Re: [PATCH][net-next] openvswitch: eliminate cpu_used_mask from sw_flow
From: Li RongQing Date: Fri, 27 Jul 2018 16:03:57 +0800 > The size of struct cpumask varies with CONFIG_NR_CPUS, some config > CONFIG_NR_CPUS is very larger, like 5120, struct cpumask will take > 640 bytes, if there is thousands of flows, it will take lots of > memory > > cpu_used_mask has two purposes > 1: Assume first cpu as cpu0 which maybe not true; now use >cpumask_first(cpu_possible_mask) > 2: when get/clear statistic, reduce the iteratation; but it >is not hot path, so use for_each_possible_cpu > > Signed-off-by: Zhang Yu > Signed-off-by: Li RongQing This seems to completely undo the optimization done by: commit c4b2bf6b4a35348fe6d1eb06928eb68d7b9d99a9 Author: Tonghao Zhang Date: Mon Jul 17 23:28:06 2017 -0700 openvswitch: Optimize operations for OvS flow_stats. And in that commit message it states clearly that flow_free() performance matters, and that the iteration over cpu_possible_mask in the for() loop is the problem. At a minimum, we can't apply this unless you explain why the above performance issue won't be reintroudced by your change. Thank you.
Re: [PATCH][net-next] openvswitch: eliminate cpu_used_mask from sw_flow
On Fri, Jul 27, 2018 at 1:03 AM, Li RongQing wrote: > The size of struct cpumask varies with CONFIG_NR_CPUS, some config > CONFIG_NR_CPUS is very larger, like 5120, struct cpumask will take > 640 bytes, if there is thousands of flows, it will take lots of > memory > I am fine with removing cpumask bitmap from flow struct. > cpu_used_mask has two purposes > 1: Assume first cpu as cpu0 which maybe not true; now use >cpumask_first(cpu_possible_mask) I am not sure about this, most of system would have cpu zero, so why this change is done in this patch ? This adds overhead of calculating first cpu when updating stats in fast path. > 2: when get/clear statistic, reduce the iteratation; but it >is not hot path, so use for_each_possible_cpu >
