Re: [PATCH net-next v8 1/6] net: rtnetlink: add pacing_offload_horizon attribute to net_device

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Jakub Kicinski wrote:
> On Tue, 8 Sep 2026 18:39:20 -0400 Willem de Bruijn wrote:
> > On Tue, Sep 8, 2026 at 5:48 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > It's just a way of avoiding netdev_features_t becoming larger than 64b.
> > > At the uAPI level we have a bitmap so we can express any number of bits.
> > > But in the kernel dev->features is u64 / ull so if we run out of bits
> > > the refactor will be rather painful.
> > >
> > > Is it really that much worse to add the support for "high feature bits"
> > > which don't go thru netdev_features_t to ethtool, than sprinkling extra
> > > one off booleans into already bloated RTM_SETLINK? Not sure.  
> > 
> > How would this work with hw_features, wanted_features, vlan_features
> > and other such feature masks?
> > 
> > There is potentially quite a bit of logic that needs to be duplicated
> > for a secondary set of features. Or, the risk that these features end
> > up not quite like normal features.
> 
> No different than a loose bit in SETLINK?

At least then there is no expectation of netdev_features_t behavior.
 
> What I was wondering is - we don't want to implement full handling for
> high feature bits, agreed, it doesn't seem needed today. But is it
> better to add such a bit in ethtool regardless, even tho it doesn't get
> any infra for propagation to uppers etc. Or is it better to start adding
> one-off bits in rt-link.
> 
> Think about it some more, since the max-horizon attr is already in
> rt-link I guess putting this bit in rt-link does make more sense.

Ok, I'll leave this in rt-link then.

Even then, running out of feature bits will come back to haunt us
soon enough. A good idea to look into it some more now.

The main issue here is not wanting to grow netdev_features_t for
hot cachelines, right? Which besides growing dev->features has a
cascading effect on all the other fields of that type in net_device
too.

Extending struct net_device itself is fine, as long as it happens in
some cold cacheline at the end.

Perhaps something like what Paolo did for virtio features in the
series of 3b17aa13015c ("virtio_net: add supports for extended
offloads"). With non-contiguous bitmap fields. Everything beyond 64
is mapped to a new field at the end of the struct. A thin API to
avoid open-coding that check everywhere. And selective conversion
only of code/drivers that need to access the extended features.

> > > I hate both notifiers everywhere and the idea that we have to be able
> > > to quietly revoke device features "on firmware rollout". It leads to
> > > unmaintainable code which almost never runs so it's buggy half of the
> > > time. Whatever.  
> > 
> > I don't like it, but firmware roll-outs that remove features
> > unfortunately are a real thing. Especially roll-backs.
> > 
> > For pacing offload specifically, I considered the risk low enough to
> > rely on the admin to manually revert the FQ settings when such an
> > event happens. But the bots kept complaining. And in fairness a
> > notifier based auto disable is indeed much more robust than a manual
> > correlated roll-out. OTOH, it is rarely exercised code in practice and
> > thus more prone to latent bugs.
> 
> TBH I'm not sure what you have in mind with the notifier.
> What netdev event does the FW reset generate? 
>
> > A simpler approach for pacing offload is to check the dev fields
> > directly in fq. It is likely that that cacheline is warm.
> 
> Right, there's ~30b of unused flag space in the first cache line
> of struct net_device. Should be warm.

I can move dev->pacing_offload there.

max_pacing_offload is in a cold line. Finding a way to squeeze that
somewhere warm can perhaps be left for later.