| Message ID | 20210208171917.1088230-10-atenart@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: xps: improve the xps maps handling | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 3 maintainers not CCed: christian.brauner@ubuntu.com alexanderduyck@fb.com edumazet@google.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 67 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Mon, Feb 8, 2021 at 9:19 AM Antoine Tenart <atenart@kernel.org> wrote: > > 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. > @@ -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 >
Quoting Alexander Duyck (2021-02-08 23:20:58) > On Mon, Feb 8, 2021 at 9:19 AM Antoine Tenart <atenart@kernel.org> wrote: > > @@ -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. Yet another possible race :-) Good catch, I thought about the one we fixed already but not this one. As the sb_dev pointer is protected by the rtnl lock, we'll have to keep the lock. I'll move that patch at the end of the series (it'll be easier to add the get_device/put_device logic with the xps_queue_show function). I'll also move netdev_txq_to_tc out of xps_queue_show, to call it under the rtnl lock taken. Thanks, Antoine
Quoting Antoine Tenart (2021-02-09 10:12:11) > > As the sb_dev pointer is protected by the rtnl lock, we'll have to keep > the lock. I'll move that patch at the end of the series (it'll be easier > to add the get_device/put_device logic with the xps_queue_show > function). I'll also move netdev_txq_to_tc out of xps_queue_show, to > call it under the rtnl lock taken. That was not very clear. I meant I won't remove the rtnl lock, but will try not to take it for too long, using get_device/put_device. We'll see if I'll have a dedicated patch for that or not.
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]); @@ -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; }
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(-)