Message-ID: <CAAywjhRCYJ2+=ei3jhbTg-p+MRTpgj6p1f8jj8ba43Y21OW-bw@mail.gmail.com>
Date: Mon, 14 Apr 2025 10:16:12 -0700
From: Samiullah Khawaja <skhawaja@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Joe Damato <jdamato@...tly.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, Apr 1, 2025 at 11:27 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> 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).
Our current approach for napi configuration is using "write all"
semantics as Joe mentioned above and I am going to change the "napi
threaded" to that also for consistency. To do the "default" semantics,
we would need something that tracks the "dirty" configuration state in
each napi. Also if we do this, we might also need to decide on a
mechanism from the user that propagates the intent that the fine
grained configuration is not needed anymore. I can send another patch
later to switch to the "default" semantics if this is decided.

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