Re: [PATCH net-next v9 3/3] net: add sysfs attribute to control napi threaded mode
From: Jakub Kicinski <kuba@kernel.org>
Date: 2021-02-03 00:29:36
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 <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>
+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?
quoted hunk ↗ jump to hunk
+ 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.
+ 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);
+}