> From: Tyler Retzlaff [mailto:roret...@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.
