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?
