Re: [PATCH net-next v9 3/3] net: add sysfs attribute to control napi threaded mode

[Alexander Duyck <alexander.duyck@gmail.com>]

On Tue, Feb 2, 2021 at 5:01 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 29 Jan 2021 10:18:12 -0800 Wei Wang wrote:
> > This patch adds a new sysfs attribute to the network device class.
> > Said attribute provides a per-device control to enable/disable the
> > threaded mode for all the napi instances of the given network device,
> > without the need for a device up/down.
> > User sets it to 1 or 0 to enable or disable threaded mode.
> >
> > Co-developed-by: Paolo Abeni <pabeni@redhat.com>
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > Co-developed-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > Co-developed-by: Felix Fietkau <nbd@nbd.name>
> > Signed-off-by: Felix Fietkau <nbd@nbd.name>
> > Signed-off-by: Wei Wang <weiwan@google.com>
>
> > +static int napi_set_threaded(struct napi_struct *n, bool threaded)
> > +{
> > +     int err = 0;
> > +
> > +     if (threaded == !!test_bit(NAPI_STATE_THREADED, &n->state))
> > +             return 0;
> > +
> > +     if (!threaded) {
> > +             clear_bit(NAPI_STATE_THREADED, &n->state);
>
> Can we put a note in the commit message saying that stopping the
> threads is slightly tricky but we'll do it if someone complains?
>
> Or is there a stronger reason than having to wait for thread to finish
> up with the NAPI not to stop them?

Normally if we are wanting to shut down NAPI we would have to go
through a coordinated process with us setting the NAPI_STATE_DISABLE
bit and then having to sit on NAPI_STATE_SCHED. Doing that would
likely cause a traffic hiccup if somebody toggles this while the NIC
is active so probably best to not interfere.

I suspect this should be more than enough to have us switch in and out
of the threaded setup. I don't think leaving the threads allocated
after someone has enabled it once should be much of an issue. As far
as using just the bit to do the disable, I think the most it would
probably take is a second or so for the queues to switch over from
threaded to normal NAPI again.

> > +             return 0;
> > +     }
> > +
> > +     if (!n->thread) {
> > +             err = napi_kthread_create(n);
> > +             if (err)
> > +                     return err;
> > +     }
> > +
> > +     /* Make sure kthread is created before THREADED bit
> > +      * is set.
> > +      */
> > +     smp_mb__before_atomic();
> > +     set_bit(NAPI_STATE_THREADED, &n->state);
> > +
> > +     return 0;
> > +}
> > +
> > +static void dev_disable_threaded_all(struct net_device *dev)
> > +{
> > +     struct napi_struct *napi;
> > +
> > +     list_for_each_entry(napi, &dev->napi_list, dev_list)
> > +             napi_set_threaded(napi, false);
> > +     dev->threaded = 0;
> > +}
> > +
> > +int dev_set_threaded(struct net_device *dev, bool threaded)
> > +{
> > +     struct napi_struct *napi;
> > +     int ret;
> > +
> > +     dev->threaded = threaded;
> > +     list_for_each_entry(napi, &dev->napi_list, dev_list) {
> > +             ret = napi_set_threaded(napi, threaded);
> > +             if (ret) {
> > +                     /* Error occurred on one of the napi,
> > +                      * reset threaded mode on all napi.
> > +                      */
> > +                     dev_disable_threaded_all(dev);
> > +                     break;
> > +             }
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> >  void netif_napi_add(struct net_device *dev, struct napi_struct *napi,
> >                   int (*poll)(struct napi_struct *, int), int weight)
> >  {
> > diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> > index daf502c13d6d..884f049ee395 100644
> > --- a/net/core/net-sysfs.c
> > +++ b/net/core/net-sysfs.c
> > @@ -538,6 +538,55 @@ static ssize_t phys_switch_id_show(struct device *dev,
> >  }
> >  static DEVICE_ATTR_RO(phys_switch_id);
> >
> > +static ssize_t threaded_show(struct device *dev,
> > +                          struct device_attribute *attr, char *buf)
> > +{
> > +     struct net_device *netdev = to_net_dev(dev);
> > +     int ret;
> > +
> > +     if (!rtnl_trylock())
> > +             return restart_syscall();
> > +
> > +     if (!dev_isalive(netdev)) {
> > +             ret = -EINVAL;
> > +             goto unlock;
> > +     }
> > +
> > +     if (list_empty(&netdev->napi_list)) {
> > +             ret = -EOPNOTSUPP;
> > +             goto unlock;
> > +     }
>
> Maybe others disagree but I'd take this check out. What's wrong with
> letting users see that threaded napi is disabled for devices without
> NAPI?
>
> This will also help a little devices which remove NAPIs when they are
> down.
>
> I've been caught off guard in the past by the fact that kernel returns
> -ENOENT for XPS map when device has a single queue.

I agree there isn't any point to the check. I think this is a
hold-over from the original code that was querying each napi structure
assigned to the device.

> > +     ret = sprintf(buf, fmt_dec, netdev->threaded);
> > +
> > +unlock:
> > +     rtnl_unlock();
> > +     return ret;
> > +}
> > +
> > +static int modify_napi_threaded(struct net_device *dev, unsigned long val)
> > +{
> > +     int ret;
> > +
> > +     if (list_empty(&dev->napi_list))
> > +             return -EOPNOTSUPP;
> > +
> > +     if (val != 0 && val != 1)
> > +             return -EOPNOTSUPP;
> > +
> > +     ret = dev_set_threaded(dev, val);
> > +
> > +     return ret;
>
> return dev_set_threaded(dev, val);
>
> > +}