On 11/4/22 23:02, 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.
>
> This patch makes the number of cpus be calculated every time 5 seconds
> which seems a reasonable middle point.
> It generates an impact in the main loop duration of <1% and a worst-case
> scenario impact in throughput of < 5% [1].
Thanks, Adrian! See some comments below.
>
> As a result of these changes (assuming the patch is backported):
> - >=2.16: a change in the cpu affinity reflects on the number of threads
> in (at most) 5 seconds.
> - < 2.16: a change in the cpu affinity will be reflected on
> the number of threads the next time there is a call to
> bridge_reconfigure() (e.g: on the next DB change), and 5 seconds
> have passed.
>
> The difference in behavior is because on older versions the thread
> number calculation was done on bridge reconfiguration while newer
> versions moved this logic down to the dpif layer and is run on
> dpif->run() stage.
> Considering it has not been a huge problem up to today and that the
> cpu change would be reflected sooner or later (e.g the user could
> force a recalculation with a simple ovs-vsctl command), I think it
> might be OK to leave like that.
I don't think we should actually backport that change as it changing
the behavior of the ovs-vswitchd in a way that may not be expected
the the user. It's more like a new feature. So, above paragraphs can
be dropped from the commit message.
And since it's a new behavior change, it should have a record in the
NEWS file. Something like this, maybe:
- ovs-vswitchd now detects changes in CPU affinity and adjusts the number
of handler and revalidator threads if necessary.
>
> [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.
>
> Fixes: be15ec48d766 ("lib: Use a more accurate value for CPU count
> (sched_getaffinity).")
And this tag is not really needed.
> Cc: [email protected]
> Signed-off-by: Adrian Moreno <[email protected]>
> ---
> lib/ovs-thread.c | 61 +++++++++++++++++++++++++++++-------------------
> 1 file changed, 37 insertions(+), 24 deletions(-)
>
> diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
> index 78ed3e970..d1deb9c52 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 5000
> +/* 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.*/
There is a missing space at the end. But, more importantly, this
assumption is not correct. The function is called from the system
stats thread, i.e. system_stats_thread_func(). So, updates inside
should be guarded by some form of a mutex.
> +int count_cpu_cores(void) {
Braces should be on a new line.
> + 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
Bets regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
