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

On Wed, Jun 09, 2021 at 02:03:48AM +0300, Dmitry Kozlyuk wrote:

2021-06-04 16:44 (UTC-0700), Narcisa Ana Maria Vasile:

quoted

From: Narcisa Vasile <redacted>

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 <redacted>
---
 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.

quoted

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

[...]

quoted

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.