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

[David Marchand <david.marchand@redhat.com>]

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.


> + * 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.


> + *   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