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.no...@gmail.com>
>>>>>
>>>>> Openvswitch opencodes for_each_cpu_from(). Fix it and drop some
>>>>> housekeeping code.
>>>>>
>>>>> Signed-off-by: Yury Norov (NVIDIA) <yury.no...@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 :).
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
