[dpdk-dev] [PATCH v4 04/17] eal: add support parsing socket_id from cpuset

[Olivier MATZ]

Hi,

On 02/09/2015 01:26 PM, Liang, Cunming wrote:
>>> @@ -50,4 +54,52 @@ __attribute__((noreturn)) void *eal_thread_loop(void
>> *arg);
>>>   */
>>>  void eal_thread_init_master(unsigned lcore_id);
>>>
>>> +/**
>>> + * Get the NUMA socket id from cpu id.
>>> + * This function is private to EAL.
>>> + *
>>> + * @param cpu_id
>>> + *   The logical process id.
>>> + * @return
>>> + *   socket_id or SOCKET_ID_ANY
>>> + */
>>> +unsigned eal_cpu_socket_id(unsigned cpu_id);
>>
>> Wouldn't it be better to rename the existing function cpu_socket_id()
>> in eal_cpu_socket_id() and export it in eal_thread.h?
>>
>> In case of bsd where cpu_socket_id() is implemented using a #define,
>> a new function should be created returning 0.
> [LCM] In eal_lcore.c, the cpu_socket_id()/cpu_core_id() defined as static and only used in rte_eal_cpu_init().
> I suppose the purpose of origin design is to make the sysfs parsing only visible in the file.
> No matter remove the 'static' prefix of cpu_core_id() or add a new wrap eal_cpu_socket_id(), it results in a new extern EAL API.
> So I prefer not change the visibility of the origin static function but have one as extern interface.

Yes, but I don't see what is the advantage of using a wrapper.
If there is no advantage, I think the one with the less code is
better.



>>> +static inline int
>>> +eal_cpuset_socket_id(rte_cpuset_t *cpusetp)
>>> +{
>>> +	unsigned cpu = 0;
>>> +	int socket_id = SOCKET_ID_ANY;
>>> +	int sid;
>>> +
>>> +	if (cpusetp == NULL)
>>> +		return SOCKET_ID_ANY;
>>> +
>>> +	do {
>>> +		if (!CPU_ISSET(cpu, cpusetp))
>>> +			continue;
>>> +
>>> +		if (socket_id == SOCKET_ID_ANY)
>>> +			socket_id = eal_cpu_socket_id(cpu);
>>> +
>>> +		sid = eal_cpu_socket_id(cpu);
>>> +		if (socket_id != sid) {
>>> +			socket_id = SOCKET_ID_ANY;
>>> +			break;
>>> +		}
>>> +
>>> +	} while (++cpu < RTE_MAX_LCORE);
>>> +
>>> +	return socket_id;
>>> +}
>>
>>
>> I don't think this function should be inlined.
>>
>> As this function is not used, it could be interesting for reviewers
>> to understand when
> [LCM] It's used in eal_thread_set_affinity() of eal_thread.c.

As it's not visible in the patch, could you add an explanation in
the commit log?