On 8/9/22 09:18, 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, or the largest core_id on the system, which
> ever one is greatest. This is to take care of systems with non-continous
> core cpus.
>
> In the above example we would create a
> corrected array in a round-robin(assuming prime bias) fashion as follows:
> [S0, S1, S2, S3, S4, S5, S6, S0, S1, S2, S3, S4]
>
> 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]>
> ---
> lib/dpif-netlink.c | 31 +++++++++++++++++++++++++------
> lib/ovs-numa.c | 28 ++++++++++++++++++++++++++++
> lib/ovs-numa.h | 1 +
> 3 files changed, 54 insertions(+), 6 deletions(-)
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 90be7c52d..76ff84050 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -801,13 +801,26 @@ dpif_netlink_set_handler_pids(struct dpif *dpif_, const
> uint32_t *upcall_pids,
> uint32_t n_upcall_pids)
> {
> struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
> + int largest_cpu_id = ovs_numa_get_largest_core_id();
> 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;
> +
> + if (largest_cpu_id == OVS_NUMA_UNSPEC) {
> + largest_cpu_id = -1;
> + }
> +
> + /* Some systems have non-continuous cpu cores ids. count_total_cores()
> + * would return an accurate number, however, this number cannot be used.
> + * e.g. If the largest core_id of a system is cpu9, but the system only
> + * has 4 cpus then the OVS kernel module would throw a "CPU mismatch"
> + * warning. With the MAX() in place in this example we send an array of
> + * size 10 and prevent the warning. This has no bearing on the number of
> + * threads created.
> + */
> + n_cores = MAX(count_total_cores(), largest_cpu_id + 1);
> VLOG_DBG("Dispatch mode(per-cpu): Number of CPUs is %d", n_cores);
Hi, Michael and Aaron. This version looks fine to me, except for the log
message here. Aaron, I think, you pointed this out in reply to v6 already,
we need to change the log message since it's not accurate. It's not really
the number of CPUs, at least not in case of non-continuous core numbering.
Suggesting the following change:
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index cc77b7ba8..9f9d10055 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -821,7 +821,8 @@ dpif_netlink_set_handler_pids(struct dpif *dpif_, const
uint32_t *upcall_pids,
* threads created.
*/
n_cores = MAX(count_total_cores(), largest_cpu_id + 1);
- VLOG_DBG("Dispatch mode(per-cpu): Number of CPUs is %d", n_cores);
+ VLOG_DBG("Dispatch mode(per-cpu): Setting up handler PIDs for %d cores",
+ n_cores);
dpif_netlink_dp_init(&request);
request.cmd = OVS_DP_CMD_SET;
---
If that looks good to you, I can fold that in on commit. I'm testing the
series right now.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
