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

[Liang, Cunming]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz at 6wind.com]
> Sent: Tuesday, February 10, 2015 1:26 AM
> To: Liang, Cunming; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 05/17] eal: new TLS definition and API
> declaration
> 
> 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.
[LCM] Got you.
> 
> 
> 
> >>> @@ -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.
[LCM] Will reorder them in next version.