DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
From: David Marchand <david.marchand@redhat.com>
To: Olivier Matz <olivier.matz@6wind.com>
Cc: dev <dev@dpdk.org>, Jerin Jacob <jerinjacobk@gmail.com>,
	 Bruce Richardson <bruce.richardson@intel.com>,
	Ray Kinsella <mdr@ashroe.eu>,
	 Thomas Monjalon <thomas@monjalon.net>,
	Andrew Rybchenko <arybchenko@solarflare.com>,
	 Kevin Traynor <ktraynor@redhat.com>,
	Ian Stokes <ian.stokes@intel.com>,
	 Ilya Maximets <i.maximets@ovn.org>,
	John McNamara <john.mcnamara@intel.com>,
	 Marko Kovacevic <marko.kovacevic@intel.com>,
	Anatoly Burakov <anatoly.burakov@intel.com>,
	 Neil Horman <nhorman@tuxdriver.com>
Subject: Re: [dpdk-dev] [PATCH v4 6/9] eal: register non-EAL threads as lcores
Date: Wed, 1 Jul 2020 09:13:36 +0200
Message-ID: <CAJFAV8xGT-4-Yy672d8GmOrNxHpJJHXhzZ=VRNt0jFpNmgJjmA@mail.gmail.com> (raw)
In-Reply-To: <20200630100717.GF5869@platinum>

