On 5/27/22 02:37, Michael Santana wrote:
The handler and CPU mapping in upcalls are incorrect, and this is
specially noticeable systems with cpu isolation enabled.
Say we have a 12 core system where only every even number CPU is enabled
C0, C2, C4, C6, C8, C10
This means we will create an array of size 6 that will be sent to
kernel that is populated with sockets [S0, S1, S2, S3, S4, S5]
The problem is when the kernel does an upcall it checks the socket array
via the index of the CPU, effectively adding additional load on some
CPUs while leaving no work on other CPUs.
e.g.
C0 indexes to S0
C2 indexes to S2 (should be S1)
C4 indexes to S4 (should be S2)
Modulo of 6 (size of socket array) is applied, so we wrap back to S0
C6 indexes to S0 (should be S3)
C8 indexes to S2 (should be S4)
C10 indexes to S4 (should be S5)
Effectively sockets S0, S2, S4 get overloaded while sockets S1, S3, S5
get no work assigned to them
This leads to the kernel to throw the following message:
"openvswitch: cpu_id mismatch with handler threads"
Instead we will send the kernel a corrected array of sockets the size
of all CPUs in the system. In the above example we would create a
corrected array in a round-robin fashion as follows:
[S0, S1, S2, S3, S4, S5, S0, S1, S2, S3, S4, S5]
Fixes: b1e517bd2f81 ("dpif-netlink: Introduce per-cpu upcall dispatch.")
Co-authored-by: Aaron Conole <[email protected]>
signed-off-by: Aaron Conole <[email protected]>
Signed-off-by: Michael Santana <[email protected]>
Hi Ilya,
I sent this patch to get the conversation moving again. It implements
the round-robin schema you had proposed.
However, there is a problem with the round robin-schema. But dont worry,
I think I have come up with an schema that will make everyone happy
The problem with the round-robin schema (+magic prime number) is that it
is active-core agnostic. My understanding from conversations I have had
with flavio and aaron is that active cores have a high probability to
see traffic (by configuring the NIC to send traffic specifically to
those active cores) as this would be the "correctly" configured
methodology. We should ideally give priority to these active cores. So
we should avoid sharing the same handler thread among cores, specially
active cores, as you had also mentioned in this thread.
With the round-robin schema you can easily deliberately create a worse
case scenario where 4 active cores are serviced by a single handler
thread (s0).
Say you have a 16 core system, with only cores c0, c5, c10, c15 active.
Using the prime number schema we would have 5 handler threads. The
mapping would look like this
## ## ## ##
c0 c1 c2 c3 c4 c5 c6 c7 c8 c9 c10 c11 c12 c13 c14 c15
s0 s1 s2 s3 s4 s0 s1 s2 s3 s4 s0 s1 s2 s3 s4 s0
Instead we can combine the design of the original patch and the
round-robin design. Doing so we avoid sharing handler threads among
active cores.
First we need more threads than active cores, like in the round-robin
(+magic prime number) implementation. I would like to change the prime
number to instead something more simple. I would like the total number
of handler threads to be: (this obviously only applies when we know we
have inactive cores (active_cores < total_cores))
handlers_n = (active_cores)+ min(active_cores/4 +1, inactive_cores/4 +1)
The idea is to assigned each active core a unique handler (active_cores)
that will not be shared with any other core. The other handlers
min(active_cores/4 +1, inactive_cores/4 +1) will be distributed among
the inactive cores in a round-robin fashion. Actually, how we distribute
handlers for inactive cores is really up to you. We can optimize for
whatever you think would be best. The key take away is that active cores
will always get unique handlers that are not shared with any other core.
And because we are adding only a small number of additional handlers
(mathematically speaking we would always only be adding less than 1/8 of
the total cores) for the inactive cores we should not see any additional
overhead.
Redoing the above example we would have a total of 6 handlers, 4 are
dedicated for the 4 active cores, and 2 shared among the inactive cores
in a round-robin fashion
## ## ## ##
c0 c1 c2 c3 c4 c5 c6 c7 c8 c9 c10 c11 c12 c13 c14 c15
s0 s4 s5 s4 s5 s1 s4 s5 s4 s5 s2 s4 s5 s4 s5 s3
We could alternatively distribute the handlers for inactive cores to
## ## ## ##
c0 c1 c2 c3 c4 c5 c6 c7 c8 c9 c10 c11 c12 c13 c14 c15
s0 s4 s4 s4 s4 s1 s4 s4 s5 s5 s2 s5 s5 s5 s5 s3
Again, how we distribute the handlers for the inactive cores is up to
you. And of course we can play around with the 1/4 +1. If you want more
handlers we could do 1/3 +1, or if you want less, 1/5 +1. Whatever you
like, we can do :D
But of course if you dont like this implementation we can always fall
back to the round-robin implementation in this patch
---
lib/dpif-netlink.c | 18 ++++++++++++------
lib/ovs-thread.c | 16 ++++++++++++++++
lib/ovs-thread.h | 1 +
3 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 71e35ccdd..77a97beac 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -803,11 +803,11 @@ dpif_netlink_set_handler_pids(struct dpif *dpif_, const
uint32_t *upcall_pids,
struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
struct dpif_netlink_dp request, reply;
struct ofpbuf *bufp;
- int error;
- int n_cores;
- n_cores = count_cpu_cores();
- ovs_assert(n_cores == n_upcall_pids);
+ uint32_t *corrected;
+ int error, i, n_cores;
+
+ n_cores = count_total_cores();
VLOG_DBG("Dispatch mode(per-cpu): Number of CPUs is %d", n_cores);
dpif_netlink_dp_init(&request);
@@ -817,7 +817,12 @@ dpif_netlink_set_handler_pids(struct dpif *dpif_, const
uint32_t *upcall_pids,
request.user_features = dpif->user_features |
OVS_DP_F_DISPATCH_UPCALL_PER_CPU;
- request.upcall_pids = upcall_pids;
+ corrected = xcalloc(n_cores, sizeof(uint32_t));
+
+ for (i = 0; i < n_cores; i++) {
+ corrected[i] = upcall_pids[i % n_upcall_pids];
+ }
+ request.upcall_pids = corrected;
request.n_upcall_pids = n_cores;
error = dpif_netlink_dp_transact(&request, &reply, &bufp);
@@ -825,9 +830,10 @@ dpif_netlink_set_handler_pids(struct dpif *dpif_, const
uint32_t *upcall_pids,
dpif->user_features = reply.user_features;
ofpbuf_delete(bufp);
if (!dpif_netlink_upcall_per_cpu(dpif)) {
- return -EOPNOTSUPP;
+ error = -EOPNOTSUPP;
}
}
+ free(corrected);
return error;
}
diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
index 805cba622..2172b3d3f 100644
--- a/lib/ovs-thread.c
+++ b/lib/ovs-thread.c
@@ -663,6 +663,22 @@ count_cpu_cores(void)
return n_cores > 0 ? n_cores : 0;
}
+/* Returns the total number of cores on the system, or 0 if the
+ * number cannot be determined. */
+int
+count_total_cores(void) {
+ long int n_cores;
+
+#ifndef _WIN32
+ n_cores = sysconf(_SC_NPROCESSORS_CONF);
+#else
+ n_cores = 0;
+ errno = ENOTSUP;
+#endif
+
+ return n_cores > 0 ? n_cores : 0;
+}
+
/* Returns 'true' if current thread is PMD thread. */
bool
thread_is_pmd(void)
diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
index 3b444ccdc..aac5e19c9 100644
--- a/lib/ovs-thread.h
+++ b/lib/ovs-thread.h
@@ -522,6 +522,7 @@ bool may_fork(void);
/* Useful functions related to threading. */
int count_cpu_cores(void);
+int count_total_cores(void);
bool thread_is_pmd(void);
#endif /* ovs-thread.h */
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
