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

["Morten Brørup" <mb@smartsharesystems.com>]

> 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".