I will answer Morten in another mail, because I got his point on the AUTONEG as a separate bit, and it _makes_ sense to me.
But Neilo, 2015-09-15 10:25 GMT+02:00 N?lio Laranjeiro <nelio.laranjeiro at 6wind.com>: > On Tue, Sep 15, 2015 at 12:50:11AM +0200, Morten Br?rup wrote: > > Comments inline, marked MB>. > > > > Med venlig hilsen / kind regards > > - Morten Br?rup > > > > Marc Sune <marcdevel at gmail.com> on 14. september 2015 23:34 wrote: > > > > 2015-09-14 12:52 GMT+02:00 Morten Br?rup <mb at smartsharesystems.com>: > > > It is important to consider that a multipath link (bonding etc.) is > not a physical link, but a logical link (built on top of multiple physical > links). Regardless whether it is a Layer2 link aggregate (IEEE 802.1ad, > Ethernet bonding, EtherChannel, DSL pair bonding, etc.) or a Layer3 > multipath link (e.g. simultaneously using Wi-Fi and mobile networks). So it > doesn't make sense trying to impose physical link properties on a purely > logical link. Likewise, it doesn't make sense to impose logical link > properties on physical links. In other words: Don't consider bonding or any > other logical link types when designing the PHY API. > > > > +1 > > +1. > > > > > > > > I think there is consensus that 1/ (PHY capabilities) and 2/ (PHY > advertisements) should use the same definitions, specifically a bitmap > field. And when you disregard bonding, I don't see any reason to use > different definitions for 3/ (PHY negotiation result). This makes it one > unified API for all three purposes. > > > > Agree. > > I don't agree with this one, some PMDs don't use the advertise of > autoneg result to get the speed or the duplex. You make a > generality from your case above all PMDs. > can you please explain how a particular PMD is recovering the actual link speed and the duplex has to do with the design of the (general) API? > > Mellanox get the speed, duplex and status information from IOCTLs > which are not related to your bitmap. So at least for this PMD, there > is already a conversion from 3 fields to a bitmap, knowing that it will > use the speed as an integer after. What is the benefit of your solution? > I said already I don't have a strong preference for 3/. But steering the design of an API through a "minimum common denominator" principle is not a good idea, specially since we are talking about a super simple mapping issue for this specific PMD. > > > > Nelio suggested adding a support function to convert the bitmap field > to a speed value as an integer. I strongly support this, because you cannot > expect the bitmap to be ordered by speed. > > > > Agree with Nelio&you. This is useful. > > It was exactly the extreme opposite, a function which takes a > rte_eth_link to a bitmap i.e. speed_to_bm (rte_eth_link link) because, > the speed is mostly used as an integer and not some kind of bitmap. > > > > This support function will be able to determine which speed is higher > when exotic speeds are added to the bitmap. Please extend this conversion > function to give three output parameters: speed, full/half duplex, auto > negotiation/non-auto negotiation, or add two separate functions to get the > duplex and auto-negotiation. > > > > Since, Full/Half duplex is for legacy 10/100Mbps only (afaik), I have my > doubts on using a bit for all speeds. I would suggest to define (unroll) > 100M (or 100M_FD) and 100M_HD, and the same 10Mbps/1gbps, as Thomas was > suggesting some mails ago. > > > > This was done in v4 (implicitely 100M == 100M_FD). See below. > > > > MB> I didn't intend two bits to be allocated in the bitmap for all > speeds to support full/half duplex, only for the relevant speeds. Since > full duplex is dominant, I agree with the previous decision (originally > suggested by Thomas, I think) to make full duplex implicit unless half > duplex is explicitly specified. E.g. 10M_HD, 10M (alias 10M_FD), 100M_HD, > 100M (alias 100M_FD), 1000M (or 1G), 2500M, 10G, 40G, 100G, etc. > > > > > > > I haven't read the suggested code, but there should be some means in > 2/ (advertisements) to disable auto negotiation, e.g. a single bit in the > bitmap to indicate if the speed/duplex-indicating bits in the bitmap means > forced speed/duplex (in which case only a single speed/duplex-bit should be > set) or auto negotiation advertised speed/duplex (in which case multiple > speed/duplex-bits can be set). > > > > Agree. > > > > v3/4 of this patch adds the bitmap in the advertised, as per discussed, > to select a group of speeds This is not implemented by drivers yet (!). > > > > So, as of v4 of this patch, there could be: a) autoneg any supported > speed (=> bitmap == 0) b) autoneg over group of speeds (=> more than one > bit set in the bitmap) c) forced speed (one and only one set in the bitmap). > > > > I think this is precisely what you meant + b) as a bonus > > > > MB> This was not what I meant, but it wasn't very clearly written, so > I'll try again: Add an additional single bit "NO_AUTONEG" (or whatever you > want to name it) to the 2/ (advertisements) bitmap that explicitly turns > off auto negotiation and tries to force the selected speed/duplex (i.e. > only one other bit can be set in the bitmap when the NO_AUTONEG bit is > set). Your c) makes it impossible to use auto negotiation to advertise a > specific speed/duplex, e.g. 100M_FD. My suggested NO_AUTONEG bit can also > be used in 3/ (result) to indicate that the speed was a result of Parallel > Detection, i.e. that auto negotiation failed or was disabled in either end > of the link. > > > > MB> However, I like your suggestion a). > > > > > > > And some means in 3/ (result) and maybe 2/ (advertisements) to select > and/or indicate physical interface in dual-personality ports (e.g. ports > where the PHY has both an SFP and a RJ45 connector, but only one of the two > can be used at any time). > > > > For rte_eth_link_get() I don't have such a strong opinion. You either > > > > * encode the link speed and duplex as of now, separating duplex and > numeric speed. I would suggest to add the encoded speed+duplex bitmap flag > for consistency (although redundant). > > * or you return a single value, the bitmap with a single flag set of the > unrolled speeds, and then have the helpers int rte_eth_speed_from_bm(int > val_bm) and bool rte_eth_duplex_from_bm(int val_bm). > > > > MB> I prefer the latter of the two, only because it makes 3/ (result) > consistent with 1/ (capabilities) and 2/ (advertisements). > > So I agree for 1/ capabilities and 2/ advertisements. > > But, I don't agree to modify rte_eth_link_get API > (and rte_eth_link structure) thus 3/ result. > We don't need a "consistent" result, we need something usable. This is > not the case of the bitmap and using some conversion functions. > Remember that the speed and duplex will not change until the next link > down and there is a lot of code using speeds as integers. > Your solution will just increase the number of instruction to get the > same result, is that a benefit? > So do you think we should care about roughly 10cycles (at very most) which is what a unique switch case mapping will take? We are not talking about the dataplane here, Neilo. The benefit that Morten is arguing here is to have a unified API, consistent for the user. Once more, I don't have a preference, though I see what he means. I am not sure if the bitmap for retrieving the link is really more usable than the current API, which is IMHO what should steer the discussion, not performance. marc > > In addition, some PMDs need the speed to make some stuff with it, > so this structure will be replicated all over DPDK. > > -- > N?lio Laranjeiro > 6WIND >
