From:   Eelco Chaudron <echaudro@redhat.com>
To:     Eddy Tao <taoyuan_eddy@hotmail.com>
Cc:     netdev@vger.kernel.org, dev@openvswitch.org,
        linux-kernel@vger.kernel.org, Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "David S. Miller" <davem@davemloft.net>
Subject: Re: [ovs-dev] [PATCH net-next v3 1/1] net:openvswitch:reduce
 cpu_used_mask memory
Date:   Thu, 02 Feb 2023 12:05:44 +0100
Message-ID: <561547E5-D4C2-4FD6-9B25-100719D4D379@redhat.com>
In-Reply-To: <OS3P286MB2295FA2701BCE468E367607AF5D69@OS3P286MB2295.JPNP286.PROD.OUTLOOK.COM>
References: <OS3P286MB2295FA2701BCE468E367607AF5D69@OS3P286MB2295.JPNP286.PROD.OUTLOOK.COM>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On 2 Feb 2023, at 11:32, Eddy Tao wrote:

> Use actual CPU number instead of hardcoded value to decide the size
> of 'cpu_used_mask' in 'struct sw_flow'. Below is the reason.
>
> 'struct cpumask cpu_used_mask' is embedded in struct sw_flow.
> Its size is hardcoded to CONFIG_NR_CPUS bits, which can be
> 8192 by default, it costs memory and slows down ovs_flow_alloc
> as well as the iteration of bits in cpu_used_mask when handling
> netlink message from ofproto

I=E2=80=99m trying to understand how this will decrease memory usage. The=
 size of the flow_cache stayed the same (actually it=E2=80=99s large due =
to the extra pointer).

Also do not understand why the iteration is less, as the mask is initiali=
zed the same.

Cheers,

Eelco

> To address this, redefine cpu_used_mask to pointer
> append cpumask_size() bytes after 'stat' to hold cpumask
>
> cpumask APIs like cpumask_next and cpumask_set_cpu never access
> bits beyond cpu count, cpumask_size() bytes of memory is enough
>
> Signed-off-by: Eddy Tao <taoyuan_eddy@hotmail.com>
> ---
>  net/openvswitch/flow.c       | 8 +++++---
>  net/openvswitch/flow.h       | 2 +-
>  net/openvswitch/flow_table.c | 8 +++++---
>  3 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index e20d1a973417..0109a5f86f6a 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -107,7 +107,7 @@ void ovs_flow_stats_update(struct sw_flow *flow, __=
be16 tcp_flags,
>
>  					rcu_assign_pointer(flow->stats[cpu],
>  							   new_stats);
> -					cpumask_set_cpu(cpu, &flow->cpu_used_mask);
> +					cpumask_set_cpu(cpu, flow->cpu_used_mask);
>  					goto unlock;
>  				}
>  			}
> @@ -135,7 +135,8 @@ void ovs_flow_stats_get(const struct sw_flow *flow,=

>  	memset(ovs_stats, 0, sizeof(*ovs_stats));
>
>  	/* We open code this to make sure cpu 0 is always considered */
> -	for (cpu =3D 0; cpu < nr_cpu_ids; cpu =3D cpumask_next(cpu, &flow->cp=
u_used_mask)) {
> +	for (cpu =3D 0; cpu < nr_cpu_ids;
> +	     cpu =3D cpumask_next(cpu, flow->cpu_used_mask)) {
>  		struct sw_flow_stats *stats =3D rcu_dereference_ovsl(flow->stats[cpu=
]);
>
>  		if (stats) {
> @@ -159,7 +160,8 @@ void ovs_flow_stats_clear(struct sw_flow *flow)
>  	int cpu;
>
>  	/* We open code this to make sure cpu 0 is always considered */
> -	for (cpu =3D 0; cpu < nr_cpu_ids; cpu =3D cpumask_next(cpu, &flow->cp=
u_used_mask)) {
> +	for (cpu =3D 0; cpu < nr_cpu_ids;
> +	     cpu =3D cpumask_next(cpu, flow->cpu_used_mask)) {
>  		struct sw_flow_stats *stats =3D ovsl_dereference(flow->stats[cpu]);
>
>  		if (stats) {
> diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
> index 073ab73ffeaa..b5711aff6e76 100644
> --- a/net/openvswitch/flow.h
> +++ b/net/openvswitch/flow.h
> @@ -229,7 +229,7 @@ struct sw_flow {
>  					 */
>  	struct sw_flow_key key;
>  	struct sw_flow_id id;
> -	struct cpumask cpu_used_mask;
> +	struct cpumask *cpu_used_mask;
>  	struct sw_flow_mask *mask;
>  	struct sw_flow_actions __rcu *sf_acts;
>  	struct sw_flow_stats __rcu *stats[]; /* One for each CPU.  First one
> diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.=
c
> index 0a0e4c283f02..dc6a174c3194 100644
> --- a/net/openvswitch/flow_table.c
> +++ b/net/openvswitch/flow_table.c
> @@ -87,11 +87,12 @@ struct sw_flow *ovs_flow_alloc(void)
>  	if (!stats)
>  		goto err;
>
> +	flow->cpu_used_mask =3D (struct cpumask *)&flow->stats[nr_cpu_ids];
>  	spin_lock_init(&stats->lock);
>
>  	RCU_INIT_POINTER(flow->stats[0], stats);
>
> -	cpumask_set_cpu(0, &flow->cpu_used_mask);
> +	cpumask_set_cpu(0, flow->cpu_used_mask);
>
>  	return flow;
>  err:
> @@ -115,7 +116,7 @@ static void flow_free(struct sw_flow *flow)
>  					  flow->sf_acts);
>  	/* We open code this to make sure cpu 0 is always considered */
>  	for (cpu =3D 0; cpu < nr_cpu_ids;
> -	     cpu =3D cpumask_next(cpu, &flow->cpu_used_mask)) {
> +	     cpu =3D cpumask_next(cpu, flow->cpu_used_mask)) {
>  		if (flow->stats[cpu])
>  			kmem_cache_free(flow_stats_cache,
>  					(struct sw_flow_stats __force *)flow->stats[cpu]);
> @@ -1196,7 +1197,8 @@ int ovs_flow_init(void)
>
>  	flow_cache =3D kmem_cache_create("sw_flow", sizeof(struct sw_flow)
>  				       + (nr_cpu_ids
> -					  * sizeof(struct sw_flow_stats *)),
> +					  * sizeof(struct sw_flow_stats *))
> +				       + cpumask_size(),
>  				       0, 0, NULL);
>  	if (flow_cache =3D=3D NULL)
>  		return -ENOMEM;
> -- =

> 2.27.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev