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 D95F942943; Fri, 14 Apr 2023 19:02:28 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7360A410F6; Fri, 14 Apr 2023 19:02:28 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 310EC40144 for ; Fri, 14 Apr 2023 19:02:27 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id 7C6AF217925E; Fri, 14 Apr 2023 10:02:26 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 7C6AF217925E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1681491746; bh=UoriJ71irF2wmJ1g4BWK6wzvl+0FYSzDhm/eEJDD9qM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=BZuIjX115QVbmpSFeWIBxHLKFxjGcN4fjHffpqMYWWKhDMniy9PhhDHT6fGp1wGVT krnvd81Hzfs21BfzId/aOfo4NKvqW/Gg0ZDV2JE1CScbE4lLFNlhezkD/vdkKM0Jy+ ekzn1Ulqnj4Rqz3V96eL/QvINmnzZwScx9oNt/H4= Date: Fri, 14 Apr 2023 10:02:26 -0700 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: dev@dpdk.org, bruce.richardson@intel.com, david.marchand@redhat.com, thomas@monjalon.net, konstantin.ananyev@huawei.com Subject: Re: [PATCH v5 11/14] eal: expand most macros to empty when using MSVC Message-ID: <20230414170226.GA10264@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1680558751-17931-1-git-send-email-roretzla@linux.microsoft.com> <1681421163-18578-1-git-send-email-roretzla@linux.microsoft.com> <1681421163-18578-12-git-send-email-roretzla@linux.microsoft.com> <98CBD80474FA8B44BF855DF32C47DC35D87878@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87878@smartserver.smartshare.dk> User-Agent: Mutt/1.5.21 (2010-09-15) 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 On Fri, Apr 14, 2023 at 08:45:17AM +0200, Morten Brørup wrote: > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > Sent: Thursday, 13 April 2023 23.26 > > > > For now expand a lot of common rte macros empty. The catch here is we > > need to test that most of the macros do what they should but at the same > > time they are blocking work needed to bootstrap of the unit tests. > > > > Later we will return and provide (where possible) expansions that work > > correctly for msvc and where not possible provide some alternate macros > > to achieve the same outcome. > > > > Signed-off-by: Tyler Retzlaff > > --- > > lib/eal/include/rte_branch_prediction.h | 8 ++++++ > > lib/eal/include/rte_common.h | 45 > > +++++++++++++++++++++++++++++++++ > > lib/eal/include/rte_compat.h | 20 +++++++++++++++ > > 3 files changed, 73 insertions(+) > > > > diff --git a/lib/eal/include/rte_branch_prediction.h > > b/lib/eal/include/rte_branch_prediction.h > > index 0256a9d..d9a0224 100644 > > --- a/lib/eal/include/rte_branch_prediction.h > > +++ b/lib/eal/include/rte_branch_prediction.h > > @@ -25,7 +25,11 @@ > > * > > */ > > #ifndef likely > > +#ifndef RTE_TOOLCHAIN_MSVC > > #define likely(x) __builtin_expect(!!(x), 1) > > +#else > > +#define likely(x) (x) > > This must be (!!(x)), because x may be non-Boolean, e.g. likely(n & 0x10), and likely() must return Boolean (0 or 1). yes, you're right. will fix. > > > +#endif > > #endif /* likely */ > > > > /** > > @@ -39,7 +43,11 @@ > > * > > */ > > #ifndef unlikely > > +#ifndef RTE_TOOLCHAIN_MSVC > > #define unlikely(x) __builtin_expect(!!(x), 0) > > +#else > > +#define unlikely(x) (x) > > This must also be (!!(x)), for the same reason as above. ack > > > +#endif > > #endif /* unlikely */ > > > > #ifdef __cplusplus > > diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h > > index 2f464e3..1bdaa2d 100644 > > --- a/lib/eal/include/rte_common.h > > +++ b/lib/eal/include/rte_common.h > > @@ -65,7 +65,11 @@ > > /** > > * Force alignment > > */ > > +#ifndef RTE_TOOLCHAIN_MSVC > > #define __rte_aligned(a) __attribute__((__aligned__(a))) > > +#else > > +#define __rte_aligned(a) > > +#endif > > It should be reviewed that __rte_aligned() is only used for optimization purposes, and is not required for DPDK to function properly. so to expand on what i have in mind (and explain why i leave it expanded empty for now) while msvc has a __declspec for align there is a mismatch between where gcc and msvc want it placed to control alignment of objects. msvc support won't be functional in 23.07 because of atomics. so once we reach the 23.11 cycle (where we can merge c11 changes) it means we can also use standard _Alignas which can accomplish the same thing but portably. full disclosure the catch is i still have to properly locate the that does the alignment and some small questions about the expansion and use of the existing macro. on the subject of DPDK requiring proper alignment, you're right it is generally for performance but also for pre-c11 atomics. one question i have been asking myself is would the community see value in more compile time assertions / testing of the size and alignment of structures and offset of structure fields? we have a few key RTE_BUILD_BUG_ON() assertions but i've discovered they don't offer comprehensive protection. > > > > > #ifdef RTE_ARCH_STRICT_ALIGN > > typedef uint64_t unaligned_uint64_t __rte_aligned(1); > > @@ -80,16 +84,29 @@ > > /** > > * Force a structure to be packed > > */ > > +#ifndef RTE_TOOLCHAIN_MSVC > > #define __rte_packed __attribute__((__packed__)) > > +#else > > +#define __rte_packed > > +#endif > > Similar comment as for __rte_aligned(); however, I consider it more likely that structure packing is a functional requirement, and not just used for optimization. Based on my experience, it may be used for packing network structures; perhaps not in DPDK itself but maybe in DPDK applications. so interestingly i've discovered this is kind of a mess and as you note some places we can't just "fix" it for abi compatibility reasons. in some instances the packing is being applied to structures where it is essentially a noop. i.e. natural alignment gets you the same thing so it is superfluous. in some instances the packing is being applied to structures that are private and it appears to be completely unnecessary e.g. some structure that isn't nested into something else and sizeof() or offsetof() fields don't matter in the context of their use. in some instances it is completely necessary usually when type punning buffers containing network framing etc... unfortunately the standard doesn't offer me an out here as there is an issue of placement of the pragma/attributes that do the packing. for places it isn't needed it, whatever i just expand empty. for places it is superfluous again because msvc has no stable abi (we're not established yet) again i just expand empty. finally for the places where it is needed i'll probably need to expand conditionally but i think the instances are far fewer than current use. > > The same risk applies to __rte_aligned(), but with lower probability. so that's the long winded story of why they are both expanded empty for now for msvc. but when the time comes i want to submit patch series that focus on each specifically to generate robust discussion. ty