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

  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(-)
  

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
>
  

> 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
  

On Thu, Sep 7, 2023 at 10:50 AM Morten Brørup <mb@smartsharesystems.com> wrote:
> > 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.");

And how will it catch the comment about 10 characters ?
  

On Thu, Sep 7, 2023 at 10:53 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Thu, Sep 7, 2023 at 10:50 AM Morten Brørup <mb@smartsharesystems.com> wrote:
> > > 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.");
>
> And how will it catch the comment about 10 characters ?

I mean you still have to re-read the whole documentation and look for
some reference somewhere about 10 characters.
  

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Thursday, 7 September 2023 10.55
> 
> On Thu, Sep 7, 2023 at 10:53 AM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Thu, Sep 7, 2023 at 10:50 AM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> > > > 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.");
> >
> > And how will it catch the comment about 10 characters ?
> 
> I mean you still have to re-read the whole documentation and look for
> some reference somewhere about 10 characters.

The trick is to put the _Static_assert close to where the expectation occurs. That makes it easier to find where changes are necessary.

And the _Static_assert can be added at all the locations where changes would be necessary. (Generally, we should add a lot more _Static_assert to the code where it makes assumptions about e.g. the ordering of fields in a struct, such as the vector optimized code.)

Also, the failure message could be improved to include help about what to look for.

PS: The reference to RTE_THREAD_INTERNAL_NAME_SIZE should remain in the documentation, so perhaps look for "RTE_THREAD_INTERNAL_NAME_SIZE".
  

07/09/2023 13:10, Morten Brørup:
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Thursday, 7 September 2023 10.55
> > 
> > On Thu, Sep 7, 2023 at 10:53 AM David Marchand
> > <david.marchand@redhat.com> wrote:
> > >
> > > On Thu, Sep 7, 2023 at 10:50 AM Morten Brørup <mb@smartsharesystems.com>
> > wrote:
> > > > > 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.");
> > >
> > > And how will it catch the comment about 10 characters ?
> > 
> > I mean you still have to re-read the whole documentation and look for
> > some reference somewhere about 10 characters.
> 
> The trick is to put the _Static_assert close to where the expectation occurs. That makes it easier to find where changes are necessary.
> 
> And the _Static_assert can be added at all the locations where changes would be necessary. (Generally, we should add a lot more _Static_assert to the code where it makes assumptions about e.g. the ordering of fields in a struct, such as the vector optimized code.)
> 
> Also, the failure message could be improved to include help about what to look for.
> 
> PS: The reference to RTE_THREAD_INTERNAL_NAME_SIZE should remain in the documentation, so perhaps look for "RTE_THREAD_INTERNAL_NAME_SIZE".

I agree with David, it is easier to maintain if not mentioning
the exact value in the doc,
and having a mention to RTE_THREAD_INTERNAL_NAME_SIZE is enough
if it defined as "11".
Then we need only one static assert (or RTE_BUILD_BUG_ON)
below the definition to make sure the number is still valid.
  

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, 11 September 2023 17.26
> 
> 07/09/2023 13:10, Morten Brørup:
> > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > Sent: Thursday, 7 September 2023 10.55
> > >
> > > On Thu, Sep 7, 2023 at 10:53 AM David Marchand
> > > <david.marchand@redhat.com> wrote:
> > > >
> > > > On Thu, Sep 7, 2023 at 10:50 AM Morten Brørup
> <mb@smartsharesystems.com>
> > > wrote:
> > > > > > 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.");
> > > >
> > > > And how will it catch the comment about 10 characters ?
> > >
> > > I mean you still have to re-read the whole documentation and look
> for
> > > some reference somewhere about 10 characters.
> >
> > The trick is to put the _Static_assert close to where the expectation
> occurs. That makes it easier to find where changes are necessary.
> >
> > And the _Static_assert can be added at all the locations where changes
> would be necessary. (Generally, we should add a lot more _Static_assert
> to the code where it makes assumptions about e.g. the ordering of fields
> in a struct, such as the vector optimized code.)
> >
> > Also, the failure message could be improved to include help about what
> to look for.
> >
> > PS: The reference to RTE_THREAD_INTERNAL_NAME_SIZE should remain in
> the documentation, so perhaps look for "RTE_THREAD_INTERNAL_NAME_SIZE".
> 
> I agree with David, it is easier to maintain if not mentioning
> the exact value in the doc,
> and having a mention to RTE_THREAD_INTERNAL_NAME_SIZE is enough
> if it defined as "11".
> Then we need only one static assert (or RTE_BUILD_BUG_ON)
> below the definition to make sure the number is still valid.

Sorry. Bad choice of words. David is obviously right that it is easy to miss if the prefix changes.

My intention was to suggest a mitigation, so we could keep the exact value in the comment, making the documentation easier to read.

Anyway, no objections from me.