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

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

[Tyler Retzlaff]

Now that we require a C11 conformant toolchain we are able to improve
portability by further adoption of C11 features.

Adapt EAL to use C11 alignas replacing __rte_cache_aligned and
__rte_aligned(a) that expand to __attribute__((__aligned__(a))).

Note: it appears that use of alignas has exposed a bug in
      lib/eal/riscv/include/rte_vect.h where the alignment
      specified was reduced to 8 for xmm_t.

Please comment, subject to the outcome I will submit further series for
lib/*

Thanks

Tyler Retzlaff (1):
  eal: use C11 alignas instead of GCC attribute aligned

 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(-)

-- 
1.8.3.1

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

[Tyler Retzlaff]

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.

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)];
 	uint16_t u16[XMM_SIZE / sizeof(uint16_t)];
 	uint32_t u32[XMM_SIZE / sizeof(uint32_t)];
 	uint64_t u64[XMM_SIZE / sizeof(uint64_t)];
 	double   pd[XMM_SIZE / sizeof(double)];
-} __rte_aligned(16) rte_xmm_t;
+} rte_xmm_t;
 
 #if defined(RTE_ARCH_ARM) && defined(RTE_ARCH_32)
 /* NEON intrinsic vqtbl1q_u8() is not supported in ARMv7-A(AArch32) */
diff --git a/lib/eal/common/malloc_elem.h b/lib/eal/common/malloc_elem.h
index 952ce73..c2c336e 100644
--- a/lib/eal/common/malloc_elem.h
+++ b/lib/eal/common/malloc_elem.h
@@ -5,6 +5,7 @@
 #ifndef MALLOC_ELEM_H_
 #define MALLOC_ELEM_H_
 
+#include <stdalign.h>
 #include <stdbool.h>
 
 #include <rte_common.h>
@@ -21,6 +22,7 @@ enum elem_state {
 };
 
 struct malloc_elem {
+	alignas(RTE_CACHE_LINE_SIZE)
 	struct malloc_heap *heap;
 	struct malloc_elem *volatile prev;
 	/**< points to prev elem in memseg */
@@ -48,7 +50,7 @@ struct malloc_elem {
 	size_t user_size;
 	uint64_t asan_cookie[2]; /* must be next to header_cookie */
 #endif
-} __rte_cache_aligned;
+};
 
 static const unsigned int MALLOC_ELEM_HEADER_LEN = sizeof(struct malloc_elem);
 
diff --git a/lib/eal/common/malloc_heap.h b/lib/eal/common/malloc_heap.h
index 8f3ab57..a724bfb 100644
--- a/lib/eal/common/malloc_heap.h
+++ b/lib/eal/common/malloc_heap.h
@@ -5,6 +5,7 @@
 #ifndef MALLOC_HEAP_H_
 #define MALLOC_HEAP_H_
 
+#include <stdalign.h>
 #include <stdbool.h>
 #include <sys/queue.h>
 
@@ -22,6 +23,7 @@
  * Structure to hold malloc heap
  */
 struct malloc_heap {
+	alignas(RTE_CACHE_LINE_SIZE)
 	rte_spinlock_t lock;
 	LIST_HEAD(, malloc_elem) free_head[RTE_HEAP_NUM_FREELISTS];
 	struct malloc_elem *volatile first;
@@ -31,7 +33,7 @@ struct malloc_heap {
 	unsigned int socket_id;
 	size_t total_size;
 	char name[RTE_HEAP_NAME_MAX_LEN];
-} __rte_cache_aligned;
+};
 
 void *
 malloc_heap_alloc(const char *type, size_t size, int socket, unsigned int flags,
diff --git a/lib/eal/common/rte_keepalive.c b/lib/eal/common/rte_keepalive.c
index e0494b2..67a898d 100644
--- a/lib/eal/common/rte_keepalive.c
+++ b/lib/eal/common/rte_keepalive.c
@@ -3,6 +3,7 @@
  */
 
 #include <inttypes.h>
+#include <stdalign.h>
 
 #include <rte_common.h>
 #include <rte_cycles.h>
@@ -17,7 +18,8 @@ struct rte_keepalive {
 		/*
 		 * Each element must be cache aligned to prevent false sharing.
 		 */
-		enum rte_keepalive_state core_state __rte_cache_aligned;
+		alignas(RTE_CACHE_LINE_SIZE)
+		enum rte_keepalive_state core_state;
 	} live_data[RTE_KEEPALIVE_MAXCORES];
 
 	/** Last-seen-alive timestamps */
diff --git a/lib/eal/common/rte_random.c b/lib/eal/common/rte_random.c
index 7709b8f..c04917e 100644
--- a/lib/eal/common/rte_random.c
+++ b/lib/eal/common/rte_random.c
@@ -2,6 +2,8 @@
  * Copyright(c) 2019 Ericsson AB
  */
 
+#include <stdalign.h>
+
 #ifdef __RDSEED__
 #include <x86intrin.h>
 #endif
@@ -14,13 +16,14 @@
 #include <rte_random.h>
 
 struct rte_rand_state {
+	alignas(RTE_CACHE_LINE_SIZE)
 	uint64_t z1;
 	uint64_t z2;
 	uint64_t z3;
 	uint64_t z4;
 	uint64_t z5;
 	RTE_CACHE_GUARD;
-} __rte_cache_aligned;
+};
 
 /* One instance each for every lcore id-equipped thread, and one
  * additional instance to be shared by all others threads (i.e., all
diff --git a/lib/eal/common/rte_service.c b/lib/eal/common/rte_service.c
index e183d2e..861ae31 100644
--- a/lib/eal/common/rte_service.c
+++ b/lib/eal/common/rte_service.c
@@ -2,6 +2,7 @@
  * Copyright(c) 2017 Intel Corporation
  */
 
+#include <stdalign.h>
 #include <stdio.h>
 #include <inttypes.h>
 #include <string.h>
@@ -33,6 +34,7 @@
 
 /* internal representation of a service */
 struct rte_service_spec_impl {
+	alignas(RTE_CACHE_LINE_SIZE)
 	/* public part of the struct */
 	struct rte_service_spec spec;
 
@@ -53,7 +55,7 @@ struct rte_service_spec_impl {
 	 * on currently.
 	 */
 	RTE_ATOMIC(uint32_t) num_mapped_cores;
-} __rte_cache_aligned;
+};
 
 struct service_stats {
 	RTE_ATOMIC(uint64_t) calls;
@@ -62,6 +64,7 @@ struct service_stats {
 
 /* the internal values of a service core */
 struct core_state {
+	alignas(RTE_CACHE_LINE_SIZE)
 	/* map of services IDs are run on this core */
 	uint64_t service_mask;
 	RTE_ATOMIC(uint8_t) runstate; /* running or stopped */
@@ -71,7 +74,7 @@ struct core_state {
 	RTE_ATOMIC(uint64_t) loops;
 	RTE_ATOMIC(uint64_t) cycles;
 	struct service_stats service_stats[RTE_SERVICE_NUM_MAX];
-} __rte_cache_aligned;
+};
 
 static uint32_t rte_service_count;
 static struct rte_service_spec_impl *rte_services;
diff --git a/lib/eal/include/generic/rte_atomic.h b/lib/eal/include/generic/rte_atomic.h
index 0e639da..bc9213c 100644
--- a/lib/eal/include/generic/rte_atomic.h
+++ b/lib/eal/include/generic/rte_atomic.h
@@ -12,6 +12,7 @@
  * This file defines a generic API for atomic operations.
  */
 
+#include <stdalign.h>
 #include <stdint.h>
 
 #include <rte_common.h>
@@ -1096,6 +1097,7 @@ static inline void rte_atomic64_clear(rte_atomic64_t *v)
  */
 typedef struct {
 	union {
+		alignas(16)
 		uint64_t val[2];
 #ifdef RTE_ARCH_64
 #ifndef RTE_TOOLCHAIN_MSVC
@@ -1103,7 +1105,7 @@ static inline void rte_atomic64_clear(rte_atomic64_t *v)
 #endif
 #endif
 	};
-} __rte_aligned(16) rte_int128_t;
+} rte_int128_t;
 
 #ifdef __DOXYGEN__
 
diff --git a/lib/eal/loongarch/include/rte_vect.h b/lib/eal/loongarch/include/rte_vect.h
index 1546515..856d87b 100644
--- a/lib/eal/loongarch/include/rte_vect.h
+++ b/lib/eal/loongarch/include/rte_vect.h
@@ -5,6 +5,7 @@
 #ifndef RTE_VECT_LOONGARCH_H
 #define RTE_VECT_LOONGARCH_H
 
+#include <stdalign.h>
 #include <stdint.h>
 #include "generic/rte_vect.h"
 #include "rte_common.h"
@@ -16,6 +17,7 @@
 #define RTE_VECT_DEFAULT_SIMD_BITWIDTH RTE_VECT_SIMD_DISABLED
 
 typedef union xmm {
+	alignas(16)
 	int8_t   i8[16];
 	int16_t  i16[8];
 	int32_t  i32[4];
@@ -25,19 +27,20 @@
 	uint32_t u32[4];
 	uint64_t u64[2];
 	double   pd[2];
-} __rte_aligned(16) xmm_t;
+} xmm_t;
 
 #define XMM_SIZE        (sizeof(xmm_t))
 #define XMM_MASK        (XMM_SIZE - 1)
 
 typedef union rte_xmm {
+	alignas(16)
 	xmm_t	 x;
 	uint8_t	 u8[XMM_SIZE / sizeof(uint8_t)];
 	uint16_t u16[XMM_SIZE / sizeof(uint16_t)];
 	uint32_t u32[XMM_SIZE / sizeof(uint32_t)];
 	uint64_t u64[XMM_SIZE / sizeof(uint64_t)];
 	double   pd[XMM_SIZE / sizeof(double)];
-} __rte_aligned(16) rte_xmm_t;
+} rte_xmm_t;
 
 static inline xmm_t
 vect_load_128(void *p)
diff --git a/lib/eal/ppc/include/rte_vect.h b/lib/eal/ppc/include/rte_vect.h
index a5f009b..e6702a4 100644
--- a/lib/eal/ppc/include/rte_vect.h
+++ b/lib/eal/ppc/include/rte_vect.h
@@ -6,6 +6,8 @@
 #ifndef _RTE_VECT_PPC_64_H_
 #define _RTE_VECT_PPC_64_H_
 
+#include <stdalign.h>
+
 #include "rte_altivec.h"
 
 #include "generic/rte_vect.h"
@@ -23,13 +25,14 @@
 #define	XMM_MASK	(XMM_SIZE - 1)
 
 typedef union rte_xmm {
+	alignas(16)
 	xmm_t    x;
 	uint8_t  u8[XMM_SIZE / sizeof(uint8_t)];
 	uint16_t u16[XMM_SIZE / sizeof(uint16_t)];
 	uint32_t u32[XMM_SIZE / sizeof(uint32_t)];
 	uint64_t u64[XMM_SIZE / sizeof(uint64_t)];
 	double   pd[XMM_SIZE / sizeof(double)];
-} __rte_aligned(16) rte_xmm_t;
+} rte_xmm_t;
 
 #ifdef __cplusplus
 }
diff --git a/lib/eal/riscv/include/rte_vect.h b/lib/eal/riscv/include/rte_vect.h
index 2f97f43..32d4386 100644
--- a/lib/eal/riscv/include/rte_vect.h
+++ b/lib/eal/riscv/include/rte_vect.h
@@ -7,6 +7,7 @@
 #ifndef RTE_VECT_RISCV_H
 #define RTE_VECT_RISCV_H
 
+#include <stdalign.h>
 #include <stdint.h>
 #include "generic/rte_vect.h"
 #include "rte_common.h"
@@ -23,13 +24,14 @@
 #define XMM_MASK	(XMM_SIZE - 1)
 
 typedef union rte_xmm {
+	alignas(16) /* !! NOTE !! changed to 16 it looks like this was a bug? */
 	xmm_t		x;
 	uint8_t		u8[XMM_SIZE / sizeof(uint8_t)];
 	uint16_t	u16[XMM_SIZE / sizeof(uint16_t)];
 	uint32_t	u32[XMM_SIZE / sizeof(uint32_t)];
 	uint64_t	u64[XMM_SIZE / sizeof(uint64_t)];
 	double		pd[XMM_SIZE / sizeof(double)];
-} __rte_aligned(8) rte_xmm_t;
+} rte_xmm_t;
 
 static inline xmm_t
 vect_load_128(void *p)
diff --git a/lib/eal/x86/include/rte_vect.h b/lib/eal/x86/include/rte_vect.h
index 560f9e4..2e5669d 100644
--- a/lib/eal/x86/include/rte_vect.h
+++ b/lib/eal/x86/include/rte_vect.h
@@ -11,6 +11,7 @@
  * RTE SSE/AVX related header.
  */
 
+#include <stdalign.h>
 #include <stdint.h>
 #include <rte_config.h>
 #include <rte_common.h>
@@ -92,6 +93,7 @@
 #define RTE_X86_ZMM_MASK	(RTE_X86_ZMM_SIZE - 1)
 
 typedef union __rte_x86_zmm {
+	alignas(RTE_X86_ZMM_SIZE)
 	__m512i	 z;
 	ymm_t    y[RTE_X86_ZMM_SIZE / sizeof(ymm_t)];
 	xmm_t    x[RTE_X86_ZMM_SIZE / sizeof(xmm_t)];
@@ -100,7 +102,7 @@
 	uint32_t u32[RTE_X86_ZMM_SIZE / sizeof(uint32_t)];
 	uint64_t u64[RTE_X86_ZMM_SIZE / sizeof(uint64_t)];
 	double   pd[RTE_X86_ZMM_SIZE / sizeof(double)];
-} __rte_aligned(RTE_X86_ZMM_SIZE) __rte_x86_zmm_t;
+} __rte_x86_zmm_t;
 
 #endif /* __AVX512F__ */
 
diff --git a/lib/eal/x86/rte_power_intrinsics.c b/lib/eal/x86/rte_power_intrinsics.c
index 532a2e6..5636543 100644
--- a/lib/eal/x86/rte_power_intrinsics.c
+++ b/lib/eal/x86/rte_power_intrinsics.c
@@ -2,6 +2,8 @@
  * Copyright(c) 2020 Intel Corporation
  */
 
+#include <stdalign.h>
+
 #include <rte_common.h>
 #include <rte_lcore.h>
 #include <rte_rtm.h>
@@ -13,9 +15,10 @@
  * Per-lcore structure holding current status of C0.2 sleeps.
  */
 static struct power_wait_status {
+	alignas(RTE_CACHE_LINE_SIZE)
 	rte_spinlock_t lock;
 	volatile void *monitor_addr; /**< NULL if not currently sleeping */
-} __rte_cache_aligned wait_status[RTE_MAX_LCORE];
+} wait_status[RTE_MAX_LCORE];
 
 /*
  * This function uses UMONITOR/UMWAIT instructions and will enter C0.2 state.
@@ -86,9 +89,10 @@ static void amd_mwaitx(const uint64_t timeout)
 }
 
 static struct {
+	alignas(RTE_CACHE_LINE_SIZE)
 	void (*mmonitor)(volatile void *addr);
 	void (*mwait)(const uint64_t timeout);
-} __rte_cache_aligned power_monitor_ops;
+} power_monitor_ops;
 
 static inline void
 __umwait_wakeup(volatile void *addr)
-- 
1.8.3.1

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

[Bruce Richardson]

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.

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.
2. Alternatively, how about putting the alignas on the same line as the
   struct/union e.g.

	struct rte_xyz {   alignas(16)
		...
	}

   In this case, or perhaps generally, perhaps we want to define
rte_aliases with underscores for these alignas to further visually separate
them.

Thoughts?

/Bruce

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

[Tyler Retzlaff]

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

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

[Morten Brørup]

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Wednesday, 15 November 2023 18.40
> 
> 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.
> 

[...]

>  typedef union rte_xmm {
> +	alignas(16)
>  	xmm_t    x;
>  	uint8_t  u8[XMM_SIZE / sizeof(uint8_t)];
>  	uint16_t u16[XMM_SIZE / sizeof(uint16_t)];
>  	uint32_t u32[XMM_SIZE / sizeof(uint32_t)];
>  	uint64_t u64[XMM_SIZE / sizeof(uint64_t)];
>  	double   pd[XMM_SIZE / sizeof(double)];
> -} __rte_aligned(16) rte_xmm_t;
> +} rte_xmm_t;

Your patch message should mention that C11 doesn't allow alignas() being applied to the declarations of struct/union types, so it is applied to the first field in the struct/union, which has the same effect.

Someone unfamiliar with alignas() would expect:

-typedef union rte_xmm {
+typedef alignas(16) union rte_xmm {
[...]
-} __rte_aligned(16) rte_xmm_t;
+} rte_xmm_t;

[...]

>  #ifndef RTE_VECT_RISCV_H
>  #define RTE_VECT_RISCV_H
> 
> +#include <stdalign.h>
>  #include <stdint.h>
>  #include "generic/rte_vect.h"
>  #include "rte_common.h"
> @@ -23,13 +24,14 @@
>  #define XMM_MASK	(XMM_SIZE - 1)
> 
>  typedef union rte_xmm {
> +	alignas(16) /* !! NOTE !! changed to 16 it looks like this was a
> bug? */
>  	xmm_t		x;
>  	uint8_t		u8[XMM_SIZE / sizeof(uint8_t)];
>  	uint16_t	u16[XMM_SIZE / sizeof(uint16_t)];
>  	uint32_t	u32[XMM_SIZE / sizeof(uint32_t)];
>  	uint64_t	u64[XMM_SIZE / sizeof(uint64_t)];
>  	double		pd[XMM_SIZE / sizeof(double)];
> -} __rte_aligned(8) rte_xmm_t;
> +} rte_xmm_t;

Yes, this looks very much like a bug.
Even if a RISC-V CPU could handle alignment like that, it might interact with other software/hardware expecting type-sized alignment, i.e. 16-byte alignment, so partially using 8-byte alignment would cause bugs.

It should be a separate patch with a Fixes tag.

We need to urgently decide if this bug should live on in DPDK 23.11, or if the fix should be included although we are very late in the release process.

Stanislaw, what do you think?

Furthermore, I wonder if it can be backported to stable, and to what extent backporting it would break the ABI/API.

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

[Tyler Retzlaff]

