Re: [dpdk-dev] [PATCH v4 06/17] eal: add eal_common_thread.c for common thread API

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

Hi,

On 02/02/2015 03:02 AM, Cunming Liang wrote:
> The API works for both EAL thread and none EAL thread.
> When calling rte_thread_set_affinity, the *_socket_id* and
> *_cpuset* of calling thread will be updated if the thread
> successful set the cpu affinity.
> 
> [...]
> +int
> +rte_thread_set_affinity(rte_cpuset_t *cpusetp)
> +{
> +	int s;
> +	unsigned lcore_id;
> +	pthread_t tid;
> +
> +	if (!cpusetp)
> +		return -1;

Is it really needed to test that cpusetp is not NULL?

> +
> +	lcore_id = rte_lcore_id();
> +	if (lcore_id != (unsigned)LCORE_ID_ANY) {

This is strange to see something that cannot happen:
lcore_id == LCORE_ID_ANY is only possible after your patch is 12/17
is added. Maybe it can be reordered to avoid this inconsistency?

> +		/* EAL thread */
> +		tid = lcore_config[lcore_id].thread_id;
> +
> +		s = pthread_setaffinity_np(tid, sizeof(rte_cpuset_t), cpusetp);
> +		if (s != 0) {
> +			RTE_LOG(ERR, EAL, "pthread_setaffinity_np failed\n");
> +			return -1;
> +		}
> +
> +		/* store socket_id in TLS for quick access */
> +		RTE_PER_LCORE(_socket_id) =
> +			eal_cpuset_socket_id(cpusetp);
> +
> +		/* store cpuset in TLS for quick access */
> +		rte_memcpy(&RTE_PER_LCORE(_cpuset), cpusetp,
> +			   sizeof(rte_cpuset_t));
> +
> +		/* update lcore_config */
> +		lcore_config[lcore_id].socket_id = RTE_PER_LCORE(_socket_id);
> +		rte_memcpy(&lcore_config[lcore_id].cpuset, cpusetp,
> +			   sizeof(rte_cpuset_t));
> +	} else {
> +		/* none EAL thread */
> +		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;
> +		}
> +
> +		/* store cpuset in TLS for quick access */
> +		rte_memcpy(&RTE_PER_LCORE(_cpuset), cpusetp,
> +			   sizeof(rte_cpuset_t));
> +
> +		/* store socket_id in TLS for quick access */
> +		RTE_PER_LCORE(_socket_id) =
> +			eal_cpuset_socket_id(cpusetp);
> +	}

Why not always using pthread_self() to get the tid?

I think most of the code could be factorized here. The only difference
(which is hard to see as is as code is not exactly ordered in the same
manner) is that the config is updated in case it's an EAL thread.



> +
> +	return 0;
> +}
> +
> +int
> +rte_thread_get_affinity(rte_cpuset_t *cpusetp)
> +{
> +	if (!cpusetp)
> +		return -1;

Same here. This is the only reason why rte_thread_get_affinity() could
fail. Removing this test would allow to change the API to return void
instead. It will avoid a useless test below in
eal_thread_dump_affinity().

> +
> +	rte_memcpy(cpusetp, &RTE_PER_LCORE(_cpuset),
> +		   sizeof(rte_cpuset_t));
> +
> +	return 0;
> +}
> +
> +void
> +eal_thread_dump_affinity(char str[], unsigned size)
> +{
> +	rte_cpuset_t cpuset;
> +	unsigned cpu;
> +	int ret;
> +	unsigned int out = 0;
> +
> +	if (rte_thread_get_affinity(&cpuset) < 0) {
> +		str[0] = '\0';
> +		return;
> +	}

This one could be removed it the (== NULL) test is removed.

> +
> +	for (cpu = 0; cpu < RTE_MAX_LCORE; cpu++) {
> +		if (!CPU_ISSET(cpu, &cpuset))
> +			continue;
> +
> +		ret = snprintf(str + out,
> +			       size - out, "%u,", cpu);
> +		if (ret < 0 || (unsigned)ret >= size - out)
> +			break;

On the contrary, I think here returning an error to the user
would be useful so he can knows that the dump is not complete.


Regards,
Olivier