Message-Id: <20211005142258.557-1-alexandr.lobakin@intel.com>
Date:   Tue,  5 Oct 2021 16:22:58 +0200
From:   Alexander Lobakin <alexandr.lobakin@...el.com>
To:     Saeed Mahameed <saeed@...nel.org>
Cc:     Alexander Lobakin <alexandr.lobakin@...el.com>,
        Andrew Lunn <andrew@...n.ch>,
        Jian Shen <shenjian15@...wei.com>, davem@...emloft.net,
        kuba@...nel.org, hkallweit1@...il.com, netdev@...r.kernel.org,
        linuxarm@...neuler.org
Subject: Re: [RFCv2 net-next 000/167] net: extend the netdev_features_t

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

> On Mon, 2021-10-04 at 16:59 +0200, Andrew Lunn wrote:
> > On Fri, Oct 01, 2021 at 05:17:10PM +0200, Alexander Lobakin wrote:
> > > From: Jian Shen <shenjian15@...wei.com>
> > > Date: Wed, 29 Sep 2021 23:50:47 +0800
> > >
> > > Hi,
> > >
> > > > 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?
> >
> > > 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.

> > Andrew

Thanks,
Al

Powered by blists - more mailing lists