Re: [PATCH net-next v2 09/12] net-sysfs: remove the rtnl lock when accessing the xps maps
From: Alexander Duyck <hidden>
Date: 2021-02-08 22:22:17
On Mon, Feb 8, 2021 at 9:19 AM Antoine Tenart [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Now that nr_ids and num_tc are stored in the xps dev_maps, which are RCU protected, we do not have the need to protect the xps_cpus_show and xps_rxqs_show functions with the rtnl lock. Signed-off-by: Antoine Tenart <atenart@kernel.org> --- net/core/net-sysfs.c | 26 ++++---------------------- 1 file changed, 4 insertions(+), 22 deletions(-)diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index c2276b589cfb..6ce5772e799e 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c@@ -1328,17 +1328,12 @@ static ssize_t xps_cpus_show(struct netdev_queue *queue, index = get_netdev_queue_index(queue); - if (!rtnl_trylock()) - return restart_syscall(); - /* If queue belongs to subordinate dev use its map */ dev = netdev_get_tx_queue(dev, index)->sb_dev ? : dev; tc = netdev_txq_to_tc(dev, index); - if (tc < 0) { - ret = -EINVAL; - goto err_rtnl_unlock; - } + if (tc < 0) + return -EINVAL; rcu_read_lock(); dev_maps = rcu_dereference(dev->xps_maps[XPS_CPUS]);
So I think we hit a snag here. The sb_dev pointer is protected by the rtnl_lock. So I don't think we can release the rtnl_lock until after we are done with the dev pointer. Also I am not sure it is safe to use netdev_txq_to_tc without holding the lock. I don't know if we ever went through and guaranteed that it will always work if the lock isn't held since in theory the device could reprogram all the map values out from under us. Odds are we should probably fix traffic_class_show as I suspect it probably also needs to be holding the rtnl_lock to prevent any possible races. I'll submit a patch for that.
quoted hunk ↗ jump to hunk
@@ -1371,16 +1366,12 @@ static ssize_t xps_cpus_show(struct netdev_queue *queue, out_no_maps: rcu_read_unlock(); - rtnl_unlock(); - len = bitmap_print_to_pagebuf(false, buf, mask, nr_ids); bitmap_free(mask); return len < PAGE_SIZE ? len : -EINVAL; err_rcu_unlock: rcu_read_unlock(); -err_rtnl_unlock: - rtnl_unlock(); return ret; }@@ -1435,14 +1426,9 @@ static ssize_t xps_rxqs_show(struct netdev_queue *queue, char *buf) index = get_netdev_queue_index(queue); - if (!rtnl_trylock()) - return restart_syscall(); - tc = netdev_txq_to_tc(dev, index); - if (tc < 0) { - ret = -EINVAL; - goto err_rtnl_unlock; - } + if (tc < 0) + return -EINVAL; rcu_read_lock(); dev_maps = rcu_dereference(dev->xps_maps[XPS_RXQS]);@@ -1475,8 +1461,6 @@ static ssize_t xps_rxqs_show(struct netdev_queue *queue, char *buf) out_no_maps: rcu_read_unlock(); - rtnl_unlock(); - len = bitmap_print_to_pagebuf(false, buf, mask, nr_ids); bitmap_free(mask);@@ -1484,8 +1468,6 @@ static ssize_t xps_rxqs_show(struct netdev_queue *queue, char *buf) err_rcu_unlock: rcu_read_unlock(); -err_rtnl_unlock: - rtnl_unlock(); return ret; } --2.29.2