| Message ID | 20210128144405.4157244-4-atenart@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| 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 | 5 maintainers not CCed: alexanderduyck@fb.com edumazet@google.com christian.brauner@ubuntu.com yebin10@huawei.com abelits@marvell.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, 221 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 Thu, Jan 28, 2021 at 6:44 AM Antoine Tenart <atenart@kernel.org> wrote: > > Most of the xps_cpus_show and xps_rxqs_show functions share the same > logic. Having it in two different functions does not help maintenance > and we can already see small implementation differences. This should not > be the case and this patch moves their common logic into a new function, > xps_queue_show, to improve maintenance. > > While the rtnl lock could be held in the new xps_queue_show, it is still > held in xps_cpus_show and xps_rxqs_show as this is an important > information when looking at those two functions. This does not add > complexity. > > Signed-off-by: Antoine Tenart <atenart@kernel.org> > --- > net/core/net-sysfs.c | 168 ++++++++++++++++++++----------------------- > 1 file changed, 79 insertions(+), 89 deletions(-) > > diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c > index 5a39e9b38e5f..6e6bc05181f6 100644 > --- a/net/core/net-sysfs.c > +++ b/net/core/net-sysfs.c > @@ -1314,77 +1314,98 @@ static const struct attribute_group dql_group = { > #endif /* CONFIG_BQL */ > > #ifdef CONFIG_XPS > -static ssize_t xps_cpus_show(struct netdev_queue *queue, > - char *buf) > +/* Should be called with the rtnl lock held. */ > +static int xps_queue_show(struct net_device *dev, unsigned long **mask, > + unsigned int index, bool is_rxqs_map) > { > - int cpu, len, ret, num_tc = 1, tc = 0; > - struct net_device *dev = queue->dev; > + const unsigned long *possible_mask = NULL; > + int j, num_tc = 0, tc = 0, ret = 0; > struct xps_dev_maps *dev_maps; > - unsigned long *mask; > - unsigned int index; > - > - if (!netif_is_multiqueue(dev)) > - return -ENOENT; > - > - index = get_netdev_queue_index(queue); > - > - if (!rtnl_trylock()) > - return restart_syscall(); > + unsigned int nr_ids; > > if (dev->num_tc) { > /* Do not allow XPS on subordinate device directly */ > num_tc = dev->num_tc; > - if (num_tc < 0) { > - ret = -EINVAL; > - goto err_rtnl_unlock; > - } > + if (num_tc < 0) > + return -EINVAL; > > /* 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; > } > > - mask = bitmap_zalloc(nr_cpu_ids, GFP_KERNEL); > - if (!mask) { > - ret = -ENOMEM; > - goto err_rtnl_unlock; > + rcu_read_lock(); > + > + if (is_rxqs_map) { > + dev_maps = rcu_dereference(dev->xps_rxqs_map); > + nr_ids = dev->num_rx_queues; > + } else { > + dev_maps = rcu_dereference(dev->xps_cpus_map); > + nr_ids = nr_cpu_ids; > + if (num_possible_cpus() > 1) > + possible_mask = cpumask_bits(cpu_possible_mask); > } I was good with what we had up until this point. THe issue is we are allocating the nr_ids for the bitmap in one location, and then populating it here. In order to keep this consisten we would need to either hold the rtnl_lock in the case of the number of Rx queues, or we would need to call cpus_read_lock in order to prevent the number of CPUs from changing. It may be better to look at encoding the number of IDs into the map first, and then using that value. Otherwise we need to be holding the appropriate lock or passing the number of IDs ourselves as an argument. Also we can just drop the possible_mask. No point in carrying it and it will simplify the loop below as we shouldn't have added the CPU if it wasn't possible to access it. > + if (!dev_maps) > + goto rcu_unlock; > > - rcu_read_lock(); > - dev_maps = rcu_dereference(dev->xps_cpus_map); > - if (dev_maps) { > - for_each_possible_cpu(cpu) { > - int i, tci = cpu * num_tc + tc; > - struct xps_map *map; > - > - map = rcu_dereference(dev_maps->attr_map[tci]); > - if (!map) > - continue; > - > - for (i = map->len; i--;) { > - if (map->queues[i] == index) { > - set_bit(cpu, mask); > - break; > - } > + for (j = -1; j = netif_attrmask_next(j, possible_mask, nr_ids), > + j < nr_ids;) { I would drop the mask check and just work from 0 to nr_ids - 1. > + int i, tci = j * num_tc + tc; > + struct xps_map *map; > + > + map = rcu_dereference(dev_maps->attr_map[tci]); > + if (!map) > + continue; > + > + for (i = map->len; i--;) { > + if (map->queues[i] == index) { > + set_bit(j, *mask); > + break; > } > } > } > + > +rcu_unlock: > rcu_read_unlock(); > > + return ret; > +} > + > +static ssize_t xps_cpus_show(struct netdev_queue *queue, char *buf) > +{ > + struct net_device *dev = queue->dev; > + unsigned long *mask; > + unsigned int index; > + int len, ret; > + > + if (!netif_is_multiqueue(dev)) > + return -ENOENT; > + > + index = get_netdev_queue_index(queue); > + > + mask = bitmap_zalloc(nr_cpu_ids, GFP_KERNEL); The nr_cpu_ids could change from this read to the one in the xps_queue_show function. We should either wrap this in a READ_ONCE and use a local variable to track the nr_ids or we need to wrap this in cpus_read_lock. > + if (!mask) > + return -ENOMEM; > + > + if (!rtnl_trylock()) { > + bitmap_free(mask); > + return restart_syscall(); > + } > + > + ret = xps_queue_show(dev, &mask, index, false); > rtnl_unlock(); > > + if (ret) { > + bitmap_free(mask); > + return ret; > + } > + > len = bitmap_print_to_pagebuf(false, buf, mask, nr_cpu_ids); > bitmap_free(mask); > return len < PAGE_SIZE ? len : -EINVAL; > - > -err_rtnl_unlock: > - rtnl_unlock(); > - return ret; > } > > static ssize_t xps_cpus_store(struct netdev_queue *queue, > @@ -1430,65 +1451,34 @@ static struct netdev_queue_attribute xps_cpus_attribute __ro_after_init > > static ssize_t xps_rxqs_show(struct netdev_queue *queue, char *buf) > { > - int j, len, ret, num_tc = 1, tc = 0; > struct net_device *dev = queue->dev; > - struct xps_dev_maps *dev_maps; > unsigned long *mask; > unsigned int index; > + int len, ret; > > index = get_netdev_queue_index(queue); > > - if (!rtnl_trylock()) > - return restart_syscall(); > - > - if (dev->num_tc) { > - num_tc = dev->num_tc; > - tc = netdev_txq_to_tc(dev, index); > - if (tc < 0) { > - ret = -EINVAL; > - goto err_rtnl_unlock; > - } > - } > mask = bitmap_zalloc(dev->num_rx_queues, GFP_KERNEL); The dev->num_rx_queues could change without the rtnl lock being held. As such we should either use a READ_ONCE or we need to encapsulate the entire thing in an rtnl_lock. > - if (!mask) { > - ret = -ENOMEM; > - goto err_rtnl_unlock; > - } > - > - rcu_read_lock(); > - dev_maps = rcu_dereference(dev->xps_rxqs_map); > - if (!dev_maps) > - goto out_no_maps; > - > - for (j = -1; j = netif_attrmask_next(j, NULL, dev->num_rx_queues), > - j < dev->num_rx_queues;) { > - int i, tci = j * num_tc + tc; > - struct xps_map *map; > - > - map = rcu_dereference(dev_maps->attr_map[tci]); > - if (!map) > - continue; > + if (!mask) > + return -ENOMEM; > > - for (i = map->len; i--;) { > - if (map->queues[i] == index) { > - set_bit(j, mask); > - break; > - } > - } > + if (!rtnl_trylock()) { > + bitmap_free(mask); > + return restart_syscall(); > } > -out_no_maps: > - rcu_read_unlock(); > > + ret = xps_queue_show(dev, &mask, index, true); > rtnl_unlock(); > > + if (ret) { > + bitmap_free(mask); > + return ret; > + } > + > len = bitmap_print_to_pagebuf(false, buf, mask, dev->num_rx_queues); > bitmap_free(mask); > > return len < PAGE_SIZE ? len : -EINVAL; > - > -err_rtnl_unlock: > - rtnl_unlock(); > - return ret; > } > > static ssize_t xps_rxqs_store(struct netdev_queue *queue, const char *buf, > -- > 2.29.2 >
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index 5a39e9b38e5f..6e6bc05181f6 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -1314,77 +1314,98 @@ static const struct attribute_group dql_group = { #endif /* CONFIG_BQL */ #ifdef CONFIG_XPS -static ssize_t xps_cpus_show(struct netdev_queue *queue, - char *buf) +/* Should be called with the rtnl lock held. */ +static int xps_queue_show(struct net_device *dev, unsigned long **mask, + unsigned int index, bool is_rxqs_map) { - int cpu, len, ret, num_tc = 1, tc = 0; - struct net_device *dev = queue->dev; + const unsigned long *possible_mask = NULL; + int j, num_tc = 0, tc = 0, ret = 0; struct xps_dev_maps *dev_maps; - unsigned long *mask; - unsigned int index; - - if (!netif_is_multiqueue(dev)) - return -ENOENT; - - index = get_netdev_queue_index(queue); - - if (!rtnl_trylock()) - return restart_syscall(); + unsigned int nr_ids; if (dev->num_tc) { /* Do not allow XPS on subordinate device directly */ num_tc = dev->num_tc; - if (num_tc < 0) { - ret = -EINVAL; - goto err_rtnl_unlock; - } + if (num_tc < 0) + return -EINVAL; /* 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; } - mask = bitmap_zalloc(nr_cpu_ids, GFP_KERNEL); - if (!mask) { - ret = -ENOMEM; - goto err_rtnl_unlock; + rcu_read_lock(); + + if (is_rxqs_map) { + dev_maps = rcu_dereference(dev->xps_rxqs_map); + nr_ids = dev->num_rx_queues; + } else { + dev_maps = rcu_dereference(dev->xps_cpus_map); + nr_ids = nr_cpu_ids; + if (num_possible_cpus() > 1) + possible_mask = cpumask_bits(cpu_possible_mask); } + if (!dev_maps) + goto rcu_unlock; - rcu_read_lock(); - dev_maps = rcu_dereference(dev->xps_cpus_map); - if (dev_maps) { - for_each_possible_cpu(cpu) { - int i, tci = cpu * num_tc + tc; - struct xps_map *map; - - map = rcu_dereference(dev_maps->attr_map[tci]); - if (!map) - continue; - - for (i = map->len; i--;) { - if (map->queues[i] == index) { - set_bit(cpu, mask); - break; - } + for (j = -1; j = netif_attrmask_next(j, possible_mask, nr_ids), + j < nr_ids;) { + int i, tci = j * num_tc + tc; + struct xps_map *map; + + map = rcu_dereference(dev_maps->attr_map[tci]); + if (!map) + continue; + + for (i = map->len; i--;) { + if (map->queues[i] == index) { + set_bit(j, *mask); + break; } } } + +rcu_unlock: rcu_read_unlock(); + return ret; +} + +static ssize_t xps_cpus_show(struct netdev_queue *queue, char *buf) +{ + struct net_device *dev = queue->dev; + unsigned long *mask; + unsigned int index; + int len, ret; + + if (!netif_is_multiqueue(dev)) + return -ENOENT; + + index = get_netdev_queue_index(queue); + + mask = bitmap_zalloc(nr_cpu_ids, GFP_KERNEL); + if (!mask) + return -ENOMEM; + + if (!rtnl_trylock()) { + bitmap_free(mask); + return restart_syscall(); + } + + ret = xps_queue_show(dev, &mask, index, false); rtnl_unlock(); + if (ret) { + bitmap_free(mask); + return ret; + } + len = bitmap_print_to_pagebuf(false, buf, mask, nr_cpu_ids); bitmap_free(mask); return len < PAGE_SIZE ? len : -EINVAL; - -err_rtnl_unlock: - rtnl_unlock(); - return ret; } static ssize_t xps_cpus_store(struct netdev_queue *queue, @@ -1430,65 +1451,34 @@ static struct netdev_queue_attribute xps_cpus_attribute __ro_after_init static ssize_t xps_rxqs_show(struct netdev_queue *queue, char *buf) { - int j, len, ret, num_tc = 1, tc = 0; struct net_device *dev = queue->dev; - struct xps_dev_maps *dev_maps; unsigned long *mask; unsigned int index; + int len, ret; index = get_netdev_queue_index(queue); - if (!rtnl_trylock()) - return restart_syscall(); - - if (dev->num_tc) { - num_tc = dev->num_tc; - tc = netdev_txq_to_tc(dev, index); - if (tc < 0) { - ret = -EINVAL; - goto err_rtnl_unlock; - } - } mask = bitmap_zalloc(dev->num_rx_queues, GFP_KERNEL); - if (!mask) { - ret = -ENOMEM; - goto err_rtnl_unlock; - } - - rcu_read_lock(); - dev_maps = rcu_dereference(dev->xps_rxqs_map); - if (!dev_maps) - goto out_no_maps; - - for (j = -1; j = netif_attrmask_next(j, NULL, dev->num_rx_queues), - j < dev->num_rx_queues;) { - int i, tci = j * num_tc + tc; - struct xps_map *map; - - map = rcu_dereference(dev_maps->attr_map[tci]); - if (!map) - continue; + if (!mask) + return -ENOMEM; - for (i = map->len; i--;) { - if (map->queues[i] == index) { - set_bit(j, mask); - break; - } - } + if (!rtnl_trylock()) { + bitmap_free(mask); + return restart_syscall(); } -out_no_maps: - rcu_read_unlock(); + ret = xps_queue_show(dev, &mask, index, true); rtnl_unlock(); + if (ret) { + bitmap_free(mask); + return ret; + } + len = bitmap_print_to_pagebuf(false, buf, mask, dev->num_rx_queues); bitmap_free(mask); return len < PAGE_SIZE ? len : -EINVAL; - -err_rtnl_unlock: - rtnl_unlock(); - return ret; } static ssize_t xps_rxqs_store(struct netdev_queue *queue, const char *buf,
Most of the xps_cpus_show and xps_rxqs_show functions share the same logic. Having it in two different functions does not help maintenance and we can already see small implementation differences. This should not be the case and this patch moves their common logic into a new function, xps_queue_show, to improve maintenance. While the rtnl lock could be held in the new xps_queue_show, it is still held in xps_cpus_show and xps_rxqs_show as this is an important information when looking at those two functions. This does not add complexity. Signed-off-by: Antoine Tenart <atenart@kernel.org> --- net/core/net-sysfs.c | 168 ++++++++++++++++++++----------------------- 1 file changed, 79 insertions(+), 89 deletions(-)