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

["Stanisław Kardach" <kda@semihalf.com>]

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