On Tue, Jun 30, 2020 at 12:07 PM Olivier Matz <olivier.matz@6wind.com> wrote:
>
> On Fri, Jun 26, 2020 at 04:47:33PM +0200, David Marchand wrote:
> > DPDK allows calling some part of its API from a non-EAL thread but this
> > has some limitations.
> > OVS (and other applications) has its own thread management but still
> > want to avoid such limitations by hacking RTE_PER_LCORE(_lcore_id) and
> > faking EAL threads potentially unknown of some DPDK component.
> >
> > Introduce a new API to register non-EAL thread and associate them to a
> > free lcore with a new NON_EAL role.
> > This role denotes lcores that do not run DPDK mainloop and as such
> > prevents use of rte_eal_wait_lcore() and consorts.
> >
> > Multiprocess is not supported as the need for cohabitation with this new
> > feature is unclear at the moment.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> > ---
> > Changes since v2:
> > - refused multiprocess init once rte_thread_register got called, and
> >   vice versa,
> > - added warning on multiprocess in rte_thread_register doxygen,
> >
> > Changes since v1:
> > - moved cleanup on lcore role code in patch 5,
> > - added unit test,
> > - updated documentation,
> > - changed naming from "external thread" to "registered non-EAL thread"
> >
> > ---
> >  MAINTAINERS                                   |   1 +
> >  app/test/Makefile                             |   1 +
> >  app/test/autotest_data.py                     |   6 +
> >  app/test/meson.build                          |   2 +
> >  app/test/test_lcores.c                        | 139 ++++++++++++++++++
> >  doc/guides/howto/debug_troubleshoot.rst       |   5 +-
> >  .../prog_guide/env_abstraction_layer.rst      |  22 +--
> >  doc/guides/prog_guide/mempool_lib.rst         |   2 +-
> >  lib/librte_eal/common/eal_common_lcore.c      |  50 ++++++-
> >  lib/librte_eal/common/eal_common_mcfg.c       |  36 +++++
> >  lib/librte_eal/common/eal_common_thread.c     |  33 +++++
> >  lib/librte_eal/common/eal_memcfg.h            |  10 ++
> >  lib/librte_eal/common/eal_private.h           |  18 +++
> >  lib/librte_eal/freebsd/eal.c                  |   4 +
> >  lib/librte_eal/include/rte_lcore.h            |  25 +++-
> >  lib/librte_eal/linux/eal.c                    |   4 +
> >  lib/librte_eal/rte_eal_version.map            |   2 +
> >  lib/librte_mempool/rte_mempool.h              |  11 +-
> >  18 files changed, 349 insertions(+), 22 deletions(-)
> >  create mode 100644 app/test/test_lcores.c
> >
>
> [...]
>
> > diff --git a/app/test/test_lcores.c b/app/test/test_lcores.c
> > new file mode 100644
> > index 0000000000..864bcbade7
> > --- /dev/null
> > +++ b/app/test/test_lcores.c
> > @@ -0,0 +1,139 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright (c) 2020 Red Hat, Inc.
> > + */
> > +
> > +#include <pthread.h>
> > +#include <string.h>
> > +
> > +#include <rte_lcore.h>
> > +
> > +#include "test.h"
> > +
> > +struct thread_context {
> > +     enum { INIT, ERROR, DONE } state;
> > +     bool lcore_id_any;
> > +     pthread_t id;
> > +     unsigned int *registered_count;
> > +};
> > +static void *thread_loop(void *arg)
> > +{
>
> missing an empty line here
>
> > +     struct thread_context *t = arg;
> > +     unsigned int lcore_id;
> > +
> > +     lcore_id = rte_lcore_id();
> > +     if (lcore_id != LCORE_ID_ANY) {
> > +             printf("Incorrect lcore id for new thread %u\n", lcore_id);
> > +             t->state = ERROR;
> > +     }
> > +     rte_thread_register();
> > +     lcore_id = rte_lcore_id();
> > +     if ((t->lcore_id_any && lcore_id != LCORE_ID_ANY) ||
> > +                     (!t->lcore_id_any && lcore_id == LCORE_ID_ANY)) {
> > +             printf("Could not register new thread, got %u while %sexpecting %u\n",
> > +                     lcore_id, t->lcore_id_any ? "" : "not ", LCORE_ID_ANY);
> > +             t->state = ERROR;
> > +     }
>
> To check if rte_thread_register() succedeed, we need to look at
> lcore_id. I wonder if rte_thread_register() shouldn't return the lcore
> id on success, and -1 on error (rte_errno could be set to give some
> info on the error).

lcore_id are unsigned integers with the special value LCORE_ID_ANY
mapped to UINT32_MAX (should be UINT_MAX? anyway...).

rte_thread_register could return an error code as there are no ERROR
level logs about why a lcore allocation failed.
We could then distinguish a shortage of lcore (or init callback
refusal) from an invalid call before rte_eal_init() or when mp is in
use.

About returning the lcore_id as part of the return code, this would
map to -1 for LCORE_ID_ANY.
This is probably not a problem but still odd.


>
> The same could be done for rte_thread_init()

?
Not sure where this one could fail.


>
> [...]
>
> > diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c
> > index a7ae0691bf..1cbddc4b5b 100644
> > --- a/lib/librte_eal/common/eal_common_thread.c
> > +++ b/lib/librte_eal/common/eal_common_thread.c
> > @@ -236,3 +236,36 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
> >       pthread_join(*thread, NULL);
> >       return -ret;
> >  }
> > +
> > +void
> > +rte_thread_register(void)
> > +{
> > +     unsigned int lcore_id;
> > +     rte_cpuset_t cpuset;
> > +
> > +     /* EAL init flushes all lcores, we can't register before. */
> > +     assert(internal_config.init_complete == 1);
> > +     if (pthread_getaffinity_np(pthread_self(), sizeof(cpuset),
> > +                     &cpuset) != 0)
> > +             CPU_ZERO(&cpuset);
> > +     lcore_id = eal_lcore_non_eal_allocate();
> > +     if (lcore_id >= RTE_MAX_LCORE)
> > +             lcore_id = LCORE_ID_ANY;
> > +     rte_thread_init(lcore_id, &cpuset);
> > +     if (lcore_id != LCORE_ID_ANY)
> > +             RTE_LOG(DEBUG, EAL, "Registered non-EAL thread as lcore %u.\n",
> > +                     lcore_id);
> > +}
>
> So, in this case, the affinity of the pthread is kept and saved, in other
> words there is no link between the lcore id and the affinity. It means we
> are allowing an application to register lcores for dataplane with conflicting
> affinities.

