Re: [PATCH v5 1/3] eal: add basic thread ID and current thread identifier API

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

On Wed, May 04, 2022 at 11:55:57PM +0100, Konstantin Ananyev wrote:
> 04/05/2022 16:46, Tyler Retzlaff пишет:
> >Provide a portable type-safe thread identifier.
> >Provide rte_thread_self for obtaining current thread identifier.
> >
> >Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
> >Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> >Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> >---
> >  lib/eal/include/rte_thread.h | 22 ++++++++++++++++++++++
> >  lib/eal/unix/rte_thread.c    | 11 +++++++++++
> >  lib/eal/version.map          |  3 +++
> >  lib/eal/windows/rte_thread.c | 10 ++++++++++
> >  4 files changed, 46 insertions(+)
> >
> >diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
> >index 8be8ed8..14478ba 100644
> >--- a/lib/eal/include/rte_thread.h
> >+++ b/lib/eal/include/rte_thread.h
> >@@ -1,7 +1,10 @@
> >  /* SPDX-License-Identifier: BSD-3-Clause
> >   * Copyright(c) 2021 Mellanox Technologies, Ltd
> >+ * Copyright (C) 2022 Microsoft Corporation
> >   */
> >+#include <stdint.h>
> >+
> >  #include <rte_os.h>
> >  #include <rte_compat.h>
> >@@ -21,10 +24,29 @@
> >  #endif
> >  /**
> >+ * Thread id descriptor.
> >+ */
> >+typedef struct {
> >+	uintptr_t opaque_id; /**< thread identifier */
> 
> 
> I know that currently on linux typeof(pthread_id) == unsigned long int.
> Though wouldn't it be safer and cleaner to use pthread_t explicitly
> on posix-like systems?

i believe the previous discussions are.

* preference for reduced or no conditional compilation.
* preference for sizeof(type) to be `the same' on all platforms.
* preference for platform agnostic headers. i.e. don't drag
  platform specific headers into the application namespace when
  including rte_xxx.h headers.

> Something like:
> typedef struct {
> #ifdef WINDOWS
> 	uintptr_t opaque_id;
> #else
> 	pthread_t opaque_id;
> #endif
> };
> AFAIK POSIX itself doesn't require pthread_t to be an 'arithmetic type'.

yes, this is correct. newer posix introduced this to allow the use of
structs. i assume prior reviewers are aware of the recent posix
standard (or should be).

this type makes no attempt to be usable on platforms that use
a handle > sizeof(uintptr_t). though any platform that does is free
to shove a pointer to struct into the handle at the cost of a
dereference if that is their implementation.

> 
> 
> >+} rte_thread_t;
> >+
> >+/**
> >   * TLS key type, an opaque pointer.
> >   */
> >  typedef struct eal_tls_key *rte_thread_key;
> >+/**
> >+ * @warning
> >+ * @b EXPERIMENTAL: this API may change without prior notice.
> >+ *
> >+ * 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);
> >+
> >  #ifdef RTE_HAS_CPUSET
> >  /**
> >diff --git a/lib/eal/unix/rte_thread.c b/lib/eal/unix/rte_thread.c
> >index c34ede9..82e008f 100644
> >--- a/lib/eal/unix/rte_thread.c
> >+++ b/lib/eal/unix/rte_thread.c
> >@@ -1,5 +1,6 @@
> >  /* SPDX-License-Identifier: BSD-3-Clause
> >   * Copyright 2021 Mellanox Technologies, Ltd
> >+ * Copyright (C) 2022 Microsoft Corporation
> >   */
> >  #include <errno.h>
> >@@ -15,6 +16,16 @@ struct eal_tls_key {
> >  	pthread_key_t thread_index;
> >  };
> >+rte_thread_t
> >+rte_thread_self(void)
> >+{
> >+	rte_thread_t thread_id;
> >+
> >+	thread_id.opaque_id = (uintptr_t)pthread_self();
> >+
> >+	return thread_id;
> >+}
> >+
> >  int
> >  rte_thread_key_create(rte_thread_key *key, void (*destructor)(void *))
> >  {
> >diff --git a/lib/eal/version.map b/lib/eal/version.map
> >index b53eeb3..05ce8f9 100644
> >--- a/lib/eal/version.map
> >+++ b/lib/eal/version.map
> >@@ -420,6 +420,9 @@ EXPERIMENTAL {
> >  	rte_intr_instance_free;
> >  	rte_intr_type_get;
> >  	rte_intr_type_set;
> >+
> >+	# added in 22.07
> >+	rte_thread_self;
> >  };
> >  INTERNAL {
> >diff --git a/lib/eal/windows/rte_thread.c b/lib/eal/windows/rte_thread.c
> >index 667287c..59fed3c 100644
> >--- a/lib/eal/windows/rte_thread.c
> >+++ b/lib/eal/windows/rte_thread.c
> >@@ -11,6 +11,16 @@ struct eal_tls_key {
> >  	DWORD thread_index;
> >  };
> >+rte_thread_t
> >+rte_thread_self(void)
> >+{
> >+	rte_thread_t thread_id;
> >+
> >+	thread_id.opaque_id = GetCurrentThreadId();
> >+
> >+	return thread_id;
> >+}
> >+
> >  int
> >  rte_thread_key_create(rte_thread_key *key,
> >  		__rte_unused void (*destructor)(void *))