Re: [PATCH] net: openvswitch: Use for_each_cpu_from() where appropriate

[Yury Norov <yury.norov@gmail.com>]

On Thu, Aug 14, 2025 at 11:21:02PM +0200, Ilya Maximets wrote:
> On 8/14/25 11:05 PM, Yury Norov wrote:
> > On Thu, Aug 14, 2025 at 10:49:30PM +0200, Ilya Maximets wrote:
> >> On 8/14/25 9:58 PM, Yury Norov wrote:
> >>> From: Yury Norov (NVIDIA) <yury.norov@gmail.com>
> >>>
> >>> Openvswitch opencodes for_each_cpu_from(). Fix it and drop some
> >>> housekeeping code.
> >>>
> >>> Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
> >>> ---
> >>>  net/openvswitch/flow.c       | 14 ++++++--------
> >>>  net/openvswitch/flow_table.c |  8 ++++----
> >>>  2 files changed, 10 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> >>> index b80bd3a90773..b464ab120731 100644
> >>> --- a/net/openvswitch/flow.c
> >>> +++ b/net/openvswitch/flow.c
> >>> @@ -129,15 +129,14 @@ void ovs_flow_stats_get(const struct sw_flow *flow,
> >>>  			struct ovs_flow_stats *ovs_stats,
> >>>  			unsigned long *used, __be16 *tcp_flags)
> >>>  {
> >>> -	int cpu;
> >>> +	/* CPU 0 is always considered */
> >>> +	unsigned int cpu = 1;
> >>
> >> Hmm.  I'm a bit confused here.  Where is CPU 0 considered if we start
> >> iteration from 1?
> > 
> > I didn't touch this part of the original comment, as you see, and I'm
> > not a domain expert, so don't know what does this wording mean.
> > 
> > Most likely 'always considered' means that CPU0 is not accounted in this
> > statistics.
> >   
> >>>  	*used = 0;
> >>>  	*tcp_flags = 0;
> >>>  	memset(ovs_stats, 0, sizeof(*ovs_stats));
> >>>  
> >>> -	/* We open code this to make sure cpu 0 is always considered */
> >>> -	for (cpu = 0; cpu < nr_cpu_ids;
> >>> -	     cpu = cpumask_next(cpu, flow->cpu_used_mask)) {
> >>> +	for_each_cpu_from(cpu, flow->cpu_used_mask) {
> >>
> >> And why it needs to be a for_each_cpu_from() and not just for_each_cpu() ?
> > 
> > The original code explicitly ignores CPU0.
> 
> No, it's not.  The loop explicitly starts from zero.  And the comments
> are saying that the loop is open-coded specifically to always have zero
> in the iteration.

OK, I see now. That indentation has fooled me. So the comment means
that CPU0 is included even if flow->cpu_used_mask has it cleared. And
to avoid opencoding, we need to do like:
        
        for_each_cpu_or(cpu, flow->cpu_used_mask, cpumask_of(0))

I'll send v2 shortly.

Thanks for pointing to this, eagle eye :).