| Message ID | 20240812145633.52911-2-jdamato@fastly.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Cleanup IRQ affinity checks in several drivers | expand |
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?
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). 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. 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/
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/
On Mon, Aug 12, 2024 at 03:36:42PM -0700, Stanislav Fomichev wrote: > 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. > > > > [...] > > > > +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. OK, I'll hack on that for rfcv2 and see what it looks like. Thanks for the suggestion. Hopefully the maintainers (or other folks) will chime in on whether or not I should submit fixes for patches 4 - 6 for the type mismatch stuff first or just handle it all together.
On Tue, Aug 13, 2024 at 10:11:09AM +0100, Joe Damato wrote: > On Mon, Aug 12, 2024 at 03:36:42PM -0700, Stanislav Fomichev wrote: > > 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. > > > > > > > [...] > > > > > > +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. > > OK, I'll hack on that for rfcv2 and see what it looks like. Thanks > for the suggestion. Yea, I just did this for rfcv2 and it looks a lot nicer/fewer changes. Will hold off on sending an rfc v2 until the 48 hour timer expires ;) > Hopefully the maintainers (or other folks) will chime in on whether > or not I should submit fixes for patches 4 - 6 for the type mismatch > stuff first or just handle it all together.
On Tue, Aug 13, 2024 at 10:11:09AM +0100, Joe Damato wrote: > On Mon, Aug 12, 2024 at 03:36:42PM -0700, Stanislav Fomichev wrote: > > 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. > > > > > > > [...] > > > > > > +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. > > OK, I'll hack on that for rfcv2 and see what it looks like. Thanks > for the suggestion. > > Hopefully the maintainers (or other folks) will chime in on whether > or not I should submit fixes for patches 4 - 6 for the type mismatch > stuff first or just handle it all together. <2c> Patches 4 - 6 seem more like clean-ups that fixes to me: they aren't fixing any bugs are they? So I would just keep them as part of this patchset unless it becomes unwieldy. </2c> In any case thanks for all your good work in this area.
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); + if (unlikely(!aff_mask)) + return true; + + return cpumask_test_cpu(cpu_curr, aff_mask); +} +EXPORT_SYMBOL(napi_affinity_no_change); + bool napi_complete_done(struct napi_struct *n, int work_done) { unsigned long flags, val, new, timeout = 0;
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(+)