On Wed, Nov 15, 2023 at 09:08:05PM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Wednesday, 15 November 2023 18.40
> > 
> > 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.
> > 
> 
> [...]
> 
> >  typedef union rte_xmm {
> > +	alignas(16)
> >  	xmm_t    x;
> >  	uint8_t  u8[XMM_SIZE / sizeof(uint8_t)];
> >  	uint16_t u16[XMM_SIZE / sizeof(uint16_t)];
> >  	uint32_t u32[XMM_SIZE / sizeof(uint32_t)];
> >  	uint64_t u64[XMM_SIZE / sizeof(uint64_t)];
> >  	double   pd[XMM_SIZE / sizeof(double)];
> > -} __rte_aligned(16) rte_xmm_t;
> > +} rte_xmm_t;
> 
> Your patch message should mention that C11 doesn't allow alignas() being applied to the declarations of struct/union types, so it is applied to the first field in the struct/union, which has the same effect.

no problem, will add a note.

> 
> Someone unfamiliar with alignas() would expect:
> 
> -typedef union rte_xmm {
> +typedef alignas(16) union rte_xmm {
> [...]
> -} __rte_aligned(16) rte_xmm_t;
> +} rte_xmm_t;
> 
> [...]
> 
> >  #ifndef RTE_VECT_RISCV_H
> >  #define RTE_VECT_RISCV_H
> > 
> > +#include <stdalign.h>
> >  #include <stdint.h>
> >  #include "generic/rte_vect.h"
> >  #include "rte_common.h"
> > @@ -23,13 +24,14 @@
> >  #define XMM_MASK	(XMM_SIZE - 1)
> > 
> >  typedef union rte_xmm {
> > +	alignas(16) /* !! NOTE !! changed to 16 it looks like this was a
> > bug? */
> >  	xmm_t		x;
> >  	uint8_t		u8[XMM_SIZE / sizeof(uint8_t)];
> >  	uint16_t	u16[XMM_SIZE / sizeof(uint16_t)];
> >  	uint32_t	u32[XMM_SIZE / sizeof(uint32_t)];
> >  	uint64_t	u64[XMM_SIZE / sizeof(uint64_t)];
> >  	double		pd[XMM_SIZE / sizeof(double)];
> > -} __rte_aligned(8) rte_xmm_t;
> > +} rte_xmm_t;
> 
> Yes, this looks very much like a bug.
> Even if a RISC-V CPU could handle alignment like that, it might interact with other software/hardware expecting type-sized alignment, i.e. 16-byte alignment, so partially using 8-byte alignment would cause bugs.
> 
> It should be a separate patch with a Fixes tag.

i'll submit a patch/fix for this so it is available and others can
discuss if it should or shouldn't be merged for 23.11.

> 
> We need to urgently decide if this bug should live on in DPDK 23.11, or if the fix should be included although we are very late in the release process.
> 
> Stanislaw, what do you think?
> 
> Furthermore, I wonder if it can be backported to stable, and to what extent backporting it would break the ABI/API.
> 

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

[Stanisław Kardach]

On Wed, Nov 15, 2023 at 10:03 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> On Wed, Nov 15, 2023 at 09:08:05PM +0100, Morten Brørup wrote:
> > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > Sent: Wednesday, 15 November 2023 18.40
> > >
> > > 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.
> > >
> >
> > [...]
> >
> > >  typedef union rte_xmm {
> > > +   alignas(16)
> > >     xmm_t    x;
> > >     uint8_t  u8[XMM_SIZE / sizeof(uint8_t)];
> > >     uint16_t u16[XMM_SIZE / sizeof(uint16_t)];
> > >     uint32_t u32[XMM_SIZE / sizeof(uint32_t)];
> > >     uint64_t u64[XMM_SIZE / sizeof(uint64_t)];
> > >     double   pd[XMM_SIZE / sizeof(double)];
> > > -} __rte_aligned(16) rte_xmm_t;
> > > +} rte_xmm_t;
> >
> > Your patch message should mention that C11 doesn't allow alignas() being applied to the declarations of struct/union types, so it is applied to the first field in the struct/union, which has the same effect.
>
> no problem, will add a note.
>
> >
> > Someone unfamiliar with alignas() would expect:
> >
> > -typedef union rte_xmm {
> > +typedef alignas(16) union rte_xmm {
> > [...]
> > -} __rte_aligned(16) rte_xmm_t;
> > +} rte_xmm_t;
> >
> > [...]
> >
> > >  #ifndef RTE_VECT_RISCV_H
> > >  #define RTE_VECT_RISCV_H
> > >
> > > +#include <stdalign.h>
> > >  #include <stdint.h>
> > >  #include "generic/rte_vect.h"
> > >  #include "rte_common.h"
> > > @@ -23,13 +24,14 @@
> > >  #define XMM_MASK   (XMM_SIZE - 1)
> > >
> > >  typedef union rte_xmm {
> > > +   alignas(16) /* !! NOTE !! changed to 16 it looks like this was a
> > > bug? */
> > >     xmm_t           x;
> > >     uint8_t         u8[XMM_SIZE / sizeof(uint8_t)];
> > >     uint16_t        u16[XMM_SIZE / sizeof(uint16_t)];
> > >     uint32_t        u32[XMM_SIZE / sizeof(uint32_t)];
> > >     uint64_t        u64[XMM_SIZE / sizeof(uint64_t)];
> > >     double          pd[XMM_SIZE / sizeof(double)];
> > > -} __rte_aligned(8) rte_xmm_t;
> > > +} rte_xmm_t;
> >
> > Yes, this looks very much like a bug.
> > Even if a RISC-V CPU could handle alignment like that, it might interact with other software/hardware expecting type-sized alignment, i.e. 16-byte alignment, so partially using 8-byte alignment would cause bugs.
> >
> > It should be a separate patch with a Fixes tag.
>
> i'll submit a patch/fix for this so it is available and others can
> discuss if it should or shouldn't be merged for 23.11.
It is definitely a bug. Good catch. Since we did not have vector
extensions on our bring-up board, all xmm_t handling was essentially
scalar.
>
> >
> > We need to urgently decide if this bug should live on in DPDK 23.11, or if the fix should be included although we are very late in the release process.
> >
> > Stanislaw, what do you think?
> >
> > Furthermore, I wonder if it can be backported to stable, and to what extent backporting it would break the ABI/API.
> >



-- 
Best Regards,
Stanisław Kardach

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

[Mattias Rönnblom]

On 2023-11-15 18:39, 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.
> 
> 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)];
>   	uint16_t u16[XMM_SIZE / sizeof(uint16_t)];
>   	uint32_t u32[XMM_SIZE / sizeof(uint32_t)];
>   	uint64_t u64[XMM_SIZE / sizeof(uint64_t)];
>   	double   pd[XMM_SIZE / sizeof(double)];
> -} __rte_aligned(16) rte_xmm_t;
> +} rte_xmm_t;
>   
>   #if defined(RTE_ARCH_ARM) && defined(RTE_ARCH_32)
>   /* NEON intrinsic vqtbl1q_u8() is not supported in ARMv7-A(AArch32) */
> diff --git a/lib/eal/common/malloc_elem.h b/lib/eal/common/malloc_elem.h
> index 952ce73..c2c336e 100644
> --- a/lib/eal/common/malloc_elem.h
> +++ b/lib/eal/common/malloc_elem.h
> @@ -5,6 +5,7 @@
>   #ifndef MALLOC_ELEM_H_
>   #define MALLOC_ELEM_H_
>   
> +#include <stdalign.h>
>   #include <stdbool.h>
>   
>   #include <rte_common.h>
> @@ -21,6 +22,7 @@ enum elem_state {
>   };
>   
>   struct malloc_elem {
> +	alignas(RTE_CACHE_LINE_SIZE)
>   	struct malloc_heap *heap;
>   	struct malloc_elem *volatile prev;
>   	/**< points to prev elem in memseg */
> @@ -48,7 +50,7 @@ struct malloc_elem {
>   	size_t user_size;
>   	uint64_t asan_cookie[2]; /* must be next to header_cookie */
>   #endif
> -} __rte_cache_aligned;
> +};
>   
>   static const unsigned int MALLOC_ELEM_HEADER_LEN = sizeof(struct malloc_elem);
>   
> diff --git a/lib/eal/common/malloc_heap.h b/lib/eal/common/malloc_heap.h
> index 8f3ab57..a724bfb 100644
> --- a/lib/eal/common/malloc_heap.h
> +++ b/lib/eal/common/malloc_heap.h
> @@ -5,6 +5,7 @@
>   #ifndef MALLOC_HEAP_H_
>   #define MALLOC_HEAP_H_
>   
> +#include <stdalign.h>
>   #include <stdbool.h>
>   #include <sys/queue.h>
>   
> @@ -22,6 +23,7 @@
>    * Structure to hold malloc heap
>    */
>   struct malloc_heap {
> +	alignas(RTE_CACHE_LINE_SIZE)
>   	rte_spinlock_t lock;
>   	LIST_HEAD(, malloc_elem) free_head[RTE_HEAP_NUM_FREELISTS];
>   	struct malloc_elem *volatile first;
> @@ -31,7 +33,7 @@ struct malloc_heap {
>   	unsigned int socket_id;
>   	size_t total_size;
>   	char name[RTE_HEAP_NAME_MAX_LEN];
> -} __rte_cache_aligned;
> +};
>   
>   void *
>   malloc_heap_alloc(const char *type, size_t size, int socket, unsigned int flags,
> diff --git a/lib/eal/common/rte_keepalive.c b/lib/eal/common/rte_keepalive.c
> index e0494b2..67a898d 100644
> --- a/lib/eal/common/rte_keepalive.c
> +++ b/lib/eal/common/rte_keepalive.c
> @@ -3,6 +3,7 @@
>    */
>   
>   #include <inttypes.h>
> +#include <stdalign.h>
>   
>   #include <rte_common.h>
>   #include <rte_cycles.h>
> @@ -17,7 +18,8 @@ struct rte_keepalive {
>   		/*
>   		 * Each element must be cache aligned to prevent false sharing.
>   		 */
> -		enum rte_keepalive_state core_state __rte_cache_aligned;
> +		alignas(RTE_CACHE_LINE_SIZE)
> +		enum rte_keepalive_state core_state;
>   	} live_data[RTE_KEEPALIVE_MAXCORES];
>   
>   	/** Last-seen-alive timestamps */
> diff --git a/lib/eal/common/rte_random.c b/lib/eal/common/rte_random.c
> index 7709b8f..c04917e 100644
> --- a/lib/eal/common/rte_random.c
> +++ b/lib/eal/common/rte_random.c
> @@ -2,6 +2,8 @@
>    * Copyright(c) 2019 Ericsson AB
>    */
>   
> +#include <stdalign.h>
> +
>   #ifdef __RDSEED__
>   #include <x86intrin.h>
>   #endif
> @@ -14,13 +16,14 @@
>   #include <rte_random.h>
>   
>   struct rte_rand_state {
> +	alignas(RTE_CACHE_LINE_SIZE)
>   	uint64_t z1;

Formatting convention question: the alignas(n) and the field shouldn't 
be on the same line?

It could be useful to have a macro, so it would be:

RTE_CACHE_ALIGNAS uint64_t z1;

...which is horter than:

alignas(RTE_CACHE_LINE_SIZE) uint64_t z1;

and by tomorrow, it will feel as natural and obvious as the open-coded 
version.

I don't know. Just some thoughts.

>   	uint64_t z2;
>   	uint64_t z3;
>   	uint64_t z4;
>   	uint64_t z5;
>   	RTE_CACHE_GUARD;
> -} __rte_cache_aligned;
> +};
>   
>   /* One instance each for every lcore id-equipped thread, and one
>    * additional instance to be shared by all others threads (i.e., all
> diff --git a/lib/eal/common/rte_service.c b/lib/eal/common/rte_service.c
> index e183d2e..861ae31 100644
> --- a/lib/eal/common/rte_service.c
> +++ b/lib/eal/common/rte_service.c
> @@ -2,6 +2,7 @@
>    * Copyright(c) 2017 Intel Corporation
>    */
>   
> +#include <stdalign.h>
>   #include <stdio.h>
>   #include <inttypes.h>
>   #include <string.h>
> @@ -33,6 +34,7 @@
>   
>   /* internal representation of a service */
>   struct rte_service_spec_impl {
> +	alignas(RTE_CACHE_LINE_SIZE)
>   	/* public part of the struct */
>   	struct rte_service_spec spec;
>   
> @@ -53,7 +55,7 @@ struct rte_service_spec_impl {
>   	 * on currently.
>   	 */
>   	RTE_ATOMIC(uint32_t) num_mapped_cores;
> -} __rte_cache_aligned;
> +};
>   
>   struct service_stats {
>   	RTE_ATOMIC(uint64_t) calls;
> @@ -62,6 +64,7 @@ struct service_stats {
>   
>   /* the internal values of a service core */
>   struct core_state {
> +	alignas(RTE_CACHE_LINE_SIZE)
>   	/* map of services IDs are run on this core */
>   	uint64_t service_mask;
>   	RTE_ATOMIC(uint8_t) runstate; /* running or stopped */
> @@ -71,7 +74,7 @@ struct core_state {
>   	RTE_ATOMIC(uint64_t) loops;
>   	RTE_ATOMIC(uint64_t) cycles;
>   	struct service_stats service_stats[RTE_SERVICE_NUM_MAX];
> -} __rte_cache_aligned;
> +};
>   
>   static uint32_t rte_service_count;
>   static struct rte_service_spec_impl *rte_services;
> diff --git a/lib/eal/include/generic/rte_atomic.h b/lib/eal/include/generic/rte_atomic.h
> index 0e639da..bc9213c 100644
> --- a/lib/eal/include/generic/rte_atomic.h
> +++ b/lib/eal/include/generic/rte_atomic.h
> @@ -12,6 +12,7 @@
>    * This file defines a generic API for atomic operations.
>    */
>   
> +#include <stdalign.h>
>   #include <stdint.h>
>   
>   #include <rte_common.h>
> @@ -1096,6 +1097,7 @@ static inline void rte_atomic64_clear(rte_atomic64_t *v)
>    */
>   typedef struct {
>   	union {
> +		alignas(16)
>   		uint64_t val[2];
>   #ifdef RTE_ARCH_64
>   #ifndef RTE_TOOLCHAIN_MSVC
> @@ -1103,7 +1105,7 @@ static inline void rte_atomic64_clear(rte_atomic64_t *v)
>   #endif
>   #endif
>   	};
> -} __rte_aligned(16) rte_int128_t;
> +} rte_int128_t;
>   
>   #ifdef __DOXYGEN__
>   
> diff --git a/lib/eal/loongarch/include/rte_vect.h b/lib/eal/loongarch/include/rte_vect.h
> index 1546515..856d87b 100644
> --- a/lib/eal/loongarch/include/rte_vect.h
> +++ b/lib/eal/loongarch/include/rte_vect.h
> @@ -5,6 +5,7 @@
>   #ifndef RTE_VECT_LOONGARCH_H
>   #define RTE_VECT_LOONGARCH_H
>   
> +#include <stdalign.h>
>   #include <stdint.h>
>   #include "generic/rte_vect.h"
>   #include "rte_common.h"
> @@ -16,6 +17,7 @@
>   #define RTE_VECT_DEFAULT_SIMD_BITWIDTH RTE_VECT_SIMD_DISABLED
>   
>   typedef union xmm {
> +	alignas(16)
>   	int8_t   i8[16];
>   	int16_t  i16[8];
>   	int32_t  i32[4];
> @@ -25,19 +27,20 @@
>   	uint32_t u32[4];
>   	uint64_t u64[2];
>   	double   pd[2];
> -} __rte_aligned(16) xmm_t;
> +} xmm_t;
>   
>   #define XMM_SIZE        (sizeof(xmm_t))
>   #define XMM_MASK        (XMM_SIZE - 1)
>   
>   typedef union rte_xmm {
> +	alignas(16)
>   	xmm_t	 x;
>   	uint8_t	 u8[XMM_SIZE / sizeof(uint8_t)];
>   	uint16_t u16[XMM_SIZE / sizeof(uint16_t)];
>   	uint32_t u32[XMM_SIZE / sizeof(uint32_t)];
>   	uint64_t u64[XMM_SIZE / sizeof(uint64_t)];
>   	double   pd[XMM_SIZE / sizeof(double)];
> -} __rte_aligned(16) rte_xmm_t;
> +} rte_xmm_t;
>   
>   static inline xmm_t
>   vect_load_128(void *p)
> diff --git a/lib/eal/ppc/include/rte_vect.h b/lib/eal/ppc/include/rte_vect.h
> index a5f009b..e6702a4 100644
> --- a/lib/eal/ppc/include/rte_vect.h
> +++ b/lib/eal/ppc/include/rte_vect.h
> @@ -6,6 +6,8 @@
>   #ifndef _RTE_VECT_PPC_64_H_
>   #define _RTE_VECT_PPC_64_H_
>   
> +#include <stdalign.h>
> +
>   #include "rte_altivec.h"
>   
>   #include "generic/rte_vect.h"
> @@ -23,13 +25,14 @@
>   #define	XMM_MASK	(XMM_SIZE - 1)
>   
>   typedef union rte_xmm {
> +	alignas(16)
>   	xmm_t    x;
>   	uint8_t  u8[XMM_SIZE / sizeof(uint8_t)];
>   	uint16_t u16[XMM_SIZE / sizeof(uint16_t)];
>   	uint32_t u32[XMM_SIZE / sizeof(uint32_t)];
>   	uint64_t u64[XMM_SIZE / sizeof(uint64_t)];
>   	double   pd[XMM_SIZE / sizeof(double)];
> -} __rte_aligned(16) rte_xmm_t;
> +} rte_xmm_t;
>   
>   #ifdef __cplusplus
>   }
> diff --git a/lib/eal/riscv/include/rte_vect.h b/lib/eal/riscv/include/rte_vect.h
> index 2f97f43..32d4386 100644
> --- a/lib/eal/riscv/include/rte_vect.h
> +++ b/lib/eal/riscv/include/rte_vect.h
> @@ -7,6 +7,7 @@
>   #ifndef RTE_VECT_RISCV_H
>   #define RTE_VECT_RISCV_H
>   
> +#include <stdalign.h>
>   #include <stdint.h>
>   #include "generic/rte_vect.h"
>   #include "rte_common.h"
> @@ -23,13 +24,14 @@
>   #define XMM_MASK	(XMM_SIZE - 1)
>   
>   typedef union rte_xmm {
> +	alignas(16) /* !! NOTE !! changed to 16 it looks like this was a bug? */
>   	xmm_t		x;
>   	uint8_t		u8[XMM_SIZE / sizeof(uint8_t)];
>   	uint16_t	u16[XMM_SIZE / sizeof(uint16_t)];
>   	uint32_t	u32[XMM_SIZE / sizeof(uint32_t)];
>   	uint64_t	u64[XMM_SIZE / sizeof(uint64_t)];
>   	double		pd[XMM_SIZE / sizeof(double)];
> -} __rte_aligned(8) rte_xmm_t;
> +} rte_xmm_t;
>   
>   static inline xmm_t
>   vect_load_128(void *p)
> diff --git a/lib/eal/x86/include/rte_vect.h b/lib/eal/x86/include/rte_vect.h
> index 560f9e4..2e5669d 100644
> --- a/lib/eal/x86/include/rte_vect.h
> +++ b/lib/eal/x86/include/rte_vect.h
> @@ -11,6 +11,7 @@
>    * RTE SSE/AVX related header.
>    */
>   
> +#include <stdalign.h>
>   #include <stdint.h>
>   #include <rte_config.h>
>   #include <rte_common.h>
> @@ -92,6 +93,7 @@
>   #define RTE_X86_ZMM_MASK	(RTE_X86_ZMM_SIZE - 1)
>   
>   typedef union __rte_x86_zmm {
> +	alignas(RTE_X86_ZMM_SIZE)
>   	__m512i	 z;
>   	ymm_t    y[RTE_X86_ZMM_SIZE / sizeof(ymm_t)];
>   	xmm_t    x[RTE_X86_ZMM_SIZE / sizeof(xmm_t)];
> @@ -100,7 +102,7 @@
>   	uint32_t u32[RTE_X86_ZMM_SIZE / sizeof(uint32_t)];
>   	uint64_t u64[RTE_X86_ZMM_SIZE / sizeof(uint64_t)];
>   	double   pd[RTE_X86_ZMM_SIZE / sizeof(double)];
> -} __rte_aligned(RTE_X86_ZMM_SIZE) __rte_x86_zmm_t;
> +} __rte_x86_zmm_t;
>   
>   #endif /* __AVX512F__ */
>   
> diff --git a/lib/eal/x86/rte_power_intrinsics.c b/lib/eal/x86/rte_power_intrinsics.c
> index 532a2e6..5636543 100644
> --- a/lib/eal/x86/rte_power_intrinsics.c
> +++ b/lib/eal/x86/rte_power_intrinsics.c
> @@ -2,6 +2,8 @@
>    * Copyright(c) 2020 Intel Corporation
>    */
>   
> +#include <stdalign.h>
> +
>   #include <rte_common.h>
>   #include <rte_lcore.h>
>   #include <rte_rtm.h>
> @@ -13,9 +15,10 @@
>    * Per-lcore structure holding current status of C0.2 sleeps.
>    */
>   static struct power_wait_status {
> +	alignas(RTE_CACHE_LINE_SIZE)
>   	rte_spinlock_t lock;
>   	volatile void *monitor_addr; /**< NULL if not currently sleeping */
> -} __rte_cache_aligned wait_status[RTE_MAX_LCORE];
> +} wait_status[RTE_MAX_LCORE];
>   
>   /*
>    * This function uses UMONITOR/UMWAIT instructions and will enter C0.2 state.
> @@ -86,9 +89,10 @@ static void amd_mwaitx(const uint64_t timeout)
>   }
>   
>   static struct {
> +	alignas(RTE_CACHE_LINE_SIZE)
>   	void (*mmonitor)(volatile void *addr);
>   	void (*mwait)(const uint64_t timeout);
> -} __rte_cache_aligned power_monitor_ops;
> +} power_monitor_ops;
>   
>   static inline void
>   __umwait_wakeup(volatile void *addr)

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

[Tyler Retzlaff]

ping.

Please review this thread if you have time, the main point of discussion
I would like to receive consensus on the following questions.

1. Should we continue to expand common alignments behind an __rte_macro

  i.e. what do we prefer to appear in code

  alignas(RTE_CACHE_LINE_MIN_SIZE)

  -- or --

  __rte_cache_aligned

One of the benefits of dropping the macro is it provides a clear visual
indicator that it is not placed in the same location or get applied
to types as is done with __attribute__((__aligned__(n))).

2. where should we place alignas(n) or __rte_macro (if we use a macro)

Should it be on the same line as the variable or field or on the
preceeding line?

  /* same line example struct */
  struct T {
      /* alignas(64) applies to field0 *not* struct T type declaration */
      alignas(64) void *field0;
      void *field1;

      ... other fields ...

      alignas(64) uint64_t field5;
      uint32_t field6;

      ... more fields ...

  };

  /* same line example array */
  alignas(64) static const uint32_t array[4] = { ... };

  -- or --

  /* preceeding line example struct */
  struct T {
      /* alignas(64) applies to field0 *not* struct T type declaration */
      alignas(64)
      void *field0;
      void *field1;

      ... other fields ...

      alignas(64)
      uint64_t field5;
      uint32_t field6;

      ... more fields ...

  };

  /* preceeding line example array */
  alignas(64)
  static const uint32_t array[4] = { ... };


