| Message ID | 20210208171917.1088230-8-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 | 9 maintainers not CCed: xiyou.wangcong@gmail.com christian.brauner@ubuntu.com ap420073@gmail.com edumazet@google.com ast@kernel.org daniel@iogearbox.net bjorn@kernel.org andriin@fb.com alexanderduyck@fb.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: 10 this patch: 10 |
| 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, 130 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 10 this patch: 10 |
| 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: > > Remove the xps possible_mask. It was an optimization but we can just > loop from 0 to nr_ids now that it is embedded in the xps dev_maps. That > simplifies the code a bit. > > Suggested-by: Alexander Duyck <alexander.duyck@gmail.com> > Signed-off-by: Antoine Tenart <atenart@kernel.org> > --- > net/core/dev.c | 43 ++++++++++++++----------------------------- > net/core/net-sysfs.c | 4 ++-- > 2 files changed, 16 insertions(+), 31 deletions(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index abbb2ae6b3ed..d0c07ccea2e5 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -2505,33 +2505,27 @@ static void reset_xps_maps(struct net_device *dev, > kfree_rcu(dev_maps, rcu); > } > > -static void clean_xps_maps(struct net_device *dev, const unsigned long *mask, > +static void clean_xps_maps(struct net_device *dev, > struct xps_dev_maps *dev_maps, u16 offset, u16 count, > bool is_rxqs_map) > { > - unsigned int nr_ids = dev_maps->nr_ids; > bool active = false; > int i, j; > > - for (j = -1; j = netif_attrmask_next(j, mask, nr_ids), j < nr_ids;) > - active |= remove_xps_queue_cpu(dev, dev_maps, j, offset, > - count); > + for (j = 0; j < dev_maps->nr_ids; j++) > + active |= remove_xps_queue_cpu(dev, dev_maps, j, offset, count); > if (!active) > reset_xps_maps(dev, dev_maps, is_rxqs_map); > > - if (!is_rxqs_map) { > - for (i = offset + (count - 1); count--; i--) { > + if (!is_rxqs_map) > + for (i = offset + (count - 1); count--; i--) > netdev_queue_numa_node_write( > - netdev_get_tx_queue(dev, i), > - NUMA_NO_NODE); > - } > - } > + netdev_get_tx_queue(dev, i), NUMA_NO_NODE); > } > This violates the coding-style guide for the kernel. The if statement should still have braces as the for loop and netdev_queue_numa_node_write are more than a single statement. I'd be curious to see if checkpatch also complains about this because it probably should. For reference see the end of section 3.0 in Documentation/process/coding-style.rst. Other than that the rest of the patch seemed to be fine.
Quoting Alexander Duyck (2021-02-08 22:43:39) > On Mon, Feb 8, 2021 at 9:19 AM Antoine Tenart <atenart@kernel.org> wrote: > > > > -static void clean_xps_maps(struct net_device *dev, const unsigned long *mask, > > +static void clean_xps_maps(struct net_device *dev, > > struct xps_dev_maps *dev_maps, u16 offset, u16 count, > > bool is_rxqs_map) > > { > > - unsigned int nr_ids = dev_maps->nr_ids; > > bool active = false; > > int i, j; > > > > - for (j = -1; j = netif_attrmask_next(j, mask, nr_ids), j < nr_ids;) > > - active |= remove_xps_queue_cpu(dev, dev_maps, j, offset, > > - count); > > + for (j = 0; j < dev_maps->nr_ids; j++) > > + active |= remove_xps_queue_cpu(dev, dev_maps, j, offset, count); > > if (!active) > > reset_xps_maps(dev, dev_maps, is_rxqs_map); > > > > - if (!is_rxqs_map) { > > - for (i = offset + (count - 1); count--; i--) { > > + if (!is_rxqs_map) > > + for (i = offset + (count - 1); count--; i--) > > netdev_queue_numa_node_write( > > - netdev_get_tx_queue(dev, i), > > - NUMA_NO_NODE); > > - } > > - } > > + netdev_get_tx_queue(dev, i), NUMA_NO_NODE); > > } > > This violates the coding-style guide for the kernel. The if statement > should still have braces as the for loop and > netdev_queue_numa_node_write are more than a single statement. I'd be > curious to see if checkpatch also complains about this because it > probably should. You're right, I'll remove that change to comply with the coding style. I reran checkpatch, even with --strict, and it did not complain. Maybe because it's a rework, not strictly new code. Thanks, Antoine
diff --git a/net/core/dev.c b/net/core/dev.c index abbb2ae6b3ed..d0c07ccea2e5 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2505,33 +2505,27 @@ static void reset_xps_maps(struct net_device *dev, kfree_rcu(dev_maps, rcu); } -static void clean_xps_maps(struct net_device *dev, const unsigned long *mask, +static void clean_xps_maps(struct net_device *dev, struct xps_dev_maps *dev_maps, u16 offset, u16 count, bool is_rxqs_map) { - unsigned int nr_ids = dev_maps->nr_ids; bool active = false; int i, j; - for (j = -1; j = netif_attrmask_next(j, mask, nr_ids), j < nr_ids;) - active |= remove_xps_queue_cpu(dev, dev_maps, j, offset, - count); + for (j = 0; j < dev_maps->nr_ids; j++) + active |= remove_xps_queue_cpu(dev, dev_maps, j, offset, count); if (!active) reset_xps_maps(dev, dev_maps, is_rxqs_map); - if (!is_rxqs_map) { - for (i = offset + (count - 1); count--; i--) { + if (!is_rxqs_map) + for (i = offset + (count - 1); count--; i--) netdev_queue_numa_node_write( - netdev_get_tx_queue(dev, i), - NUMA_NO_NODE); - } - } + netdev_get_tx_queue(dev, i), NUMA_NO_NODE); } static void netif_reset_xps_queues(struct net_device *dev, u16 offset, u16 count) { - const unsigned long *possible_mask = NULL; struct xps_dev_maps *dev_maps; if (!static_key_false(&xps_needed)) @@ -2543,17 +2537,14 @@ static void netif_reset_xps_queues(struct net_device *dev, u16 offset, if (static_key_false(&xps_rxqs_needed)) { dev_maps = xmap_dereference(dev->xps_rxqs_map); if (dev_maps) - clean_xps_maps(dev, possible_mask, dev_maps, offset, - count, true); + clean_xps_maps(dev, dev_maps, offset, count, true); } dev_maps = xmap_dereference(dev->xps_cpus_map); if (!dev_maps) goto out_no_maps; - if (num_possible_cpus() > 1) - possible_mask = cpumask_bits(cpu_possible_mask); - clean_xps_maps(dev, possible_mask, dev_maps, offset, count, false); + clean_xps_maps(dev, dev_maps, offset, count, false); out_no_maps: mutex_unlock(&xps_map_mutex); @@ -2609,8 +2600,8 @@ static struct xps_map *expand_xps_map(struct xps_map *map, int attr_index, int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask, u16 index, bool is_rxqs_map) { - const unsigned long *online_mask = NULL, *possible_mask = NULL; struct xps_dev_maps *dev_maps, *new_dev_maps = NULL; + const unsigned long *online_mask = NULL; bool active = false, copy = false; int i, j, tci, numa_node_id = -2; int maps_sz, num_tc = 1, tc = 0; @@ -2640,10 +2631,8 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask, nr_ids = dev->num_rx_queues; } else { maps_sz = XPS_CPU_DEV_MAPS_SIZE(num_tc); - if (num_possible_cpus() > 1) { + if (num_possible_cpus() > 1) online_mask = cpumask_bits(cpu_online_mask); - possible_mask = cpumask_bits(cpu_possible_mask); - } dev_maps = xmap_dereference(dev->xps_cpus_map); nr_ids = nr_cpu_ids; } @@ -2693,8 +2682,7 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask, static_key_slow_inc_cpuslocked(&xps_rxqs_needed); } - for (j = -1; j = netif_attrmask_next(j, possible_mask, nr_ids), - j < nr_ids;) { + for (j = 0; j < nr_ids; j++) { /* copy maps belonging to foreign traffic classes */ for (i = tc, tci = j * num_tc; copy && i--; tci++) { /* fill in the new device map from the old device map */ @@ -2749,8 +2737,7 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask, if (!dev_maps) goto out_no_old_maps; - for (j = -1; j = netif_attrmask_next(j, possible_mask, nr_ids), - j < nr_ids;) { + for (j = 0; j < nr_ids; j++) { for (i = num_tc, tci = j * dev_maps->num_tc; i--; tci++) { map = xmap_dereference(dev_maps->attr_map[tci]); if (!map) @@ -2784,8 +2771,7 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask, goto out_no_maps; /* removes tx-queue from unused CPUs/rx-queues */ - for (j = -1; j = netif_attrmask_next(j, possible_mask, dev_maps->nr_ids), - j < dev_maps->nr_ids;) { + for (j = 0; j < dev_maps->nr_ids; j++) { for (i = tc, tci = j * dev_maps->num_tc; i--; tci++) active |= remove_xps_queue(dev_maps, tci, index); if (!netif_attr_test_mask(j, mask, dev_maps->nr_ids) || @@ -2805,8 +2791,7 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask, return 0; error: /* remove any maps that we added */ - for (j = -1; j = netif_attrmask_next(j, possible_mask, nr_ids), - j < nr_ids;) { + for (j = 0; j < nr_ids; j++) { for (i = num_tc, tci = j * num_tc; i--; tci++) { new_map = xmap_dereference(new_dev_maps->attr_map[tci]); map = copy ? diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index e544e5f2467c..6180c35d5108 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -1353,7 +1353,7 @@ static ssize_t xps_cpus_show(struct netdev_queue *queue, if (!dev_maps || tc >= dev_maps->num_tc) goto out_no_maps; - for (j = -1; j = netif_attrmask_next(j, NULL, nr_ids), j < nr_ids;) { + for (j = 0; j < nr_ids; j++) { int i, tci = j * dev_maps->num_tc + tc; struct xps_map *map; @@ -1457,7 +1457,7 @@ static ssize_t xps_rxqs_show(struct netdev_queue *queue, char *buf) if (!dev_maps || tc >= dev_maps->num_tc) goto out_no_maps; - for (j = -1; j = netif_attrmask_next(j, NULL, nr_ids), j < nr_ids;) { + for (j = 0; j < nr_ids; j++) { int i, tci = j * dev_maps->num_tc + tc; struct xps_map *map;
Remove the xps possible_mask. It was an optimization but we can just loop from 0 to nr_ids now that it is embedded in the xps dev_maps. That simplifies the code a bit. Suggested-by: Alexander Duyck <alexander.duyck@gmail.com> Signed-off-by: Antoine Tenart <atenart@kernel.org> --- net/core/dev.c | 43 ++++++++++++++----------------------------- net/core/net-sysfs.c | 4 ++-- 2 files changed, 16 insertions(+), 31 deletions(-)