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 84996A050A; Sat, 7 May 2022 15:57:58 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 237A74068A; Sat, 7 May 2022 15:57:58 +0200 (CEST) Received: from forward501j.mail.yandex.net (forward501j.mail.yandex.net [5.45.198.251]) by mails.dpdk.org (Postfix) with ESMTP id B1E6840395 for ; Sat, 7 May 2022 15:57:56 +0200 (CEST) Received: from sas2-bcc9105f7ecc.qloud-c.yandex.net (sas2-bcc9105f7ecc.qloud-c.yandex.net [IPv6:2a02:6b8:c08:b7ac:0:640:bcc9:105f]) by forward501j.mail.yandex.net (Yandex) with ESMTP id EBCBA623187; Sat, 7 May 2022 16:57:55 +0300 (MSK) Received: from sas1-1f4a002bb12a.qloud-c.yandex.net (sas1-1f4a002bb12a.qloud-c.yandex.net [2a02:6b8:c14:3908:0:640:1f4a:2b]) by sas2-bcc9105f7ecc.qloud-c.yandex.net (mxback/Yandex) with ESMTP id SBS8lYOI3B-vtgemfvQ; Sat, 07 May 2022 16:57:55 +0300 X-Yandex-Fwd: 2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yandex.ru; s=mail; t=1651931875; bh=Gim6mQ5xTCjYJUyHpxOBYA7kXZRRwTW46kXK1vUbRSM=; h=In-Reply-To:From:Subject:Cc:References:Date:Message-ID:To; b=BebEOJWHbpQ4VQPUtLqc0Gc0yI/VG0F8tLDKr68/1cnSGps9rnkz+GsBBNkK5I6nr tX7VTkW7yVb6fjVoJbZEV+VlWrmZpmazupsAmUMuthpPIiSLZwkrkA02Z0lrLcQKgW +swvkBQSVuQS//2Yb7iGCYnmoY6xVQLaR5VNVRLM= Authentication-Results: sas2-bcc9105f7ecc.qloud-c.yandex.net; dkim=pass header.i=@yandex.ru Received: by sas1-1f4a002bb12a.qloud-c.yandex.net (smtp/Yandex) with ESMTPSA id tNwT77uDgj-vrMmsrvR; Sat, 07 May 2022 16:57:54 +0300 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (Client certificate not present) Message-ID: <0fa2b370-d5db-45d9-93f8-267ddc3184ec@yandex.ru> Date: Sat, 7 May 2022 14:57:52 +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> From: Konstantin Ananyev In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87047@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 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. > >> >>> * 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. >> > > 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 > >> >>> >>>> >>>> >>>>> +} 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 >>>>> @@ -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 *)) >> >