[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: Monday, February 09, 2015 4:00 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/02/2015 03:02 AM, Cunming Liang wrote:
> > 1. add two TLS *_socket_id* and *_cpuset*
> > 2. add two external API rte_thread_set/get_affinity
> > 3. add one internal API eal_thread_dump_affinity
> 
> To me, it's a bit strage to add an API withtout the associated code.
> Maybe you have a good reason to do that, but I think in this case it
> should be explained in the commit log.
[LCM] Accept.
> 
> >
> > Signed-off-by: Cunming Liang <cunming.liang at intel.com>
> > ---
> >  lib/librte_eal/bsdapp/eal/eal_thread.c    |  2 ++
> >  lib/librte_eal/common/eal_thread.h        | 14 ++++++++++++++
> >  lib/librte_eal/common/include/rte_lcore.h | 29
> +++++++++++++++++++++++++++--
> >  lib/librte_eal/linuxapp/eal/eal_thread.c  |  2 ++
> >  4 files changed, 45 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_eal/bsdapp/eal/eal_thread.c
> b/lib/librte_eal/bsdapp/eal/eal_thread.c
> > index ab05368..10220c7 100644
> > --- a/lib/librte_eal/bsdapp/eal/eal_thread.c
> > +++ b/lib/librte_eal/bsdapp/eal/eal_thread.c
> > @@ -56,6 +56,8 @@
> >  #include "eal_thread.h"
> >
> >  RTE_DEFINE_PER_LCORE(unsigned, _lcore_id);
> > +RTE_DEFINE_PER_LCORE(unsigned, _socket_id);
> > +RTE_DEFINE_PER_LCORE(rte_cpuset_t, _cpuset);
> >
> >  /*
> >   * Send a message to a slave lcore identified by slave_id to call a
> > diff --git a/lib/librte_eal/common/eal_thread.h
> b/lib/librte_eal/common/eal_thread.h
> > index a25ee86..28edf51 100644
> > --- a/lib/librte_eal/common/eal_thread.h
> > +++ b/lib/librte_eal/common/eal_thread.h
> > @@ -102,4 +102,18 @@ eal_cpuset_socket_id(rte_cpuset_t *cpusetp)
> >  	return socket_id;
> >  }
> >
> > +/**
> > + * 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()
> 
> What occurs if the size of the dump is greater than the size of the
> given buffer? Is the string truncated? Is there a \0 at the end?
[LCM] Yes, always have a '\0' in the end.
> This should be described in the API comments.
[LCM] Accept.
> Maybe adding a return
> value could help the user to determine if the string was truncated.
[LCM] Good idea, so the user can continue to print '...' for the truncated part.
> 
> > +
> > +
> >  #endif /* EAL_THREAD_H */
> > diff --git a/lib/librte_eal/common/include/rte_lcore.h
> b/lib/librte_eal/common/include/rte_lcore.h
> > index 4c7d6bb..facdbdc 100644
> > --- a/lib/librte_eal/common/include/rte_lcore.h
> > +++ b/lib/librte_eal/common/include/rte_lcore.h
> > @@ -43,6 +43,7 @@
> >  #include <rte_per_lcore.h>
> >  #include <rte_eal.h>
> >  #include <rte_launch.h>
> > +#include <rte_memory.h>
> >
> >  #ifdef __cplusplus
> >  extern "C" {
> > @@ -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.
> 
> Regards,
> Olivier