On Wed, Sep 6, 2023 at 6:23 PM Thomas Monjalon <tho...@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 <tho...@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
