On Tue, Sep 15, 2015 at 11:20:03PM +0200, Marc Sune wrote: > Adrien, > > 2015-09-15 12:04 GMT+02:00 Adrien Mazarguil <adrien.mazarguil at 6wind.com>: [...] > > It's not so much about the way PMDs recover link information, rather about > > the amount of changes required to switch to a bit-field API for the current > > link speed with no clear advantage. > > > See below; these are trivial changes. > > > All PMDs must be modified, the initial > > set of patches isn't complete in this regard. > > > > Thanks for pointing out this. There are a couple missing. > [...] > > I think Nelio was using mlx4 as an example, all PMDs have their own > > particular method to recover it and several must perform calculations to > > get > > the final value. Using integers for this task is certainly easier than > > going > > through bit-field conversions. > > > > Drivers have their *own* way to extract the link speed from the HW, because > the way is stored is anyway HW specific. That a driver encodes their link > speed as a numeric value is just a coincidence, and the exception.
I concede that most non-virtual drivers (including all Intel drivers) only perform conversions between bit-field values, for those it's just a matter of updating a macro name in a succession of if/else or switch/case statements which is indeed trivial. Exceptions are currently af_packet, bnx2x, bonding (does not care actually), enic, mlx4, null, pcap, ring, xenvirt (these last four are purely virtual and use hard-coded values, no calculations involved but still). > Specifically, and putting an example of e1000 (which you are right it is > buggy in v4, see below): [...] > Can you please tell me which exact extra conversions are needed on the PMD > side? It only needs to be fixed: [...] > Other drivers, like i40 are already ooing it correctly: Sure, I was only pointing out the extra work required to make sure all of them were behaving as expected. [...] > > Everyone agrees that a link speed bit-field is useful as an input value to > > advertise, request and allow a set of speeds. We do not agree with its > > usage > > as the current link speed which is often the result of a computation. We > > aren't talking about performance. > > > > A given link cannot be simultaneously at 10 Gbps and 1 Gbps right? Using a > > bit-field for the current link speed is confusing at best. Output values do > > not need to be included in the unified API, they are never converted back > > into enum values. > > > > Although I agree it is unlikely that this would happen, we shouldn't > anticipate what users will do, so in either approach you would need utility > functions to translate from numerical to bitmap and viceversa. Yes, obviously. > > I'm stressing again the fact that doing so would require a changes in all > > applications that use the current speed and in PMDs for no good reason. > > Well any change in the API will. This patch (v1,v2) started as speed caps > only, and now we are refactoring the link API. How much code has to be > changed or how is easier for PMDs is irrelevant IMHO. What matters is if > the API makes sense for the user. > > And for that you are probably right; it might be more comprehensible to > have "a" speed value as a result of rte_eth_get_link(), provided that we > give utility functions to go back and forth from numerical constants and > bitmap constants (both have to be defined then in rte_eth.h). I fully agree with this. Which means ETH_LINK_SPEED_* macros can be dropped (as in your patchset) and replaced with a function or a macro that simply converts their ETH_SPEED_* counterparts to integer values. That way we don't have to keep two confusing sets of macros. Probably obvious but in the reverse function, I suggest returning ETH_SPEED_UNKNOWN for invalid values instead of the nearest match. About struct rte_eth_link, Nelio intends to submit a patch to store link_speed in a uint32_t for 100+Gbps links and update a few PMDs affected by the change. Perhaps his patch should be included in your patchset? -- Adrien Mazarguil 6WIND
