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

[Ilya Maximets <i.maximets@ovn.org>]

On 8/14/25 11:37 PM, Yury Norov wrote:
> 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.

Yeah, it is fairly puzzling. :)

> 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))

We don't really need to do that, a plain for_each_cpu() should be enough,
because 0 is always set in the mask at the moment we allocate the flow
structure, see:

net/openvswitch/flow_table.c: ovs_flow_alloc():

   cpumask_set_cpu(0, flow->cpu_used_mask);

This is true since commit:

  c4b2bf6b4a35 ("openvswitch: Optimize operations for OvS flow_stats.")

The open-coded loop is just an artifact of the previous implementation.

> 
> I'll send v2 shortly.
> 
> Thanks for pointing to this, eagle eye :).