Re: [ovs-dev] [PATCH net-next v3 1/1] net:openvswitch:reduce cpu_used_mask memory

On 2 Feb 2023, at 12:26, 陶 缘 wrote:

Hi, Eelco:

      Thanks for your time going through the detail.
The thing is: sizeof(struct cpumask), with default CONFIG_NR_CPUS 8192, has a size of 1024 bytes even on a system with only 4 cpus.
While in practice the cpumask APIs like cpumask_next and cpumask_set_cpu never access more than cpumask_size() of bytes in the bitmap
My change used cpumask_size() (in above example, consume 8 bytes after alignement for the cpumask, it saved 1016 bytes for every flow.

I looked at the wrong nr_cpumask_bits definition, so thanks for this education :)

Your question reminded me to revisit the description "as well as the iteration of bits in cpu_used_mask", after a second think, this statement is not valid and should be removed.
since the iteration API will not access the number of bytes decided by nr_cpu_ids(running CPUs)

I will remove this statement after solving a final style issue in the next submission.

Thanks!

Thanks
eddy
________________________________
发件人: Eelco Chaudron [off-list ref]
发送时间: 2023年2月2日 11:05
收件人: Eddy Tao [off-list ref]
抄送: netdev@vger.kernel.org [off-list ref]; dev@openvswitch.org [off-list ref]; linux-kernel@vger.kernel.org [off-list ref]; Eric Dumazet [off-list ref]; Jakub Kicinski [off-list ref]; Paolo Abeni [off-list ref]; David S. Miller [off-list ref]
主题: Re: [ovs-dev] [PATCH net-next v3 1/1] net:openvswitch:reduce cpu_used_mask memory



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

quoted

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’m trying to understand how this will decrease memory usage. The size of the flow_cache stayed the same (actually it’s large due to the extra pointer).

Also do not understand why the iteration is less, as the mask is initialized the same.

Cheers,

Eelco

quoted

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 <redacted>
---
 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 = 0; cpu < nr_cpu_ids; cpu = cpumask_next(cpu, &flow->cpu_used_mask)) {
+     for (cpu = 0; cpu < nr_cpu_ids;
+          cpu = cpumask_next(cpu, flow->cpu_used_mask)) {
               struct sw_flow_stats *stats = 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 = 0; cpu < nr_cpu_ids; cpu = cpumask_next(cpu, &flow->cpu_used_mask)) {
+     for (cpu = 0; cpu < nr_cpu_ids;
+          cpu = cpumask_next(cpu, flow->cpu_used_mask)) {
               struct sw_flow_stats *stats = 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 = (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 = 0; cpu < nr_cpu_ids;
-          cpu = cpumask_next(cpu, &flow->cpu_used_mask)) {
+          cpu = 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 = 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 == NULL)
               return -ENOMEM;
--

2.27.0

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