From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 9897BA0093; Tue, 10 May 2022 23:52:39 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7BF4E410EE; Tue, 10 May 2022 23:52:39 +0200 (CEST) Received: from forward104p.mail.yandex.net (forward104p.mail.yandex.net [77.88.28.107]) by mails.dpdk.org (Postfix) with ESMTP id F0FFC406B4 for ; Tue, 10 May 2022 23:52:37 +0200 (CEST) Received: from sas1-399a5a23b7c8.qloud-c.yandex.net (sas1-399a5a23b7c8.qloud-c.yandex.net [IPv6:2a02:6b8:c08:61a:0:640:399a:5a23]) by forward104p.mail.yandex.net (Yandex) with ESMTP id 8DCD73C1E6A2; Wed, 11 May 2022 00:52:37 +0300 (MSK) Received: from sas2-1cbd504aaa99.qloud-c.yandex.net (sas2-1cbd504aaa99.qloud-c.yandex.net [2a02:6b8:c14:7101:0:640:1cbd:504a]) by sas1-399a5a23b7c8.qloud-c.yandex.net (mxback/Yandex) with ESMTP id 3lhTqdxTqx-qahWPchL; Wed, 11 May 2022 00:52:37 +0300 X-Yandex-Fwd: 2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yandex.ru; s=mail; t=1652219557; bh=OCom+0j4nl9W5fyATpVVoTeJqJlRdFOCK1EUgELuLNA=; h=In-Reply-To:From:To:Subject:Cc:References:Date:Message-ID; b=Q2ux0t0/Svhe0pJ/UsK27/9sAWyPzF32TF3oyejbYf69rxfgHIrf6rzdnYAIbdqnq atbsa7qkM53lBu899Qflvo2EM3zK+rnKastuFaBitbWvhf4pEcAaKDzIyiF1Am5dSx SixAzuTmUgPJJKhqtN6GebXoFEljs0fMWPgtgnYg= Authentication-Results: sas1-399a5a23b7c8.qloud-c.yandex.net; dkim=pass header.i=@yandex.ru Received: by sas2-1cbd504aaa99.qloud-c.yandex.net (smtp/Yandex) with ESMTPSA id 6zYQu8efba-qZM4CNTG; Wed, 11 May 2022 00:52:36 +0300 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (Client certificate not present) Message-ID: Date: Tue, 10 May 2022 22:52:34 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 Subject: Re: [PATCH v5 1/3] eal: add basic thread ID and current thread identifier API Content-Language: en-US To: =?UTF-8?Q?Morten_Br=c3=b8rup?= , Tyler Retzlaff Cc: dev@dpdk.org, thomas@monjalon.net, dmitry.kozliuk@gmail.com, anatoly.burakov@intel.com, Narcisa Vasile References: <1648819793-18948-1-git-send-email-roretzla@linux.microsoft.com> <1651679185-23260-1-git-send-email-roretzla@linux.microsoft.com> <1651679185-23260-2-git-send-email-roretzla@linux.microsoft.com> <11c226e0-2e11-5367-8cb6-b76f471ea2a5@yandex.ru> <20220505071136.GA27566@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35D87047@smartserver.smartshare.dk> <0fa2b370-d5db-45d9-93f8-267ddc3184ec@yandex.ru> <98CBD80474FA8B44BF855DF32C47DC35D8704A@smartserver.smartshare.dk> From: Konstantin Ananyev In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D8704A@smartserver.smartshare.dk> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 >>>>>>> Signed-off-by: Tyler Retzlaff >>>>>>> Acked-by: Dmitry Kozlyuk >>>>>>> --- >>>>>>> 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 >>>>>>> + >>>>>>> #include >>>>>>> #include >>>>>>> @@ -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 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. */ 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).