DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Tyler Retzlaff" <roretzla@linux.microsoft.com>,
	"Konstantin Ananyev" <konstantin.v.ananyev@yandex.ru>
Cc: <dev@dpdk.org>, <thomas@monjalon.net>, <dmitry.kozliuk@gmail.com>,
	<anatoly.burakov@intel.com>,
	"Narcisa Vasile" <navasile@microsoft.com>
Subject: RE: [PATCH v5 1/3] eal: add basic thread ID and current thread identifier API
Date: Wed, 11 May 2022 09:36:24 +0200	[thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D8705A@smartserver.smartshare.dk> (raw)
In-Reply-To: <20220511071707.GA7974@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>

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



  reply	other threads:[~2022-05-11  7:36 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-01 13:29 [PATCH 0/3] add eal functions for thread affinity Tyler Retzlaff
2022-04-01 13:29 ` [PATCH 1/3] eal/windows: translate Windows errors to errno-style errors Tyler Retzlaff
2022-04-01 13:29 ` [PATCH 2/3] eal: implement functions for get/set thread affinity Tyler Retzlaff
2022-04-08 14:01   ` Dmitry Kozlyuk
2022-04-09  8:02     ` Tyler Retzlaff
2022-04-01 13:29 ` [PATCH 3/3] test/threads: add unit test for thread API Tyler Retzlaff
2022-04-08 14:01   ` Dmitry Kozlyuk
2022-04-09  8:56     ` Tyler Retzlaff
2022-04-11 22:52       ` Dmitry Kozlyuk
2022-04-08  8:57 ` [PATCH 0/3] add eal functions for thread affinity David Marchand
2022-04-08 13:46   ` Tyler Retzlaff
2022-04-11  7:32     ` David Marchand
2022-04-12 10:43 ` [PATCH v2 0/4] add eal functions for thread affinity and self Tyler Retzlaff
2022-04-12 10:43   ` [PATCH v2 1/4] eal/windows: translate Windows errors to errno-style errors Tyler Retzlaff
2022-04-12 17:26     ` Menon, Ranjit
2022-04-13  7:07       ` Tyler Retzlaff
2022-04-25  8:25     ` David Marchand
2022-04-25  8:52       ` Tyler Retzlaff
2022-04-12 10:43   ` [PATCH v2 2/4] eal: add basic thread ID and current thread identifier API Tyler Retzlaff
2022-04-12 10:43   ` [PATCH v2 3/4] eal: implement functions for get/set thread affinity Tyler Retzlaff
2022-04-13  7:30     ` Tyler Retzlaff
2022-04-12 10:43   ` [PATCH v2 4/4] test/threads: add unit test for thread API Tyler Retzlaff
2022-04-13  7:43 ` [PATCH v3 0/4] add eal functions for thread affinity and self Tyler Retzlaff
2022-04-13  7:43   ` [PATCH v3 1/4] eal/windows: translate Windows errors to errno-style errors Tyler Retzlaff
2022-04-13  7:43   ` [PATCH v3 2/4] eal: add basic thread ID and current thread identifier API Tyler Retzlaff
2022-04-25  8:26     ` David Marchand
2022-04-25  8:53       ` Tyler Retzlaff
2022-04-13  7:43   ` [PATCH v3 3/4] eal: implement functions for get/set thread affinity Tyler Retzlaff
2022-04-25  8:26     ` David Marchand
2022-04-25  8:55       ` Tyler Retzlaff
2022-04-13  7:43   ` [PATCH v3 4/4] test/threads: add unit test for thread API Tyler Retzlaff
2022-04-26  7:50 ` [PATCH v4 0/3] add eal functions for thread affinity and self Tyler Retzlaff
2022-04-26  7:50   ` [PATCH v4 1/3] eal: add basic thread ID and current thread identifier API Tyler Retzlaff
2022-05-01  9:18     ` Dmitry Kozlyuk
2022-04-26  7:50   ` [PATCH v4 2/3] eal: implement functions for get/set thread affinity Tyler Retzlaff
2022-05-01  9:18     ` Dmitry Kozlyuk
2022-04-26  7:50   ` [PATCH v4 3/3] test/threads: add unit test for thread API Tyler Retzlaff
2022-05-01  9:18     ` Dmitry Kozlyuk
2022-05-03  9:38       ` Tyler Retzlaff
2022-05-04 15:46 ` [PATCH v5 0/3] add eal functions for thread affinity and self Tyler Retzlaff
2022-05-04 15:46   ` [PATCH v5 1/3] eal: add basic thread ID and current thread identifier API Tyler Retzlaff
2022-05-04 22:55     ` Konstantin Ananyev
2022-05-05  7:11       ` Tyler Retzlaff
2022-05-06 19:37         ` Konstantin Ananyev
2022-05-07  8:25           ` Morten Brørup
2022-05-07 13:57             ` Konstantin Ananyev
2022-05-07 19:47               ` Morten Brørup
2022-05-10 21:52                 ` Konstantin Ananyev
2022-05-11  7:17                   ` Tyler Retzlaff
2022-05-11  7:36                     ` Morten Brørup [this message]
2022-05-11 22:27                     ` Konstantin Ananyev
2022-05-04 15:46   ` [PATCH v5 2/3] eal: implement functions for get/set thread affinity Tyler Retzlaff
2022-05-04 15:46   ` [PATCH v5 3/3] test/threads: add unit test for thread API Tyler Retzlaff
2022-05-12 13:14 ` [PATCH v6 0/3] add eal functions for thread affinity and self Tyler Retzlaff
2022-05-12 13:14   ` [PATCH v6 1/3] eal: add basic thread ID and current thread identifier API Tyler Retzlaff
2022-05-15 22:02     ` Konstantin Ananyev
2022-05-16  6:21       ` Tyler Retzlaff
2022-05-12 13:14   ` [PATCH v6 2/3] eal: implement functions for get/set thread affinity Tyler Retzlaff
2022-05-12 13:14   ` [PATCH v6 3/3] test/threads: add unit test for thread API Tyler Retzlaff
2022-05-19 15:05   ` [PATCH v6 0/3] add eal functions for thread affinity and self David Marchand

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=98CBD80474FA8B44BF855DF32C47DC35D8705A@smartserver.smartshare.dk \
    --to=mb@smartsharesystems.com \
    --cc=anatoly.burakov@intel.com \
    --cc=dev@dpdk.org \
    --cc=dmitry.kozliuk@gmail.com \
    --cc=konstantin.v.ananyev@yandex.ru \
    --cc=navasile@microsoft.com \
    --cc=roretzla@linux.microsoft.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).