I'll submit patches for lib/* once the discussion is concluded.

thanks folks

On Wed, Nov 15, 2023 at 09:39:56AM -0800, Tyler Retzlaff wrote:
> Now that we require a C11 conformant toolchain we are able to improve
> portability by further adoption of C11 features.
> 
> Adapt EAL to use C11 alignas replacing __rte_cache_aligned and
> __rte_aligned(a) that expand to __attribute__((__aligned__(a))).
> 
> Note: it appears that use of alignas has exposed a bug in
>       lib/eal/riscv/include/rte_vect.h where the alignment
>       specified was reduced to 8 for xmm_t.
> 
> Please comment, subject to the outcome I will submit further series for
> lib/*
> 
> Thanks
> 
> Tyler Retzlaff (1):
>   eal: use C11 alignas instead of GCC attribute aligned
> 
>  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(-)
> 
> -- 
> 1.8.3.1

RE: [PATCH] RFC: use C11 alignas instead of GCC attribute aligned

[Morten Brørup]

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Thursday, 25 January 2024 19.37
> 
> ping.
> 
> Please review this thread if you have time, the main point of
> discussion
> I would like to receive consensus on the following questions.
> 
> 1. Should we continue to expand common alignments behind an __rte_macro
> 
>   i.e. what do we prefer to appear in code
> 
>   alignas(RTE_CACHE_LINE_MIN_SIZE)
> 
>   -- or --
> 
>   __rte_cache_aligned
> 
> One of the benefits of dropping the macro is it provides a clear visual
> indicator that it is not placed in the same location or get applied
> to types as is done with __attribute__((__aligned__(n))).

We don't want our own proprietary variant of something that already exists in the C standard. Now that we have moved to C11, the __rte alignment macros should be considered obsolete.

Note: I don't mind convenience macros for common use cases, so we could also introduce the macro suggested by Mattias [1]:

#define RTE_CACHE_ALIGNAS alignas(RTE_CACHE_LINE_SIZE)

[1]: https://inbox.dpdk.org/dev/dc3f3131-38e6-4219-861e-b31ec10c08bb@lysator.liu.se/

> 
> 2. where should we place alignas(n) or __rte_macro (if we use a macro)
> 
> Should it be on the same line as the variable or field or on the
> preceeding line?
> 
>   /* same line example struct */
>   struct T {
>       /* alignas(64) applies to field0 *not* struct T type declaration
> */
>       alignas(64) void *field0;
>       void *field1;
> 
>       ... other fields ...
> 
>       alignas(64) uint64_t field5;
>       uint32_t field6;
> 
>       ... more fields ...
> 
>   };
> 
>   /* same line example array */
>   alignas(64) static const uint32_t array[4] = { ... };
> 
>   -- or --
> 
>   /* preceeding line example struct */
>   struct T {
>       /* alignas(64) applies to field0 *not* struct T type declaration
> */
>       alignas(64)
>       void *field0;
>       void *field1;
> 
>       ... other fields ...
> 
>       alignas(64)
>       uint64_t field5;
>       uint32_t field6;
> 
>       ... more fields ...
> 
>   };
> 
>   /* preceeding line example array */
>   alignas(64)
>   static const uint32_t array[4] = { ... };
> 

Searching the net for what other projects do, I came across this required placement [2]:

uint64_t alignas(64) field5;

[2]: https://lore.kernel.org/buildroot/20230730000851.6faa3391@windsurf/T/

So let's follow the standard's intention and put them on the same line.
On an case-by-case basis, we can wrap lines if it improves readability, like we do with function headers that have a lot of attributes.

> 
> I'll submit patches for lib/* once the discussion is concluded.
> 
> thanks folks

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

[Tyler Retzlaff]

On Thu, Jan 25, 2024 at 11:53:04PM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Thursday, 25 January 2024 19.37
> > 
> > ping.
> > 
> > Please review this thread if you have time, the main point of
> > discussion
> > I would like to receive consensus on the following questions.
> > 
> > 1. Should we continue to expand common alignments behind an __rte_macro
> > 
> >   i.e. what do we prefer to appear in code
> > 
> >   alignas(RTE_CACHE_LINE_MIN_SIZE)
> > 
> >   -- or --
> > 
> >   __rte_cache_aligned
> > 
> > One of the benefits of dropping the macro is it provides a clear visual
> > indicator that it is not placed in the same location or get applied
> > to types as is done with __attribute__((__aligned__(n))).
> 
> We don't want our own proprietary variant of something that already exists in the C standard. Now that we have moved to C11, the __rte alignment macros should be considered obsolete.
> 
> Note: I don't mind convenience macros for common use cases, so we could also introduce the macro suggested by Mattias [1]:

ack

> 
> #define RTE_CACHE_ALIGNAS alignas(RTE_CACHE_LINE_SIZE)
> 
> [1]: https://inbox.dpdk.org/dev/dc3f3131-38e6-4219-861e-b31ec10c08bb@lysator.liu.se/

i'm good with this, it satisfies that it is a different name than the
original and therefore achieves the same intent. i'll spin the patch
series with this macro.

> 
> > 
> > 2. where should we place alignas(n) or __rte_macro (if we use a macro)
> > 
> > Should it be on the same line as the variable or field or on the
> > preceeding line?
> > 
> >   /* same line example struct */
> >   struct T {
> >       /* alignas(64) applies to field0 *not* struct T type declaration
> > */
> >       alignas(64) void *field0;
> >       void *field1;
> > 
> >       ... other fields ...
> > 
> >       alignas(64) uint64_t field5;
> >       uint32_t field6;
> > 
> >       ... more fields ...
> > 
> >   };
> > 
> >   /* same line example array */
> >   alignas(64) static const uint32_t array[4] = { ... };
> > 
> >   -- or --
> > 
> >   /* preceeding line example struct */
> >   struct T {
> >       /* alignas(64) applies to field0 *not* struct T type declaration
> > */
> >       alignas(64)
> >       void *field0;
> >       void *field1;
> > 
> >       ... other fields ...
> > 
> >       alignas(64)
> >       uint64_t field5;
> >       uint32_t field6;
> > 
> >       ... more fields ...
> > 
> >   };
> > 
> >   /* preceeding line example array */
> >   alignas(64)
> >   static const uint32_t array[4] = { ... };
> > 
> 
> Searching the net for what other projects do, I came across this required placement [2]:
> 
> uint64_t alignas(64) field5;
> 
> [2]: https://lore.kernel.org/buildroot/20230730000851.6faa3391@windsurf/T/
>
> So let's follow the standard's intention and put them on the same line.
> On an case-by-case basis, we can wrap lines if it improves readability, like we do with function headers that have a lot of attributes.

just fyi.

the linked code is c++ and standard c++ has both semantic and syntactic
differences from standard c. notably standard c is moving away
from the notion that you can alignas types and instead you align
variables/fields/members.

further restricting placement is the need to choose an intersecting
placement that works when consumed in either a c or c++ translation
unit. so the options i present above are that intersection.

ty

> 
> 
> > 
> > I'll submit patches for lib/* once the discussion is concluded.
> > 
> > thanks folks

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

[Mattias Rönnblom]

On 2024-01-25 23:53, Morten Brørup wrote:
>> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
>> Sent: Thursday, 25 January 2024 19.37
>>
>> ping.
>>
>> Please review this thread if you have time, the main point of
>> discussion
>> I would like to receive consensus on the following questions.
>>
>> 1. Should we continue to expand common alignments behind an __rte_macro
>>
>>    i.e. what do we prefer to appear in code
>>
>>    alignas(RTE_CACHE_LINE_MIN_SIZE)
>>
>>    -- or --
>>
>>    __rte_cache_aligned
>>
>> One of the benefits of dropping the macro is it provides a clear visual
>> indicator that it is not placed in the same location or get applied
>> to types as is done with __attribute__((__aligned__(n))).
> 
> We don't want our own proprietary variant of something that already exists in the C standard. Now that we have moved to C11, the __rte alignment macros should be considered obsolete.

Making so something cache-line aligned is not in C11.

__rte_cache_aligned is shorter, provides a tiny bit of abstraction, and 
is already an established DPDK standard. So just keep the macro. If it 
would change, I would argue for it to be changed to rte_cache_aligned 
(i.e., just moving it out of __ namespace, and maybe making it 
all-uppercase).

Non-trivial C programs wrap things all the time, standard or not. It's 
not something to be overly concerned about, imo.

> 
> Note: I don't mind convenience macros for common use cases, so we could also introduce the macro suggested by Mattias [1]:
> 
> #define RTE_CACHE_ALIGNAS alignas(RTE_CACHE_LINE_SIZE)
> 
> [1]: https://inbox.dpdk.org/dev/dc3f3131-38e6-4219-861e-b31ec10c08bb@lysator.liu.se/
> 
>>
>> 2. where should we place alignas(n) or __rte_macro (if we use a macro)
>>
>> Should it be on the same line as the variable or field or on the
>> preceeding line?
>>
>>    /* same line example struct */
>>    struct T {
>>        /* alignas(64) applies to field0 *not* struct T type declaration
>> */
>>        alignas(64) void *field0;
>>        void *field1;
>>
>>        ... other fields ...
>>
>>        alignas(64) uint64_t field5;
>>        uint32_t field6;
>>
>>        ... more fields ...
>>
>>    };
>>
>>    /* same line example array */
>>    alignas(64) static const uint32_t array[4] = { ... };
>>
>>    -- or --
>>
>>    /* preceeding line example struct */
>>    struct T {
>>        /* alignas(64) applies to field0 *not* struct T type declaration
>> */
>>        alignas(64)
>>        void *field0;
>>        void *field1;
>>
>>        ... other fields ...
>>
>>        alignas(64)
>>        uint64_t field5;
>>        uint32_t field6;
>>
>>        ... more fields ...
>>
>>    };
>>
>>    /* preceeding line example array */
>>    alignas(64)
>>    static const uint32_t array[4] = { ... };
>>
> 
> Searching the net for what other projects do, I came across this required placement [2]:
> 
> uint64_t alignas(64) field5;
> 
> [2]: https://lore.kernel.org/buildroot/20230730000851.6faa3391@windsurf/T/
> 
> So let's follow the standard's intention and put them on the same line.
> On an case-by-case basis, we can wrap lines if it improves readability, like we do with function headers that have a lot of attributes.
> 
>>
>> I'll submit patches for lib/* once the discussion is concluded.
>>
>> thanks folks
> 

RE: [PATCH] RFC: use C11 alignas instead of GCC attribute aligned

[Morten Brørup]

> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> Sent: Friday, 26 January 2024 11.05
> 
> On 2024-01-25 23:53, Morten Brørup wrote:
> >> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> >> Sent: Thursday, 25 January 2024 19.37
> >>
> >> ping.
> >>
> >> Please review this thread if you have time, the main point of
> >> discussion
> >> I would like to receive consensus on the following questions.
> >>
> >> 1. Should we continue to expand common alignments behind an
> __rte_macro
> >>
> >>    i.e. what do we prefer to appear in code
> >>
> >>    alignas(RTE_CACHE_LINE_MIN_SIZE)
> >>
> >>    -- or --
> >>
> >>    __rte_cache_aligned
> >>
> >> One of the benefits of dropping the macro is it provides a clear
> visual
> >> indicator that it is not placed in the same location or get applied
> >> to types as is done with __attribute__((__aligned__(n))).
> >
> > We don't want our own proprietary variant of something that already
> exists in the C standard. Now that we have moved to C11, the __rte
> alignment macros should be considered obsolete.
> 
> Making so something cache-line aligned is not in C11.

We are talking about the __rte_aligned() macro, not the cache alignment macro.

> 
> __rte_cache_aligned is shorter, provides a tiny bit of abstraction, and
> is already an established DPDK standard. So just keep the macro. If it
> would change, I would argue for it to be changed to rte_cache_aligned
> (i.e., just moving it out of __ namespace, and maybe making it
> all-uppercase).
> 
> Non-trivial C programs wrap things all the time, standard or not. It's
> not something to be overly concerned about, imo.

Using the cache alignment macro was obviously a bad example for discussing the __rte_aligned() macro.

FYI, Tyler later agreed to introducing the RTE_CACHE_ALIGNAS you had proposed in an earlier correspondence.

