| Message ID | 20250128164646.4009-3-yury.norov@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | cpumask: cleanup cpumask_next_wrap() implementation and usage | expand |
On Tue, Jan 28, 2025 at 11:46:31AM -0500, Yury Norov wrote: > The inner loop may be replaced with the dedicated for_each_online_cpu_wrap. > It helps to avoid setting the same bits in the @mask more than once, in > case of group_size is greater than number of online CPUs. nit: Looking at the previous logic of how group_stride is calculated, I don't think there is possibility of "setting the same bits in the @mask more than once". group_stride = n_cpu / n_queues nit: I see this more as 2 patches. The introduction of a new core helper function is a bit buried. > > CC: Nick Child <nnac123@linux.ibm.com> > Signed-off-by: Yury Norov <yury.norov@gmail.com> Don't know if my comments alone merit a v3 and I think the patch does simplify the codebase so: Reviewed-by: Nick Child <nnac123@linux.ibm.com> > --- > drivers/net/virtio_net.c | 12 +++++++----- > include/linux/cpumask.h | 4 ++++ > 2 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 7646ddd9bef7..9d7c37e968b5 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -3826,7 +3826,7 @@ static void virtnet_set_affinity(struct virtnet_info *vi) > cpumask_var_t mask; > int stragglers; > int group_size; > - int i, j, cpu; > + int i, start = 0, cpu; > int num_cpu; > int stride; > > @@ -3840,16 +3840,18 @@ static void virtnet_set_affinity(struct virtnet_info *vi) > stragglers = num_cpu >= vi->curr_queue_pairs ? > num_cpu % vi->curr_queue_pairs : > 0; > - cpu = cpumask_first(cpu_online_mask); > > for (i = 0; i < vi->curr_queue_pairs; i++) { > group_size = stride + (i < stragglers ? 1 : 0); > > - for (j = 0; j < group_size; j++) { > + for_each_online_cpu_wrap(cpu, start) { > + if (!group_size--) { > + start = cpu; > + break; > + } > cpumask_set_cpu(cpu, mask); > - cpu = cpumask_next_wrap(cpu, cpu_online_mask, > - nr_cpu_ids, false); > } > + > virtqueue_set_affinity(vi->rq[i].vq, mask); > virtqueue_set_affinity(vi->sq[i].vq, mask); > __netif_set_xps_queue(vi->dev, cpumask_bits(mask), i, XPS_CPUS); > diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h > index 5cf69a110c1c..30042351f15f 100644 > --- a/include/linux/cpumask.h > +++ b/include/linux/cpumask.h > @@ -1036,6 +1036,8 @@ extern const DECLARE_BITMAP(cpu_all_bits, NR_CPUS); > > #define for_each_possible_cpu_wrap(cpu, start) \ > for ((void)(start), (cpu) = 0; (cpu) < 1; (cpu)++) > +#define for_each_online_cpu_wrap(cpu, start) \ > + for ((void)(start), (cpu) = 0; (cpu) < 1; (cpu)++) > #else > #define for_each_possible_cpu(cpu) for_each_cpu((cpu), cpu_possible_mask) > #define for_each_online_cpu(cpu) for_each_cpu((cpu), cpu_online_mask) > @@ -1044,6 +1046,8 @@ extern const DECLARE_BITMAP(cpu_all_bits, NR_CPUS); > > #define for_each_possible_cpu_wrap(cpu, start) \ > for_each_cpu_wrap((cpu), cpu_possible_mask, (start)) > +#define for_each_online_cpu_wrap(cpu, start) \ > + for_each_cpu_wrap((cpu), cpu_online_mask, (start)) > #endif > > /* Wrappers for arch boot code to manipulate normally-constant masks */ > -- > 2.43.0 >
Thanks for review and testing, Nick! On Wed, Feb 05, 2025 at 05:00:23PM -0600, Nick Child wrote: > On Tue, Jan 28, 2025 at 11:46:31AM -0500, Yury Norov wrote: > > The inner loop may be replaced with the dedicated for_each_online_cpu_wrap. > > It helps to avoid setting the same bits in the @mask more than once, in > > case of group_size is greater than number of online CPUs. > > nit: Looking at the previous logic of how group_stride is calculated, I don't > think there is possibility of "setting the same bits in the @mask more > than once". group_stride = n_cpu / n_queues > > nit: I see this more as 2 patches. The introduction of a new core > helper function is a bit buried. > > > > > CC: Nick Child <nnac123@linux.ibm.com> > > Signed-off-by: Yury Norov <yury.norov@gmail.com> > > Don't know if my comments alone merit a v3 and I think the patch > does simplify the codebase so: > Reviewed-by: Nick Child <nnac123@linux.ibm.com> I fixed the comments to #2 and #3 as you suggested and split-out new for_each() loops to the new patch. I also think those are trivial changes not worth v3. So it's in bitmap-for-next: https://github.com/norov/linux/tree/bitmap-for-next Thanks for review, Nick! Thanks, Yury > > --- > > drivers/net/virtio_net.c | 12 +++++++----- > > include/linux/cpumask.h | 4 ++++ > > 2 files changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 7646ddd9bef7..9d7c37e968b5 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -3826,7 +3826,7 @@ static void virtnet_set_affinity(struct virtnet_info *vi) > > cpumask_var_t mask; > > int stragglers; > > int group_size; > > - int i, j, cpu; > > + int i, start = 0, cpu; > > int num_cpu; > > int stride; > > > > @@ -3840,16 +3840,18 @@ static void virtnet_set_affinity(struct virtnet_info *vi) > > stragglers = num_cpu >= vi->curr_queue_pairs ? > > num_cpu % vi->curr_queue_pairs : > > 0; > > - cpu = cpumask_first(cpu_online_mask); > > > > for (i = 0; i < vi->curr_queue_pairs; i++) { > > group_size = stride + (i < stragglers ? 1 : 0); > > > > - for (j = 0; j < group_size; j++) { > > + for_each_online_cpu_wrap(cpu, start) { > > + if (!group_size--) { > > + start = cpu; > > + break; > > + } > > cpumask_set_cpu(cpu, mask); > > - cpu = cpumask_next_wrap(cpu, cpu_online_mask, > > - nr_cpu_ids, false); > > } > > + > > virtqueue_set_affinity(vi->rq[i].vq, mask); > > virtqueue_set_affinity(vi->sq[i].vq, mask); > > __netif_set_xps_queue(vi->dev, cpumask_bits(mask), i, XPS_CPUS); > > diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h > > index 5cf69a110c1c..30042351f15f 100644 > > --- a/include/linux/cpumask.h > > +++ b/include/linux/cpumask.h > > @@ -1036,6 +1036,8 @@ extern const DECLARE_BITMAP(cpu_all_bits, NR_CPUS); > > > > #define for_each_possible_cpu_wrap(cpu, start) \ > > for ((void)(start), (cpu) = 0; (cpu) < 1; (cpu)++) > > +#define for_each_online_cpu_wrap(cpu, start) \ > > + for ((void)(start), (cpu) = 0; (cpu) < 1; (cpu)++) > > #else > > #define for_each_possible_cpu(cpu) for_each_cpu((cpu), cpu_possible_mask) > > #define for_each_online_cpu(cpu) for_each_cpu((cpu), cpu_online_mask) > > @@ -1044,6 +1046,8 @@ extern const DECLARE_BITMAP(cpu_all_bits, NR_CPUS); > > > > #define for_each_possible_cpu_wrap(cpu, start) \ > > for_each_cpu_wrap((cpu), cpu_possible_mask, (start)) > > +#define for_each_online_cpu_wrap(cpu, start) \ > > + for_each_cpu_wrap((cpu), cpu_online_mask, (start)) > > #endif > > > > /* Wrappers for arch boot code to manipulate normally-constant masks */ > > -- > > 2.43.0 > >
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 7646ddd9bef7..9d7c37e968b5 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -3826,7 +3826,7 @@ static void virtnet_set_affinity(struct virtnet_info *vi) cpumask_var_t mask; int stragglers; int group_size; - int i, j, cpu; + int i, start = 0, cpu; int num_cpu; int stride; @@ -3840,16 +3840,18 @@ static void virtnet_set_affinity(struct virtnet_info *vi) stragglers = num_cpu >= vi->curr_queue_pairs ? num_cpu % vi->curr_queue_pairs : 0; - cpu = cpumask_first(cpu_online_mask); for (i = 0; i < vi->curr_queue_pairs; i++) { group_size = stride + (i < stragglers ? 1 : 0); - for (j = 0; j < group_size; j++) { + for_each_online_cpu_wrap(cpu, start) { + if (!group_size--) { + start = cpu; + break; + } cpumask_set_cpu(cpu, mask); - cpu = cpumask_next_wrap(cpu, cpu_online_mask, - nr_cpu_ids, false); } + virtqueue_set_affinity(vi->rq[i].vq, mask); virtqueue_set_affinity(vi->sq[i].vq, mask); __netif_set_xps_queue(vi->dev, cpumask_bits(mask), i, XPS_CPUS); diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h index 5cf69a110c1c..30042351f15f 100644 --- a/include/linux/cpumask.h +++ b/include/linux/cpumask.h @@ -1036,6 +1036,8 @@ extern const DECLARE_BITMAP(cpu_all_bits, NR_CPUS); #define for_each_possible_cpu_wrap(cpu, start) \ for ((void)(start), (cpu) = 0; (cpu) < 1; (cpu)++) +#define for_each_online_cpu_wrap(cpu, start) \ + for ((void)(start), (cpu) = 0; (cpu) < 1; (cpu)++) #else #define for_each_possible_cpu(cpu) for_each_cpu((cpu), cpu_possible_mask) #define for_each_online_cpu(cpu) for_each_cpu((cpu), cpu_online_mask) @@ -1044,6 +1046,8 @@ extern const DECLARE_BITMAP(cpu_all_bits, NR_CPUS); #define for_each_possible_cpu_wrap(cpu, start) \ for_each_cpu_wrap((cpu), cpu_possible_mask, (start)) +#define for_each_online_cpu_wrap(cpu, start) \ + for_each_cpu_wrap((cpu), cpu_online_mask, (start)) #endif /* Wrappers for arch boot code to manipulate normally-constant masks */
The inner loop may be replaced with the dedicated for_each_online_cpu_wrap. It helps to avoid setting the same bits in the @mask more than once, in case of group_size is greater than number of online CPUs. CC: Nick Child <nnac123@linux.ibm.com> Signed-off-by: Yury Norov <yury.norov@gmail.com> --- drivers/net/virtio_net.c | 12 +++++++----- include/linux/cpumask.h | 4 ++++ 2 files changed, 11 insertions(+), 5 deletions(-)