[dpdk-dev] [PATCH v9 01/10] eal: add thread id and simple thread functions

[Narcisa Ana Maria Vasile]

On Wed, Jun 09, 2021 at 02:03:48AM +0300, Dmitry Kozlyuk wrote:
> 2021-06-04 16:44 (UTC-0700), Narcisa Ana Maria Vasile:
> > From: Narcisa Vasile <navasile at microsoft.com>
> > 
> > Use a portable, type-safe representation for the thread identifier.
> > Add functions for comparing thread ids and obtaining the thread id
> > for the current thread.
> > 
> > Signed-off-by: Narcisa Vasile <navasile at microsoft.com>
> > ---
> >  lib/eal/common/rte_thread.c                   | 105 ++++++++++++++++++
> >  lib/eal/include/rte_thread.h                  |  53 +++++++--
> >  lib/eal/include/rte_thread_types.h            |  10 ++
> >  .../include/rte_windows_thread_types.h        |  10 ++
> >  lib/eal/windows/rte_thread.c                  |  17 +++
> >  5 files changed, 186 insertions(+), 9 deletions(-)
> >  create mode 100644 lib/eal/common/rte_thread.c
> >  create mode 100644 lib/eal/include/rte_thread_types.h
> >  create mode 100644 lib/eal/windows/include/rte_windows_thread_types.h
> 
> It is strange that new files are being filled in the series, but are neither
> compiled nor installed until the last patch. Any reason not to replace
> lib/eal/unix/rte_thread.c starting from this patch?
> 
Replaced unix/rte_thread.c starting from here.

> A better name for rte_thread_types.h would be rte_posix_thread_types.h
> to indicate this file is not really common.
> 
> > 
> > +rte_thread_t
> > +rte_thread_self(void)
> > +{
> > +	rte_thread_t thread_id = { 0 };
> 
> (Applies to entire series.)
> Please do not initialize variables when you intend to overwrite them.
> This prevents compiler from reporting code paths where the variable is used
> before a proper assignment.

Thanks for the explanation, Dmitry!
> 
> 
> [...]
> > diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
> > index 8be8ed8f36..347df1a6ae 100644
> directly, see lib/eal/$arch/rte_atomic_64.h for examples.
> 
> Note that rte_*_thread_types.h should be `indirect_headers`, not `headers`
> in `meson.build` so that they're not checked by `buildtools/chkincs` when is
> gets enabled for Windows. This is especially
> true for lib/eal/common/rte_thread.h that cannot work without pthread.
> 
> However, in later patches these headers only contain mutex bits,
> see the comment to pathc 07/10 about them. Maybe we don't need these files
> after all.

Agreed, I was able to remove them after reimplementing the mutex functions.