RE: [PATCH 05/11] eal: force prefix for internal threads

["Morten Brørup" <mb@smartsharesystems.com>]

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Thursday, 7 September 2023 10.28
> 
> On Wed, Sep 6, 2023 at 6:23 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > In order to make sure all threads created in DPDK drivers and libraries
> > have the same prefix in their name, some wrapper functions are added
> > for internal use when creating a control thread or setting a thread name:
> >         - rte_thread_create_internal_control
> >         - rte_thread_set_prefixed_name
> >
> > The equivalent public functions are then forbidden for internal use:
> >         - rte_thread_create_control
> >         - rte_thread_set_name
> >
> > Note: the libraries and drivers conversion is done in next patches,
> > while doing other thread-related changes.
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> >  devtools/checkpatches.sh           |  8 +++++
> >  lib/eal/common/eal_common_thread.c | 30 ++++++++++++++++
> >  lib/eal/include/rte_thread.h       | 57 +++++++++++++++++++++++++++++-
> >  lib/eal/version.map                |  2 ++
> >  4 files changed, 96 insertions(+), 1 deletion(-)
> >
> > diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
> > index 131ffbcebe..18ad6fbb7f 100755
> > --- a/devtools/checkpatches.sh
> > +++ b/devtools/checkpatches.sh
> > @@ -159,6 +159,14 @@ check_forbidden_additions() { # <patch>
> >                 -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> >                 "$1" || res=1
> >
> > +       # forbid non-internal thread in drivers and libs
> > +       awk -v FOLDERS='lib drivers' \
> > +               -v EXPRESSIONS="rte_thread_(set_name|create_control)\\\(" \
> > +               -v RET_ON_FAIL=1 \
> > +               -v MESSAGE='Prefer
> rte_thread_(set_prefixed_name|create_internal_control) in lib & drivers' \
> > +               -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> > +               "$1" || res=1
> > +
> >         # forbid inclusion of driver specific headers in apps and examples
> >         awk -v FOLDERS='app examples' \
> >                 -v EXPRESSIONS='include.*_driver\\.h include.*_pmd\\.h' \
> > diff --git a/lib/eal/common/eal_common_thread.c
> b/lib/eal/common/eal_common_thread.c
> > index 07ac721da1..31c37e3102 100644
> > --- a/lib/eal/common/eal_common_thread.c
> > +++ b/lib/eal/common/eal_common_thread.c
> > @@ -392,6 +392,36 @@ rte_thread_create_control(rte_thread_t *thread, const
> char *name,
> >         return ret;
> >  }
> >
> > +static void
> > +add_internal_prefix(char *prefixed_name, const char *name, size_t size)
> > +{
> > +       const char *prefix = "dpdk-";
> 
> Can you add some BUILD_BUG_ON to check RTE_THREAD_INTERNAL_NAME_SIZE
> is still relevant to the prefix?
> 
> 
> > +       size_t prefixlen;
> > +
> > +       prefixlen = strlen(prefix);
> > +       strlcpy(prefixed_name, prefix, size);
> > +       strlcpy(prefixed_name + prefixlen, name, size - prefixlen);
> > +}
> > +
> > +int
> > +rte_thread_create_internal_control(rte_thread_t *id, const char *name,
> > +               rte_thread_func func, void *arg)
> > +{
> > +       char prefixed_name[RTE_THREAD_NAME_SIZE];
> > +
> > +       add_internal_prefix(prefixed_name, name, sizeof(prefixed_name));
> > +       return rte_thread_create_control(id, prefixed_name, func, arg);
> > +}
> > +
> > +void
> > +rte_thread_set_prefixed_name(rte_thread_t id, const char *name)
> > +{
> > +       char prefixed_name[RTE_THREAD_NAME_SIZE];
> > +
> > +       add_internal_prefix(prefixed_name, name, sizeof(prefixed_name));
> > +       rte_thread_set_name(id, prefixed_name);
> > +}
> > +
> >  int
> >  rte_thread_register(void)
> >  {
> > diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
> > index dd1f62523f..4b1135df4f 100644
> > --- a/lib/eal/include/rte_thread.h
> > +++ b/lib/eal/include/rte_thread.h
> > @@ -28,6 +28,9 @@ extern "C" {
> >  /* Old definition, aliased for compatibility. */
> >  #define RTE_MAX_THREAD_NAME_LEN RTE_THREAD_NAME_SIZE
> >
> > +/** Maximum internal thread name length (including '\0'). */
> > +#define RTE_THREAD_INTERNAL_NAME_SIZE 11
> > +
> >  /**
> >   * Thread id descriptor.
> >   */
> > @@ -112,7 +115,7 @@ int rte_thread_create(rte_thread_t *thread_id,
> >   * @param thread_func
> >   *   Function to be executed by the new thread.
> >   * @param arg
> > - *   Argument passed to start_routine.
> > + *   Argument passed to thread_func.
> >   * @return
> >   *   On success, returns 0; on error, it returns a negative value
> >   *   corresponding to the error number.
> > @@ -121,6 +124,36 @@ int
> >  rte_thread_create_control(rte_thread_t *thread, const char *name,
> >                 rte_thread_func thread_func, void *arg);
> >
> > +/**
> > + * Create an internal control thread.
> > + *
> > + * Creates a control thread with the given name prefixed with "dpdk-".
> 
> I don't like having a hardcoded comment here.
> Plus try to grep dpdk- and you will see it is likely we will miss some
> if we change the prefix.

You could do something like the memzone name prefixes, which for the ring looks like this [1]:

#define RTE_RING_MZ_PREFIX "RG_"
/** The maximum length of a ring name. */
#define RTE_RING_NAMESIZE (RTE_MEMZONE_NAMESIZE - \
			   sizeof(RTE_RING_MZ_PREFIX) + 1)

So, for the thread names:

#define RTE_THREAD_NAME_PREFIX "dpdk-"
#define RTE_THREAD_INTERNAL_NAME_SIZE (RTE_THREAD_NAME_SIZE - \
		sizeof(RTE_THREAD_NAME_PREFIX) + 1)

> 
> 
> > + * If setting the name of the thread fails, the error is ignored and
> logged.
> > + *
> > + * The affinity of the new thread is based on the CPU affinity retrieved
> > + * at the time rte_eal_init() was called, the EAL threads are then
> excluded.
> > + *
> > + * @param id
> > + *   Filled with the thread ID of the new created thread.
> > + * @param name
> > + *   The name of the control thread
> > + *   (maximum 10 characters excluding terminating '\0').
> 
> This 10 value in the comment is easy to miss if some change with the
> prefix is done.
> Mentionning RTE_THREAD_INTERNAL_NAME_SIZE is enough.

I disagree with David's comment to this.

The function documentation is easier to read if the actual number is also mentioned.

For the best of both worlds, you can add something like this nearby:

_Static_assert(sizeof(RTE_THREAD_NAME_PREFIX) == sizeof("dpdk-"),
		"Length of RTE_THREAD_NAME_PREFIX has changed; "
		"the documentation needs updating.");

> 
> 
> > + *   See RTE_THREAD_INTERNAL_NAME_SIZE.
> > + *   The name of the driver or library should be first,
> > + *   then followed by a hyphen and more details.
> > + *   It will be prefixed with "dpdk-" by this function.
> > + * @param func
> > + *   Function to be executed by the new thread.
> > + * @param arg
> > + *   Argument passed to func.
> > + * @return
> > + *   On success, returns 0; a negative value otherwise.
> > + */
> > +__rte_internal
> > +int
> > +rte_thread_create_internal_control(rte_thread_t *id, const char *name,
> > +               rte_thread_func func, void *arg);
> > +
> >  /**
> >   * Waits for the thread identified by 'thread_id' to terminate
> >   *
> > @@ -159,6 +192,7 @@ rte_thread_t rte_thread_self(void);
> >
> >  /**
> >   * Set the name of the thread.
> > + *
> >   * This API is a noop if the underlying platform does not
> >   * support setting the thread name or the platform-specific
> >   * API used to set the thread name fails.
> > @@ -173,6 +207,27 @@ rte_thread_t rte_thread_self(void);
> >  void
> >  rte_thread_set_name(rte_thread_t thread_id, const char *thread_name);
> >
> > +/**
> > + * Set the name of an internal thread with adding a "dpdk-" prefix.
> > + *
> > + * This API is a noop if the underlying platform does not support
> > + * setting the thread name, or if it fails.
> > + *
> > + * @param id
> > + *   The ID of the thread to set name.
> > + *
> > + * @param name
> > + *   The name to set after being prefixed
> > + *   (maximum 10 characters excluding terminating '\0').
> > + *   See RTE_THREAD_INTERNAL_NAME_SIZE.
> > + *   The name of the driver or library should be first,
> > + *   then followed by a hyphen and more details.
> > + *   It will be prefixed with "dpdk-" by this function.
> > + */
> > +__rte_internal
> > +void
> > +rte_thread_set_prefixed_name(rte_thread_t id, const char *name);
> > +
> >  /**
> >   * Check if 2 thread ids are equal.
> >   *
> > diff --git a/lib/eal/version.map b/lib/eal/version.map
> > index 33b853d7be..6d32c19286 100644
> > --- a/lib/eal/version.map
> > +++ b/lib/eal/version.map
> > @@ -462,4 +462,6 @@ INTERNAL {
> >         rte_mem_map;
> >         rte_mem_page_size;
> >         rte_mem_unmap;
> > +       rte_thread_create_internal_control;
> > +       rte_thread_set_prefixed_name;
> >  };
> > --
> > 2.42.0
> >
> 
> 
> --
> David Marchand