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

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

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