> 
> >
> > Note: I don't mind convenience macros for common use cases, so we
> could also introduce the macro suggested by Mattias [1]:
> >
> > #define RTE_CACHE_ALIGNAS alignas(RTE_CACHE_LINE_SIZE)
> >
> > [1]: https://inbox.dpdk.org/dev/dc3f3131-38e6-4219-861e-
> b31ec10c08bb@lysator.liu.se/
> >
> >>
> >> 2. where should we place alignas(n) or __rte_macro (if we use a
> macro)
> >>
> >> Should it be on the same line as the variable or field or on the
> >> preceeding line?
> >>
> >>    /* same line example struct */
> >>    struct T {
> >>        /* alignas(64) applies to field0 *not* struct T type
> declaration
> >> */
> >>        alignas(64) void *field0;
> >>        void *field1;
> >>
> >>        ... other fields ...
> >>
> >>        alignas(64) uint64_t field5;
> >>        uint32_t field6;
> >>
> >>        ... more fields ...
> >>
> >>    };
> >>
> >>    /* same line example array */
> >>    alignas(64) static const uint32_t array[4] = { ... };
> >>
> >>    -- or --
> >>
> >>    /* preceeding line example struct */
> >>    struct T {
> >>        /* alignas(64) applies to field0 *not* struct T type
> declaration
> >> */
> >>        alignas(64)
> >>        void *field0;
> >>        void *field1;
> >>
> >>        ... other fields ...
> >>
> >>        alignas(64)
> >>        uint64_t field5;
> >>        uint32_t field6;
> >>
> >>        ... more fields ...
> >>
> >>    };
> >>
> >>    /* preceeding line example array */
> >>    alignas(64)
> >>    static const uint32_t array[4] = { ... };
> >>
> >
> > Searching the net for what other projects do, I came across this
> required placement [2]:
> >
> > uint64_t alignas(64) field5;
> >
> > [2]:
> https://lore.kernel.org/buildroot/20230730000851.6faa3391@windsurf/T/
> >
> > So let's follow the standard's intention and put them on the same
> line.
> > On an case-by-case basis, we can wrap lines if it improves
> readability, like we do with function headers that have a lot of
> attributes.
> >
> >>
> >> I'll submit patches for lib/* once the discussion is concluded.
> >>
> >> thanks folks
> >

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

[Mattias Rönnblom]

On 2024-01-26 11:18, Morten Brørup wrote:
>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
>> Sent: Friday, 26 January 2024 11.05
>>
>> On 2024-01-25 23:53, Morten Brørup wrote:
>>>> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
>>>> Sent: Thursday, 25 January 2024 19.37
>>>>
>>>> ping.
>>>>
>>>> Please review this thread if you have time, the main point of
>>>> discussion
>>>> I would like to receive consensus on the following questions.
>>>>
>>>> 1. Should we continue to expand common alignments behind an
>> __rte_macro
>>>>
>>>>     i.e. what do we prefer to appear in code
>>>>
>>>>     alignas(RTE_CACHE_LINE_MIN_SIZE)
>>>>
>>>>     -- or --
>>>>
>>>>     __rte_cache_aligned
>>>>
>>>> One of the benefits of dropping the macro is it provides a clear
>> visual
>>>> indicator that it is not placed in the same location or get applied
>>>> to types as is done with __attribute__((__aligned__(n))).
>>>
>>> We don't want our own proprietary variant of something that already
>> exists in the C standard. Now that we have moved to C11, the __rte
>> alignment macros should be considered obsolete.
>>
>> Making so something cache-line aligned is not in C11.
> 
> We are talking about the __rte_aligned() macro, not the cache alignment macro.
> 

OK, in that case, what is the relevance of question 1 above?

>>
>> __rte_cache_aligned is shorter, provides a tiny bit of abstraction, and
>> is already an established DPDK standard. So just keep the macro. If it
>> would change, I would argue for it to be changed to rte_cache_aligned
>> (i.e., just moving it out of __ namespace, and maybe making it
>> all-uppercase).
>>
>> Non-trivial C programs wrap things all the time, standard or not. It's
>> not something to be overly concerned about, imo.
> 
> Using the cache alignment macro was obviously a bad example for discussing the __rte_aligned() macro.
> 
> FYI, Tyler later agreed to introducing the RTE_CACHE_ALIGNAS you had proposed in an earlier correspondence.
> 
>>
>>>
>>> Note: I don't mind convenience macros for common use cases, so we
>> could also introduce the macro suggested by Mattias [1]:
>>>
>>> #define RTE_CACHE_ALIGNAS alignas(RTE_CACHE_LINE_SIZE)
>>>
>>> [1]: https://inbox.dpdk.org/dev/dc3f3131-38e6-4219-861e-
>> b31ec10c08bb@lysator.liu.se/
>>>
>>>>
>>>> 2. where should we place alignas(n) or __rte_macro (if we use a
>> macro)
>>>>
>>>> Should it be on the same line as the variable or field or on the
>>>> preceeding line?
>>>>
>>>>     /* same line example struct */
>>>>     struct T {
>>>>         /* alignas(64) applies to field0 *not* struct T type
>> declaration
>>>> */
>>>>         alignas(64) void *field0;
>>>>         void *field1;
>>>>
>>>>         ... other fields ...
>>>>
>>>>         alignas(64) uint64_t field5;
>>>>         uint32_t field6;
>>>>
>>>>         ... more fields ...
>>>>
>>>>     };
>>>>
>>>>     /* same line example array */
>>>>     alignas(64) static const uint32_t array[4] = { ... };
>>>>
>>>>     -- or --
>>>>
>>>>     /* preceeding line example struct */
>>>>     struct T {
>>>>         /* alignas(64) applies to field0 *not* struct T type
>> declaration
>>>> */
>>>>         alignas(64)
>>>>         void *field0;
>>>>         void *field1;
>>>>
>>>>         ... other fields ...
>>>>
>>>>         alignas(64)
>>>>         uint64_t field5;
>>>>         uint32_t field6;
>>>>
>>>>         ... more fields ...
>>>>
>>>>     };
>>>>
>>>>     /* preceeding line example array */
>>>>     alignas(64)
>>>>     static const uint32_t array[4] = { ... };
>>>>
>>>
>>> Searching the net for what other projects do, I came across this
>> required placement [2]:
>>>
>>> uint64_t alignas(64) field5;
>>>
>>> [2]:
>> https://lore.kernel.org/buildroot/20230730000851.6faa3391@windsurf/T/
>>>
>>> So let's follow the standard's intention and put them on the same
>> line.
>>> On an case-by-case basis, we can wrap lines if it improves
>> readability, like we do with function headers that have a lot of
>> attributes.
>>>
>>>>
>>>> I'll submit patches for lib/* once the discussion is concluded.
>>>>
>>>> thanks folks
>>>

RE: [PATCH] RFC: use C11 alignas instead of GCC attribute aligned

[Morten Brørup]

> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> Sent: Saturday, 27 January 2024 20.15
> 
> On 2024-01-26 11:18, Morten Brørup wrote:
> >> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> >> Sent: Friday, 26 January 2024 11.05
> >>
> >> On 2024-01-25 23:53, Morten Brørup wrote:
> >>>> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> >>>> Sent: Thursday, 25 January 2024 19.37
> >>>>
> >>>> ping.
> >>>>
> >>>> Please review this thread if you have time, the main point of
> >>>> discussion
> >>>> I would like to receive consensus on the following questions.
> >>>>
> >>>> 1. Should we continue to expand common alignments behind an
> >> __rte_macro
> >>>>
> >>>>     i.e. what do we prefer to appear in code
> >>>>
> >>>>     alignas(RTE_CACHE_LINE_MIN_SIZE)
> >>>>
> >>>>     -- or --
> >>>>
> >>>>     __rte_cache_aligned
> >>>>
> >>>> One of the benefits of dropping the macro is it provides a clear
> >> visual
> >>>> indicator that it is not placed in the same location or get
> applied
> >>>> to types as is done with __attribute__((__aligned__(n))).
> >>>
> >>> We don't want our own proprietary variant of something that already
> >> exists in the C standard. Now that we have moved to C11, the __rte
> >> alignment macros should be considered obsolete.
> >>
> >> Making so something cache-line aligned is not in C11.
> >
> > We are talking about the __rte_aligned() macro, not the cache
> alignment macro.
> >
> 
> OK, in that case, what is the relevance of question 1 above?

With this in mind, try re-reading Tyler's clarifications in this tread.

Briefly: alignas() can be attached to variables and structure fields, but not to types (like __rte_aligned()), so to align a structure:

struct foo {
	int alignas(64) bar; /* alignas(64) must be here */
	int             baz;
}; /* __rte_aligned(64) was here, but alignas(64) cannot be here. */

So the question is: Do we want to eliminate the __rte_aligned() macro - which relies on compiler attributes - and migrate to using the C11 standard alignas()?

I think yes; after updating to C11, the workaround for pre-C11 not offering alignment is obsolete, and its removal should be on the roadmap.

Continuously cleaning up old cruft in DPDK is important, especially for reducing the learning curve for newcomers to the project.

For backwards compatibility, we can still keep the obsolete macros, but they should be removed from the project itself.

> 
> >>
> >> __rte_cache_aligned is shorter, provides a tiny bit of abstraction,
> and
> >> is already an established DPDK standard. So just keep the macro. If
> it
> >> would change, I would argue for it to be changed to
> rte_cache_aligned
> >> (i.e., just moving it out of __ namespace, and maybe making it
> >> all-uppercase).
> >>
> >> Non-trivial C programs wrap things all the time, standard or not.
> It's
> >> not something to be overly concerned about, imo.
> >
> > Using the cache alignment macro was obviously a bad example for
> discussing the __rte_aligned() macro.
> >
> > FYI, Tyler later agreed to introducing the RTE_CACHE_ALIGNAS you had
> proposed in an earlier correspondence.
> >
> >>
> >>>
> >>> Note: I don't mind convenience macros for common use cases, so we
> >> could also introduce the macro suggested by Mattias [1]:
> >>>
> >>> #define RTE_CACHE_ALIGNAS alignas(RTE_CACHE_LINE_SIZE)
> >>>
> >>> [1]: https://inbox.dpdk.org/dev/dc3f3131-38e6-4219-861e-
> >> b31ec10c08bb@lysator.liu.se/
> >>>
> >>>>
> >>>> 2. where should we place alignas(n) or __rte_macro (if we use a
> >> macro)
> >>>>
> >>>> Should it be on the same line as the variable or field or on the
> >>>> preceeding line?
> >>>>
> >>>>     /* same line example struct */
> >>>>     struct T {
> >>>>         /* alignas(64) applies to field0 *not* struct T type
> >> declaration
> >>>> */
> >>>>         alignas(64) void *field0;
> >>>>         void *field1;
> >>>>
> >>>>         ... other fields ...
> >>>>
> >>>>         alignas(64) uint64_t field5;
> >>>>         uint32_t field6;
> >>>>
> >>>>         ... more fields ...
> >>>>
> >>>>     };
> >>>>
> >>>>     /* same line example array */
> >>>>     alignas(64) static const uint32_t array[4] = { ... };
> >>>>
> >>>>     -- or --
> >>>>
> >>>>     /* preceeding line example struct */
> >>>>     struct T {
> >>>>         /* alignas(64) applies to field0 *not* struct T type
> >> declaration
> >>>> */
> >>>>         alignas(64)
> >>>>         void *field0;
> >>>>         void *field1;
> >>>>
> >>>>         ... other fields ...
> >>>>
> >>>>         alignas(64)
> >>>>         uint64_t field5;
> >>>>         uint32_t field6;
> >>>>
> >>>>         ... more fields ...
> >>>>
> >>>>     };
> >>>>
> >>>>     /* preceeding line example array */
> >>>>     alignas(64)
> >>>>     static const uint32_t array[4] = { ... };
> >>>>
> >>>
> >>> Searching the net for what other projects do, I came across this
> >> required placement [2]:
> >>>
> >>> uint64_t alignas(64) field5;
> >>>
> >>> [2]:
> >>
> https://lore.kernel.org/buildroot/20230730000851.6faa3391@windsurf/T/
> >>>
> >>> So let's follow the standard's intention and put them on the same
> >> line.
> >>> On an case-by-case basis, we can wrap lines if it improves
> >> readability, like we do with function headers that have a lot of
> >> attributes.
> >>>
> >>>>
> >>>> I'll submit patches for lib/* once the discussion is concluded.
> >>>>
> >>>> thanks folks
> >>>

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

[Mattias Rönnblom]

On 2024-01-28 09:57, Morten Brørup wrote:
>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
>> Sent: Saturday, 27 January 2024 20.15
>>
>> On 2024-01-26 11:18, Morten Brørup wrote:
>>>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
>>>> Sent: Friday, 26 January 2024 11.05
>>>>
>>>> On 2024-01-25 23:53, Morten Brørup wrote:
>>>>>> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
>>>>>> Sent: Thursday, 25 January 2024 19.37
>>>>>>
>>>>>> ping.
>>>>>>
>>>>>> Please review this thread if you have time, the main point of
>>>>>> discussion
>>>>>> I would like to receive consensus on the following questions.
>>>>>>
>>>>>> 1. Should we continue to expand common alignments behind an
>>>> __rte_macro
>>>>>>
>>>>>>      i.e. what do we prefer to appear in code
>>>>>>
>>>>>>      alignas(RTE_CACHE_LINE_MIN_SIZE)
>>>>>>
>>>>>>      -- or --
>>>>>>
>>>>>>      __rte_cache_aligned
>>>>>>
>>>>>> One of the benefits of dropping the macro is it provides a clear
>>>> visual
>>>>>> indicator that it is not placed in the same location or get
>> applied
>>>>>> to types as is done with __attribute__((__aligned__(n))).
>>>>>
>>>>> We don't want our own proprietary variant of something that already
>>>> exists in the C standard. Now that we have moved to C11, the __rte
>>>> alignment macros should be considered obsolete.
>>>>
>>>> Making so something cache-line aligned is not in C11.
>>>
>>> We are talking about the __rte_aligned() macro, not the cache
>> alignment macro.
>>>
>>
>> OK, in that case, what is the relevance of question 1 above?
> 
> With this in mind, try re-reading Tyler's clarifications in this tread.
> 
> Briefly: alignas() can be attached to variables and structure fields, but not to types (like __rte_aligned()), so to align a structure:
> 
> struct foo {
> 	int alignas(64) bar; /* alignas(64) must be here */
> 	int             baz;
> }; /* __rte_aligned(64) was here, but alignas(64) cannot be here. */
> 
> So the question is: Do we want to eliminate the __rte_aligned() macro - which relies on compiler attributes - and migrate to using the C11 standard alignas()?
> 
> I think yes; after updating to C11, the workaround for pre-C11 not offering alignment is obsolete, and its removal should be on the roadmap.
> 

OK, thanks for the explanation. Interesting limitation in the standard.

If the construct the standard is offering is less effective (in this 
case, less readable) and the non-standard-based option is possible to 
implement on all compilers (i.e., on MSVC too), then we should keep the 
custom option. Especially if it's already there, but also in cases where 
it isn't.

In fact, one could argue *everything* related to alignment should go 
through something rte_, __rte_ or RTE_-prefixed. So, "int 
RTE_ALIGNAS(64) bar;". Maybe that would be silly, but it would be 
consistent with RTE_CACHE_ALIGNAS.

I would worry more about allowing DPDK developers writing clean and 
readable code, than very slightly lowering the bar for the fraction of 
newcomers experienced with the latest and greatest from the C standard, 
and *not* familiar with age-old GCC extensions.

> Continuously cleaning up old cruft in DPDK is important, especially for reducing the learning curve for newcomers to the project.
> 
> For backwards compatibility, we can still keep the obsolete macros, but they should be removed from the project itself.
> 
>>
>>>>
>>>> __rte_cache_aligned is shorter, provides a tiny bit of abstraction,
>> and
>>>> is already an established DPDK standard. So just keep the macro. If
>> it
>>>> would change, I would argue for it to be changed to
>> rte_cache_aligned
>>>> (i.e., just moving it out of __ namespace, and maybe making it
>>>> all-uppercase).
>>>>
>>>> Non-trivial C programs wrap things all the time, standard or not.
>> It's
>>>> not something to be overly concerned about, imo.
>>>
>>> Using the cache alignment macro was obviously a bad example for
>> discussing the __rte_aligned() macro.
>>>
>>> FYI, Tyler later agreed to introducing the RTE_CACHE_ALIGNAS you had
>> proposed in an earlier correspondence.
>>>
>>>>
>>>>>
>>>>> Note: I don't mind convenience macros for common use cases, so we
>>>> could also introduce the macro suggested by Mattias [1]:
>>>>>
>>>>> #define RTE_CACHE_ALIGNAS alignas(RTE_CACHE_LINE_SIZE)
>>>>>
>>>>> [1]: https://inbox.dpdk.org/dev/dc3f3131-38e6-4219-861e-
>>>> b31ec10c08bb@lysator.liu.se/
>>>>>
>>>>>>
>>>>>> 2. where should we place alignas(n) or __rte_macro (if we use a
>>>> macro)
>>>>>>
>>>>>> Should it be on the same line as the variable or field or on the
>>>>>> preceeding line?
>>>>>>
>>>>>>      /* same line example struct */
>>>>>>      struct T {
>>>>>>          /* alignas(64) applies to field0 *not* struct T type
>>>> declaration
>>>>>> */
>>>>>>          alignas(64) void *field0;
>>>>>>          void *field1;
>>>>>>
>>>>>>          ... other fields ...
>>>>>>
>>>>>>          alignas(64) uint64_t field5;
>>>>>>          uint32_t field6;
>>>>>>
>>>>>>          ... more fields ...
>>>>>>
>>>>>>      };
>>>>>>
>>>>>>      /* same line example array */
>>>>>>      alignas(64) static const uint32_t array[4] = { ... };
>>>>>>
>>>>>>      -- or --
>>>>>>
>>>>>>      /* preceeding line example struct */
>>>>>>      struct T {
>>>>>>          /* alignas(64) applies to field0 *not* struct T type
>>>> declaration
>>>>>> */
>>>>>>          alignas(64)
>>>>>>          void *field0;
>>>>>>          void *field1;
>>>>>>
>>>>>>          ... other fields ...
>>>>>>
>>>>>>          alignas(64)
>>>>>>          uint64_t field5;
>>>>>>          uint32_t field6;
>>>>>>
>>>>>>          ... more fields ...
>>>>>>
>>>>>>      };
>>>>>>
>>>>>>      /* preceeding line example array */
>>>>>>      alignas(64)
>>>>>>      static const uint32_t array[4] = { ... };
>>>>>>
>>>>>
>>>>> Searching the net for what other projects do, I came across this
>>>> required placement [2]:
>>>>>
>>>>> uint64_t alignas(64) field5;
>>>>>
>>>>> [2]:
>>>>
>> https://lore.kernel.org/buildroot/20230730000851.6faa3391@windsurf/T/
>>>>>
>>>>> So let's follow the standard's intention and put them on the same
>>>> line.
>>>>> On an case-by-case basis, we can wrap lines if it improves
>>>> readability, like we do with function headers that have a lot of
>>>> attributes.
>>>>>
>>>>>>
>>>>>> I'll submit patches for lib/* once the discussion is concluded.
>>>>>>
>>>>>> thanks folks
>>>>>

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

