Re: [PATCH v3 08/39] mbuf: remove unnecessary explicit alignment

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

On Wed, Feb 14, 2024 at 02:12:17PM +0100, David Marchand wrote:
> On Wed, Feb 14, 2024 at 8:07 AM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > Remove explicit alignment with __rte_aligned(sizeof(T)) on buf_iova
> > field in the absence of packing the field should be correctly aligned.
> >
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> >  lib/mbuf/rte_mbuf_core.h | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
> > index 5688683..7369e3e 100644
> > --- a/lib/mbuf/rte_mbuf_core.h
> > +++ b/lib/mbuf/rte_mbuf_core.h
> > @@ -463,7 +463,7 @@ enum {
> >  /**
> >   * The generic rte_mbuf, containing a packet mbuf.
> >   */
> > -struct rte_mbuf {
> > +struct __rte_cache_aligned rte_mbuf {
> >         RTE_MARKER cacheline0;
> >
> >         void *buf_addr;           /**< Virtual address of segment buffer. */
> > @@ -476,7 +476,7 @@ struct rte_mbuf {
> >          * same mbuf cacheline0 layout for 32-bit and 64-bit. This makes
> >          * working on vector drivers easier.
> >          */
> > -       rte_iova_t buf_iova __rte_aligned(sizeof(rte_iova_t));
> > +       rte_iova_t buf_iova;
> >  #else
> 
> Before the change, for 32bits build:
> 
> struct rte_mbuf {
>     RTE_MARKER                 cacheline0;           /*     0     0 */
>     void *                     buf_addr;             /*     0     4 */
> 
>     /* XXX 4 bytes hole, try to pack */
> 
>     rte_iova_t                 buf_iova
> __attribute__((__aligned__(8))); /*     8     8 */
>     RTE_MARKER64               rearm_data;           /*    16     0 */
> ...
> 
> After the change:
> 
> struct rte_mbuf {
>     RTE_MARKER                 cacheline0;           /*     0     0 */
>     void *                     buf_addr;             /*     0     4 */
>     rte_iova_t                 buf_iova;             /*     4     8 */
>     RTE_MARKER64               rearm_data;           /*    12     0 */
> ...
> 
> So it looks like uint64_t is not naturally aligned on 8 bytes for x86
> 32 bits, which explains the current explicit constraint (and comment
> in the header).

How I love x86, I forgot that uint64_t can be 4 byte aligned on x86.

I've done this in 3 places I will restore them all. I wonder why my test
builds didn't build bug out (something separate I better investigate).

Thanks for calling it out!

> See also 586ec205bcbb ("mbuf: fix 64-bit address alignment in 32-bit builds").
> 
> 
> This results in a cascading offset changes triggering multiple build
> errors in vectorised code:
> 
> In file included from
> ../../../git/pub/dpdk.org/main/lib/eal/x86/include/rte_vect.h:16,
>                  from
> ../../../git/pub/dpdk.org/main/drivers/common/idpf/idpf_common_rxtx_avx512.c:5:
> ../../../git/pub/dpdk.org/main/drivers/common/idpf/idpf_common_rxtx_avx512.c:
> In function ‘idpf_singleq_rearm_common’:
> ../../../git/pub/dpdk.org/main/lib/eal/include/rte_common.h:509:55:
> error: size of unnamed array is negative
>   509 | #define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 -
> 2*!!(condition)]))
>       |                                                       ^
> ../../../git/pub/dpdk.org/main/drivers/common/idpf/idpf_common_rxtx_avx512.c:68:17:
> note: in expansion of macro ‘RTE_BUILD_BUG_ON’
>    68 |                 RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf, buf_iova) !=
>       |                 ^~~~~~~~~~~~~~~~
> ../../../git/pub/dpdk.org/main/drivers/common/idpf/idpf_common_rxtx_avx512.c:
> In function ‘_idpf_singleq_recv_raw_pkts_avx512’:
> ../../../git/pub/dpdk.org/main/lib/eal/include/rte_common.h:509:55:
> error: size of unnamed array is negative
>   509 | #define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 -
> 2*!!(condition)]))
>       |                                                       ^
> ../../../git/pub/dpdk.org/main/drivers/common/idpf/idpf_common_rxtx_avx512.c:461:17:
> note: in expansion of macro ‘RTE_BUILD_BUG_ON’
>   461 |                 RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf,
> rearm_data) !=
>       |                 ^~~~~~~~~~~~~~~~
> ../../../git/pub/dpdk.org/main/drivers/common/idpf/idpf_common_rxtx_avx512.c:
> In function ‘_idpf_splitq_recv_raw_pkts_avx512’:
> ../../../git/pub/dpdk.org/main/lib/eal/include/rte_common.h:509:55:
> error: size of unnamed array is negative
>   509 | #define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 -
> 2*!!(condition)]))
>       |                                                       ^
> ../../../git/pub/dpdk.org/main/drivers/common/idpf/idpf_common_rxtx_avx512.c:921:17:
> note: in expansion of macro ‘RTE_BUILD_BUG_ON’
>   921 |                 RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf,
> rearm_data) !=
>       |                 ^~~~~~~~~~~~~~~~
> 
> 
> -- 
> David Marchand