RE: [PATCH 01/83] examples: move alignment attribute on types

["Morten Brørup" <mb@smartsharesystems.com>]

+To: David Hunt, Distributor maintainer
+To: Radu Nicolau and Akhil Goyal, IPsec security gateway example maintainers

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Wednesday, 20 March 2024 16.37
> 
> Move location of __rte_aligned(a) to new conventional location. The new
> placement between {struct,union} and the tag allows the desired
> alignment to be imparted on the type regardless of the toolchain being
> used for both C and C++. Additionally, it avoids confusion by Doxygen
> when generating documentation.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---

Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

A few comments for the above mentioned maintainers inline below.

> diff --git a/examples/distributor/main.c b/examples/distributor/main.c
> index 542f76c..ddbc387 100644
> --- a/examples/distributor/main.c
> +++ b/examples/distributor/main.c
> @@ -44,39 +44,39 @@
>  unsigned int num_workers;
> 
>  static volatile struct app_stats {
> -	struct {
> +	alignas(RTE_CACHE_LINE_SIZE) struct {
>  		uint64_t rx_pkts;
>  		uint64_t returned_pkts;
>  		uint64_t enqueued_pkts;
>  		uint64_t enqdrop_pkts;
> -	} rx __rte_cache_aligned;
> -	int pad1 __rte_cache_aligned;
> +	} rx;
> +	alignas(RTE_CACHE_LINE_SIZE) int pad1;

pad1/2/3/4/5 should be replaced by RTE_CACHE_GUARD, if that is their purpose.

@David: You might want to change this in a future patch.

> 
> -	struct {
> +	alignas(RTE_CACHE_LINE_SIZE) struct {
>  		uint64_t in_pkts;
>  		uint64_t ret_pkts;
>  		uint64_t sent_pkts;
>  		uint64_t enqdrop_pkts;
> -	} dist __rte_cache_aligned;
> -	int pad2 __rte_cache_aligned;
> +	} dist;
> +	alignas(RTE_CACHE_LINE_SIZE) int pad2;
> 
> -	struct {
> +	alignas(RTE_CACHE_LINE_SIZE) struct {
>  		uint64_t dequeue_pkts;
>  		uint64_t tx_pkts;
>  		uint64_t enqdrop_pkts;
> -	} tx __rte_cache_aligned;
> -	int pad3 __rte_cache_aligned;
> +	} tx;
> +	alignas(RTE_CACHE_LINE_SIZE) int pad3;
> 
> -	uint64_t worker_pkts[64] __rte_cache_aligned;
> +	alignas(RTE_CACHE_LINE_SIZE) uint64_t worker_pkts[64];
> 
> -	int pad4 __rte_cache_aligned;
> +	alignas(RTE_CACHE_LINE_SIZE) int pad4;
> 
> -	uint64_t worker_bursts[64][8] __rte_cache_aligned;
> +	alignas(RTE_CACHE_LINE_SIZE) uint64_t worker_bursts[64][8];
> 
> -	int pad5 __rte_cache_aligned;
> +	alignas(RTE_CACHE_LINE_SIZE) int pad5;
> 
> -	uint64_t port_rx_pkts[64] __rte_cache_aligned;
> -	uint64_t port_tx_pkts[64] __rte_cache_aligned;
> +	alignas(RTE_CACHE_LINE_SIZE) uint64_t port_rx_pkts[64];
> +	alignas(RTE_CACHE_LINE_SIZE) uint64_t port_tx_pkts[64];
>  } app_stats;
> 
>  struct app_stats prev_app_stats;

[...]

> diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
> index bdcada1..4cf4c9d 100644
> --- a/examples/ipsec-secgw/ipsec.h
> +++ b/examples/ipsec-secgw/ipsec.h
> @@ -112,7 +112,7 @@ enum {
>  	return (struct ipsec_sa *)i;
>  }
> 
> -struct ipsec_sa {
> +struct __rte_cache_aligned ipsec_sa {
>  	struct rte_ipsec_session sessions[IPSEC_SESSION_MAX];
>  	uint32_t spi;
>  	struct cdev_qp *cqp[RTE_MAX_LCORE];
> @@ -170,7 +170,7 @@ struct ipsec_sa {
>  	struct rte_flow_item_esp esp_spec;
>  	struct rte_flow *flow;
>  	struct rte_security_session_conf sess_conf;
> -} __rte_cache_aligned;
> +};
> 
>  struct ipsec_xf {
>  	struct rte_crypto_sym_xform a;
> @@ -190,12 +190,12 @@ struct sa_ctx {
>  	struct ipsec_sa sa[];
>  };
> 
> -struct ipsec_mbuf_metadata {
> +struct __rte_cache_aligned ipsec_mbuf_metadata {
>  	struct ipsec_sa *sa;
>  	struct rte_crypto_op cop;
>  	struct rte_crypto_sym_op sym_cop;
>  	uint8_t buf[32];
> -} __rte_cache_aligned;
> +};
> 
>  #define IS_TRANSPORT(flags) ((flags) & TRANSPORT)
> 
> @@ -224,7 +224,7 @@ struct cdev_qp {
>  	uint16_t qp;
>  	uint16_t in_flight;
>  	uint16_t len;
> -	struct rte_crypto_op *buf[MAX_PKT_BURST] __rte_aligned(sizeof(void
> *));
> +	alignas(sizeof(void *)) struct rte_crypto_op *buf[MAX_PKT_BURST];

Aligning a pointer to the size of a pointer is superfluous, unless the structure is packed.

@Radu, @Akhil: You might want to remove these in a future patch.

>  };
> 
>  struct ipsec_ctx {
> @@ -235,7 +235,7 @@ struct ipsec_ctx {
>  	uint16_t nb_qps;
>  	uint16_t last_qp;
>  	struct cdev_qp tbl[MAX_QP_PER_LCORE];
> -	struct rte_mbuf *ol_pkts[MAX_PKT_BURST] __rte_aligned(sizeof(void
> *));
> +	alignas(sizeof(void *)) struct rte_mbuf *ol_pkts[MAX_PKT_BURST];
>  	uint16_t ol_pkts_cnt;
>  	uint64_t ipv4_offloads;
>  	uint64_t ipv6_offloads;
> @@ -283,18 +283,18 @@ struct cnt_blk {
>  	uint32_t cnt;
>  } __rte_packed;
> 
> -struct lcore_rx_queue {
> +struct __rte_cache_aligned lcore_rx_queue {
>  	uint16_t port_id;
>  	uint8_t queue_id;
>  	void *sec_ctx;
> -} __rte_cache_aligned;
> +};
> 
>  struct buffer {
>  	uint16_t len;
> -	struct rte_mbuf *m_table[MAX_PKT_BURST] __rte_aligned(sizeof(void
> *));
> +	alignas(sizeof(void *)) struct rte_mbuf *m_table[MAX_PKT_BURST];
>  };
> 
> -struct lcore_conf {
> +struct __rte_cache_aligned lcore_conf {
>  	uint16_t nb_rx_queue;
>  	struct lcore_rx_queue rx_queue_list[MAX_RX_QUEUE_PER_LCORE];
>  	uint16_t tx_queue_id[RTE_MAX_ETHPORTS];