On 12/19/22 17:20, Adrian Moreno wrote:
> Currently, things like the number of handler and revalidator threads are
> calculated based on the number of available CPUs. However, this number
> is considered static and only calculated once, hence ignoring events
> such as cpus being hotplugged, switched on/off or affinity mask
> changing.
>
> On the other hand, checking the number of available CPUs multiple times
> per second seems like an overkill.
> Affinity should not change that often and, even if it does, the impact
> of destroying and recreating all the threads so often is probably a
> price too expensive to pay.
>
> I tested the impact of updating the threads every 5 seconds and saw
> an impact in the main loop duration of <1% and a worst-case scenario
> impact in throughput of < 5% [1]. This patch sets the default period to
> 10 seconds just to be safer.
>
> [1] Tested in the worst-case scenario of disabling the kernel cache
> (other_config:flow-size=0), modifying ovs-vswithd's affinity so the
> number of handlers go up and down every 5 seconds and calculated the
> difference in netperf's ops/sec.
>
> Signed-off-by: Adrian Moreno <[email protected]>
> ---
> NEWS | 2 ++
> lib/ovs-thread.c | 61 +++++++++++++++++++++++++++++-------------------
> 2 files changed, 39 insertions(+), 24 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 265375e1c..9f0a17bcc 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,5 +1,7 @@
> Post-v3.0.0
> --------------------
> + - ovs-vswitchd now detects changes in CPU affinity and adjusts the number
> + of handler and revalidator threads if necessary.
> - ovs-appctl:
> * "ovs-appctl ofproto/trace" command can now display port names with the
> "--names" option.
> diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
> index 78ed3e970..ba05b90ec 100644
> --- a/lib/ovs-thread.c
> +++ b/lib/ovs-thread.c
> @@ -31,6 +31,7 @@
> #include "openvswitch/poll-loop.h"
> #include "seq.h"
> #include "socket-util.h"
> +#include "timeval.h"
> #include "util.h"
>
> #ifdef __CHECKER__
> @@ -627,42 +628,54 @@ ovs_thread_stats_next_bucket(const struct
> ovsthread_stats *stats, size_t i)
> }
>
>
> -/* Returns the total number of cores available to this process, or 0 if the
> - * number cannot be determined. */
> -int
> -count_cpu_cores(void)
> +static int
> +count_cpu_cores__(void)
> {
> - static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> - static long int n_cores;
> + long int n_cores;
>
> - if (ovsthread_once_start(&once)) {
> #ifndef _WIN32
> - n_cores = sysconf(_SC_NPROCESSORS_ONLN);
> + n_cores = sysconf(_SC_NPROCESSORS_ONLN);
> +#else
> + SYSTEM_INFO sysinfo;
> + GetSystemInfo(&sysinfo);
> + n_cores = sysinfo.dwNumberOfProcessors;
> +#endif
> #ifdef __linux__
> - if (n_cores > 0) {
> - cpu_set_t *set = CPU_ALLOC(n_cores);
> + if (n_cores > 0) {
> + cpu_set_t *set = CPU_ALLOC(n_cores);
>
> - if (set) {
> - size_t size = CPU_ALLOC_SIZE(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);
> + if (!sched_getaffinity(0, size, set)) {
> + n_cores = CPU_COUNT_S(size, set);
> }
> + CPU_FREE(set);
> }
> -#endif
> -#else
> - SYSTEM_INFO sysinfo;
> - GetSystemInfo(&sysinfo);
> - n_cores = sysinfo.dwNumberOfProcessors;
> -#endif
> - ovsthread_once_done(&once);
> }
> -
> +#endif
> return n_cores > 0 ? n_cores : 0;
> }
>
> +/* It's unlikely that the available cpus change several times per second and
> + * even if it does, it's not needed (or desired) to react to such changes so
> + * quickly.*/
> +#define COUNT_CPU_UPDATE_TIME_MS 10000
> +/* Returns the current total number of cores available to this process, or 0
> + * if the number cannot be determined.
> + * It is assumed that this function is only called from the main thread.*/
Hi, Adrian. There was a thread-safety comment from me for this function
in v3 as it is actually called from multiple threads. Could you, please,
re-check?
Best regards, Ilya Maximets.
> +int count_cpu_cores(void) {
> + static long long int last_updated = 0;
> + long long int now = time_msec();
> + static int cpu_cores;
> +
> + if (now - last_updated >= COUNT_CPU_UPDATE_TIME_MS) {
> + last_updated = now;
> + cpu_cores = count_cpu_cores__();
> + }
> + return cpu_cores;
> +}
> +
> /* Returns the total number of cores on the system, or 0 if the
> * number cannot be determined. */
> int
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
