Re: [dpdk-dev] [PATCH v4 3/9] eal: introduce thread init helper

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

Hi David,

one minor comment below

On Fri, Jun 26, 2020 at 04:47:30PM +0200, David Marchand wrote:
> Introduce a helper responsible for initialising the per thread context.
> We can then have a unified context for EAL and non-EAL threads and
> remove copy/paste'd OS-specific helpers.
> 
> Per EAL thread CPU affinity setting is separated from the thread init.
> It is to accommodate with Windows EAL where CPU affinity is not set at
> the moment.
> Besides, having affinity set by the master lcore in FreeBSD and Linux
> will make it possible to detect errors rather than panic in the child
> thread. But the cleanup when such an event happens is left for later.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v1:
> - rebased on master, removed Windows workarounds wrt gettid and traces
>   support,
> 
> ---
>  lib/librte_eal/common/eal_common_thread.c | 51 +++++++++++++----------
>  lib/librte_eal/common/eal_thread.h        |  8 ++--
>  lib/librte_eal/freebsd/eal.c              | 14 ++++++-
>  lib/librte_eal/freebsd/eal_thread.c       | 32 +-------------
>  lib/librte_eal/linux/eal.c                | 15 ++++++-
>  lib/librte_eal/linux/eal_thread.c         | 32 +-------------
>  lib/librte_eal/windows/eal.c              |  3 +-
>  lib/librte_eal/windows/eal_thread.c       | 10 +----
>  8 files changed, 66 insertions(+), 99 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c
> index 280c64bb76..afb30236c5 100644
> --- a/lib/librte_eal/common/eal_common_thread.c
> +++ b/lib/librte_eal/common/eal_common_thread.c
> @@ -71,20 +71,10 @@ eal_cpuset_socket_id(rte_cpuset_t *cpusetp)
>  	return socket_id;
>  }
>  
> -int
> -rte_thread_set_affinity(rte_cpuset_t *cpusetp)
> +static void
> +thread_update_affinity(rte_cpuset_t *cpusetp)
>  {
> -	int s;
> -	unsigned lcore_id;
> -	pthread_t tid;
> -
> -	tid = pthread_self();
> -
> -	s = pthread_setaffinity_np(tid, sizeof(rte_cpuset_t), cpusetp);
> -	if (s != 0) {
> -		RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n");
> -		return -1;
> -	}
> +	unsigned int lcore_id = rte_lcore_id();
>  
>  	/* store socket_id in TLS for quick access */
>  	RTE_PER_LCORE(_socket_id) =
> @@ -94,14 +84,24 @@ rte_thread_set_affinity(rte_cpuset_t *cpusetp)
>  	memmove(&RTE_PER_LCORE(_cpuset), cpusetp,
>  		sizeof(rte_cpuset_t));
>  
> -	lcore_id = rte_lcore_id();
>  	if (lcore_id != (unsigned)LCORE_ID_ANY) {
>  		/* EAL thread will update lcore_config */
>  		lcore_config[lcore_id].socket_id = RTE_PER_LCORE(_socket_id);
>  		memmove(&lcore_config[lcore_id].cpuset, cpusetp,
>  			sizeof(rte_cpuset_t));
>  	}
> +}
>  
> +int
> +rte_thread_set_affinity(rte_cpuset_t *cpusetp)
> +{
> +	if (pthread_setaffinity_np(pthread_self(), sizeof(rte_cpuset_t),
> +			cpusetp) != 0) {
> +		RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n");
> +		return -1;
> +	}
> +
> +	thread_update_affinity(cpusetp);
>  	return 0;
>  }
>  
> @@ -147,6 +147,19 @@ eal_thread_dump_affinity(char *str, unsigned size)
>  	return ret;
>  }
>  
> +void
> +rte_thread_init(unsigned int lcore_id, rte_cpuset_t *cpuset)
> +{
> +	/* set the lcore ID in per-lcore memory area */
> +	RTE_PER_LCORE(_lcore_id) = lcore_id;
> +
> +	/* acquire system unique id  */
> +	rte_gettid();

If I understand properly, rte_gettid() is now also called for control
thread. I don't think this behavior change can break anything, but it may
be good to highlight it in the commit log.

also, there are 2 spaces before "*/"