Re: [dpdk-dev] [PATCH v4 6/9] eal: register non-EAL threads as lcores

[Olivier Matz <olivier.matz@6wind.com>]

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).

The same could be done for rte_thread_init()

[...]

> 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.

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.

This could be done later however, just an idea.

[...]
> 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

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