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 B794A42953; Sat, 15 Apr 2023 22:52:31 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A6B60410F6; Sat, 15 Apr 2023 22:52:29 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id F2738410F1 for ; Sat, 15 Apr 2023 22:52:27 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id D3989217A96A; Sat, 15 Apr 2023 13:52:26 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com D3989217A96A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1681591946; bh=5xyUgVf2Z125GvOgaZAZWNI5YZI8KaCjsNwvJY7IU9M=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=QXrhy+k6Fr8BpPUGMrkB0QQWFUC337zI9BK6qbhcvPXic+cL+UgZILx99YiNNXEr8 99tB/kbCu/8z7/uXvJtnXndn4TMCRArphebGoUOOJNILuRitHeEDKQhlAoRiJdjQq2 m2i1405f2Qish8RE6IVHkMl7uZwMIL/rUUSE+TI4= Date: Sat, 15 Apr 2023 13:52:26 -0700 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: bruce.richardson@intel.com, david.marchand@redhat.com, thomas@monjalon.net, konstantin.ananyev@huawei.com, dev@dpdk.org Subject: Re: [PATCH v5 11/14] eal: expand most macros to empty when using MSVC Message-ID: <20230415205226.GA9699@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> <20230414170226.GA10264@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35D8787D@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: <98CBD80474FA8B44BF855DF32C47DC35D8787D@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 Sat, Apr 15, 2023 at 09:16:21AM +0200, Morten Brørup wrote: > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > Sent: Friday, 14 April 2023 19.02 > > > > 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 > > [...] > > > > > /** > > > > * 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. > > That (C11 standard _Alignas) should be the roadmap for solving the alignment requirements. > > This should be a general principle for DPDK... if the C standard offers something, don't reinvent our own. And as a consequence of the upgrade to C11, we should deprecate all our own now-obsolete substitutes for these. > > > > > 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. > > Absolutely. Catching bugs at build time is much better than any alternative! that's handy feedback. i am now encouraged to include more compile time checks in advance of or along with changes related to structure abi. follow on question, once we do get to use c11 would something like _Static_assert be preferable over RTE_BUILD_BUG_ON? structures sensitive to layout could be co-located with the asserts right at the point of definition. or is there something extra RTE_BUILD_BUG_ON gives us? > > > > > /** > > > > * 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. > > Optimally, we will have a common macro (or other solution) to support both GCC/CLANG and MSVC to replace or supplement __rte_packed. However, the cost of this may be an API break if we replace __rte_packed. > > > > > > > > > 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. > > Sounds like the right path to take. > > Now, I'm thinking ahead here... > > We should be prepared to accept a major ABI/API break at one point in time, to replace our home-grown macros with C11 standard solutions and to fully support MSVC. This is not happening anytime soon, but the Techboard should acknowledge that this is going to happen (with an unspecified release), so it can be formally announced. The sooner it is announced, the more time developers will have to prepare for it. so, just to avoid any confusion i want to make it clear that i am not planning to submit changes that would change abi as a part of supporting msvc (aside from changing to standard atomics which we agreed on). in general there are some cleanups we could make in the area of code maintainability and portability and we may want to discuss the advantages or disadvantages of making those changes. but i think those changes are a topic unrelated to windows or msvc specifically. > > All the details do not need to be known at the time of the announcement; they can be added along the way, based on the discussions from your future patches. > > > > > ty