This is not something new, applications using --lcores option already
live with this.
We have warnings in the documentation about non-EAL threads and about
the dangers of conflicting affinities.
Hopefully, the users of this API know what they are doing since they
chose not to use EAL threads.


>
> I wonder if it could be useful to have an API that automatically sets
> the affinity according to the lcore_id. Or a function that creates a
> pthread using the specified lcore id, and setting the correct affinity.
> I could simplify the work for applications that want to create/destroy
> dataplane threads dynamically.

Do you mean EAL threads dynamic creation/suppression?


>
> This could be done later however, just an idea.

For now, I don't see the need.


>
> [...]
> > diff --git a/lib/librte_eal/freebsd/eal.c b/lib/librte_eal/freebsd/eal.c
> > index 13e5de006f..32a3d999b8 100644
> > --- a/lib/librte_eal/freebsd/eal.c
> > +++ b/lib/librte_eal/freebsd/eal.c
> > @@ -424,6 +424,10 @@ rte_config_init(void)
> >               }
> >               if (rte_eal_config_reattach() < 0)
> >                       return -1;
> > +             if (!eal_mcfg_enable_multiprocess()) {
> > +                     RTE_LOG(ERR, EAL, "Primary process refused secondary attachment\n");
> > +                     return -1;
> > +             }
> >               eal_mcfg_update_internal();
> >               break;
> >       case RTE_PROC_AUTO:
> > diff --git a/lib/librte_eal/include/rte_lcore.h b/lib/librte_eal/include/rte_lcore.h
> > index 3968c40693..43747e88df 100644
> > --- a/lib/librte_eal/include/rte_lcore.h
> > +++ b/lib/librte_eal/include/rte_lcore.h
> > @@ -31,6 +31,7 @@ enum rte_lcore_role_t {
> >       ROLE_RTE,
> >       ROLE_OFF,
> >       ROLE_SERVICE,
> > +     ROLE_NON_EAL,
> >  };
>
> If find the name ROLE_NON_EAL a bit heavy (this was also my impression
> when reading the doc part).
>
> I understand that there are several types of threads:
>
> - eal (pthread created by eal): ROLE_RTE and ROLE_SERVICE
> - unregistered (pthread not created by eal, and not registered): ROLE_OFF
>   (note that ROLE_OFF also applies for unexistant threads)
> - dynamic: pthread not created by eal, but registered

Last two cases both are non-EAL threads as described in the doc so far.


>
> What about using ROLE_DYN ? I'm not sure about this name either, it's just
> to open the discussion :)
>

Well, at the moment, all those new lcores are mapped only to non-EAL threads.
A dynamic role feels like you want to take dynamic EAL threads into
account from the start.
I prefer to stick to non-EAL.


-- 
David Marchand


  reply index

