[v9,01/10] eal: add thread id and simple thread functions
Checks
Commit Message
From: Narcisa Vasile <navasile@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@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
Comments
2021-06-04 16:44 (UTC-0700), Narcisa Ana Maria Vasile:
> From: Narcisa Vasile <navasile@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@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?
A better name for rte_thread_types.h would be rte_posix_thread_types.h
to indicate this file is not really common.
>
> 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;
> +}
> +
[...]
> 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.
[...]
> 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.
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@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@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.
new file mode 100644
@@ -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 };
+
+ thread_id.opaque_id = pthread_self();
+
+ return thread_id;
+}
+
+int
+rte_thread_equal(rte_thread_t t1, rte_thread_t t2)
+{
+ return pthread_equal(t1.opaque_id, t2.opaque_id);
+}
+
+int
+rte_thread_key_create(rte_thread_key *key, void (*destructor)(void *))
+{
+ int err;
+ rte_thread_key k;
+
+ k = malloc(sizeof(*k));
+ if (k == NULL) {
+ RTE_LOG(DEBUG, EAL, "Cannot allocate TLS key.\n");
+ return EINVAL;
+ }
+ err = pthread_key_create(&(k->thread_index), destructor);
+ if (err != 0) {
+ RTE_LOG(DEBUG, EAL, "pthread_key_create failed: %s\n",
+ strerror(err));
+ free(k);
+ return err;
+ }
+ *key = k;
+ return 0;
+}
+
+int
+rte_thread_key_delete(rte_thread_key key)
+{
+ int err;
+
+ if (key == NULL) {
+ RTE_LOG(DEBUG, EAL, "Invalid TLS key.\n");
+ return EINVAL;
+ }
+ err = pthread_key_delete(key->thread_index);
+ if (err != 0) {
+ RTE_LOG(DEBUG, EAL, "pthread_key_delete failed: %s\n",
+ strerror(err));
+ free(key);
+ return err;
+ }
+ free(key);
+ return 0;
+}
+
+int
+rte_thread_value_set(rte_thread_key key, const void *value)
+{
+ int err;
+
+ if (key == NULL) {
+ RTE_LOG(DEBUG, EAL, "Invalid TLS key.\n");
+ return EINVAL;
+ }
+ err = pthread_setspecific(key->thread_index, value);
+ if (err != 0) {
+ RTE_LOG(DEBUG, EAL, "pthread_setspecific failed: %s\n",
+ strerror(err));
+ return err;
+ }
+ return 0;
+}
+
+void *
+rte_thread_value_get(rte_thread_key key)
+{
+ if (key == NULL) {
+ RTE_LOG(DEBUG, EAL, "Invalid TLS key.\n");
+ rte_errno = EINVAL;
+ return NULL;
+ }
+ return pthread_getspecific(key->thread_index);
+}
@@ -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)
+#include <rte_windows_thread_types.h>
+#else
+#include <rte_thread_types.h>
+#endif
+
+/**
+ * Thread id descriptor.
+ */
+typedef struct rte_thread_tag {
+ uintptr_t opaque_id; /**< thread identifier */
+} rte_thread_t;
+
/**
* TLS key type, an opaque pointer.
*/
typedef struct eal_tls_key *rte_thread_key;
+/**
+ * Get the id of the calling thread.
+ *
+ * @return
+ * Return the thread id of the calling thread.
+ */
+__rte_experimental
+rte_thread_t rte_thread_self(void);
+
+/**
+ * Check if 2 thread ids are equal.
+ *
+ * @param t1
+ * First thread id.
+ *
+ * @param t2
+ * Second thread id.
+ *
+ * @return
+ * If the ids are equal, return nonzero.
+ * Otherwise, return 0.
+ */
+__rte_experimental
+int rte_thread_equal(rte_thread_t t1, rte_thread_t t2);
+
#ifdef RTE_HAS_CPUSET
/**
@@ -63,9 +104,7 @@ void rte_thread_get_affinity(rte_cpuset_t *cpusetp);
*
* @return
* On success, zero.
- * On failure, a negative number and an error number is set in rte_errno.
- * rte_errno can be: ENOMEM - Memory allocation error.
- * ENOEXEC - Specific OS error.
+ * On failure, return a positive errno-style error number.
*/
__rte_experimental
@@ -80,9 +119,7 @@ int rte_thread_key_create(rte_thread_key *key,
*
* @return
* On success, zero.
- * On failure, a negative number and an error number is set in rte_errno.
- * rte_errno can be: EINVAL - Invalid parameter passed.
- * ENOEXEC - Specific OS error.
+ * On failure, return a positive errno-style error number.
*/
__rte_experimental
int rte_thread_key_delete(rte_thread_key key);
@@ -97,9 +134,7 @@ int rte_thread_key_delete(rte_thread_key key);
*
* @return
* On success, zero.
- * On failure, a negative number and an error number is set in rte_errno.
- * rte_errno can be: EINVAL - Invalid parameter passed.
- * ENOEXEC - Specific OS error.
+ * On failure, return a positive errno-style error number.
*/
__rte_experimental
int rte_thread_value_set(rte_thread_key key, const void *value);
new file mode 100644
@@ -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_ */
new file mode 100644
@@ -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_ */
@@ -1,5 +1,6 @@
/* SPDX-License-Identifier: BSD-3-Clause
* Copyright 2021 Mellanox Technologies, Ltd
+ * Copyright(c) 2021 Microsoft Corporation
*/
#include <rte_common.h>
@@ -11,6 +12,22 @@ struct eal_tls_key {
DWORD thread_index;
};
+rte_thread_t
+rte_thread_self(void)
+{
+ rte_thread_t thread_id = { 0 };
+
+ thread_id.opaque_id = GetCurrentThreadId();
+
+ return thread_id;
+}
+
+int
+rte_thread_equal(rte_thread_t t1, rte_thread_t t2)
+{
+ return t1.opaque_id == t2.opaque_id;
+}
+
int
rte_thread_key_create(rte_thread_key *key,
__rte_unused void (*destructor)(void *))