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

[Liang, Cunming]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz at 6wind.com]
> Sent: Monday, February 09, 2015 4:00 AM
> To: Liang, Cunming; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 06/17] eal: add eal_common_thread.c for
> common thread API
> 
> 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?
[LCM] Accept, we can ignore it and depend on pthread_setaffinity_np() to return failure.
> 
> > +
> > +	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?
[LCM] You're right, here do some re-order.
The point is to make everything ready before switching the default value to -1.
And we can have the whole function implement in one patch.
It just won't take effect, but won't bring additional risk.
> 
> > +		/* 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?
[LCM] Good point, I haven't notice it.
> 
> 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.
[LCM] Accept.
> 
> 
> 
> > +
> > +	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().
[LCM] The cpusetp is used as destination of memcpy and the function suppose an EAL API.
I don't think it's a good idea to remove the check, do you ?
> 
> > +
> > +	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.
[LCM] accept.
> 
> 
> Regards,
> Olivier