| Message ID | 20210208171917.1088230-1-atenart@kernel.org |
|---|---|
| Headers | show |
| Series | net: xps: improve the xps maps handling | expand |
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
Hello, A small series moving the xps cpus/rxqs retrieval logic in net-sysfs was sent[1] and while there was no comments asking for modifications in the patches themselves, we had discussions about other xps related reworks. In addition to the patches sent in the previous series[1] (included), this series has extra patches introducing the modifications we discussed. The main change is moving dev->num_tc and dev->nr_ids in the xps maps, to avoid out-of-bound accesses as those two fields can be updated after the maps have been allocated. This allows further reworks, to improve the xps code readability and allow to stop taking the rtnl lock when reading the maps in sysfs. Finally, the maps are moved to an array in net_device, which simplifies the code a lot. One future improvement may be to remove the use of xps_map_mutex from net/core/dev.c, but that may require extra care. Thanks! Antoine [1] https://lore.kernel.org/netdev/20210106180428.722521-1-atenart@kernel.org/ Since v1: - Reordered the patches to improve readability and avoid introducing issues in between patches. - Use dev_maps->nr_ids to allocate the mask in xps_queue_show but still default to nr_cpu_ids/dev->num_rx_queues in xps_queue_show when dev_maps hasn't been allocated yet for backward compatibility. Antoine Tenart (12): net-sysfs: convert xps_cpus_show to bitmap_zalloc net-sysfs: store the return of get_netdev_queue_index in an unsigned int net-sysfs: make xps_cpus_show and xps_rxqs_show consistent net: embed num_tc in the xps maps net: embed nr_ids in the xps maps net: assert the rtnl lock is held when calling __netif_set_xps_queue net: remove the xps possible_mask net: move the xps maps to an array net-sysfs: remove the rtnl lock when accessing the xps maps net: add an helper to copy xps maps to the new dev_maps net: improve queue removal readability in __netif_set_xps_queue net-sysfs: move the xps cpus/rxqs retrieval in a common function drivers/net/virtio_net.c | 2 +- include/linux/netdevice.h | 27 ++++- net/core/dev.c | 233 +++++++++++++++++++------------------- net/core/net-sysfs.c | 165 +++++++++------------------ 4 files changed, 194 insertions(+), 233 deletions(-)