[ovs-dev] [PATCH] dpdk: Refuse running on cpu >= RTE_MAX_LCORE.

[Ilya Maximets]

On 6/26/20 2:27 PM, David Marchand wrote:
> DPDK lcores range is [0, RTE_MAX_LCORE[.
> Trying to use a lcore out of this range expose OVS to crashes in DPDK mempool
> local cache array.
> Prefer a "clean" abort so that users know that something is wrong with
> their configuration.
> 
> Signed-off-by: David Marchand <david.marchand at redhat.com>
> ---
>  lib/dpdk.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/lib/dpdk.c b/lib/dpdk.c
> index 98d0e2643e..55ce9a9221 100644
> --- a/lib/dpdk.c
> +++ b/lib/dpdk.c
> @@ -617,7 +617,14 @@ dpdk_set_lcore_id(unsigned cpu)
>  {
>      /* NON_PMD_CORE_ID is reserved for use by non pmd threads. */
>      ovs_assert(cpu != NON_PMD_CORE_ID);
> +    if (cpu >= RTE_MAX_LCORE) {
> +        cpu = LCORE_ID_ANY;
> +    }
>      RTE_PER_LCORE(_lcore_id) = cpu;
> +    if (rte_lcore_id() == LCORE_ID_ANY) {
> +        ovs_abort(0, "PMD thread init failed, trying to use more cores than "
> +                  "DPDK supports (RTE_MAX_LCORE %u).", RTE_MAX_LCORE);

I don't think that random OVS abort after pmd-cpu-mask change or port addition
is a good thing to have.  Maybe it's better to limit CPU core discovery?
This way OVS will never try to create thread on unwanted cores.

Since ovs-numa module is mostly used by userspace datapath this will not
affect non-DPDK setups.

Something like this (not tested):

diff --git a/lib/ovs-numa.c b/lib/ovs-numa.c
index 6d0a68522..7c8a3b036 100644
--- a/lib/ovs-numa.c
+++ b/lib/ovs-numa.c
@@ -27,6 +27,7 @@
 #include <unistd.h>
 #endif /* __linux__ */
 
+#include "dpdk.h"
 #include "hash.h"
 #include "openvswitch/hmap.h"
 #include "openvswitch/list.h"
@@ -58,6 +59,13 @@ VLOG_DEFINE_THIS_MODULE(ovs_numa);
 
 #define MAX_NUMA_NODES 128
 
+#ifdef DPDK_NETDEV
+/* DPDK will not work correctly for cores with core_id >= RTE_MAX_LCORE. */
+#define MAX_CORE_ID    (RTE_MAX_LCORE - 1)
+#else
+#define MAX_CORE_ID    INT_MAX
+#endif
+
 /* numa node. */
 struct numa_node {
     struct hmap_node hmap_node;     /* In the 'all_numa_nodes'. */
@@ -112,6 +120,12 @@ insert_new_numa_node(int numa_id)
 static struct cpu_core *
 insert_new_cpu_core(struct numa_node *n, unsigned core_id)
 {
+    if (core_id > MAX_CORE_ID) {
+        VLOG_DBG("Ignoring CPU core with id %u ( %u > %d).",
+                 core_id, core_id, MAX_CORE_ID);
+        return NULL;
+    }
+
     struct cpu_core *c = xzalloc(sizeof *c);
 
     hmap_insert(&all_cpu_cores, &c->hmap_node, hash_int(core_id, 0));
@@ -278,6 +292,11 @@ ovs_numa_init(void)
     if (ovsthread_once_start(&once)) {
         const struct numa_node *n;
 
+        if (MAX_CORE_ID != INT_MAX) {
+            VLOG_INFO("Maximum allowed CPU core id is %d. "
+                      "Other cores will not be available.", MAX_CORE_ID);
+        }
+
         if (dummy_numa) {
             discover_numa_and_core_dummy();
         } else {
---

What do you think?

Best regards, Ilya Maximets.