[Tyler Retzlaff]

On Sun, Jan 28, 2024 at 11:00:31AM +0100, Mattias Rönnblom wrote:
> On 2024-01-28 09:57, Morten Brørup wrote:
> >>From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> >>Sent: Saturday, 27 January 2024 20.15
> >>
> >>On 2024-01-26 11:18, Morten Brørup wrote:
> >>>>From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> >>>>Sent: Friday, 26 January 2024 11.05
> >>>>
> >>>>On 2024-01-25 23:53, Morten Brørup wrote:
> >>>>>>From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> >>>>>>Sent: Thursday, 25 January 2024 19.37
> >>>>>>
> >>>>>>ping.
> >>>>>>
> >>>>>>Please review this thread if you have time, the main point of
> >>>>>>discussion
> >>>>>>I would like to receive consensus on the following questions.
> >>>>>>
> >>>>>>1. Should we continue to expand common alignments behind an
> >>>>__rte_macro
> >>>>>>
> >>>>>>     i.e. what do we prefer to appear in code
> >>>>>>
> >>>>>>     alignas(RTE_CACHE_LINE_MIN_SIZE)
> >>>>>>
> >>>>>>     -- or --
> >>>>>>
> >>>>>>     __rte_cache_aligned
> >>>>>>
> >>>>>>One of the benefits of dropping the macro is it provides a clear
> >>>>visual
> >>>>>>indicator that it is not placed in the same location or get
> >>applied
> >>>>>>to types as is done with __attribute__((__aligned__(n))).
> >>>>>
> >>>>>We don't want our own proprietary variant of something that already
> >>>>exists in the C standard. Now that we have moved to C11, the __rte
> >>>>alignment macros should be considered obsolete.
> >>>>
> >>>>Making so something cache-line aligned is not in C11.
> >>>
> >>>We are talking about the __rte_aligned() macro, not the cache
> >>alignment macro.
> >>>
> >>
> >>OK, in that case, what is the relevance of question 1 above?
> >
> >With this in mind, try re-reading Tyler's clarifications in this tread.
> >
> >Briefly: alignas() can be attached to variables and structure fields, but not to types (like __rte_aligned()), so to align a structure:
> >
> >struct foo {
> >	int alignas(64) bar; /* alignas(64) must be here */
> >	int             baz;
> >}; /* __rte_aligned(64) was here, but alignas(64) cannot be here. */
> >
> >So the question is: Do we want to eliminate the __rte_aligned() macro - which relies on compiler attributes - and migrate to using the C11 standard alignas()?
> >
> >I think yes; after updating to C11, the workaround for pre-C11 not offering alignment is obsolete, and its removal should be on the roadmap.
> >
> 
> OK, thanks for the explanation. Interesting limitation in the standard.
> 
> If the construct the standard is offering is less effective (in this
> case, less readable) and the non-standard-based option is possible
> to implement on all compilers (i.e., on MSVC too), then we should
> keep the custom option. Especially if it's already there, but also
> in cases where it isn't.
> 
> In fact, one could argue *everything* related to alignment should go
> through something rte_, __rte_ or RTE_-prefixed. So, "int
> RTE_ALIGNAS(64) bar;". Maybe that would be silly, but it would be
> consistent with RTE_CACHE_ALIGNAS.
> 
> I would worry more about allowing DPDK developers writing clean and
> readable code, than very slightly lowering the bar for the fraction
> of newcomers experienced with the latest and greatest from the C
> standard, and *not* familiar with age-old GCC extensions.

I’d just like to summarize where my understanding is at after reviewing
this discussion and my downstream branch. But I also want to make it
clear that we probably need to use both standard C and non-standard
attribute/declspec for object and struct/union type alignment
respectively.

I've assumed we prefer avoiding per-compiler conditional expansion when
possible through the use of standard C mechanisms. But there are
instances when alignas is awkward.

So I think the following is consistent with what Mattias is advocating
sans any discussions related to actual naming of macros.

We should have 2 macros, upon which others may be built to expand to
well-known values for e.g. cache line size.

RTE_ALIGNAS(n) object;

* This macro is used to align C objects i.e. variable, array, struct/union
  fields etc.
* Trivially expands to alignas(n) for all toolchains.
* Placed in a location that both C and C++ translation units accept that
  is on the same line preceeding the object type.
  example:
  // RTE_ALIGNAS(n) object;
  RTE_ALIGNAS(16) char somearray[16];

RTE_ALIGN_TYPE(n)

* This macro is used to align struct/union types.
* Conditionally expands to __declspec(align(n)) (msvc) and
  __attribute__((__aligned__(n))) (for all other toolchains)
* Placed in a location that for all gcc,clang,msvc and both C and C++
  translation units accept.
  example:
  // {struct,union} RTE_ALIGN_TYPE(n) tag { ... };
  struct RTE_ALIGN_TYPE(64) sometype { ... };

I'm not picky about what the names actualy are if you have better
suggestions i'm happy to adopt them.

Thoughts? Comments?

Appreciate the discussion this has been helpful.

ty

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

[Mattias Rönnblom]

On 2024-01-29 20:43, Tyler Retzlaff wrote:
> On Sun, Jan 28, 2024 at 11:00:31AM +0100, Mattias Rönnblom wrote:
>> On 2024-01-28 09:57, Morten Brørup wrote:
>>>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
>>>> Sent: Saturday, 27 January 2024 20.15
>>>>
>>>> On 2024-01-26 11:18, Morten Brørup wrote:
>>>>>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
>>>>>> Sent: Friday, 26 January 2024 11.05
>>>>>>
>>>>>> On 2024-01-25 23:53, Morten Brørup wrote:
>>>>>>>> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
>>>>>>>> Sent: Thursday, 25 January 2024 19.37
>>>>>>>>
>>>>>>>> ping.
>>>>>>>>
>>>>>>>> Please review this thread if you have time, the main point of
>>>>>>>> discussion
>>>>>>>> I would like to receive consensus on the following questions.
>>>>>>>>
>>>>>>>> 1. Should we continue to expand common alignments behind an
>>>>>> __rte_macro
>>>>>>>>
>>>>>>>>      i.e. what do we prefer to appear in code
>>>>>>>>
>>>>>>>>      alignas(RTE_CACHE_LINE_MIN_SIZE)
>>>>>>>>
>>>>>>>>      -- or --
>>>>>>>>
>>>>>>>>      __rte_cache_aligned
>>>>>>>>
>>>>>>>> One of the benefits of dropping the macro is it provides a clear
>>>>>> visual
>>>>>>>> indicator that it is not placed in the same location or get
>>>> applied
>>>>>>>> to types as is done with __attribute__((__aligned__(n))).
>>>>>>>
>>>>>>> We don't want our own proprietary variant of something that already
>>>>>> exists in the C standard. Now that we have moved to C11, the __rte
>>>>>> alignment macros should be considered obsolete.
>>>>>>
>>>>>> Making so something cache-line aligned is not in C11.
>>>>>
>>>>> We are talking about the __rte_aligned() macro, not the cache
>>>> alignment macro.
>>>>>
>>>>
>>>> OK, in that case, what is the relevance of question 1 above?
>>>
>>> With this in mind, try re-reading Tyler's clarifications in this tread.
>>>
>>> Briefly: alignas() can be attached to variables and structure fields, but not to types (like __rte_aligned()), so to align a structure:
>>>
>>> struct foo {
>>> 	int alignas(64) bar; /* alignas(64) must be here */
>>> 	int             baz;
>>> }; /* __rte_aligned(64) was here, but alignas(64) cannot be here. */
>>>
>>> So the question is: Do we want to eliminate the __rte_aligned() macro - which relies on compiler attributes - and migrate to using the C11 standard alignas()?
>>>
>>> I think yes; after updating to C11, the workaround for pre-C11 not offering alignment is obsolete, and its removal should be on the roadmap.
>>>
>>
>> OK, thanks for the explanation. Interesting limitation in the standard.
>>
>> If the construct the standard is offering is less effective (in this
>> case, less readable) and the non-standard-based option is possible
>> to implement on all compilers (i.e., on MSVC too), then we should
>> keep the custom option. Especially if it's already there, but also
>> in cases where it isn't.
>>
>> In fact, one could argue *everything* related to alignment should go
>> through something rte_, __rte_ or RTE_-prefixed. So, "int
>> RTE_ALIGNAS(64) bar;". Maybe that would be silly, but it would be
>> consistent with RTE_CACHE_ALIGNAS.
>>
>> I would worry more about allowing DPDK developers writing clean and
>> readable code, than very slightly lowering the bar for the fraction
>> of newcomers experienced with the latest and greatest from the C
>> standard, and *not* familiar with age-old GCC extensions.
> 
> I’d just like to summarize where my understanding is at after reviewing
> this discussion and my downstream branch. But I also want to make it
> clear that we probably need to use both standard C and non-standard
> attribute/declspec for object and struct/union type alignment
> respectively.
> 
> I've assumed we prefer avoiding per-compiler conditional expansion when
> possible through the use of standard C mechanisms. But there are
> instances when alignas is awkward.
> 
> So I think the following is consistent with what Mattias is advocating
> sans any discussions related to actual naming of macros.
> 
> We should have 2 macros, upon which others may be built to expand to
> well-known values for e.g. cache line size.
> 
> RTE_ALIGNAS(n) object;
> 
> * This macro is used to align C objects i.e. variable, array, struct/union
>    fields etc.
> * Trivially expands to alignas(n) for all toolchains.
> * Placed in a location that both C and C++ translation units accept that
>    is on the same line preceeding the object type.
>    example:
>    // RTE_ALIGNAS(n) object;
>    RTE_ALIGNAS(16) char somearray[16];
> 
> RTE_ALIGN_TYPE(n)
> 
> * This macro is used to align struct/union types.
> * Conditionally expands to __declspec(align(n)) (msvc) and
>    __attribute__((__aligned__(n))) (for all other toolchains)
> * Placed in a location that for all gcc,clang,msvc and both C and C++
>    translation units accept.
>    example:
>    // {struct,union} RTE_ALIGN_TYPE(n) tag { ... };
>    struct RTE_ALIGN_TYPE(64) sometype { ... };
> 

Sorry if I've missed some discussion on the list, but the current 
pattern of putting __rte_aligned(X) at the end doesn't work with MSVC, 
or why are we doing this? C11 purism doesn't seem like much of a driving 
force.

If one defined a macro as __declspec(align(X)) on MSVC and 
__attribute__(__aligned__(X)) on other compilers, could it do the work 
of both the above RTE_ALIGNAS() and RTE_ALIGN_TYPE()?

<a> struct <b> { int a; } <c>;

You would have to mandate the placement of such a __rte_aligned plug-in 
replacement being at <b> rather than (the more intuitive?) <a>, since 
clang doesn't like __attribute__s before the struct/union keyword, correct?

What about other <rte_common.h> __attribute__ wrappers like 
__rte_packed; would they also need to change placement to make DPDK work 
with MSVC?

> I'm not picky about what the names actualy are if you have better
> suggestions i'm happy to adopt them.
> 
> Thoughts? Comments?
> 
> Appreciate the discussion this has been helpful.
> 
> ty
> 

RE: [PATCH] RFC: use C11 alignas instead of GCC attribute aligned

[Morten Brørup]

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Monday, 29 January 2024 20.44
> 
> On Sun, Jan 28, 2024 at 11:00:31AM +0100, Mattias Rönnblom wrote:
> > On 2024-01-28 09:57, Morten Brørup wrote:
> > >>From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> > >>Sent: Saturday, 27 January 2024 20.15
> > >>
> > >>On 2024-01-26 11:18, Morten Brørup wrote:
> > >>>>From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> > >>>>Sent: Friday, 26 January 2024 11.05
> > >>>>
> > >>>>On 2024-01-25 23:53, Morten Brørup wrote:
> > >>>>>>From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > >>>>>>Sent: Thursday, 25 January 2024 19.37
> > >>>>>>
> > >>>>>>ping.
> > >>>>>>
> > >>>>>>Please review this thread if you have time, the main point of
> > >>>>>>discussion
> > >>>>>>I would like to receive consensus on the following questions.
> > >>>>>>
> > >>>>>>1. Should we continue to expand common alignments behind an
> > >>>>__rte_macro
> > >>>>>>
> > >>>>>>     i.e. what do we prefer to appear in code
> > >>>>>>
> > >>>>>>     alignas(RTE_CACHE_LINE_MIN_SIZE)
> > >>>>>>
> > >>>>>>     -- or --
> > >>>>>>
> > >>>>>>     __rte_cache_aligned
> > >>>>>>
> > >>>>>>One of the benefits of dropping the macro is it provides a
> clear
> > >>>>visual
> > >>>>>>indicator that it is not placed in the same location or get
> > >>applied
> > >>>>>>to types as is done with __attribute__((__aligned__(n))).
> > >>>>>
> > >>>>>We don't want our own proprietary variant of something that
> already
> > >>>>exists in the C standard. Now that we have moved to C11, the
> __rte
> > >>>>alignment macros should be considered obsolete.
> > >>>>
> > >>>>Making so something cache-line aligned is not in C11.
> > >>>
> > >>>We are talking about the __rte_aligned() macro, not the cache
> > >>alignment macro.
> > >>>
> > >>
> > >>OK, in that case, what is the relevance of question 1 above?
> > >
> > >With this in mind, try re-reading Tyler's clarifications in this
> tread.
> > >
> > >Briefly: alignas() can be attached to variables and structure
> fields, but not to types (like __rte_aligned()), so to align a
> structure:
> > >
> > >struct foo {
> > >	int alignas(64) bar; /* alignas(64) must be here */
> > >	int             baz;
> > >}; /* __rte_aligned(64) was here, but alignas(64) cannot be here. */
> > >
> > >So the question is: Do we want to eliminate the __rte_aligned()
> macro - which relies on compiler attributes - and migrate to using the
> C11 standard alignas()?
> > >
> > >I think yes; after updating to C11, the workaround for pre-C11 not
> offering alignment is obsolete, and its removal should be on the
> roadmap.
> > >
> >
> > OK, thanks for the explanation. Interesting limitation in the
> standard.
> >
> > If the construct the standard is offering is less effective (in this
> > case, less readable) and the non-standard-based option is possible
> > to implement on all compilers (i.e., on MSVC too), then we should
> > keep the custom option. Especially if it's already there, but also
> > in cases where it isn't.
> >
> > In fact, one could argue *everything* related to alignment should go
> > through something rte_, __rte_ or RTE_-prefixed. So, "int
> > RTE_ALIGNAS(64) bar;". Maybe that would be silly, but it would be
> > consistent with RTE_CACHE_ALIGNAS.
> >
> > I would worry more about allowing DPDK developers writing clean and
> > readable code, than very slightly lowering the bar for the fraction
> > of newcomers experienced with the latest and greatest from the C
> > standard, and *not* familiar with age-old GCC extensions.
> 
> I’d just like to summarize where my understanding is at after reviewing
> this discussion and my downstream branch. But I also want to make it
> clear that we probably need to use both standard C and non-standard
> attribute/declspec for object and struct/union type alignment
> respectively.
> 
> I've assumed we prefer avoiding per-compiler conditional expansion when
> possible through the use of standard C mechanisms. But there are
> instances when alignas is awkward.
> 
> So I think the following is consistent with what Mattias is advocating
> sans any discussions related to actual naming of macros.
> 
> We should have 2 macros, upon which others may be built to expand to
> well-known values for e.g. cache line size.
> 
> RTE_ALIGNAS(n) object;
> 
> * This macro is used to align C objects i.e. variable, array,
> struct/union
>   fields etc.
> * Trivially expands to alignas(n) for all toolchains.
> * Placed in a location that both C and C++ translation units accept
> that
>   is on the same line preceeding the object type.
>   example:
>   // RTE_ALIGNAS(n) object;
>   RTE_ALIGNAS(16) char somearray[16];

Shouldn't the location be:

[static] [const] char RTE_ALIGNAS(16) somearray[16];

> 
> RTE_ALIGN_TYPE(n)
> 
> * This macro is used to align struct/union types.
> * Conditionally expands to __declspec(align(n)) (msvc) and
>   __attribute__((__aligned__(n))) (for all other toolchains)
> * Placed in a location that for all gcc,clang,msvc and both C and C++
>   translation units accept.
>   example:
>   // {struct,union} RTE_ALIGN_TYPE(n) tag { ... };
>   struct RTE_ALIGN_TYPE(64) sometype { ... };
> 
> I'm not picky about what the names actualy are if you have better
> suggestions i'm happy to adopt them.

