Re: [PATCH net-next 1/6] net: Add sysfs parameter irq_suspend_timeout

[Joe Damato <jdamato@fastly.com>]

On Fri, Aug 23, 2024 at 07:39:56PM +0200, Eric Dumazet wrote:
> On Fri, Aug 23, 2024 at 7:31 PM Joe Damato <jdamato@fastly.com> wrote:
> >
> > From: Martin Karsten <mkarsten@uwaterloo.ca>
> >
> > This patch doesn't change any behavior but prepares the code for other
> > changes in the following commits which use irq_suspend_timeout as a
> > timeout for IRQ suspension.
> >
> > Signed-off-by: Martin Karsten <mkarsten@uwaterloo.ca>
> > Co-developed-by: Joe Damato <jdamato@fastly.com>
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > Tested-by: Joe Damato <jdamato@fastly.com>
> > Tested-by: Martin Karsten <mkarsten@uwaterloo.ca>
> > ---
> >  rfc -> v1:
> >    - Removed napi.rst documentation from this patch; added to patch 6.
> >
> >  include/linux/netdevice.h |  2 ++
> >  net/core/dev.c            |  3 ++-
> >  net/core/net-sysfs.c      | 18 ++++++++++++++++++
> >  3 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 0ef3eaa23f4b..31867bb2ff65 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1857,6 +1857,7 @@ enum netdev_reg_state {
> >   *     @gro_flush_timeout:     timeout for GRO layer in NAPI
> >   *     @napi_defer_hard_irqs:  If not zero, provides a counter that would
> >   *                             allow to avoid NIC hard IRQ, on busy queues.
> > + *     @irq_suspend_timeout:   IRQ suspension timeout
> >   *
> >   *     @rx_handler:            handler for received packets
> >   *     @rx_handler_data:       XXX: need comments on this one
> > @@ -2060,6 +2061,7 @@ struct net_device {
> >         struct netdev_rx_queue  *_rx;
> >         unsigned long           gro_flush_timeout;
> >         int                     napi_defer_hard_irqs;
> > +       unsigned long           irq_suspend_timeout;
> >         unsigned int            gro_max_size;
> >         unsigned int            gro_ipv4_max_size;
> >         rx_handler_func_t __rcu *rx_handler;
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index e7260889d4cb..3bf325ec25a3 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -11945,6 +11945,7 @@ static void __init net_dev_struct_check(void)
> >         CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_rx, _rx);
> >         CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_rx, gro_flush_timeout);
> >         CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_rx, napi_defer_hard_irqs);
> > +       CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_rx, irq_suspend_timeout);
> >         CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_rx, gro_max_size);
> >         CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_rx, gro_ipv4_max_size);
> >         CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_rx, rx_handler);
> > @@ -11956,7 +11957,7 @@ static void __init net_dev_struct_check(void)
> >  #ifdef CONFIG_NET_XGRESS
> >         CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_rx, tcx_ingress);
> >  #endif
> > -       CACHELINE_ASSERT_GROUP_SIZE(struct net_device, net_device_read_rx, 104);
> > +       CACHELINE_ASSERT_GROUP_SIZE(struct net_device, net_device_read_rx, 112);
> >  }
> >
> >  /*
> > diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> > index 0e2084ce7b75..fb6f3327310f 100644
> > --- a/net/core/net-sysfs.c
> > +++ b/net/core/net-sysfs.c
> > @@ -440,6 +440,23 @@ static ssize_t napi_defer_hard_irqs_store(struct device *dev,
> >  }
> >  NETDEVICE_SHOW_RW(napi_defer_hard_irqs, fmt_dec);
> >
> > +static int change_irq_suspend_timeout(struct net_device *dev, unsigned long val)
> > +{
> > +       WRITE_ONCE(dev->irq_suspend_timeout, val);
> > +       return 0;
> > +}
> > +
> > +static ssize_t irq_suspend_timeout_store(struct device *dev,
> > +                                        struct device_attribute *attr,
> > +                                        const char *buf, size_t len)
> > +{
> > +       if (!capable(CAP_NET_ADMIN))
> > +               return -EPERM;
> > +
> > +       return netdev_store(dev, attr, buf, len, change_irq_suspend_timeout);
> > +}
> > +NETDEVICE_SHOW_RW(irq_suspend_timeout, fmt_ulong);
> > +
> >  static ssize_t ifalias_store(struct device *dev, struct device_attribute *attr,
> >                              const char *buf, size_t len)
> >  {
> > @@ -664,6 +681,7 @@ static struct attribute *net_class_attrs[] __ro_after_init = {
> >         &dev_attr_tx_queue_len.attr,
> >         &dev_attr_gro_flush_timeout.attr,
> >         &dev_attr_napi_defer_hard_irqs.attr,
> > +       &dev_attr_irq_suspend_timeout.attr,
> >         &dev_attr_phys_port_id.attr,
> >         &dev_attr_phys_port_name.attr,
> >         &dev_attr_phys_switch_id.attr,
> > --
> > 2.25.1
> 
> 
> Please no more per-device sysfs entry, shared by all the users of the device.
> 
> Let's not repeat past mistakes.
> 
> Nowadays, we need/want per receive-queue tuning, preferably set with netlink.

Thanks for the feedback, Eric. We appreciate your consideration
and guidance.

May we ask what your thoughts are, overall, about getting a
mechanism like this accepted?

We want to make sure that this, in principle, is acceptable before
iterating further and going down the path of netlink, if required.

On the specific netlink bit in your comment, we agree in principle,
however:

  1. Our code integrates directly with existing sysfs parameters.
     If we make our parameter settable via netlink, but the others
     remain as sysfs parameters then the interface for users
     becomes a bit cumbersome.

     And, so the urge will be to move all parameters to netlink
     for ease of use for the user.

     As we mentioned in our cover letter: we agree that doing so is
     a good idea, but we hope to convince you (and the other
     maintainers) that the netlink work can come later as a separate
     change which affects the existing parameters we integrate with
     as well as the parameter we are introducing, at the same time.

  2. The proposed mechanism yields a substantial performance and
     efficiency improvement which would be valuable to users. It would
     be unfortunate to block until all of this could be moved to
     netlink, first.

  3. While adding a new sysfs parameter does affect the ABI
     permanently, it doesn't prevent us from making these parameters
     per-NAPI in the future. This series adds strength to the argument
     that these parameters should be per-NAPI because our results show
     the impact these parameters can have on network processing very
     clearly in a range of scenarios.

We appreciate your thoughts on the above as we want to ensure we
are moving in the right direction.