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 3A64A4294B; Sat, 15 Apr 2023 09:16:26 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BBA9240A7A; Sat, 15 Apr 2023 09:16:25 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id B89ED40697 for ; Sat, 15 Apr 2023 09:16:24 +0200 (CEST) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH v5 11/14] eal: expand most macros to empty when using MSVC Date: Sat, 15 Apr 2023 09:16:21 +0200 X-MimeOLE: Produced By Microsoft Exchange V6.5 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D8787D@smartserver.smartshare.dk> In-Reply-To: <20230414170226.GA10264@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v5 11/14] eal: expand most macros to empty when using MSVC Thread-Index: Adlu8uSwrmxbZnl0TT6PQAZIGm94IQAbxVYA 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> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Tyler Retzlaff" , , , , Cc: 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 > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > Sent: Friday, 14 April 2023 19.02 >=20 > On Fri, Apr 14, 2023 at 08:45:17AM +0200, Morten Br=F8rup 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. >=20 > so to expand on what i have in mind (and explain why i leave it = expanded > empty for now) >=20 > while msvc has a __declspec for align there is a mismatch between > where gcc and msvc want it placed to control alignment of objects. >=20 > 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. >=20 > 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. >=20 > on the subject of DPDK requiring proper alignment, you're right it > is generally for performance but also for pre-c11 atomics. >=20 > 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! > > > /** > > > * 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > in some instances it is completely necessary usually when type punning > buffers containing network framing etc... >=20 > 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. >=20 > 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. >=20 > > > > The same risk applies to __rte_aligned(), but with lower = probability. >=20 > 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. 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. >=20 > ty