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

On Tue, Feb 2, 2021 at 5:01 PM Jakub Kicinski [off-list ref] wrote:

On Fri, 29 Jan 2021 10:18:12 -0800 Wei Wang wrote:

quoted

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 <redacted>
Signed-off-by: Hannes Frederic Sowa <redacted>
Co-developed-by: Felix Fietkau <nbd@nbd.name>
Signed-off-by: Felix Fietkau <nbd@nbd.name>
Signed-off-by: Wei Wang <redacted>

quoted

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

quoted

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

quoted

+     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);

quoted

+}