Re: [PATCH] eal: use C11 alignas instead of GCC attribute aligned

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

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 <roretzla@linux.microsoft.com>
> > ---
> >  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 <stdalign.h>
> >  #include <stdint.h>
> >  #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