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

[David Marchand]

On Thu, Jul 2, 2020 at 3:09 PM Ilya Maximets <i.maximets at ovn.org> wrote:
>
> 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?

pmd threads will never run on any core >= RTE_MAX_LCORE ok, it works.

Though, it binds OVS core id and DPDK lcore id.
Should OVS really care about this?


-- 
David Marchand