> From: Tyler Retzlaff [mailto:roret...@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.anan...@yandex.ru] > > >>Sent: Saturday, 7 May 2022 15.58 > > >> > > >>Hi Morten, > > >> > > >>>>From: Konstantin Ananyev [mailto:konstantin.v.anan...@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 <navas...@microsoft.com> > > >>>>>>>Signed-off-by: Tyler Retzlaff <roret...@linux.microsoft.com> > > >>>>>>>Acked-by: Dmitry Kozlyuk <dmitry.kozl...@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".
