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

[Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>]

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 *))
>>
>