linux-kernel - Re: [PATCH] net: openvswitch: Use for_each_cpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c5b583a8-65da-4adb-8cf1-73bf679c0593@ovn.org>
Date: Thu, 14 Aug 2025 22:49:30 +0200
From: Ilya Maximets <i.maximets@....org>
To: Yury Norov <yury.norov@...il.com>, Aaron Conole <aconole@...hat.com>,
 Eelco Chaudron <echaudro@...hat.com>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Simon Horman <horms@...nel.org>, netdev@...r.kernel.org,
 dev@...nvswitch.org, linux-kernel@...r.kernel.org
Cc: i.maximets@....org
Subject: Re: [PATCH] net: openvswitch: Use for_each_cpu_from() where
 appropriate

On 8/14/25 9:58 PM, Yury Norov wrote:
> From: Yury Norov (NVIDIA) <yury.norov@...il.com>
> 
> Openvswitch opencodes for_each_cpu_from(). Fix it and drop some
> housekeeping code.
> 
> Signed-off-by: Yury Norov (NVIDIA) <yury.norov@...il.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?

>  
>  	*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() ?

Note: the original logic here came from using for_each_node() back when
stats were collected per numa, and it was important to check node 0 when
the system didn't have it, so the loop was open-coded, see commit:
  40773966ccf1 ("openvswitch: fix flow stats accounting when node 0 is not possible")

Later the stats collection was changed to be per-CPU instead of per-NUMA,
th eloop was adjusted to CPUs, but remained open-coded, even though it
was probbaly safe to use for_each_cpu() macro here, as it accepts the
mask and doesn't limit it to available CPUs, unlike the for_each_node()
macro that only iterates over possible NUMA node numbers and will skip
the zero.  The zero is importnat, because it is used as long as only one
core updates the stats, regardless of the number of that core, AFAIU.

So, the comments in the code do not really make a lot of sense, especially
in this patch.

Best regards, Ilya Maximets.