Re: [RFCv2 net-next 000/167] net: extend the netdev_features_t

From: Saeed Mahameed <saeed@kernel.org>
Date: Mon, 04 Oct 2021 15:30:21 -0700

On Mon, 2021-10-04 at 16:59 +0200, Andrew Lunn wrote:

quoted

On Fri, Oct 01, 2021 at 05:17:10PM +0200, Alexander Lobakin wrote:

quoted

From: Jian Shen <shenjian15@huawei.com>
Date: Wed, 29 Sep 2021 23:50:47 +0800

Hi,

quoted

For the prototype of netdev_features_t is u64, and the number
of netdevice feature bits is 64 now. So there is no space to
introduce new feature bit.

This patchset try to solve it by change the prototype of
netdev_features_t from u64 to bitmap. With this change,
it's necessary to introduce a set of bitmap operation helpers
for netdev features. Meanwhile, the functions which use
netdev_features_t as return value are also need to be changed,
return the result as an output parameter.

With above changes, it will affect hundreds of files, and all the
nic drivers. To make it easy to be reviewed, split the changes
to 167 patches to 5 parts.

If you leave the current feature field set (features, hw_features
etc.) as is and just add new ones as bitmaps -- I mean, to place
only newly added features there -- you won't have to change this in
hundreds of drivers.

That makes things messy for the future. Two different ways to express
the same thing. And it is a trap waiting for developers to fall
into. Is this a new feature or an old feature bit? Should i add it to
the old or new bitmap? Will the compiler error out if i get it wrong,
or silently accept it?

quoted

Another option is to introduce new fields as bitmaps and mirror all
features there, but also keep the current ones. This implies some
code duplication -- to keep both sets in sync -- but it will also
allow to avoid such diffstats. Developers could switch their
drivers
one-by-one then, and once they finish converting,

Which will never happen. Most developers will say, why bother, it
works as it is, i'm too lazy. And many drivers don't have an active
developer, and so won't get converted.

Yes it is a big patchset, but at the end, we get a uniform API which
is future proof, and no traps waiting for developers to fall into.

I agree, i had to visit this topic a year ago or so, and the only
conclusion was is to solve this the hard way, introduce a totally new
mechanism, the safest way is to remove old netdev_features_t fields
from netdev and add new ones (both names and types), so compiler will
catch you if you missed to convert a place.

Makes sense! I'm more a fan of "total conversions" rather than
keeping both old/new, just wasn't sure about how long it would
take to collect acks. On the other hand, there is probably no
need to wait for every single driver team: no real logic changes,
sole convertion stuff to bitmaps.

Anyway, we ran out of bits for both netdev_features_t and
priv_flags, so such changes need to be done.

maybe hide the implementation details and abstract it away from drivers
using getters and manipulation APIs, it is not that bad since drivers
are already not supposed to modify netdev_features directly.

That can also be actual, the only thing is that we need to wrap only
basic access to netdev_features_t type itself as we have a set of
fields (hw, vlan, gso etc.) to work with.

quoted

Andrew

Thanks,
Al