Re: [PATCH v4 0/2] ethdev: add port speed capability bitmap

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

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@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