Being able to align types is very convenient, and since it works on all toolchains, replacing __rte_aligned() with RTE_ALIGN() (in present tense, like "inline" not past tense like "inlined") is perfectly acceptable with me. (I suppose MSVC requires this other location when using it, so we simply have to accept that. It's a minor change only, it could have been much worse!)

Now, if we have RTE_ALIGN[_TYPE](), what do we need RTE_ALIGNAS() for?

And what is the point of introducing RTE_ALIGNAS() when the C standard already has alignas()?

I don't know why the existing alignment macros are lower case and prefixed with double underscore (__rte_macro), instead of upper case like other macros (RTE_MACRO). If someone can explain why that code convention is still relevant, the new macros should follow it; otherwise follow the code convention for macros, i.e. RTE_MACRO.

PS: #define RTE_CACHE_ALIGN RTE_ALIGN(RTE_CACHE_LINE_SIZE) for brevity still seems like a good idea to me.

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

[Mattias Rönnblom]

On 2024-01-30 09:09, Morten Brørup wrote:
>> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
>> Sent: Monday, 29 January 2024 20.44
>>
>> On Sun, Jan 28, 2024 at 11:00:31AM +0100, Mattias Rönnblom wrote:
>>> On 2024-01-28 09:57, Morten Brørup wrote:
>>>>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
>>>>> Sent: Saturday, 27 January 2024 20.15
>>>>>
>>>>> On 2024-01-26 11:18, Morten Brørup wrote:
>>>>>>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
>>>>>>> Sent: Friday, 26 January 2024 11.05
>>>>>>>
>>>>>>> On 2024-01-25 23:53, Morten Brørup wrote:
>>>>>>>>> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
>>>>>>>>> Sent: Thursday, 25 January 2024 19.37
>>>>>>>>>
>>>>>>>>> ping.
>>>>>>>>>
>>>>>>>>> Please review this thread if you have time, the main point of
>>>>>>>>> discussion
>>>>>>>>> I would like to receive consensus on the following questions.
>>>>>>>>>
>>>>>>>>> 1. Should we continue to expand common alignments behind an
>>>>>>> __rte_macro
>>>>>>>>>
>>>>>>>>>      i.e. what do we prefer to appear in code
>>>>>>>>>
>>>>>>>>>      alignas(RTE_CACHE_LINE_MIN_SIZE)
>>>>>>>>>
>>>>>>>>>      -- or --
>>>>>>>>>
>>>>>>>>>      __rte_cache_aligned
>>>>>>>>>
>>>>>>>>> One of the benefits of dropping the macro is it provides a
>> clear
>>>>>>> visual
>>>>>>>>> indicator that it is not placed in the same location or get
>>>>> applied
>>>>>>>>> to types as is done with __attribute__((__aligned__(n))).
>>>>>>>>
>>>>>>>> We don't want our own proprietary variant of something that
>> already
>>>>>>> exists in the C standard. Now that we have moved to C11, the
>> __rte
>>>>>>> alignment macros should be considered obsolete.
>>>>>>>
>>>>>>> Making so something cache-line aligned is not in C11.
>>>>>>
>>>>>> We are talking about the __rte_aligned() macro, not the cache
>>>>> alignment macro.
>>>>>>
>>>>>
>>>>> OK, in that case, what is the relevance of question 1 above?
>>>>
>>>> With this in mind, try re-reading Tyler's clarifications in this
>> tread.
>>>>
>>>> Briefly: alignas() can be attached to variables and structure
>> fields, but not to types (like __rte_aligned()), so to align a
>> structure:
>>>>
>>>> struct foo {
>>>> 	int alignas(64) bar; /* alignas(64) must be here */
>>>> 	int             baz;
>>>> }; /* __rte_aligned(64) was here, but alignas(64) cannot be here. */
>>>>
>>>> So the question is: Do we want to eliminate the __rte_aligned()
>> macro - which relies on compiler attributes - and migrate to using the
>> C11 standard alignas()?
>>>>
>>>> I think yes; after updating to C11, the workaround for pre-C11 not
>> offering alignment is obsolete, and its removal should be on the
>> roadmap.
>>>>
>>>
>>> OK, thanks for the explanation. Interesting limitation in the
>> standard.
>>>
>>> If the construct the standard is offering is less effective (in this
>>> case, less readable) and the non-standard-based option is possible
>>> to implement on all compilers (i.e., on MSVC too), then we should
>>> keep the custom option. Especially if it's already there, but also
>>> in cases where it isn't.
>>>
>>> In fact, one could argue *everything* related to alignment should go
>>> through something rte_, __rte_ or RTE_-prefixed. So, "int
>>> RTE_ALIGNAS(64) bar;". Maybe that would be silly, but it would be
>>> consistent with RTE_CACHE_ALIGNAS.
>>>
>>> I would worry more about allowing DPDK developers writing clean and
>>> readable code, than very slightly lowering the bar for the fraction
>>> of newcomers experienced with the latest and greatest from the C
>>> standard, and *not* familiar with age-old GCC extensions.
>>
>> I’d just like to summarize where my understanding is at after reviewing
>> this discussion and my downstream branch. But I also want to make it
>> clear that we probably need to use both standard C and non-standard
>> attribute/declspec for object and struct/union type alignment
>> respectively.
>>
>> I've assumed we prefer avoiding per-compiler conditional expansion when
>> possible through the use of standard C mechanisms. But there are
>> instances when alignas is awkward.
>>
>> So I think the following is consistent with what Mattias is advocating
>> sans any discussions related to actual naming of macros.
>>
>> We should have 2 macros, upon which others may be built to expand to
>> well-known values for e.g. cache line size.
>>
>> RTE_ALIGNAS(n) object;
>>
>> * This macro is used to align C objects i.e. variable, array,
>> struct/union
>>    fields etc.
>> * Trivially expands to alignas(n) for all toolchains.
>> * Placed in a location that both C and C++ translation units accept
>> that
>>    is on the same line preceeding the object type.
>>    example:
>>    // RTE_ALIGNAS(n) object;
>>    RTE_ALIGNAS(16) char somearray[16];
> 
> Shouldn't the location be:
> 
> [static] [const] char RTE_ALIGNAS(16) somearray[16];
> 
>>
>> RTE_ALIGN_TYPE(n)
>>
>> * This macro is used to align struct/union types.
>> * Conditionally expands to __declspec(align(n)) (msvc) and
>>    __attribute__((__aligned__(n))) (for all other toolchains)
>> * Placed in a location that for all gcc,clang,msvc and both C and C++
>>    translation units accept.
>>    example:
>>    // {struct,union} RTE_ALIGN_TYPE(n) tag { ... };
>>    struct RTE_ALIGN_TYPE(64) sometype { ... };
>>
>> I'm not picky about what the names actualy are if you have better
>> suggestions i'm happy to adopt them.
> 
> Being able to align types is very convenient, and since it works on all toolchains, replacing __rte_aligned() with RTE_ALIGN() (in present tense, like "inline" not past tense like "inlined") is perfectly acceptable with me. (I suppose MSVC requires this other location when using it, so we simply have to accept that. It's a minor change only, it could have been much worse!)
> 
> Now, if we have RTE_ALIGN[_TYPE](), what do we need RTE_ALIGNAS() for?
> 
> And what is the point of introducing RTE_ALIGNAS() when the C standard already has alignas()?
> 

The argument I made, which may not be a very strong one, is if you 
needed two constructs for alignment-related purposes, they should both 
have the RTE_ prefix, for consistency reasons.

> I don't know why the existing alignment macros are lower case and prefixed with double underscore (__rte_macro), instead of upper case like other macros (RTE_MACRO). If someone can explain why that code convention is still relevant, the new macros should follow it; otherwise follow the code convention for macros, i.e. RTE_MACRO.
> 

A lot the low-level DPDK stuff looks like it's borrowed from either 
Linux or *BSD kernels. __aligned(16) (Linux, FreeBSD) -> __rte_aligned(16).

> PS: #define RTE_CACHE_ALIGN RTE_ALIGN(RTE_CACHE_LINE_SIZE) for brevity still seems like a good idea to me.
> 

RTE_CACHE_ALIGN or RTE_CACHE_LINE_ALIGN?

The former is shorter, the latter consistent with RTE_CACHE_LINE_SIZE. I 
think I prefer the former.

RE: [PATCH] RFC: use C11 alignas instead of GCC attribute aligned

[Morten Brørup]

> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> Sent: Tuesday, 30 January 2024 10.28
> 
> On 2024-01-30 09:09, Morten Brørup wrote:
> >> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> >> Sent: Monday, 29 January 2024 20.44
> >>
> >> On Sun, Jan 28, 2024 at 11:00:31AM +0100, Mattias Rönnblom wrote:
> >>> On 2024-01-28 09:57, Morten Brørup wrote:
> >>>>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> >>>>> Sent: Saturday, 27 January 2024 20.15
> >>>>>
> >>>>> On 2024-01-26 11:18, Morten Brørup wrote:
> >>>>>>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> >>>>>>> Sent: Friday, 26 January 2024 11.05
> >>>>>>>
> >>>>>>> On 2024-01-25 23:53, Morten Brørup wrote:
> >>>>>>>>> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> >>>>>>>>> Sent: Thursday, 25 January 2024 19.37
> >>>>>>>>>
> >>>>>>>>> ping.
> >>>>>>>>>
> >>>>>>>>> Please review this thread if you have time, the main point of
> >>>>>>>>> discussion
> >>>>>>>>> I would like to receive consensus on the following questions.
> >>>>>>>>>
> >>>>>>>>> 1. Should we continue to expand common alignments behind an
> >>>>>>> __rte_macro
> >>>>>>>>>
> >>>>>>>>>      i.e. what do we prefer to appear in code
> >>>>>>>>>
> >>>>>>>>>      alignas(RTE_CACHE_LINE_MIN_SIZE)
> >>>>>>>>>
> >>>>>>>>>      -- or --
> >>>>>>>>>
> >>>>>>>>>      __rte_cache_aligned
> >>>>>>>>>
> >>>>>>>>> One of the benefits of dropping the macro is it provides a
> >> clear
> >>>>>>> visual
> >>>>>>>>> indicator that it is not placed in the same location or get
> >>>>> applied
> >>>>>>>>> to types as is done with __attribute__((__aligned__(n))).
> >>>>>>>>
> >>>>>>>> We don't want our own proprietary variant of something that
> >> already
> >>>>>>> exists in the C standard. Now that we have moved to C11, the
> >> __rte
> >>>>>>> alignment macros should be considered obsolete.
> >>>>>>>
> >>>>>>> Making so something cache-line aligned is not in C11.
> >>>>>>
> >>>>>> We are talking about the __rte_aligned() macro, not the cache
> >>>>> alignment macro.
> >>>>>>
> >>>>>
> >>>>> OK, in that case, what is the relevance of question 1 above?
> >>>>
> >>>> With this in mind, try re-reading Tyler's clarifications in this
> >> tread.
> >>>>
> >>>> Briefly: alignas() can be attached to variables and structure
> >> fields, but not to types (like __rte_aligned()), so to align a
> >> structure:
> >>>>
> >>>> struct foo {
> >>>> 	int alignas(64) bar; /* alignas(64) must be here */
> >>>> 	int             baz;
> >>>> }; /* __rte_aligned(64) was here, but alignas(64) cannot be here.
> */
> >>>>
> >>>> So the question is: Do we want to eliminate the __rte_aligned()
> >> macro - which relies on compiler attributes - and migrate to using
> the
> >> C11 standard alignas()?
> >>>>
> >>>> I think yes; after updating to C11, the workaround for pre-C11 not
> >> offering alignment is obsolete, and its removal should be on the
> >> roadmap.
> >>>>
> >>>
> >>> OK, thanks for the explanation. Interesting limitation in the
> >> standard.
> >>>
> >>> If the construct the standard is offering is less effective (in
> this
> >>> case, less readable) and the non-standard-based option is possible
> >>> to implement on all compilers (i.e., on MSVC too), then we should
> >>> keep the custom option. Especially if it's already there, but also
> >>> in cases where it isn't.
> >>>
> >>> In fact, one could argue *everything* related to alignment should
> go
> >>> through something rte_, __rte_ or RTE_-prefixed. So, "int
> >>> RTE_ALIGNAS(64) bar;". Maybe that would be silly, but it would be
> >>> consistent with RTE_CACHE_ALIGNAS.
> >>>
> >>> I would worry more about allowing DPDK developers writing clean and
> >>> readable code, than very slightly lowering the bar for the fraction
> >>> of newcomers experienced with the latest and greatest from the C
> >>> standard, and *not* familiar with age-old GCC extensions.
> >>
> >> I’d just like to summarize where my understanding is at after
> reviewing
> >> this discussion and my downstream branch. But I also want to make it
> >> clear that we probably need to use both standard C and non-standard
> >> attribute/declspec for object and struct/union type alignment
> >> respectively.
> >>
> >> I've assumed we prefer avoiding per-compiler conditional expansion
> when
> >> possible through the use of standard C mechanisms. But there are
> >> instances when alignas is awkward.
> >>
> >> So I think the following is consistent with what Mattias is
> advocating
> >> sans any discussions related to actual naming of macros.
> >>
> >> We should have 2 macros, upon which others may be built to expand to
> >> well-known values for e.g. cache line size.
> >>
> >> RTE_ALIGNAS(n) object;
> >>
> >> * This macro is used to align C objects i.e. variable, array,
> >> struct/union
> >>    fields etc.
> >> * Trivially expands to alignas(n) for all toolchains.
> >> * Placed in a location that both C and C++ translation units accept
> >> that
> >>    is on the same line preceeding the object type.
> >>    example:
> >>    // RTE_ALIGNAS(n) object;
> >>    RTE_ALIGNAS(16) char somearray[16];
> >
> > Shouldn't the location be:
> >
> > [static] [const] char RTE_ALIGNAS(16) somearray[16];
> >
> >>
> >> RTE_ALIGN_TYPE(n)
> >>
> >> * This macro is used to align struct/union types.
> >> * Conditionally expands to __declspec(align(n)) (msvc) and
> >>    __attribute__((__aligned__(n))) (for all other toolchains)
> >> * Placed in a location that for all gcc,clang,msvc and both C and
> C++
> >>    translation units accept.
> >>    example:
> >>    // {struct,union} RTE_ALIGN_TYPE(n) tag { ... };
> >>    struct RTE_ALIGN_TYPE(64) sometype { ... };
> >>
> >> I'm not picky about what the names actualy are if you have better
> >> suggestions i'm happy to adopt them.
> >
> > Being able to align types is very convenient, and since it works on
> all toolchains, replacing __rte_aligned() with RTE_ALIGN() (in present
> tense, like "inline" not past tense like "inlined") is perfectly
> acceptable with me. (I suppose MSVC requires this other location when
> using it, so we simply have to accept that. It's a minor change only,
> it could have been much worse!)
> >
> > Now, if we have RTE_ALIGN[_TYPE](), what do we need RTE_ALIGNAS()
> for?
> >
> > And what is the point of introducing RTE_ALIGNAS() when the C
> standard already has alignas()?
> >
> 
> The argument I made, which may not be a very strong one, is if you
> needed two constructs for alignment-related purposes, they should both
> have the RTE_ prefix, for consistency reasons.

I don't consider such consistency a strong enough reason to introduce a macro (RTE_ALIGNAS()) for something that exists 1:1 in the C standard (alignas()). It doesn't make the code any cleaner. And since we require C11, alignas() works with all toolchains.

I guess it's a matter of taste. In this case I think it is superfluous, and prefer C11 purism. :-)

> 
> > I don't know why the existing alignment macros are lower case and
> prefixed with double underscore (__rte_macro), instead of upper case
> like other macros (RTE_MACRO). If someone can explain why that code
> convention is still relevant, the new macros should follow it;
> otherwise follow the code convention for macros, i.e. RTE_MACRO.
> >
> 
> A lot the low-level DPDK stuff looks like it's borrowed from either
> Linux or *BSD kernels. __aligned(16) (Linux, FreeBSD) ->
> __rte_aligned(16).

That seems a very likely origin.
So the questions are:
1. Do Linux kernel coding conventions trump DPDK Coding Style guidelines?
2. We must change the __rte_aligned() macro, so do we keep using lower case for the new macro, or do we take the opportunity to fix it and make it upper case?

I think macros generally should be upper case, so we should make this one upper case too.
If we want to make some macros lower case, we should document when a macro can be lower case. E.g. we could allow inline function-like macros (which - unlike inline functions - can take typeless parameters) to be lower case, if they seen from the outside behave like inline functions, i.e. if they use each of their parameters exactly once.

<irony>
We should also rename likely()/unlikely() to RTE_LIKELY()/RTE_UNLIKELY()!
</irony>

> 
> > PS: #define RTE_CACHE_ALIGN RTE_ALIGN(RTE_CACHE_LINE_SIZE) for
> brevity still seems like a good idea to me.
> >
> 
> RTE_CACHE_ALIGN or RTE_CACHE_LINE_ALIGN?
> 
> The former is shorter, the latter consistent with RTE_CACHE_LINE_SIZE.
> I
> think I prefer the former.

I prefer the shorter one too.

The meaning of CACHE_ALIGN (without _LINE) is unlikely to be misunderstood. But CACHE_SIZE (without _LINE) would mean something else than CACHE_LINE_SIZE.

No strong preference on this name, though.

RE: [PATCH] RFC: use C11 alignas instead of GCC attribute aligned

[Morten Brørup]

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Tuesday, 30 January 2024 11.17
> 
> > From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> > Sent: Tuesday, 30 January 2024 10.28
> >
> > On 2024-01-30 09:09, Morten Brørup wrote:
> > >> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > >> Sent: Monday, 29 January 2024 20.44
> > >>
> > >> On Sun, Jan 28, 2024 at 11:00:31AM +0100, Mattias Rönnblom wrote:
> > >>> On 2024-01-28 09:57, Morten Brørup wrote:
> > >>>>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> > >>>>> Sent: Saturday, 27 January 2024 20.15
> > >>>>>
> > >>>>> On 2024-01-26 11:18, Morten Brørup wrote:
> > >>>>>>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> > >>>>>>> Sent: Friday, 26 January 2024 11.05
> > >>>>>>>
> > >>>>>>> On 2024-01-25 23:53, Morten Brørup wrote:
> > >>>>>>>>> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > >>>>>>>>> Sent: Thursday, 25 January 2024 19.37
> > >>>>>>>>>
> > >>>>>>>>> ping.
> > >>>>>>>>>
> > >>>>>>>>> Please review this thread if you have time, the main point
> of
> > >>>>>>>>> discussion
> > >>>>>>>>> I would like to receive consensus on the following
> questions.
> > >>>>>>>>>
> > >>>>>>>>> 1. Should we continue to expand common alignments behind an
> > >>>>>>> __rte_macro
> > >>>>>>>>>
> > >>>>>>>>>      i.e. what do we prefer to appear in code
> > >>>>>>>>>
> > >>>>>>>>>      alignas(RTE_CACHE_LINE_MIN_SIZE)
> > >>>>>>>>>
> > >>>>>>>>>      -- or --
> > >>>>>>>>>
> > >>>>>>>>>      __rte_cache_aligned
> > >>>>>>>>>
> > >>>>>>>>> One of the benefits of dropping the macro is it provides a
> > >> clear
> > >>>>>>> visual
> > >>>>>>>>> indicator that it is not placed in the same location or get
> > >>>>> applied
> > >>>>>>>>> to types as is done with __attribute__((__aligned__(n))).
> > >>>>>>>>
> > >>>>>>>> We don't want our own proprietary variant of something that
> > >> already
> > >>>>>>> exists in the C standard. Now that we have moved to C11, the
> > >> __rte
> > >>>>>>> alignment macros should be considered obsolete.
> > >>>>>>>
> > >>>>>>> Making so something cache-line aligned is not in C11.
> > >>>>>>
> > >>>>>> We are talking about the __rte_aligned() macro, not the cache
> > >>>>> alignment macro.
> > >>>>>>
> > >>>>>
> > >>>>> OK, in that case, what is the relevance of question 1 above?
> > >>>>
> > >>>> With this in mind, try re-reading Tyler's clarifications in this
> > >> tread.
> > >>>>
> > >>>> Briefly: alignas() can be attached to variables and structure
> > >> fields, but not to types (like __rte_aligned()), so to align a
> > >> structure:
> > >>>>
> > >>>> struct foo {
> > >>>> 	int alignas(64) bar; /* alignas(64) must be here */
> > >>>> 	int             baz;
> > >>>> }; /* __rte_aligned(64) was here, but alignas(64) cannot be
> here.
> > */
> > >>>>
> > >>>> So the question is: Do we want to eliminate the __rte_aligned()
> > >> macro - which relies on compiler attributes - and migrate to using
> > the
> > >> C11 standard alignas()?
> > >>>>
> > >>>> I think yes; after updating to C11, the workaround for pre-C11
> not
> > >> offering alignment is obsolete, and its removal should be on the
> > >> roadmap.
> > >>>>
> > >>>
> > >>> OK, thanks for the explanation. Interesting limitation in the
> > >> standard.
> > >>>
> > >>> If the construct the standard is offering is less effective (in
> > this
> > >>> case, less readable) and the non-standard-based option is
> possible
> > >>> to implement on all compilers (i.e., on MSVC too), then we should
> > >>> keep the custom option. Especially if it's already there, but
> also
> > >>> in cases where it isn't.
> > >>>
> > >>> In fact, one could argue *everything* related to alignment should
> > go
> > >>> through something rte_, __rte_ or RTE_-prefixed. So, "int
> > >>> RTE_ALIGNAS(64) bar;". Maybe that would be silly, but it would be
> > >>> consistent with RTE_CACHE_ALIGNAS.
> > >>>
> > >>> I would worry more about allowing DPDK developers writing clean
> and
> > >>> readable code, than very slightly lowering the bar for the
> fraction
> > >>> of newcomers experienced with the latest and greatest from the C
> > >>> standard, and *not* familiar with age-old GCC extensions.
> > >>
> > >> I’d just like to summarize where my understanding is at after
> > reviewing
> > >> this discussion and my downstream branch. But I also want to make
> it
> > >> clear that we probably need to use both standard C and non-
> standard
> > >> attribute/declspec for object and struct/union type alignment
> > >> respectively.
> > >>
> > >> I've assumed we prefer avoiding per-compiler conditional expansion
> > when
> > >> possible through the use of standard C mechanisms. But there are
> > >> instances when alignas is awkward.
> > >>
> > >> So I think the following is consistent with what Mattias is
> > advocating
> > >> sans any discussions related to actual naming of macros.
> > >>
> > >> We should have 2 macros, upon which others may be built to expand
> to
> > >> well-known values for e.g. cache line size.
> > >>
> > >> RTE_ALIGNAS(n) object;
> > >>
> > >> * This macro is used to align C objects i.e. variable, array,
> > >> struct/union
> > >>    fields etc.
> > >> * Trivially expands to alignas(n) for all toolchains.
> > >> * Placed in a location that both C and C++ translation units
> accept
> > >> that
> > >>    is on the same line preceeding the object type.
> > >>    example:
> > >>    // RTE_ALIGNAS(n) object;
> > >>    RTE_ALIGNAS(16) char somearray[16];
> > >
> > > Shouldn't the location be:
> > >
> > > [static] [const] char RTE_ALIGNAS(16) somearray[16];
> > >
> > >>
> > >> RTE_ALIGN_TYPE(n)
> > >>
> > >> * This macro is used to align struct/union types.
> > >> * Conditionally expands to __declspec(align(n)) (msvc) and
> > >>    __attribute__((__aligned__(n))) (for all other toolchains)
> > >> * Placed in a location that for all gcc,clang,msvc and both C and
> > C++
> > >>    translation units accept.
> > >>    example:
> > >>    // {struct,union} RTE_ALIGN_TYPE(n) tag { ... };
> > >>    struct RTE_ALIGN_TYPE(64) sometype { ... };
> > >>
> > >> I'm not picky about what the names actualy are if you have better
> > >> suggestions i'm happy to adopt them.
> > >
> > > Being able to align types is very convenient, and since it works on
> > all toolchains, replacing __rte_aligned() with RTE_ALIGN() (in
> present
> > tense, like "inline" not past tense like "inlined") is perfectly
> > acceptable with me. (I suppose MSVC requires this other location when
> > using it, so we simply have to accept that. It's a minor change only,
> > it could have been much worse!)
> > >
> > > Now, if we have RTE_ALIGN[_TYPE](), what do we need RTE_ALIGNAS()
> > for?
> > >
> > > And what is the point of introducing RTE_ALIGNAS() when the C
> > standard already has alignas()?
> > >
> >
> > The argument I made, which may not be a very strong one, is if you
> > needed two constructs for alignment-related purposes, they should
> both
> > have the RTE_ prefix, for consistency reasons.
> 
> I don't consider such consistency a strong enough reason to introduce a
> macro (RTE_ALIGNAS()) for something that exists 1:1 in the C standard
> (alignas()). It doesn't make the code any cleaner. And since we require
> C11, alignas() works with all toolchains.
> 
> I guess it's a matter of taste. In this case I think it is superfluous,
> and prefer C11 purism. :-)
> 
> >
> > > I don't know why the existing alignment macros are lower case and
> > prefixed with double underscore (__rte_macro), instead of upper case
> > like other macros (RTE_MACRO). If someone can explain why that code
> > convention is still relevant, the new macros should follow it;
> > otherwise follow the code convention for macros, i.e. RTE_MACRO.
> > >
> >
> > A lot the low-level DPDK stuff looks like it's borrowed from either
> > Linux or *BSD kernels. __aligned(16) (Linux, FreeBSD) ->
> > __rte_aligned(16).
> 
> That seems a very likely origin.
> So the questions are:
> 1. Do Linux kernel coding conventions trump DPDK Coding Style
> guidelines?
> 2. We must change the __rte_aligned() macro, so do we keep using lower
> case for the new macro, or do we take the opportunity to fix it and
> make it upper case?
> 
> I think macros generally should be upper case, so we should make this
> one upper case too.

I just realized that the macros in rte_common.h related to attributes are all lower case and "__" prefixed.
I guess it's an undocumented convention, so we should probably stick with it.

That would make the new macro's name "__rte_align()", which is really close to the "__rte_aligned()" it replaces. It doesn't bother me, but let's see if anyone complains about it.

> If we want to make some macros lower case, we should document when a
> macro can be lower case. E.g. we could allow inline function-like
> macros (which - unlike inline functions - can take typeless parameters)
> to be lower case, if they seen from the outside behave like inline
> functions, i.e. if they use each of their parameters exactly once.
> 
> <irony>
> We should also rename likely()/unlikely() to
> RTE_LIKELY()/RTE_UNLIKELY()!
> </irony>
> 
> >
> > > PS: #define RTE_CACHE_ALIGN RTE_ALIGN(RTE_CACHE_LINE_SIZE) for
> > brevity still seems like a good idea to me.
> > >
> >
> > RTE_CACHE_ALIGN or RTE_CACHE_LINE_ALIGN?
> >
> > The former is shorter, the latter consistent with
> RTE_CACHE_LINE_SIZE.
> > I
> > think I prefer the former.
> 
> I prefer the shorter one too.
> 
> The meaning of CACHE_ALIGN (without _LINE) is unlikely to be
> misunderstood. But CACHE_SIZE (without _LINE) would mean something else
> than CACHE_LINE_SIZE.
> 
> No strong preference on this name, though.

The convenience macro should probably follow the attribute macro naming convention too:
#define __rte_cache_align __rte_align(RTE_CACHE_LINE_SIZE)

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

[Tyler Retzlaff]

On Tue, Jan 30, 2024 at 09:08:21AM +0100, Mattias Rönnblom wrote:
> On 2024-01-29 20:43, Tyler Retzlaff wrote:
> >On Sun, Jan 28, 2024 at 11:00:31AM +0100, Mattias Rönnblom wrote:
> >>On 2024-01-28 09:57, Morten Brørup wrote:
> >>>>From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> >>>>Sent: Saturday, 27 January 2024 20.15
> >>>>
> >>>>On 2024-01-26 11:18, Morten Brørup wrote:
> >>>>>>From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> >>>>>>Sent: Friday, 26 January 2024 11.05
> >>>>>>
> >>>>>>On 2024-01-25 23:53, Morten Brørup wrote:
> >>>>>>>>From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> >>>>>>>>Sent: Thursday, 25 January 2024 19.37
> >>>>>>>>
> >>>>>>>>ping.
> >>>>>>>>
> >>>>>>>>Please review this thread if you have time, the main point of
> >>>>>>>>discussion
> >>>>>>>>I would like to receive consensus on the following questions.
> >>>>>>>>
> >>>>>>>>1. Should we continue to expand common alignments behind an
> >>>>>>__rte_macro
> >>>>>>>>
> >>>>>>>>     i.e. what do we prefer to appear in code
> >>>>>>>>
> >>>>>>>>     alignas(RTE_CACHE_LINE_MIN_SIZE)
> >>>>>>>>
> >>>>>>>>     -- or --
> >>>>>>>>
> >>>>>>>>     __rte_cache_aligned
> >>>>>>>>
> >>>>>>>>One of the benefits of dropping the macro is it provides a clear
> >>>>>>visual
> >>>>>>>>indicator that it is not placed in the same location or get
> >>>>applied
> >>>>>>>>to types as is done with __attribute__((__aligned__(n))).
> >>>>>>>
> >>>>>>>We don't want our own proprietary variant of something that already
> >>>>>>exists in the C standard. Now that we have moved to C11, the __rte
> >>>>>>alignment macros should be considered obsolete.
> >>>>>>
> >>>>>>Making so something cache-line aligned is not in C11.
> >>>>>
> >>>>>We are talking about the __rte_aligned() macro, not the cache
> >>>>alignment macro.
> >>>>>
> >>>>
> >>>>OK, in that case, what is the relevance of question 1 above?
> >>>
> >>>With this in mind, try re-reading Tyler's clarifications in this tread.
> >>>
> >>>Briefly: alignas() can be attached to variables and structure fields, but not to types (like __rte_aligned()), so to align a structure:
> >>>
> >>>struct foo {
> >>>	int alignas(64) bar; /* alignas(64) must be here */
> >>>	int             baz;
> >>>}; /* __rte_aligned(64) was here, but alignas(64) cannot be here. */
> >>>
> >>>So the question is: Do we want to eliminate the __rte_aligned() macro - which relies on compiler attributes - and migrate to using the C11 standard alignas()?
> >>>
> >>>I think yes; after updating to C11, the workaround for pre-C11 not offering alignment is obsolete, and its removal should be on the roadmap.
> >>>
> >>
> >>OK, thanks for the explanation. Interesting limitation in the standard.
> >>
> >>If the construct the standard is offering is less effective (in this
> >>case, less readable) and the non-standard-based option is possible
> >>to implement on all compilers (i.e., on MSVC too), then we should
> >>keep the custom option. Especially if it's already there, but also
> >>in cases where it isn't.
> >>
> >>In fact, one could argue *everything* related to alignment should go
> >>through something rte_, __rte_ or RTE_-prefixed. So, "int
> >>RTE_ALIGNAS(64) bar;". Maybe that would be silly, but it would be
> >>consistent with RTE_CACHE_ALIGNAS.
> >>
> >>I would worry more about allowing DPDK developers writing clean and
> >>readable code, than very slightly lowering the bar for the fraction
> >>of newcomers experienced with the latest and greatest from the C
> >>standard, and *not* familiar with age-old GCC extensions.
> >
> >I’d just like to summarize where my understanding is at after reviewing
> >this discussion and my downstream branch. But I also want to make it
> >clear that we probably need to use both standard C and non-standard
> >attribute/declspec for object and struct/union type alignment
> >respectively.
> >
> >I've assumed we prefer avoiding per-compiler conditional expansion when
> >possible through the use of standard C mechanisms. But there are
> >instances when alignas is awkward.
> >
> >So I think the following is consistent with what Mattias is advocating
> >sans any discussions related to actual naming of macros.
> >
> >We should have 2 macros, upon which others may be built to expand to
> >well-known values for e.g. cache line size.
> >
> >RTE_ALIGNAS(n) object;
> >
> >* This macro is used to align C objects i.e. variable, array, struct/union
> >   fields etc.
> >* Trivially expands to alignas(n) for all toolchains.
> >* Placed in a location that both C and C++ translation units accept that
> >   is on the same line preceeding the object type.
> >   example:
> >   // RTE_ALIGNAS(n) object;
> >   RTE_ALIGNAS(16) char somearray[16];
> >
> >RTE_ALIGN_TYPE(n)
> >
> >* This macro is used to align struct/union types.
> >* Conditionally expands to __declspec(align(n)) (msvc) and
> >   __attribute__((__aligned__(n))) (for all other toolchains)
> >* Placed in a location that for all gcc,clang,msvc and both C and C++
> >   translation units accept.
> >   example:
> >   // {struct,union} RTE_ALIGN_TYPE(n) tag { ... };
> >   struct RTE_ALIGN_TYPE(64) sometype { ... };
> >
> 
> Sorry if I've missed some discussion on the list, but the current
> pattern of putting __rte_aligned(X) at the end doesn't work with
> MSVC, or why are we doing this? C11 purism doesn't seem like much of
> a driving force.

__rte_aligned(X) at the end doesn't work with MSVC __declspec(align(n))

> 
> If one defined a macro as __declspec(align(X)) on MSVC and
> __attribute__(__aligned__(X)) on other compilers, could it do the
> work of both the above RTE_ALIGNAS() and RTE_ALIGN_TYPE()?
> 
> <a> struct <b> { int a; } <c>;

yes for struct/union. but only when placed at location you mark as <b>
when compiling both C and C++ for all toolchains.

maybe, for objects but ideally, we prefer alignas for consistent semantics
defined by standard rather than accomodating potential implementation
differences when conditionally expanding __aligned vs __declspec. as you
have noted __declspec has limitations/variations when compared to
__attribute__((__aligned__(n))).

> 
> You would have to mandate the placement of such a __rte_aligned
> plug-in replacement being at <b> rather than (the more intuitive?)
> <a>, since clang doesn't like __attribute__s before the struct/union
> keyword, correct?

for struct/union there is a single placement accepted by all toolchains
for both C and C++ and it is <b>.

> 
> What about other <rte_common.h> __attribute__ wrappers like
> __rte_packed; would they also need to change placement to make DPDK
> work with MSVC?

packing is a different problem that needs a separate RFC and discussion
of it's own.

> 
> >I'm not picky about what the names actualy are if you have better
> >suggestions i'm happy to adopt them.
> >
> >Thoughts? Comments?
> >
> >Appreciate the discussion this has been helpful.
> >
> >ty
> >

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

[Tyler Retzlaff]

On Tue, Jan 30, 2024 at 09:09:20AM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Monday, 29 January 2024 20.44
> > 
> > On Sun, Jan 28, 2024 at 11:00:31AM +0100, Mattias Rönnblom wrote:
> > > On 2024-01-28 09:57, Morten Brørup wrote:
> > > >>From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> > > >>Sent: Saturday, 27 January 2024 20.15
> > > >>
> > > >>On 2024-01-26 11:18, Morten Brørup wrote:
> > > >>>>From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> > > >>>>Sent: Friday, 26 January 2024 11.05
> > > >>>>
> > > >>>>On 2024-01-25 23:53, Morten Brørup wrote:
> > > >>>>>>From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > >>>>>>Sent: Thursday, 25 January 2024 19.37
> > > >>>>>>
> > > >>>>>>ping.
> > > >>>>>>
> > > >>>>>>Please review this thread if you have time, the main point of
> > > >>>>>>discussion
> > > >>>>>>I would like to receive consensus on the following questions.
> > > >>>>>>
> > > >>>>>>1. Should we continue to expand common alignments behind an
> > > >>>>__rte_macro
> > > >>>>>>
> > > >>>>>>     i.e. what do we prefer to appear in code
> > > >>>>>>
> > > >>>>>>     alignas(RTE_CACHE_LINE_MIN_SIZE)
> > > >>>>>>
> > > >>>>>>     -- or --
> > > >>>>>>
> > > >>>>>>     __rte_cache_aligned
> > > >>>>>>
> > > >>>>>>One of the benefits of dropping the macro is it provides a
> > clear
> > > >>>>visual
> > > >>>>>>indicator that it is not placed in the same location or get
> > > >>applied
> > > >>>>>>to types as is done with __attribute__((__aligned__(n))).
> > > >>>>>
> > > >>>>>We don't want our own proprietary variant of something that
> > already
> > > >>>>exists in the C standard. Now that we have moved to C11, the
> > __rte
> > > >>>>alignment macros should be considered obsolete.
> > > >>>>
> > > >>>>Making so something cache-line aligned is not in C11.
> > > >>>
> > > >>>We are talking about the __rte_aligned() macro, not the cache
> > > >>alignment macro.
> > > >>>
> > > >>
> > > >>OK, in that case, what is the relevance of question 1 above?
> > > >
> > > >With this in mind, try re-reading Tyler's clarifications in this
> > tread.
> > > >
> > > >Briefly: alignas() can be attached to variables and structure
> > fields, but not to types (like __rte_aligned()), so to align a
> > structure:
> > > >
> > > >struct foo {
> > > >	int alignas(64) bar; /* alignas(64) must be here */
> > > >	int             baz;
> > > >}; /* __rte_aligned(64) was here, but alignas(64) cannot be here. */
> > > >
> > > >So the question is: Do we want to eliminate the __rte_aligned()
> > macro - which relies on compiler attributes - and migrate to using the
> > C11 standard alignas()?
> > > >
> > > >I think yes; after updating to C11, the workaround for pre-C11 not
> > offering alignment is obsolete, and its removal should be on the
> > roadmap.
> > > >
> > >
> > > OK, thanks for the explanation. Interesting limitation in the
> > standard.
> > >
> > > If the construct the standard is offering is less effective (in this
> > > case, less readable) and the non-standard-based option is possible
> > > to implement on all compilers (i.e., on MSVC too), then we should
> > > keep the custom option. Especially if it's already there, but also
> > > in cases where it isn't.
> > >
> > > In fact, one could argue *everything* related to alignment should go
> > > through something rte_, __rte_ or RTE_-prefixed. So, "int
> > > RTE_ALIGNAS(64) bar;". Maybe that would be silly, but it would be
> > > consistent with RTE_CACHE_ALIGNAS.
> > >
> > > I would worry more about allowing DPDK developers writing clean and
> > > readable code, than very slightly lowering the bar for the fraction
> > > of newcomers experienced with the latest and greatest from the C
> > > standard, and *not* familiar with age-old GCC extensions.
> > 
> > I’d just like to summarize where my understanding is at after reviewing
> > this discussion and my downstream branch. But I also want to make it
> > clear that we probably need to use both standard C and non-standard
> > attribute/declspec for object and struct/union type alignment
> > respectively.
> > 
> > I've assumed we prefer avoiding per-compiler conditional expansion when
> > possible through the use of standard C mechanisms. But there are
> > instances when alignas is awkward.
> > 
> > So I think the following is consistent with what Mattias is advocating
> > sans any discussions related to actual naming of macros.
> > 
> > We should have 2 macros, upon which others may be built to expand to
> > well-known values for e.g. cache line size.
> > 
> > RTE_ALIGNAS(n) object;
> > 
> > * This macro is used to align C objects i.e. variable, array,
> > struct/union
> >   fields etc.
> > * Trivially expands to alignas(n) for all toolchains.
> > * Placed in a location that both C and C++ translation units accept
> > that
> >   is on the same line preceeding the object type.
> >   example:
> >   // RTE_ALIGNAS(n) object;
> >   RTE_ALIGNAS(16) char somearray[16];
> 
> Shouldn't the location be:
> 
> [static] [const] char RTE_ALIGNAS(16) somearray[16];
> 
> > 
> > RTE_ALIGN_TYPE(n)
> > 
> > * This macro is used to align struct/union types.
> > * Conditionally expands to __declspec(align(n)) (msvc) and
> >   __attribute__((__aligned__(n))) (for all other toolchains)
> > * Placed in a location that for all gcc,clang,msvc and both C and C++
> >   translation units accept.
> >   example:
> >   // {struct,union} RTE_ALIGN_TYPE(n) tag { ... };
> >   struct RTE_ALIGN_TYPE(64) sometype { ... };
> > 
> > I'm not picky about what the names actualy are if you have better
> > suggestions i'm happy to adopt them.
> 
> Being able to align types is very convenient, and since it works on all toolchains, replacing __rte_aligned() with RTE_ALIGN() (in present tense, like "inline" not past tense like "inlined") is perfectly acceptable with me. (I suppose MSVC requires this other location when using it, so we simply have to accept that. It's a minor change only, it could have been much worse!)

* naming suggestion noted.
* __declspec cannot be placed after struct/union definition.
  note: there are some structs in dpdk already placing it where
        accepted by msvc (not many but a handful).
> 
> Now, if we have RTE_ALIGN[_TYPE](), what do we need RTE_ALIGNAS() for?
> 
> And what is the point of introducing RTE_ALIGNAS() when the C standard already has alignas()?

so this seems to be unresolved contention? for object alignment do we
want a macro or not which just trivially expands to alignas(). i comment
on this a bit below.

> 
> I don't know why the existing alignment macros are lower case and prefixed with double underscore (__rte_macro), instead of upper case like other macros (RTE_MACRO). If someone can explain why that code convention is still relevant, the new macros should follow it; otherwise follow the code convention for macros, i.e. RTE_MACRO.

my best guess of __rte_macro vs RTE_MACRO as a convention interpretation
has typically been.

__rte_macro
expresses that the macro is private, while exposed by dpdk publicly as a
necessity is meant for dpdk use only. it is not a supported api and
may change at any time without notice.

RTE_MACRO
expresses that the macro is public and part of the api and probably
imposes the usual obligations.

interestingly this maybe lends some weight to the argument that we
should use alignas(a) directly to dance around having to provide
something that looks like an api either privately or publicly for the
cases where we can use alignas(a).

second, it probably means we continue to use __rte_aligned/__rte_cache_align
for the expansions of the non-standard attributes since i am not
propsing promotion of these to be any kind of a supported/formal api.

> 
> PS: #define RTE_CACHE_ALIGN RTE_ALIGN(RTE_CACHE_LINE_SIZE) for brevity still seems like a good idea to me.

so given the above we would stay with __rte_align(a)

> 

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

[Bruce Richardson]

On Tue, Jan 30, 2024 at 09:39:28AM -0800, Tyler Retzlaff wrote:
> On Tue, Jan 30, 2024 at 09:08:21AM +0100, Mattias Rönnblom wrote:
<snip>
> > 
> > Sorry if I've missed some discussion on the list, but the current
> > pattern of putting __rte_aligned(X) at the end doesn't work with MSVC,
> > or why are we doing this? C11 purism doesn't seem like much of a
> > driving force.
> 
> __rte_aligned(X) at the end doesn't work with MSVC __declspec(align(n))
> 
> > 
> > If one defined a macro as __declspec(align(X)) on MSVC and
> > __attribute__(__aligned__(X)) on other compilers, could it do the work
> > of both the above RTE_ALIGNAS() and RTE_ALIGN_TYPE()?
> > 
> > <a> struct <b> { int a; } <c>;
> 
> yes for struct/union. but only when placed at location you mark as <b>
> when compiling both C and C++ for all toolchains.
> 
I can see this restriction on placement potentially causing problems. Maybe
we should consider defining macros with the "struct" keywork included. For
example, (using gcc attributes here):

#define rte_aligned_struct(n) struct __attribute((aligned(n)))

rte_aligned_struct my_struct {
	int a;
}

Probably that's taking things a bit far away from standard C, but it may
cut down on placement errors.

/Bruce

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

[Bruce Richardson]

On Tue, Jan 30, 2024 at 05:59:25PM +0000, Bruce Richardson wrote:
> On Tue, Jan 30, 2024 at 09:39:28AM -0800, Tyler Retzlaff wrote:
> > On Tue, Jan 30, 2024 at 09:08:21AM +0100, Mattias Rönnblom wrote:
> <snip>
> > > 
> > > Sorry if I've missed some discussion on the list, but the current
> > > pattern of putting __rte_aligned(X) at the end doesn't work with MSVC,
> > > or why are we doing this? C11 purism doesn't seem like much of a
> > > driving force.
> > 
> > __rte_aligned(X) at the end doesn't work with MSVC __declspec(align(n))
> > 
> > > 
> > > If one defined a macro as __declspec(align(X)) on MSVC and
> > > __attribute__(__aligned__(X)) on other compilers, could it do the work
> > > of both the above RTE_ALIGNAS() and RTE_ALIGN_TYPE()?
> > > 
> > > <a> struct <b> { int a; } <c>;
> > 
> > yes for struct/union. but only when placed at location you mark as <b>
> > when compiling both C and C++ for all toolchains.
> > 
> I can see this restriction on placement potentially causing problems. Maybe
> we should consider defining macros with the "struct" keywork included. For
> example, (using gcc attributes here):
> 

Sorry, corrected example below, though I'm sure the idea was clear from the
previous mail!

#define rte_aligned_struct(n) struct __attribute((aligned(n)))

rte_aligned_struct(32) my_struct {
	int a;
}

> 
> Probably that's taking things a bit far away from standard C, but it may
> cut down on placement errors.
> 
> /Bruce

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

[Tyler Retzlaff]

On Tue, Jan 30, 2024 at 05:59:25PM +0000, Bruce Richardson wrote:
> On Tue, Jan 30, 2024 at 09:39:28AM -0800, Tyler Retzlaff wrote:
> > On Tue, Jan 30, 2024 at 09:08:21AM +0100, Mattias Rönnblom wrote:
> <snip>
> > > 
> > > Sorry if I've missed some discussion on the list, but the current
> > > pattern of putting __rte_aligned(X) at the end doesn't work with MSVC,
> > > or why are we doing this? C11 purism doesn't seem like much of a
> > > driving force.
> > 
> > __rte_aligned(X) at the end doesn't work with MSVC __declspec(align(n))
> > 
> > > 
> > > If one defined a macro as __declspec(align(X)) on MSVC and
> > > __attribute__(__aligned__(X)) on other compilers, could it do the work
> > > of both the above RTE_ALIGNAS() and RTE_ALIGN_TYPE()?
> > > 
> > > <a> struct <b> { int a; } <c>;
> > 
> > yes for struct/union. but only when placed at location you mark as <b>
> > when compiling both C and C++ for all toolchains.
> > 
> I can see this restriction on placement potentially causing problems. Maybe
> we should consider defining macros with the "struct" keywork included. For
> example, (using gcc attributes here):

i had considered this but it might be overkill.
* it will be picked up by windows/msvc ci build.
* once established as the common visual pattern in the source it will be
  cut n' pasted at low rate of error.

> 
> #define rte_aligned_struct(n) struct __attribute((aligned(n)))
> 
> rte_aligned_struct my_struct {
> 	int a;
> }
> 
> Probably that's taking things a bit far away from standard C, but it may
> cut down on placement errors.
> 
> /Bruce

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

[Mattias Rönnblom]

On 2024-01-30 18:59, Bruce Richardson wrote:
> On Tue, Jan 30, 2024 at 09:39:28AM -0800, Tyler Retzlaff wrote:
>> On Tue, Jan 30, 2024 at 09:08:21AM +0100, Mattias Rönnblom wrote:
> <snip>
>>>
>>> Sorry if I've missed some discussion on the list, but the current
>>> pattern of putting __rte_aligned(X) at the end doesn't work with MSVC,
>>> or why are we doing this? C11 purism doesn't seem like much of a
>>> driving force.
>>
>> __rte_aligned(X) at the end doesn't work with MSVC __declspec(align(n))
>>
>>>
>>> If one defined a macro as __declspec(align(X)) on MSVC and
>>> __attribute__(__aligned__(X)) on other compilers, could it do the work
>>> of both the above RTE_ALIGNAS() and RTE_ALIGN_TYPE()?
>>>
>>> <a> struct <b> { int a; } <c>;
>>
>> yes for struct/union. but only when placed at location you mark as <b>
>> when compiling both C and C++ for all toolchains.
>>
> I can see this restriction on placement potentially causing problems. Maybe
> we should consider defining macros with the "struct" keywork included. For
> example, (using gcc attributes here):
> 
> #define rte_aligned_struct(n) struct __attribute((aligned(n)))
> 
> rte_aligned_struct my_struct {
> 	int a;
> }
> 
> Probably that's taking things a bit far away from standard C, but it may
> cut down on placement errors.

It doesn't go well with the fact alignment is just one of several 
attributes one may want to add to a struct (__rte_packed is another).

A quick scan of the DPDK source tree suggests DPDK developers are pretty 
good at putting the old __rte_cache_aligned consistently after the 
struct declaration (i.e., position <c> per above). Conservative as they 
may be, perhaps they could be rewired to consistently put it somewhere else.

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

[Mattias Rönnblom]

On 2024-01-30 18:39, Tyler Retzlaff wrote:
> On Tue, Jan 30, 2024 at 09:08:21AM +0100, Mattias Rönnblom wrote:
>> On 2024-01-29 20:43, Tyler Retzlaff wrote:
>>> On Sun, Jan 28, 2024 at 11:00:31AM +0100, Mattias Rönnblom wrote:
>>>> On 2024-01-28 09:57, Morten Brørup wrote:
>>>>>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
>>>>>> Sent: Saturday, 27 January 2024 20.15
>>>>>>
>>>>>> On 2024-01-26 11:18, Morten Brørup wrote:
>>>>>>>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
>>>>>>>> Sent: Friday, 26 January 2024 11.05
>>>>>>>>
>>>>>>>> On 2024-01-25 23:53, Morten Brørup wrote:
>>>>>>>>>> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
>>>>>>>>>> Sent: Thursday, 25 January 2024 19.37
>>>>>>>>>>
>>>>>>>>>> ping.
>>>>>>>>>>
>>>>>>>>>> Please review this thread if you have time, the main point of
>>>>>>>>>> discussion
>>>>>>>>>> I would like to receive consensus on the following questions.
>>>>>>>>>>
>>>>>>>>>> 1. Should we continue to expand common alignments behind an
>>>>>>>> __rte_macro
>>>>>>>>>>
>>>>>>>>>>      i.e. what do we prefer to appear in code
>>>>>>>>>>
>>>>>>>>>>      alignas(RTE_CACHE_LINE_MIN_SIZE)
>>>>>>>>>>
>>>>>>>>>>      -- or --
>>>>>>>>>>
>>>>>>>>>>      __rte_cache_aligned
>>>>>>>>>>
>>>>>>>>>> One of the benefits of dropping the macro is it provides a clear
>>>>>>>> visual
>>>>>>>>>> indicator that it is not placed in the same location or get
>>>>>> applied
>>>>>>>>>> to types as is done with __attribute__((__aligned__(n))).
>>>>>>>>>
>>>>>>>>> We don't want our own proprietary variant of something that already
>>>>>>>> exists in the C standard. Now that we have moved to C11, the __rte
>>>>>>>> alignment macros should be considered obsolete.
>>>>>>>>
>>>>>>>> Making so something cache-line aligned is not in C11.
>>>>>>>
>>>>>>> We are talking about the __rte_aligned() macro, not the cache
>>>>>> alignment macro.
>>>>>>>
>>>>>>
>>>>>> OK, in that case, what is the relevance of question 1 above?
>>>>>
>>>>> With this in mind, try re-reading Tyler's clarifications in this tread.
>>>>>
>>>>> Briefly: alignas() can be attached to variables and structure fields, but not to types (like __rte_aligned()), so to align a structure:
>>>>>
>>>>> struct foo {
>>>>> 	int alignas(64) bar; /* alignas(64) must be here */
>>>>> 	int             baz;
>>>>> }; /* __rte_aligned(64) was here, but alignas(64) cannot be here. */
>>>>>
>>>>> So the question is: Do we want to eliminate the __rte_aligned() macro - which relies on compiler attributes - and migrate to using the C11 standard alignas()?
>>>>>
>>>>> I think yes; after updating to C11, the workaround for pre-C11 not offering alignment is obsolete, and its removal should be on the roadmap.
>>>>>
>>>>
>>>> OK, thanks for the explanation. Interesting limitation in the standard.
>>>>
>>>> If the construct the standard is offering is less effective (in this
>>>> case, less readable) and the non-standard-based option is possible
>>>> to implement on all compilers (i.e., on MSVC too), then we should
>>>> keep the custom option. Especially if it's already there, but also
>>>> in cases where it isn't.
>>>>
>>>> In fact, one could argue *everything* related to alignment should go
>>>> through something rte_, __rte_ or RTE_-prefixed. So, "int
>>>> RTE_ALIGNAS(64) bar;". Maybe that would be silly, but it would be
>>>> consistent with RTE_CACHE_ALIGNAS.
>>>>
>>>> I would worry more about allowing DPDK developers writing clean and
>>>> readable code, than very slightly lowering the bar for the fraction
>>>> of newcomers experienced with the latest and greatest from the C
>>>> standard, and *not* familiar with age-old GCC extensions.
>>>
>>> I’d just like to summarize where my understanding is at after reviewing
>>> this discussion and my downstream branch. But I also want to make it
>>> clear that we probably need to use both standard C and non-standard
>>> attribute/declspec for object and struct/union type alignment
>>> respectively.
>>>
>>> I've assumed we prefer avoiding per-compiler conditional expansion when
>>> possible through the use of standard C mechanisms. But there are
>>> instances when alignas is awkward.
>>>
>>> So I think the following is consistent with what Mattias is advocating
>>> sans any discussions related to actual naming of macros.
>>>
>>> We should have 2 macros, upon which others may be built to expand to
>>> well-known values for e.g. cache line size.
>>>
>>> RTE_ALIGNAS(n) object;
>>>
>>> * This macro is used to align C objects i.e. variable, array, struct/union
>>>    fields etc.
>>> * Trivially expands to alignas(n) for all toolchains.
>>> * Placed in a location that both C and C++ translation units accept that
>>>    is on the same line preceeding the object type.
>>>    example:
>>>    // RTE_ALIGNAS(n) object;
>>>    RTE_ALIGNAS(16) char somearray[16];
>>>
>>> RTE_ALIGN_TYPE(n)
>>>
>>> * This macro is used to align struct/union types.
>>> * Conditionally expands to __declspec(align(n)) (msvc) and
>>>    __attribute__((__aligned__(n))) (for all other toolchains)
>>> * Placed in a location that for all gcc,clang,msvc and both C and C++
>>>    translation units accept.
>>>    example:
>>>    // {struct,union} RTE_ALIGN_TYPE(n) tag { ... };
>>>    struct RTE_ALIGN_TYPE(64) sometype { ... };
>>>
>>
>> Sorry if I've missed some discussion on the list, but the current
>> pattern of putting __rte_aligned(X) at the end doesn't work with
>> MSVC, or why are we doing this? C11 purism doesn't seem like much of
>> a driving force.
> 
> __rte_aligned(X) at the end doesn't work with MSVC __declspec(align(n))
> 
>>
>> If one defined a macro as __declspec(align(X)) on MSVC and
>> __attribute__(__aligned__(X)) on other compilers, could it do the
>> work of both the above RTE_ALIGNAS() and RTE_ALIGN_TYPE()?
>>
>> <a> struct <b> { int a; } <c>;
> 
> yes for struct/union. but only when placed at location you mark as <b>
> when compiling both C and C++ for all toolchains.
> 
> maybe, for objects but ideally, we prefer alignas for consistent semantics
> defined by standard rather than accomodating potential implementation
> differences when conditionally expanding __aligned vs __declspec. as you
> have noted __declspec has limitations/variations when compared to
> __attribute__((__aligned__(n))).
> 
>>
>> You would have to mandate the placement of such a __rte_aligned
>> plug-in replacement being at <b> rather than (the more intuitive?)
>> <a>, since clang doesn't like __attribute__s before the struct/union
>> keyword, correct?
> 
> for struct/union there is a single placement accepted by all toolchains
> for both C and C++ and it is <b>.
> 
>>
>> What about other <rte_common.h> __attribute__ wrappers like
>> __rte_packed; would they also need to change placement to make DPDK
>> work with MSVC?
> 
> packing is a different problem that needs a separate RFC and discussion
> of it's own.
> 

Seems like the same kind of problem with potentially the exact same 
solution: mandate a new "__rte_xxx" placement and use MSVC __declspec.

Different RFC, yes, different discussion: not so sure.

>>
>>> I'm not picky about what the names actualy are if you have better
>>> suggestions i'm happy to adopt them.
>>>
>>> Thoughts? Comments?
>>>
>>> Appreciate the discussion this has been helpful.
>>>
>>> ty
>>>