[dpdk-dev] [PATCH v4 05/17] eal: new TLS definition and API declaration

[Olivier MATZ]

Hi,

On 02/09/2015 01:45 PM, Liang, Cunming wrote:
>>> +/**
>>> + * Dump the current pthread cpuset.
>>> + * This function is private to EAL.
>>> + *
>>> + * @param str
>>> + *   The string buffer the cpuset will dump to.
>>> + * @param size
>>> + *   The string buffer size.
>>> + */
>>> +#define CPU_STR_LEN            256
>>> +void
>>> +eal_thread_dump_affinity(char str[], unsigned size);
>>
>> Although it's equivalent for function arguments, I think "char *str" is
>> usually preferred over "char str[]". See for instance in snprintf() or
>> fgets().
> [LCM] Accept.
>>
>> What is the purpose of CPU_STR_LEN?
> [LCM] For default quick reference for str[] definition used in dump_affinity()

So the API comment of the function is not placed at the right
place.

A comment "Default buffer size to use with eal_thread_dump_affinity()"
should be added above CPU_STR_LEN. Also, it could be renamed in
RTE_CPU_STR_LEN or RTE_CPU_AFFINITY_STR_LEN.



>>> @@ -80,7 +81,9 @@ struct lcore_config {
>>>   */
>>>  extern struct lcore_config lcore_config[RTE_MAX_LCORE];
>>>
>>> -RTE_DECLARE_PER_LCORE(unsigned, _lcore_id); /**< Per core "core id". */
>>> +RTE_DECLARE_PER_LCORE(unsigned, _lcore_id);  /**< Per thread "lcore id".
>> */
>>> +RTE_DECLARE_PER_LCORE(unsigned, _socket_id); /**< Per thread "socket id".
>> */
>>> +RTE_DECLARE_PER_LCORE(rte_cpuset_t, _cpuset); /**< Per thread "cpuset".
>> */
>>>
>>>  /**
>>>   * Return the ID of the execution unit we are running on.
>>> @@ -146,7 +149,7 @@ rte_lcore_index(int lcore_id)
>>>  static inline unsigned
>>>  rte_socket_id(void)
>>>  {
>>> -	return lcore_config[rte_lcore_id()].socket_id;
>>> +	return RTE_PER_LCORE(_socket_id);
>>>  }
>>
>> I don't see where the _socket_id variable is assigned. I think there
>> is probably an issue with the splitting of the patches.
> [LCM] The value initializes as SOCKET_ID_ANY when RTE_DEFINE_PER_LCORE().
> And updated in eal_thread_set_affinity() for EAL thread and rte_thread_set_affinity() for non-EAL thread.

This is done in a later patches:

"eal: set _lcore_id and _socket_id to (-1) by default"
"eal: apply affinity of EAL thread by assigned cpuset"

That's why I said there is probably an issue with the ordering
of the patches as these values are used here but initialized
later in the series.