[v5,1/3] eal: add basic thread ID and current thread identifier API
Checks
Commit Message
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(+)
Comments
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?
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'.
> +} 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 *))
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 *))
05/05/2022 08:11, Tyler Retzlaff пишет:
> 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.
It would be the same as long as sizes of pthread_t uintptr_t are equal.
> * 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.
Using pthread_t directly still seems like a safest bet to me.
Then we can avoid doing these explicit type conversions before/after
each pthread_xxx() call and wouldn't need to worry if we'll ever have
platform with bigger pthread_t (though yes, I admit it is very unlikely).
But, if we still prefer to go ahead with 'arch-neutral' approach,
then I think we need to have compilation time check that opaque_id
is big enough to hold pthread_t value:
RTE_BUILD_BUG_ON(sizeof(pthread_t) != sizeof(opaque_id)) or so.
>
>>
>>
>>> +} 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 *))
> From: Konstantin Ananyev [mailto:konstantin.v.ananyev@yandex.ru]
> Sent: Friday, 6 May 2022 21.38
>
> 05/05/2022 08:11, Tyler Retzlaff пишет:
> > 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.
>
>
> It would be the same as long as sizes of pthread_t uintptr_t are equal.
They are not. pthread_t (Linux thread ID) and DWORD (Windows thread ID) are both 32 bit, uintptr_t is 64 or 32 bit depending on pointer size (32 or 64 bit CPU).
>
> > * 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.
>
> Using pthread_t directly still seems like a safest bet to me.
> Then we can avoid doing these explicit type conversions before/after
> each pthread_xxx() call and wouldn't need to worry if we'll ever have
> platform with bigger pthread_t (though yes, I admit it is very
> unlikely).
> But, if we still prefer to go ahead with 'arch-neutral' approach,
> then I think we need to have compilation time check that opaque_id
> is big enough to hold pthread_t value:
> RTE_BUILD_BUG_ON(sizeof(pthread_t) != sizeof(opaque_id)) or so.
>
I agree with Konstantin's concerns. All this type casting increases the risk for bugs. Also, I don't understand the need to use a 64 bit value when thread IDs are 32 bit in both Linux and Windows.
The thread handling is O/S specific code, so #ifdef is unavoidable.
Furthermore, I don't think we should wrap O/S specific integer types into structs - it adds unnecessary complexity.
I would prefer:
#ifdef WINDOWS
typedef DWORD rte_thread_t;
#else
typedef pthread_t rte_thread_t;
#endif
>
> >
> >>
> >>
> >>> +} 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 *))
>
Hi Morten,
>> From: Konstantin Ananyev [mailto:konstantin.v.ananyev@yandex.ru]
>> Sent: Friday, 6 May 2022 21.38
>>
>> 05/05/2022 08:11, Tyler Retzlaff пишет:
>>> 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.
>>
>>
>> It would be the same as long as sizes of pthread_t uintptr_t are equal.
>
> They are not. pthread_t (Linux thread ID) and DWORD (Windows thread ID) are both 32 bit, uintptr_t is 64 or 32 bit depending on pointer size (32 or 64 bit CPU).
What make you think pthread_t is 32-bit on linux?
From <pthread.h> on my box:
typedef unsigned long int pthread_t;
So it is either 64-bit or 32-bit depending on arch.
Same as uintptr_t.
>
>>
>>> * 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.
>>
>> Using pthread_t directly still seems like a safest bet to me.
>> Then we can avoid doing these explicit type conversions before/after
>> each pthread_xxx() call and wouldn't need to worry if we'll ever have
>> platform with bigger pthread_t (though yes, I admit it is very
>> unlikely).
>> But, if we still prefer to go ahead with 'arch-neutral' approach,
>> then I think we need to have compilation time check that opaque_id
>> is big enough to hold pthread_t value:
>> RTE_BUILD_BUG_ON(sizeof(pthread_t) != sizeof(opaque_id)) or so.
>>
>
> I agree with Konstantin's concerns. All this type casting increases the risk for bugs. Also, I don't understand the need to use a 64 bit value when thread IDs are 32 bit in both Linux and Windows.
>
> The thread handling is O/S specific code, so #ifdef is unavoidable.
>
> Furthermore, I don't think we should wrap O/S specific integer types into structs - it adds unnecessary complexity.
>
> I would prefer:
>
> #ifdef WINDOWS
> typedef DWORD rte_thread_t;
> #else
> typedef pthread_t rte_thread_t;
> #endif
>
>>
>>>
>>>>
>>>>
>>>>> +} 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 *))
>>
>
> From: Konstantin Ananyev [mailto:konstantin.v.ananyev@yandex.ru]
> Sent: Saturday, 7 May 2022 15.58
>
> Hi Morten,
>
> >> From: Konstantin Ananyev [mailto:konstantin.v.ananyev@yandex.ru]
> >> Sent: Friday, 6 May 2022 21.38
> >>
> >> 05/05/2022 08:11, Tyler Retzlaff пишет:
> >>> 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.
> >>
> >>
> >> It would be the same as long as sizes of pthread_t uintptr_t are
> equal.
> >
> > They are not. pthread_t (Linux thread ID) and DWORD (Windows thread
> ID) are both 32 bit, uintptr_t is 64 or 32 bit depending on pointer
> size (32 or 64 bit CPU).
>
> What make you think pthread_t is 32-bit on linux?
> From <pthread.h> on my box:
> typedef unsigned long int pthread_t;
> So it is either 64-bit or 32-bit depending on arch.
> Same as uintptr_t.
You are right, Konstantin. I had overlooked the "long" in there.
>
> >
> >>
> >>> * preference for platform agnostic headers. i.e. don't drag
> >>> platform specific headers into the application namespace when
> >>> including rte_xxx.h headers.
So this is an exception from the "don't hide the types" rule that DPDK inherited from the Kernel. In theory, this makes really good sense for an EAL that really is what it says (i.e. an abstraction layer). In reality, though, this requires that the EAL offers all the features of the underlying O/S that the application wants to use - otherwise, the application will have to access private members of the EAL structures.
> >>>> 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.
> >>
> >> Using pthread_t directly still seems like a safest bet to me.
> >> Then we can avoid doing these explicit type conversions before/after
> >> each pthread_xxx() call and wouldn't need to worry if we'll ever
> have
> >> platform with bigger pthread_t (though yes, I admit it is very
> >> unlikely).
> >> But, if we still prefer to go ahead with 'arch-neutral' approach,
> >> then I think we need to have compilation time check that opaque_id
> >> is big enough to hold pthread_t value:
> >> RTE_BUILD_BUG_ON(sizeof(pthread_t) != sizeof(opaque_id)) or so.
Yes, this should be in the O/S specific c files:
RTE_BUILD_BUG_ON(sizeof(rte_thread_t) < sizeof(pthread_t))
RTE_BUILD_BUG_ON(sizeof(rte_thread_t) < sizeof(DWORD))
> >>
> >
> > I agree with Konstantin's concerns. All this type casting increases
> the risk for bugs. Also, I don't understand the need to use a 64 bit
> value when thread IDs are 32 bit in both Linux and Windows.
> >
> > The thread handling is O/S specific code, so #ifdef is unavoidable.
> >
> > Furthermore, I don't think we should wrap O/S specific integer types
> into structs - it adds unnecessary complexity.
> >
> > I would prefer:
> >
> > #ifdef WINDOWS
> > typedef DWORD rte_thread_t;
> > #else
> > typedef pthread_t rte_thread_t;
> > #endif
OK, I was totally wrong here. But I still don't think we need to wrap the value into a structure, but can just use:
typedef uintptr_t rte_thread_t; /* And add comments about the underlying types, i.e. DWORD on Windows, pthread_t (not tid_t) on Linux/BSD. */
> >
> >>
> >>>
> >>>>
> >>>>
> >>>>> +} 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 *))
> >>
> >
>
07/05/2022 20:47, Morten Brørup пишет:
>> From: Konstantin Ananyev [mailto:konstantin.v.ananyev@yandex.ru]
>> Sent: Saturday, 7 May 2022 15.58
>>
>> Hi Morten,
>>
>>>> From: Konstantin Ananyev [mailto:konstantin.v.ananyev@yandex.ru]
>>>> Sent: Friday, 6 May 2022 21.38
>>>>
>>>> 05/05/2022 08:11, Tyler Retzlaff пишет:
>>>>> 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.
>>>>
>>>>
>>>> It would be the same as long as sizes of pthread_t uintptr_t are
>> equal.
>>>
>>> They are not. pthread_t (Linux thread ID) and DWORD (Windows thread
>> ID) are both 32 bit, uintptr_t is 64 or 32 bit depending on pointer
>> size (32 or 64 bit CPU).
>>
>> What make you think pthread_t is 32-bit on linux?
>> From <pthread.h> on my box:
>> typedef unsigned long int pthread_t;
>> So it is either 64-bit or 32-bit depending on arch.
>> Same as uintptr_t.
>
> You are right, Konstantin. I had overlooked the "long" in there.
>
>>
>>>
>>>>
>>>>> * preference for platform agnostic headers. i.e. don't drag
>>>>> platform specific headers into the application namespace when
>>>>> including rte_xxx.h headers.
>
> So this is an exception from the "don't hide the types" rule that DPDK inherited from the Kernel. In theory, this makes really good sense for an EAL that really is what it says (i.e. an abstraction layer). In reality, though, this requires that the EAL offers all the features of the underlying O/S that the application wants to use - otherwise, the application will have to access private members of the EAL structures.
>
>>>>>> 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.
>>>>
>>>> Using pthread_t directly still seems like a safest bet to me.
>>>> Then we can avoid doing these explicit type conversions before/after
>>>> each pthread_xxx() call and wouldn't need to worry if we'll ever
>> have
>>>> platform with bigger pthread_t (though yes, I admit it is very
>>>> unlikely).
>>>> But, if we still prefer to go ahead with 'arch-neutral' approach,
>>>> then I think we need to have compilation time check that opaque_id
>>>> is big enough to hold pthread_t value:
>>>> RTE_BUILD_BUG_ON(sizeof(pthread_t) != sizeof(opaque_id)) or so.
>
> Yes, this should be in the O/S specific c files:
>
> RTE_BUILD_BUG_ON(sizeof(rte_thread_t) < sizeof(pthread_t))
>
> RTE_BUILD_BUG_ON(sizeof(rte_thread_t) < sizeof(DWORD))
>
>>>>
>>>
>>> I agree with Konstantin's concerns. All this type casting increases
>> the risk for bugs. Also, I don't understand the need to use a 64 bit
>> value when thread IDs are 32 bit in both Linux and Windows.
>>>
>>> The thread handling is O/S specific code, so #ifdef is unavoidable.
>>>
>>> Furthermore, I don't think we should wrap O/S specific integer types
>> into structs - it adds unnecessary complexity.
>>>
>>> I would prefer:
>>>
>>> #ifdef WINDOWS
>>> typedef DWORD rte_thread_t;
>>> #else
>>> typedef pthread_t rte_thread_t;
>>> #endif
>
> OK, I was totally wrong here. But I still don't think we need to wrap the value into a structure, but can just use:
>
> typedef uintptr_t rte_thread_t; /* And add comments about the underlying types, i.e. DWORD on Windows, pthread_t (not tid_t) on Linux/BSD. */
I think we probably can have what you suggested above.
Though it might be better to move rte_thread_t typedef in OS-specific
header (rte_os.h).
On Tue, May 10, 2022 at 10:52:34PM +0100, Konstantin Ananyev wrote:
> 07/05/2022 20:47, Morten Brørup пишет:
> >>From: Konstantin Ananyev [mailto:konstantin.v.ananyev@yandex.ru]
> >>Sent: Saturday, 7 May 2022 15.58
> >>
> >>Hi Morten,
> >>
> >>>>From: Konstantin Ananyev [mailto:konstantin.v.ananyev@yandex.ru]
> >>>>Sent: Friday, 6 May 2022 21.38
> >>>>
> >>>>05/05/2022 08:11, Tyler Retzlaff пишет:
> >>>>>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.
> >>>>
> >>>>
> >>>>It would be the same as long as sizes of pthread_t uintptr_t are
> >>equal.
> >>>
> >>>They are not. pthread_t (Linux thread ID) and DWORD (Windows thread
> >>ID) are both 32 bit, uintptr_t is 64 or 32 bit depending on pointer
> >>size (32 or 64 bit CPU).
> >>
> >>What make you think pthread_t is 32-bit on linux?
> >> From <pthread.h> on my box:
> >>typedef unsigned long int pthread_t;
> >>So it is either 64-bit or 32-bit depending on arch.
> >>Same as uintptr_t.
> >
> >You are right, Konstantin. I had overlooked the "long" in there.
> >
> >>
> >>>
> >>>>
> >>>>>* preference for platform agnostic headers. i.e. don't drag
> >>>>> platform specific headers into the application namespace when
> >>>>> including rte_xxx.h headers.
> >
> >So this is an exception from the "don't hide the types" rule that DPDK inherited from the Kernel. In theory, this makes really good sense for an EAL that really is what it says (i.e. an abstraction layer). In reality, though, this requires that the EAL offers all the features of the underlying O/S that the application wants to use - otherwise, the application will have to access private members of the EAL structures.
> >
> >>>>>>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.
> >>>>
> >>>>Using pthread_t directly still seems like a safest bet to me.
> >>>>Then we can avoid doing these explicit type conversions before/after
> >>>>each pthread_xxx() call and wouldn't need to worry if we'll ever
> >>have
> >>>>platform with bigger pthread_t (though yes, I admit it is very
> >>>>unlikely).
> >>>>But, if we still prefer to go ahead with 'arch-neutral' approach,
> >>>>then I think we need to have compilation time check that opaque_id
> >>>>is big enough to hold pthread_t value:
> >>>>RTE_BUILD_BUG_ON(sizeof(pthread_t) != sizeof(opaque_id)) or so.
> >
> >Yes, this should be in the O/S specific c files:
> >
> >RTE_BUILD_BUG_ON(sizeof(rte_thread_t) < sizeof(pthread_t))
> >
> >RTE_BUILD_BUG_ON(sizeof(rte_thread_t) < sizeof(DWORD))
> >
> >>>>
> >>>
> >>>I agree with Konstantin's concerns. All this type casting increases
> >>the risk for bugs. Also, I don't understand the need to use a 64 bit
> >>value when thread IDs are 32 bit in both Linux and Windows.
> >>>
> >>>The thread handling is O/S specific code, so #ifdef is unavoidable.
> >>>
> >>>Furthermore, I don't think we should wrap O/S specific integer types
> >>into structs - it adds unnecessary complexity.
> >>>
> >>>I would prefer:
> >>>
> >>>#ifdef WINDOWS
> >>>typedef DWORD rte_thread_t;
> >>>#else
> >>>typedef pthread_t rte_thread_t;
> >>>#endif
> >
> >OK, I was totally wrong here. But I still don't think we need to wrap the value into a structure, but can just use:
> >
> >typedef uintptr_t rte_thread_t; /* And add comments about the underlying types, i.e. DWORD on Windows, pthread_t (not tid_t) on Linux/BSD. */
a struct will guarantee no implicit conversion since it is a real type.
i am uncertain about why you feel it makes it more complex? it improves
safety by enforcing the semantics intended i.e. that it is an opaque type.
as a side-nit it's a shame the same wasn't done for core id vs core index
because i'm forever fixing bugs where people have swapped the two where
the mistake could have been a compiler error it is now a runtime error.
>
>
> I think we probably can have what you suggested above.
> Though it might be better to move rte_thread_t typedef in OS-specific
> header (rte_os.h).
>
i don't see any benefit to this.
direct exposure of pthread_t will invariably lead to abuse by people
re-introducing dependency on pthread api leading to re-introduction of
conditionally compiled code for posix v non-posix ports.
of course the struct doesn't expressly prevent this since you can just
grovel its internals and assume the implementation but still why beg
for it by presenting no barrier entry at all.
i can address the initial concern about type sizing with a static assert
but i don't think without a much stronger argument with examples
demonstrating clear benefit i can get behind using just a typedef or
pthread_t conditionally compiled.
thanks for the feedback.
> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Wednesday, 11 May 2022 09.17
>
> On Tue, May 10, 2022 at 10:52:34PM +0100, Konstantin Ananyev wrote:
> > 07/05/2022 20:47, Morten Brørup пишет:
> > >>From: Konstantin Ananyev [mailto:konstantin.v.ananyev@yandex.ru]
> > >>Sent: Saturday, 7 May 2022 15.58
> > >>
> > >>Hi Morten,
> > >>
> > >>>>From: Konstantin Ananyev [mailto:konstantin.v.ananyev@yandex.ru]
> > >>>>Sent: Friday, 6 May 2022 21.38
> > >>>>
> > >>>>05/05/2022 08:11, Tyler Retzlaff пишет:
> > >>>>>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.
> > >>>>
> > >>>>
> > >>>>It would be the same as long as sizes of pthread_t uintptr_t are
> > >>equal.
> > >>>
> > >>>They are not. pthread_t (Linux thread ID) and DWORD (Windows
> thread
> > >>ID) are both 32 bit, uintptr_t is 64 or 32 bit depending on pointer
> > >>size (32 or 64 bit CPU).
> > >>
> > >>What make you think pthread_t is 32-bit on linux?
> > >> From <pthread.h> on my box:
> > >>typedef unsigned long int pthread_t;
> > >>So it is either 64-bit or 32-bit depending on arch.
> > >>Same as uintptr_t.
> > >
> > >You are right, Konstantin. I had overlooked the "long" in there.
> > >
> > >>
> > >>>
> > >>>>
> > >>>>>* preference for platform agnostic headers. i.e. don't drag
> > >>>>> platform specific headers into the application namespace
> when
> > >>>>> including rte_xxx.h headers.
> > >
> > >So this is an exception from the "don't hide the types" rule that
> DPDK inherited from the Kernel. In theory, this makes really good sense
> for an EAL that really is what it says (i.e. an abstraction layer). In
> reality, though, this requires that the EAL offers all the features of
> the underlying O/S that the application wants to use - otherwise, the
> application will have to access private members of the EAL structures.
> > >
> > >>>>>>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.
> > >>>>
> > >>>>Using pthread_t directly still seems like a safest bet to me.
> > >>>>Then we can avoid doing these explicit type conversions
> before/after
> > >>>>each pthread_xxx() call and wouldn't need to worry if we'll ever
> > >>have
> > >>>>platform with bigger pthread_t (though yes, I admit it is very
> > >>>>unlikely).
> > >>>>But, if we still prefer to go ahead with 'arch-neutral' approach,
> > >>>>then I think we need to have compilation time check that
> opaque_id
> > >>>>is big enough to hold pthread_t value:
> > >>>>RTE_BUILD_BUG_ON(sizeof(pthread_t) != sizeof(opaque_id)) or so.
> > >
> > >Yes, this should be in the O/S specific c files:
> > >
> > >RTE_BUILD_BUG_ON(sizeof(rte_thread_t) < sizeof(pthread_t))
> > >
> > >RTE_BUILD_BUG_ON(sizeof(rte_thread_t) < sizeof(DWORD))
> > >
> > >>>>
> > >>>
> > >>>I agree with Konstantin's concerns. All this type casting
> increases
> > >>the risk for bugs. Also, I don't understand the need to use a 64
> bit
> > >>value when thread IDs are 32 bit in both Linux and Windows.
> > >>>
> > >>>The thread handling is O/S specific code, so #ifdef is
> unavoidable.
> > >>>
> > >>>Furthermore, I don't think we should wrap O/S specific integer
> types
> > >>into structs - it adds unnecessary complexity.
> > >>>
> > >>>I would prefer:
> > >>>
> > >>>#ifdef WINDOWS
> > >>>typedef DWORD rte_thread_t;
> > >>>#else
> > >>>typedef pthread_t rte_thread_t;
> > >>>#endif
> > >
> > >OK, I was totally wrong here. But I still don't think we need to
> wrap the value into a structure, but can just use:
>
> > >
> > >typedef uintptr_t rte_thread_t; /* And add comments about the
> underlying types, i.e. DWORD on Windows, pthread_t (not tid_t) on
> Linux/BSD. */
>
> a struct will guarantee no implicit conversion since it is a real type.
That's a good point, which I didn't think about.
>
> i am uncertain about why you feel it makes it more complex? it improves
> safety by enforcing the semantics intended i.e. that it is an opaque
> type.
>
> as a side-nit it's a shame the same wasn't done for core id vs core
> index
> because i'm forever fixing bugs where people have swapped the two where
> the mistake could have been a compiler error it is now a runtime error.
I have noticed this too... I guess they started out as one, and only split into two in a later version of DPDK. This kind of legacy is difficult to improve when the API must remain stable. :-(
>
> >
> >
> > I think we probably can have what you suggested above.
> > Though it might be better to move rte_thread_t typedef in OS-specific
> > header (rte_os.h).
> >
>
> i don't see any benefit to this.
>
> direct exposure of pthread_t will invariably lead to abuse by people
> re-introducing dependency on pthread api leading to re-introduction of
> conditionally compiled code for posix v non-posix ports.
>
> of course the struct doesn't expressly prevent this since you can just
> grovel its internals and assume the implementation but still why beg
> for it by presenting no barrier entry at all.
>
> i can address the initial concern about type sizing with a static
> assert
> but i don't think without a much stronger argument with examples
> demonstrating clear benefit i can get behind using just a typedef or
> pthread_t conditionally compiled.
>
> thanks for the feedback.
I guess the differences of opinion mainly revolve about how opaque vs. transparent we want the rte_thread_t to be.
DPDK has a tradition, inherited from Linux, for keeping types transparent. In this case, considering your experience with working cross O/S, I will change my opinion and support your stance.
And to further avoid someone incorrectly using the private "id" member of the rte_thread_t struct, you could give it a different name for each O/S. Unfortunately, it's not C++, so you cannot make it "private" or "protected".
11/05/2022 08:17, Tyler Retzlaff пишет:
> On Tue, May 10, 2022 at 10:52:34PM +0100, Konstantin Ananyev wrote:
>> 07/05/2022 20:47, Morten Brørup пишет:
>>>> From: Konstantin Ananyev [mailto:konstantin.v.ananyev@yandex.ru]
>>>> Sent: Saturday, 7 May 2022 15.58
>>>>
>>>> Hi Morten,
>>>>
>>>>>> From: Konstantin Ananyev [mailto:konstantin.v.ananyev@yandex.ru]
>>>>>> Sent: Friday, 6 May 2022 21.38
>>>>>>
>>>>>> 05/05/2022 08:11, Tyler Retzlaff пишет:
>>>>>>> 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.
>>>>>>
>>>>>>
>>>>>> It would be the same as long as sizes of pthread_t uintptr_t are
>>>> equal.
>>>>>
>>>>> They are not. pthread_t (Linux thread ID) and DWORD (Windows thread
>>>> ID) are both 32 bit, uintptr_t is 64 or 32 bit depending on pointer
>>>> size (32 or 64 bit CPU).
>>>>
>>>> What make you think pthread_t is 32-bit on linux?
>>>> From <pthread.h> on my box:
>>>> typedef unsigned long int pthread_t;
>>>> So it is either 64-bit or 32-bit depending on arch.
>>>> Same as uintptr_t.
>>>
>>> You are right, Konstantin. I had overlooked the "long" in there.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>> * preference for platform agnostic headers. i.e. don't drag
>>>>>>> platform specific headers into the application namespace when
>>>>>>> including rte_xxx.h headers.
>>>
>>> So this is an exception from the "don't hide the types" rule that DPDK inherited from the Kernel. In theory, this makes really good sense for an EAL that really is what it says (i.e. an abstraction layer). In reality, though, this requires that the EAL offers all the features of the underlying O/S that the application wants to use - otherwise, the application will have to access private members of the EAL structures.
>>>
>>>>>>>> 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.
>>>>>>
>>>>>> Using pthread_t directly still seems like a safest bet to me.
>>>>>> Then we can avoid doing these explicit type conversions before/after
>>>>>> each pthread_xxx() call and wouldn't need to worry if we'll ever
>>>> have
>>>>>> platform with bigger pthread_t (though yes, I admit it is very
>>>>>> unlikely).
>>>>>> But, if we still prefer to go ahead with 'arch-neutral' approach,
>>>>>> then I think we need to have compilation time check that opaque_id
>>>>>> is big enough to hold pthread_t value:
>>>>>> RTE_BUILD_BUG_ON(sizeof(pthread_t) != sizeof(opaque_id)) or so.
>>>
>>> Yes, this should be in the O/S specific c files:
>>>
>>> RTE_BUILD_BUG_ON(sizeof(rte_thread_t) < sizeof(pthread_t))
>>>
>>> RTE_BUILD_BUG_ON(sizeof(rte_thread_t) < sizeof(DWORD))
>>>
>>>>>>
>>>>>
>>>>> I agree with Konstantin's concerns. All this type casting increases
>>>> the risk for bugs. Also, I don't understand the need to use a 64 bit
>>>> value when thread IDs are 32 bit in both Linux and Windows.
>>>>>
>>>>> The thread handling is O/S specific code, so #ifdef is unavoidable.
>>>>>
>>>>> Furthermore, I don't think we should wrap O/S specific integer types
>>>> into structs - it adds unnecessary complexity.
>>>>>
>>>>> I would prefer:
>>>>>
>>>>> #ifdef WINDOWS
>>>>> typedef DWORD rte_thread_t;
>>>>> #else
>>>>> typedef pthread_t rte_thread_t;
>>>>> #endif
>>>
>>> OK, I was totally wrong here. But I still don't think we need to wrap the value into a structure, but can just use:
>
>>>
>>> typedef uintptr_t rte_thread_t; /* And add comments about the underlying types, i.e. DWORD on Windows, pthread_t (not tid_t) on Linux/BSD. */
>
> a struct will guarantee no implicit conversion since it is a real type.
>
> i am uncertain about why you feel it makes it more complex? it improves
> safety by enforcing the semantics intended i.e. that it is an opaque type.
>
> as a side-nit it's a shame the same wasn't done for core id vs core index
> because i'm forever fixing bugs where people have swapped the two where
> the mistake could have been a compiler error it is now a runtime error.
>
>>
>>
>> I think we probably can have what you suggested above.
>> Though it might be better to move rte_thread_t typedef in OS-specific
>> header (rte_os.h).
>>
>
> i don't see any benefit to this.
>
> direct exposure of pthread_t will invariably lead to abuse by people
> re-introducing dependency on pthread api leading to re-introduction of
> conditionally compiled code for posix v non-posix ports.
>
> of course the struct doesn't expressly prevent this since you can just
> grovel its internals and assume the implementation but still why beg
> for it by presenting no barrier entry at all.
Ok, if everyone else think that opaque type is a better choice here,
I wouldn't insist.
> i can address the initial concern about type sizing with a static assert
Please do.
> but i don't think without a much stronger argument with examples
> demonstrating clear benefit i can get behind using just a typedef or
> pthread_t conditionally compiled.
>
> thanks for the feedback.
@@ -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 */
+} 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
/**
@@ -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 *))
{
@@ -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 {
@@ -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 *))