On Wed, Nov 15, 2023 at 09:08:05PM +0100, Morten Brørup wrote:
> > 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.
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.
>
