Thanks for the review David! On 8/29/22 11:32, David Marchand wrote:
On Fri, Aug 26, 2022 at 3:28 PM Adrian Moreno <[email protected]> wrote:It might be OK to consider the total number of online CPUs as a static value (although certain platforms do support CPU hot-plugging and CPUs can get disabled). However, it's much more likely that ovs-vswitchd's CPU affinity mask is modified dynamically. Fix cpu calculation to detect changes in CPU affinity dynamically. Fixes: be15ec48d766 ("lib: Use a more accurate value for CPU count (sched_getaffinity).") Cc: [email protected] Signed-off-by: Adrian Moreno <[email protected]> --- lib/ovs-thread.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c index 78ed3e970..0e52a0da7 100644 --- a/lib/ovs-thread.c +++ b/lib/ovs-thread.c @@ -633,33 +633,35 @@ int count_cpu_cores(void) { static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; - static long int n_cores; + static long int n_total_cores; + long int n_cores; if (ovsthread_once_start(&once)) { #ifndef _WIN32 - n_cores = sysconf(_SC_NPROCESSORS_ONLN); -#ifdef __linux__ - if (n_cores > 0) { - cpu_set_t *set = CPU_ALLOC(n_cores); - - if (set) { - size_t size = CPU_ALLOC_SIZE(n_cores); - - if (!sched_getaffinity(0, size, set)) { - n_cores = CPU_COUNT_S(size, set); - } - CPU_FREE(set); - } - } -#endif + n_total_cores = sysconf(_SC_NPROCESSORS_ONLN); #else SYSTEM_INFO sysinfo; GetSystemInfo(&sysinfo); - n_cores = sysinfo.dwNumberOfProcessors; + n_total_cores = sysinfo.dwNumberOfProcessors; #endif ovsthread_once_done(&once); } + n_cores = n_total_cores; +#ifdef __linux__ + if (n_cores > 0) { + cpu_set_t *set = CPU_ALLOC(n_cores); + + if (set) { + size_t size = CPU_ALLOC_SIZE(n_cores); + + if (!sched_getaffinity(0, size, set)) { + n_cores = CPU_COUNT_S(size, set); + } + CPU_FREE(set); + } + } +#endif return n_cores > 0 ? n_cores : 0; }If we are changing this helper to provide the *current* core count for Linux, I would have it behave the same for all OS. And the simpler might be to drop the once check and the static qualifier on n_cores. WDYT?
Yes, I did consider it. I don't have a very strong opinion about it. I ended up laying towards keeping the static qualifier on other OSs because the chances a CPU is hotplugged or disabled/enabled at runtime are very low compared to those of the affinity mask being changed. Cost-wise, even though this is run on every bridge loop iteration I don't think it's very expensive. All in all, I ended up preferring not enabling what seems to me as almost-useless behavior vs cross-OS homogenization.
But again, If you do think it's better to keep behavior homogeneous or see a use-case where _SC_NPROCESSORS_ONLN are likely to change, I'll update the patch.
BTW, I did notice that, in the function added later on (count_total_cores) by Michael Santana, _SC_NPROCESSORS_CONF was used. Michael, do you have any issues with changing it to _SC_NPROCESSORS_ONLN?
Thanks. -- Adrián Moreno _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
