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 2A3384333B; Wed, 15 Nov 2023 19:27:46 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BC57A402B7; Wed, 15 Nov 2023 19:27:45 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id CA5EA402B2 for ; Wed, 15 Nov 2023 19:27:43 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id EA15720B74C1; Wed, 15 Nov 2023 10:27:42 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com EA15720B74C1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1700072862; bh=v1/1YXrQiEvzfAl3H+3bUMTIHoLEX4Om61UyUEbJk0U=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=RrWBWFtwILW2joZK2/EMTAiHJUYoNXSl1WASkB/QeT4Upy0Yb8h1fSA4P35sKYUU0 caiK3Zo73gGqSs+rpIE04im/nEH0ldE6duyg2pq4ne8Lkk93GS/QbUhL0Uw2ZH7i34 1C3edG8ZdDS4ouIULiGb7KBxijow/YhiMfeQtHSQ= Date: Wed, 15 Nov 2023 10:27:42 -0800 From: Tyler Retzlaff To: Bruce Richardson Cc: dev@dpdk.org, Mattias =?iso-8859-1?Q?R=F6nnblom?= , Anatoly Burakov , David Christensen , Harry van Haaren , Konstantin Ananyev , Min Zhou , Ruifeng Wang , Stanislaw Kardach Subject: Re: [PATCH] eal: use C11 alignas instead of GCC attribute aligned Message-ID: <20231115182742.GA22439@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1700069997-4399-1-git-send-email-roretzla@linux.microsoft.com> <1700069997-4399-2-git-send-email-roretzla@linux.microsoft.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 Wed, Nov 15, 2023 at 06:13:55PM +0000, Bruce Richardson wrote: > On Wed, Nov 15, 2023 at 09:39:57AM -0800, Tyler Retzlaff wrote: > > Now that we have enabled C11 replace the use of __rte_cache_aligned > > and __rte_aligned(n) with alignas(RTE_CACHE_LINE_SIZE) and > > __rte_aligned(n) respectively. > > alignas(n) > > > > > Signed-off-by: Tyler Retzlaff > > --- > > lib/eal/arm/include/rte_vect.h | 4 +++- > > lib/eal/common/malloc_elem.h | 4 +++- > > lib/eal/common/malloc_heap.h | 4 +++- > > lib/eal/common/rte_keepalive.c | 4 +++- > > lib/eal/common/rte_random.c | 5 ++++- > > lib/eal/common/rte_service.c | 7 +++++-- > > lib/eal/include/generic/rte_atomic.h | 4 +++- > > lib/eal/loongarch/include/rte_vect.h | 7 +++++-- > > lib/eal/ppc/include/rte_vect.h | 5 ++++- > > lib/eal/riscv/include/rte_vect.h | 4 +++- > > lib/eal/x86/include/rte_vect.h | 4 +++- > > lib/eal/x86/rte_power_intrinsics.c | 8 ++++++-- > > 12 files changed, 45 insertions(+), 15 deletions(-) > > > > diff --git a/lib/eal/arm/include/rte_vect.h b/lib/eal/arm/include/rte_vect.h > > index 8cfe4bd..c7a3b2e 100644 > > --- a/lib/eal/arm/include/rte_vect.h > > +++ b/lib/eal/arm/include/rte_vect.h > > @@ -5,6 +5,7 @@ > > #ifndef _RTE_VECT_ARM_H_ > > #define _RTE_VECT_ARM_H_ > > > > +#include > > #include > > #include "generic/rte_vect.h" > > #include "rte_debug.h" > > @@ -25,13 +26,14 @@ > > #define XMM_MASK (XMM_SIZE - 1) > > > > typedef union rte_xmm { > > + alignas(16) > > xmm_t x; > > uint8_t u8[XMM_SIZE / sizeof(uint8_t)]; > > This may seem minor but I really don't like the indentation style used for > these alignas statements. To a casual glance they look like elements in the > struct. The previous macros were nice is that it was hard to mistake them > for anything other than additional info on the struct. > i'm open to whatever indentation style people choose. though as you have pointed out it might be important to be clear that the alignas is being applied to the first member. > Couple of suggestions: > 1. Put them on the same line as the definition of the first element. The > downside is that we lose the (as here) implication that it's the struct > being aligned more than just the first element. i'd be inclined to place it on the same line so we don't end up with confusion about what it is being applied to. > 2. Alternatively, how about putting the alignas on the same line as the > struct/union e.g. > > struct rte_xyz { alignas(16) > ... > } for this option what happens if there are more fields in the same struct? for the first field do we do this and then for other fields we do (1)? > > In this case, or perhaps generally, perhaps we want to define > rte_aliases with underscores for these alignas to further visually separate > them. i worry if hidden behind a macro people will continue to assume that the syntactic placement continues to be permitted anywhere __attribute__((__aligned__(a)) can go which is not the case. maybe the expansion raising a compiler error is enough though? not sure. > > Thoughts? > > /Bruce