Re: [RFC net-next 1/6] netdevice: Add napi_affinity_no_change

[Stanislav Fomichev <sdf@fomichev.me>]

On 08/12, Joe Damato wrote:
> On Mon, Aug 12, 2024 at 01:23:27PM -0700, Stanislav Fomichev wrote:
> > On 08/12, Joe Damato wrote:
> > > Several drivers have their own, very similar, implementations of
> > > determining if IRQ affinity has changed. Create napi_affinity_no_change
> > > to centralize this logic in the core.
> > > 
> > > This will be used in following commits for various drivers to eliminate
> > > duplicated code.
> > > 
> > > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > > ---
> > >  include/linux/netdevice.h |  8 ++++++++
> > >  net/core/dev.c            | 14 ++++++++++++++
> > >  2 files changed, 22 insertions(+)
> > > 
> > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > index 0ef3eaa23f4b..dc714a04b90a 100644
> > > --- a/include/linux/netdevice.h
> > > +++ b/include/linux/netdevice.h
> > > @@ -464,6 +464,14 @@ enum rx_handler_result {
> > >  typedef enum rx_handler_result rx_handler_result_t;
> > >  typedef rx_handler_result_t rx_handler_func_t(struct sk_buff **pskb);
> > >  
> > > +/**
> > > + * napi_affinity_no_change - determine if CPU affinity changed
> > > + * @irq: the IRQ whose affinity may have changed
> > > + *
> > > + * Return true if the CPU affinity has NOT changed, false otherwise.
> > > + */
> > > +bool napi_affinity_no_change(unsigned int irq);
> > > +
> > >  void __napi_schedule(struct napi_struct *n);
> > >  void __napi_schedule_irqoff(struct napi_struct *n);
> > >  
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 751d9b70e6ad..9c56ad49490c 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -89,6 +89,7 @@
> > >  #include <linux/errno.h>
> > >  #include <linux/interrupt.h>
> > >  #include <linux/if_ether.h>
> > > +#include <linux/irq.h>
> > >  #include <linux/netdevice.h>
> > >  #include <linux/etherdevice.h>
> > >  #include <linux/ethtool.h>
> > > @@ -6210,6 +6211,19 @@ void __napi_schedule_irqoff(struct napi_struct *n)
> > >  }
> > >  EXPORT_SYMBOL(__napi_schedule_irqoff);
> > >  
> > > +bool napi_affinity_no_change(unsigned int irq)
> > > +{
> > > +	int cpu_curr = smp_processor_id();
> > > +	const struct cpumask *aff_mask;
> > > +
> > 
> > [..]
> > 
> > > +	aff_mask = irq_get_effective_affinity_mask(irq);
> > 
> > Most drivers don't seem to call this on every napi_poll (and
> > cache the aff_mask somewhere instead). Should we try to keep this
> > out of the past path as well?
> 
> Hm, I see what you mean. It looks like only gve calls it on every
> poll, while the others use a cached value.
> 
> Maybe a better solution is to:
>   1. Have the helper take the cached affinity mask from the driver
>      and return true/false.
>   2. Update gve to cache the mask (like the other 4 are doing).

SG! GVE is definitely the outlier here.

> FWIW, it seems i40e added this code to solve a specific bug [1] and
> I would assume other drivers either hit the same issue (or were
> inspired by i40e).
> 
> In general: I think the logic is here to stay and other drivers may
> do something similar in the future.

+1 on pushing this logic to the core if possible.

> It'd be nice to have one helper instead of several different
> copies/implementations.
> 
> [1]: https://patchwork.ozlabs.org/project/intel-wired-lan/patch/1473895479-23035-9-git-send-email-bimmy.pujari@intel.com/