| Message ID | 20220826132754.360124-2-amorenoz@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Detect changes in cpu affinity | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Fri, Aug 26, 2022 at 3:28 PM Adrian Moreno <amorenoz@redhat.com> 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: david.marchand@redhat.com > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- > 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?
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 <amorenoz@redhat.com> 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: david.marchand@redhat.com >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >> --- >> 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.
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; }
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: david.marchand@redhat.com Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- lib/ovs-thread.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-)