Thread overview: 126+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-10 14:44 [dpdk-dev] [PATCH 0/7] Register external threads as lcore David Marchand
2020-06-10 14:45 ` [dpdk-dev] [PATCH 1/7] eal: relocate per thread symbols to common David Marchand
2020-06-10 14:45 ` [dpdk-dev] [PATCH 2/7] eal: fix multiple definition of per lcore thread id David Marchand
2020-06-15  6:46   ` Kinsella, Ray
2020-06-10 14:45 ` [dpdk-dev] [PATCH 3/7] eal: introduce thread init helper David Marchand
2020-06-10 14:45 ` [dpdk-dev] [PATCH 4/7] eal: introduce thread uninit helper David Marchand
2020-06-10 14:45 ` [dpdk-dev] [PATCH 5/7] eal: register non-EAL threads as lcore David Marchand
2020-06-15  6:43   ` Kinsella, Ray
2020-06-10 14:45 ` [dpdk-dev] [PATCH 6/7] eal: dump lcores David Marchand
2020-06-15  6:40   ` Kinsella, Ray
2020-06-10 14:45 ` [dpdk-dev] [PATCH 7/7] eal: add lcore hotplug notifications David Marchand
2020-06-15  6:34   ` Kinsella, Ray
2020-06-15  7:13     ` David Marchand
2020-06-10 15:09 ` [dpdk-dev] [PATCH 0/7] Register external threads as lcore Jerin Jacob
2020-06-10 15:13   ` Bruce Richardson
2020-06-10 15:18   ` David Marchand
2020-06-10 15:33     ` Jerin Jacob
2020-06-15  7:11       ` David Marchand
2020-06-19 16:22 ` [dpdk-dev] [PATCH v2 0/9] Register non-EAL " David Marchand
2020-06-19 16:22   ` [dpdk-dev] [PATCH v2 1/9] eal: relocate per thread symbols to common David Marchand
2020-06-19 16:22   ` [dpdk-dev] [PATCH v2 2/9] eal: fix multiple definition of per lcore thread id David Marchand
2020-06-19 16:22   ` [dpdk-dev] [PATCH v2 3/9] eal: introduce thread init helper David Marchand
2020-06-19 16:22   ` [dpdk-dev] [PATCH v2 4/9] eal: introduce thread uninit helper David Marchand
2020-06-19 16:22   ` [dpdk-dev] [PATCH v2 5/9] eal: move lcore role code David Marchand
2020-06-19 16:22   ` [dpdk-dev] [PATCH v2 6/9] eal: register non-EAL threads as lcores David Marchand
2020-06-19 16:22   ` [dpdk-dev] [PATCH v2 7/9] eal: add lcore init callbacks David Marchand
2020-06-19 16:22   ` [dpdk-dev] [PATCH v2 8/9] eal: add lcore iterators David Marchand
2020-06-20  2:21     ` Stephen Hemminger
2020-06-19 16:22   ` [dpdk-dev] [PATCH v2 9/9] mempool/bucket: handle non-EAL lcores David Marchand
2020-06-22 13:25 ` [dpdk-dev] [PATCH v3 0/9] Register non-EAL threads as lcore David Marchand
2020-06-22 13:25   ` [dpdk-dev] [PATCH v3 1/9] eal: relocate per thread symbols to common David Marchand
2020-06-22 13:25   ` [dpdk-dev] [PATCH v3 2/9] eal: fix multiple definition of per lcore thread id David Marchand
2020-06-22 13:25   ` [dpdk-dev] [PATCH v3 3/9] eal: introduce thread init helper David Marchand
2020-06-22 13:25   ` [dpdk-dev] [PATCH v3 4/9] eal: introduce thread uninit helper David Marchand
2020-06-22 13:25   ` [dpdk-dev] [PATCH v3 5/9] eal: move lcore role code David Marchand
2020-06-22 13:25   ` [dpdk-dev] [PATCH v3 6/9] eal: register non-EAL threads as lcores David Marchand
2020-06-22 15:49     ` Ananyev, Konstantin
2020-06-22 16:37       ` Ananyev, Konstantin
2020-06-23  7:49       ` David Marchand
2020-06-23  9:14         ` Bruce Richardson
2020-06-23 12:49           ` David Marchand
2020-06-23 13:15         ` Ananyev, Konstantin
2020-06-24  9:23           ` David Marchand
2020-06-24  9:56             ` Bruce Richardson
2020-06-24 10:08               ` Thomas Monjalon
2020-06-24 10:45                 ` Ananyev, Konstantin
2020-06-24 10:39             ` Ananyev, Konstantin
2020-06-24 10:48               ` David Marchand
2020-06-24 11:59                 ` Ananyev, Konstantin
2020-06-26 14:43                   ` David Marchand
2020-06-30 10:35                     ` Thomas Monjalon
2020-06-30 12:07                       ` Ananyev, Konstantin
2020-06-30 12:44                         ` Olivier Matz
2020-06-30 14:37                           ` Thomas Monjalon
2020-06-30 19:02                           ` Ananyev, Konstantin
2020-06-30 14:35                         ` Thomas Monjalon
2020-06-30 18:57                           ` Ananyev, Konstantin
2020-07-01  7:48                             ` David Marchand
2020-07-01 11:58                               ` Ananyev, Konstantin
2020-07-02 13:06                                 ` David Marchand
2020-07-03 15:15                                   ` Thomas Monjalon
2020-07-03 16:40                                   ` Ananyev, Konstantin
2020-07-04 15:00                                     ` David Marchand
2020-07-04 21:24                                       ` Ananyev, Konstantin
2020-06-23 17:02     ` Andrew Rybchenko
2020-06-22 13:25   ` [dpdk-dev] [PATCH v3 7/9] eal: add lcore init callbacks David Marchand
2020-06-22 13:25   ` [dpdk-dev] [PATCH v3 8/9] eal: add lcore iterators David Marchand
2020-06-22 13:25   ` [dpdk-dev] [PATCH v3 9/9] mempool/bucket: handle non-EAL lcores David Marchand
2020-06-23 17:28     ` Andrew Rybchenko
2020-06-26 14:13       ` David Marchand
2020-06-26 14:34         ` Andrew Rybchenko
2020-06-26 14:47 ` [dpdk-dev] [PATCH v4 0/9] Register non-EAL threads as lcore David Marchand
2020-06-26 14:47   ` [dpdk-dev] [PATCH v4 1/9] eal: relocate per thread symbols to common David Marchand
2020-06-30  9:33     ` Olivier Matz
2020-06-26 14:47   ` [dpdk-dev] [PATCH v4 2/9] eal: fix multiple definition of per lcore thread id David Marchand
2020-06-30  9:34     ` Olivier Matz
2020-06-26 14:47   ` [dpdk-dev] [PATCH v4 3/9] eal: introduce thread init helper David Marchand
2020-06-30  9:37     ` Olivier Matz
2020-06-30 12:04       ` David Marchand
2020-06-26 14:47   ` [dpdk-dev] [PATCH v4 4/9] eal: introduce thread uninit helper David Marchand
2020-06-26 15:00     ` Jerin Jacob
2020-06-29  9:07       ` David Marchand
2020-06-29  8:59     ` [dpdk-dev] [EXT] " Sunil Kumar Kori
2020-06-29  9:25       ` David Marchand
2020-06-30  9:42     ` [dpdk-dev] " Olivier Matz
2020-07-01  8:00       ` David Marchand
2020-06-26 14:47   ` [dpdk-dev] [PATCH v4 5/9] eal: move lcore role code David Marchand
2020-06-30  9:45     ` Olivier Matz
2020-06-26 14:47   ` [dpdk-dev] [PATCH v4 6/9] eal: register non-EAL threads as lcores David Marchand
2020-06-29 14:27     ` Ananyev, Konstantin
2020-06-30 10:07     ` Olivier Matz
2020-07-01  7:13       ` David Marchand [this message]
2020-07-01  9:11         ` Olivier Matz
2020-06-26 14:47   ` [dpdk-dev] [PATCH v4 7/9] eal: add lcore init callbacks David Marchand
2020-06-29 12:46     ` Ananyev, Konstantin
2020-06-30 10:09     ` Olivier Matz
2020-06-30 10:15     ` Olivier Matz
2020-06-26 14:47   ` [dpdk-dev] [PATCH v4 8/9] eal: add lcore iterators David Marchand
2020-06-30 10:11     ` Olivier Matz
2020-06-26 14:47   ` [dpdk-dev] [PATCH v4 9/9] mempool/bucket: handle non-EAL lcores David Marchand
2020-06-26 14:52     ` Andrew Rybchenko
2020-07-06 14:15 ` [dpdk-dev] [PATCH v5 00/10] Register non-EAL threads as lcore David Marchand
2020-07-06 14:15   ` [dpdk-dev] [PATCH v5 01/10] eal: relocate per thread symbols to common David Marchand
2020-07-06 14:15   ` [dpdk-dev] [PATCH v5 02/10] eal: fix multiple definition of per lcore thread id David Marchand
2020-07-06 14:15   ` [dpdk-dev] [PATCH v5 03/10] eal: introduce thread init helper David Marchand
2020-07-06 14:16   ` [dpdk-dev] [PATCH v5 04/10] eal: introduce thread uninit helper David Marchand
2020-07-06 14:16   ` [dpdk-dev] [PATCH v5 05/10] eal: move lcore role code David Marchand
2020-07-06 14:16   ` [dpdk-dev] [PATCH v5 06/10] eal: register non-EAL threads as lcores David Marchand
2020-07-06 14:16   ` [dpdk-dev] [PATCH v5 07/10] eal: add lcore init callbacks David Marchand
2020-07-06 14:16   ` [dpdk-dev] [PATCH v5 08/10] eal: add lcore iterators David Marchand
2020-07-06 14:16   ` [dpdk-dev] [PATCH v5 09/10] mempool/bucket: handle non-EAL lcores David Marchand
2020-07-06 14:16   ` [dpdk-dev] [PATCH v5 10/10] eal: add multiprocess disable API David Marchand
2020-07-06 20:52 ` [dpdk-dev] [PATCH v6 00/10] Register non-EAL threads as lcore David Marchand
2020-07-06 20:52   ` [dpdk-dev] [PATCH v6 01/10] eal: relocate per thread symbols to common David Marchand
2020-07-06 20:52   ` [dpdk-dev] [PATCH v6 02/10] eal: fix multiple definition of per lcore thread id David Marchand
2020-07-06 20:52   ` [dpdk-dev] [PATCH v6 03/10] eal: introduce thread init helper David Marchand
2020-07-06 20:52   ` [dpdk-dev] [PATCH v6 04/10] eal: introduce thread uninit helper David Marchand
2020-07-06 20:52   ` [dpdk-dev] [PATCH v6 05/10] eal: move lcore role code David Marchand
2020-07-06 20:52   ` [dpdk-dev] [PATCH v6 06/10] eal: register non-EAL threads as lcores David Marchand
2020-07-06 20:52   ` [dpdk-dev] [PATCH v6 07/10] eal: add lcore init callbacks David Marchand
2020-07-06 20:52   ` [dpdk-dev] [PATCH v6 08/10] eal: add lcore iterators David Marchand
2020-07-06 20:52   ` [dpdk-dev] [PATCH v6 09/10] mempool/bucket: handle non-EAL lcores David Marchand
2020-07-06 20:52   ` [dpdk-dev] [PATCH v6 10/10] eal: add multiprocess disable API David Marchand
2020-07-06 23:22   ` [dpdk-dev] [PATCH v6 00/10] Register non-EAL threads as lcore Ananyev, Konstantin
2020-07-08 13:05     ` David Marchand
2020-07-08 13:06   ` David Marchand

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAJFAV8xGT-4-Yy672d8GmOrNxHpJJHXhzZ=VRNt0jFpNmgJjmA@mail.gmail.com' \
    --to=david.marchand@redhat.com \
    --cc=anatoly.burakov@intel.com \
    --cc=arybchenko@solarflare.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=i.maximets@ovn.org \
    --cc=ian.stokes@intel.com \
    --cc=jerinjacobk@gmail.com \
    --cc=john.mcnamara@intel.com \
    --cc=ktraynor@redhat.com \
    --cc=marko.kovacevic@intel.com \
    --cc=mdr@ashroe.eu \
    --cc=nhorman@tuxdriver.com \
    --cc=olivier.matz@6wind.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org
	public-inbox-index dpdk-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git