diff mbox series

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

Message ID 1707894382-307-9-git-send-email-roretzla@linux.microsoft.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series use C11 alignas and normalize type alignment |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Tyler Retzlaff Feb. 14, 2024, 7:05 a.m. UTC 
  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(-)

Comments

David Marchand Feb. 14, 2024, 1:12 p.m. UTC | #1 
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).
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) !=
      |                 ^~~~~~~~~~~~~~~~
Tyler Retzlaff Feb. 14, 2024, 2:28 p.m. UTC | #2 
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
diff mbox series

Patch

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
 	/**
 	 * Next segment of scattered packet.
@@ -662,7 +662,7 @@  struct rte_mbuf {
 	uint16_t timesync;
 
 	uint32_t dynfield1[9]; /**< Reserved for dynamic fields. */
-} __rte_cache_aligned;
+};
 
 /**
  * Function typedef of callback to free externally attached buffer.