Message-ID: <20250401112706.2ff58e3d@kernel.org>
Date: Tue, 1 Apr 2025 11:27:06 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Joe Damato <jdamato@...tly.com>
Cc: Samiullah Khawaja <skhawaja@...gle.com>, "David S . Miller "
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni
 <pabeni@...hat.com>, almasrymina@...gle.com, willemb@...gle.com,
 mkarsten@...terloo.ca, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v4 1/4] Add support to set napi threaded for
 individual napi

On Tue, 25 Mar 2025 07:51:00 -0700 Jakub Kicinski wrote:
> On Fri, 21 Mar 2025 10:10:09 -0700 Joe Damato wrote:
> > > +int napi_set_threaded(struct napi_struct *napi, bool threaded)
> > > +{
> > > +	if (napi->dev->threaded)
> > > +		return -EINVAL;    
> > 
> > This works differently than the existing per-NAPI defer_hard_irqs /
> > gro_flush_timeout which are also interface wide.
> > 
> > In that implementation: 
> >   - the per-NAPI value is set when requested by the user
> >   - when the sysfs value is written, all NAPIs have their values
> >     overwritten to the sysfs value
> > 
> > I think either:
> >   - This implementation should work like the existing ones, or
> >   - The existing ones should be changed to work like this
> > 
> > I am opposed to have two different behaviors when setting per-NAPI
> > vs system/nic-wide sysfs values.
> > 
> > I don't have a preference on which behavior is chosen, but the
> > behavior should be the same for all of the things that are
> > system/nic-wide and moving to per-NAPI.  
> 
> And we should probably have a test that verifies the consistency
> for all the relevant attrs.

I was thinking about it some more in another context, and I decided 
to write down what came to mind. Does this make sense as part of
our docs?

===================================
Adding new configuration interfaces
===================================

Best practices for implementing new configuration interfaces in networking.

Multi-level configuration
=========================

In certain cases the same configuration option can be specified with different
levels of granularity, e.g. global configuration, and device-level
configuration. Finer-grained rules always take precedence. A more tricky
problem is what effect should changing the coarser settings have on already
present finer settings. Setting coarser configuration can either reset
all finer grained rules ("write all" semantics), or affect only objects
for which finer grained rules have not been specified ("default" semantics).

The "default" semantics are recommended unless clear and documented reason
exists for the interface to behave otherwise.

Configuration persistence
=========================

User configuration should always be preserved, as long as related objects
exist.

No loss on close
----------------

Closing and opening a net_device should not result in loss of configuration.
Dynamically allocated objects should be re-instantiated when the device
is opened.

No partial loss
---------------

Loss of configuration is only acceptable due to asynchronous device errors,
and in response to explicit reset requests from the user (``devlink reload``,
``ethtool --reset``, etc.). The implementation should not attempt to preserve
the objects affected by configuration loss (e.g. if some of net_device
configuration is lost, the net_device should be unregistered and re-registered
as part of the reset procedure).

Explicit default tracking
-------------------------

Network configuration is often performed in multiple steps, so it is important
that conflicting user requests cause an explicit error, rather than silent
reset of previously requested settings to defaults. For example, if user
first requests an RSS indirection table directing to queues 0, 1, and 2,
and then sets the queue count to 2 the queue count change should be rejected.

This implies that network configuration often needs to include an indication
whether given setting has been requested by a user, or is a default value
populated by the core, or the driver. What's more the user configuration API
may need to provide an ability to not only set a value but also to reset
it back to the default.

Indexed objects
---------------

Configuration related to indexed objects (queues, NAPI instances etc.)
should not be reset when device is closed, but should be reset when object
is explicitly "freed" by the user. For example reducing the queue count
should discard configuration of now-disabled queued.

Core validation
===============

For driver-facing APIs the networking stack should do its best to validate
the request (using maintained state and potentially requesting other config
from the driver via GET methods), before passing the configuration to
the driver.

Testing
=======

All new configuration APIs are required to be accompanied by tests,
including tests validating the configuration persistence, and (if applicable)
the interactions of multi-level configuration.

Tests validating the API should support execution against netdevsim,
and real drivers (netdev Python tests have this support built-in).

Powered by blists - more mailing lists