[dpdk-dev] [PATCH v4 08/17] eal: apply affinity of EAL thread by assigned cpuset

Olivier MATZ olivier.matz at 6wind.com
Sun Feb 8 21:00:41 CET 2015


Hi,

On 02/02/2015 03:02 AM, Cunming Liang wrote:
> EAL threads use assigned cpuset to set core affinity during startup.
> It keeps 1:1 mapping, if no '--lcores' option is used.
> 
> [...]
>
>  lib/librte_eal/bsdapp/eal/eal.c          | 13 ++++---
>  lib/librte_eal/bsdapp/eal/eal_thread.c   | 63 +++++++++---------------------
>  lib/librte_eal/linuxapp/eal/eal.c        |  7 +++-
>  lib/librte_eal/linuxapp/eal/eal_thread.c | 67 +++++++++++---------------------
>  4 files changed, 54 insertions(+), 96 deletions(-)
> 
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
> index 69f3c03..98c5a83 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -432,6 +432,7 @@ rte_eal_init(int argc, char **argv)
>  	int i, fctret, ret;
>  	pthread_t thread_id;
>  	static rte_atomic32_t run_once = RTE_ATOMIC32_INIT(0);
> +	char cpuset[CPU_STR_LEN];
>  
>  	if (!rte_atomic32_test_and_set(&run_once))
>  		return -1;
> @@ -502,13 +503,17 @@ rte_eal_init(int argc, char **argv)
>  	if (rte_eal_pci_init() < 0)
>  		rte_panic("Cannot init PCI\n");
>  
> -	RTE_LOG(DEBUG, EAL, "Master core %u is ready (tid=%p)\n",
> -		rte_config.master_lcore, thread_id);
> -
>  	eal_check_mem_on_local_socket();
>  
>  	rte_eal_mcfg_complete();
>  
> +	eal_thread_init_master(rte_config.master_lcore);
> +
> +	eal_thread_dump_affinity(cpuset, CPU_STR_LEN);
> +
> +	RTE_LOG(DEBUG, EAL, "Master lcore %u is ready (tid=%p;cpuset=[%s])\n",
> +		rte_config.master_lcore, thread_id, cpuset);
> +
>  	if (rte_eal_dev_init() < 0)
>  		rte_panic("Cannot init pmd devices\n");
>  
> @@ -532,8 +537,6 @@ rte_eal_init(int argc, char **argv)
>  			rte_panic("Cannot create thread\n");
>  	}
>  
> -	eal_thread_init_master(rte_config.master_lcore);
> -
>  	/*
>  	 * Launch a dummy function on all slave lcores, so that master lcore
>  	 * knows they are all ready when this function returns.

I wonder if changing this may have an impact on third-party drivers
that already use a management thread. Before the patch, the init()
function of the external library was called with default affinities,
and now it's called with the affinity from master lcore.

I think it should at least be noticed in the commit log.

Why are you doing this change? (I don't say it's a bad change, but
I don't understand why you are doing it here)


> diff --git a/lib/librte_eal/bsdapp/eal/eal_thread.c b/lib/librte_eal/bsdapp/eal/eal_thread.c
> index d0c077b..5b16302 100644
> --- a/lib/librte_eal/bsdapp/eal/eal_thread.c
> +++ b/lib/librte_eal/bsdapp/eal/eal_thread.c
> @@ -103,55 +103,27 @@ eal_thread_set_affinity(void)
>  {
>  	int s;
>  	pthread_t thread;
> -
> -/*
> - * According to the section VERSIONS of the CPU_ALLOC man page:
> - *
> - * The CPU_ZERO(), CPU_SET(), CPU_CLR(), and CPU_ISSET() macros were added
> - * in glibc 2.3.3.
> - *
> - * CPU_COUNT() first appeared in glibc 2.6.
> - *
> - * CPU_AND(),     CPU_OR(),     CPU_XOR(),    CPU_EQUAL(),    CPU_ALLOC(),
> - * CPU_ALLOC_SIZE(), CPU_FREE(), CPU_ZERO_S(),  CPU_SET_S(),  CPU_CLR_S(),
> - * CPU_ISSET_S(),  CPU_AND_S(), CPU_OR_S(), CPU_XOR_S(), and CPU_EQUAL_S()
> - * first appeared in glibc 2.7.
> - */
> -#if defined(CPU_ALLOC)
> -	size_t size;
> -	cpu_set_t *cpusetp;
> -
> -	cpusetp = CPU_ALLOC(RTE_MAX_LCORE);
> -	if (cpusetp == NULL) {
> -		RTE_LOG(ERR, EAL, "CPU_ALLOC failed\n");
> -		return -1;
> -	}
> -
> -	size = CPU_ALLOC_SIZE(RTE_MAX_LCORE);
> -	CPU_ZERO_S(size, cpusetp);
> -	CPU_SET_S(rte_lcore_id(), size, cpusetp);
> +	unsigned lcore_id = rte_lcore_id();
>  
>  	thread = pthread_self();
> -	s = pthread_setaffinity_np(thread, size, cpusetp);
> +	s = pthread_setaffinity_np(thread, sizeof(cpuset_t),
> +				   &lcore_config[lcore_id].cpuset);
>  	if (s != 0) {
>  		RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n");
> -		CPU_FREE(cpusetp);
>  		return -1;
>  	}
>  
> -	CPU_FREE(cpusetp);
> -#else /* CPU_ALLOC */
> -	cpuset_t cpuset;
> -	CPU_ZERO( &cpuset );
> -	CPU_SET( rte_lcore_id(), &cpuset );
> +	/* acquire system unique id  */
> +	rte_gettid();

As suggested in the previous patch, I think having rte_init_tid() would
be clearer here.

> +
> +	/* store socket_id in TLS for quick access */
> +	RTE_PER_LCORE(_socket_id) =
> +		eal_cpuset_socket_id(&lcore_config[lcore_id].cpuset);
> +
> +	CPU_COPY(&lcore_config[lcore_id].cpuset, &RTE_PER_LCORE(_cpuset));
> +
> +	lcore_config[lcore_id].socket_id = RTE_PER_LCORE(_socket_id);
>  
> -	thread = pthread_self();
> -	s = pthread_setaffinity_np(thread, sizeof( cpuset ), &cpuset);
> -	if (s != 0) {
> -		RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n");
> -		return -1;
> -	}
> -#endif

You are removing a lot of code that was using CPU_ALLOC().
Are we sure that the cpuset_t type is large enough to store all the
CPUs?

It looks the current value of CPU_SETSIZE is 1024 now, but I wonder
if this code was written when this value was lower. Could you check if
it can happen today (maybe with an old libc)? A problem can occur if
the size of cpuset_t is lower that the size of RTE_MAX_LCORE.


Regards,
Olivier


More information about the dev mailing list