diff mbox series

[ovs-dev,v5] ovs-thread: Detect changes in number of cpus

Message ID 20221219182906.183791-1-amorenoz@redhat.com
State Accepted
Commit 0d23948a598ac609e9865174e0874e782a48d6a8
Headers show
Series [ovs-dev,v5] ovs-thread: Detect changes in number of cpus | expand

Checks

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 fail test: fail

Commit Message

Adrian Moreno Dec. 19, 2022, 6:29 p.m. UTC
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 <amorenoz@redhat.com>
---
 NEWS             |  2 ++
 lib/ovs-thread.c | 66 ++++++++++++++++++++++++++++++------------------
 2 files changed, 44 insertions(+), 24 deletions(-)

Comments

Ilya Maximets Dec. 20, 2022, 3:28 p.m. UTC | #1
On 12/19/22 19:29, 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 <amorenoz@redhat.com>
> ---


Applied.  Thanks!

Best regards, Ilya Maximets.
diff mbox series

Patch

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..168049b4c 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,59 @@  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
+
+static struct ovs_mutex cpu_cores_mutex = OVS_MUTEX_INITIALIZER;
+
+/* Returns the current total number of cores available to this process, or 0
+ * if the number cannot be determined. */
+int count_cpu_cores(void)
+{
+    static long long int last_updated = 0;
+    long long int now = time_msec();
+    static int cpu_cores;
+
+    ovs_mutex_lock(&cpu_cores_mutex);
+    if (now - last_updated >= COUNT_CPU_UPDATE_TIME_MS) {
+        last_updated = now;
+        cpu_cores = count_cpu_cores__();
+    }
+    ovs_mutex_unlock(&cpu_cores_mutex);
+    return cpu_cores;
+}
+
 /* Returns the total number of cores on the system, or 0 if the
  * number cannot be determined. */
 int