Re: [dpdk-dev] [PATCH v9 01/10] eal: add thread id and simple thread functions
From: Dmitry Kozlyuk <hidden>
Date: 2021-06-08 23:03:53
2021-06-04 16:44 (UTC-0700), Narcisa Ana Maria Vasile:
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? A better name for rte_thread_types.h would be rte_posix_thread_types.h to indicate this file is not really common.
quoted hunk ↗ jump to hunk
diff --git a/lib/eal/common/rte_thread.c b/lib/eal/common/rte_thread.c new file mode 100644 index 0000000000..1292f7a8f8 --- /dev/null +++ b/lib/eal/common/rte_thread.c@@ -0,0 +1,105 @@ +/* SPDX-License-Identifier: BSD-3-Clause + * Copyright 2021 Mellanox Technologies, Ltd + * Copyright(c) 2021 Microsoft Corporation + */ + +#include <errno.h> +#include <pthread.h> +#include <stdlib.h> +#include <string.h> + +#include <rte_common.h> +#include <rte_errno.h> +#include <rte_log.h> +#include <rte_thread.h> + +struct eal_tls_key { + pthread_key_t thread_index; +}; + +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.
+ + thread_id.opaque_id = pthread_self(); + + return thread_id; +} +
[...]
quoted hunk ↗ jump to hunk
diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h index 8be8ed8f36..347df1a6ae 100644 --- a/lib/eal/include/rte_thread.h +++ b/lib/eal/include/rte_thread.h@@ -1,6 +1,8 @@ /* SPDX-License-Identifier: BSD-3-Clause * Copyright(c) 2021 Mellanox Technologies, Ltd + * Copyright(c) 2021 Microsoft Corporation */ +#include <stdint.h> #include <rte_os.h> #include <rte_compat.h>@@ -20,11 +22,50 @@ extern "C" { #endif +#include <sched.h> +#if defined(RTE_USE_WINDOWS_THREAD_TYPES)
Redundant braces are discouraged in DPDK codebase. [...]
quoted hunk ↗ jump to hunk
diff --git a/lib/eal/include/rte_thread_types.h b/lib/eal/include/rte_thread_types.h new file mode 100644 index 0000000000..d67b24a563 --- /dev/null +++ b/lib/eal/include/rte_thread_types.h@@ -0,0 +1,10 @@ +/* SPDX-License-Identifier: BSD-3-Clause + * Copyright(c) 2021 Microsoft Corporation + */ + +#ifndef _RTE_THREAD_TYPES_H_ +#define _RTE_THREAD_TYPES_H_ + +#include <pthread.h> + +#endif /* _RTE_THREAD_TYPES_H_ */diff --git a/lib/eal/windows/include/rte_windows_thread_types.h b/lib/eal/windows/include/rte_windows_thread_types.h new file mode 100644 index 0000000000..60e6d94553 --- /dev/null +++ b/lib/eal/windows/include/rte_windows_thread_types.h@@ -0,0 +1,10 @@ +/* SPDX-License-Identifier: BSD-3-Clause + * Copyright(c) 2021 Microsoft Corporation + */ + +#ifndef _RTE_THREAD_TYPES_H_ +#define _RTE_THREAD_TYPES_H_ + +#include <rte_windows.h> + +#endif /* _RTE_THREAD_TYPES_H_ */
Thread types headers should have a check that forbids including them 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.