On Wed, Feb 14, 2024 at 02:12:17PM +0100, David Marchand wrote:
> On Wed, Feb 14, 2024 at 8:07 AM Tyler Retzlaff
> <roret...@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 <roret...@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
