[PATCH v6 20/23] mbuf: remove and stop using rte marker fields

[Tyler Retzlaff]

On Wed, Feb 28, 2024 at 03:18:40PM +0100, David Marchand wrote:
> On Tue, Feb 27, 2024 at 6:44 AM Tyler Retzlaff
> <roretzla at linux.microsoft.com> wrote:
> >
> > RTE_MARKER typedefs are a GCC extension unsupported by MSVC. Remove
> > RTE_MARKER fields from rte_mbuf struct.
> >
> > Maintain alignment of fields after removed cacheline1 marker by placing
> > C11 alignas(RTE_CACHE_LINE_MIN_SIZE).
> >
> > Update implementation of rte_mbuf_prefetch_part1() and
> > rte_mbuf_prefetch_part2() inline functions calculate pointer for
> > prefetch of cachline0 and cachline1 without using removed markers.
> >
> > Update static_assert of rte_mbuf struct fields to reference data_off and
> > packet_type fields that occupy the original offsets of the marker
> > fields.
> >
> > Signed-off-by: Tyler Retzlaff <roretzla at linux.microsoft.com>
> > ---
> >  doc/guides/rel_notes/release_24_03.rst |  9 ++++++++
> >  lib/mbuf/rte_mbuf.h                    |  4 ++--
> >  lib/mbuf/rte_mbuf_core.h               | 39 +++++++++++++---------------------
> >  3 files changed, 26 insertions(+), 26 deletions(-)
> >
> > diff --git a/doc/guides/rel_notes/release_24_03.rst b/doc/guides/rel_notes/release_24_03.rst
> > index 879bb49..67750f2 100644
> > --- a/doc/guides/rel_notes/release_24_03.rst
> > +++ b/doc/guides/rel_notes/release_24_03.rst
> > @@ -156,6 +156,15 @@ Removed Items
> >    The application reserved statically defined logtypes ``RTE_LOGTYPE_USER1..RTE_LOGTYPE_USER8``
> >    are still defined.
> >
> > +* mbuf: ``RTE_MARKER`` fields ``cacheline0`` ``cacheline1``
> > +  ``rx_descriptor_fields1`` and ``RTE_MARKER64`` field ``rearm_data``
> > +  have been removed from ``struct rte_mbuf``.
> > +  Prefetch of ``cacheline0`` and ``cacheline1`` may be achieved through
> > +  ``rte_mbuf_prefetch_part1()`` and ``rte_mbuf_prefetch_part2()`` inline
> > +  functions respectively.
> > +  Access to ``rearm_data`` and ``rx_descriptor_fields1`` should be
> > +  through new inline functions ``rte_mbuf_rearm_data()`` and
> > +  ``rte_mbuf_rx_descriptor_fields1()`` respectively.
> >
> >  API Changes
> >  -----------
> > diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
> > index aa7495b..61cda20 100644
> > --- a/lib/mbuf/rte_mbuf.h
> > +++ b/lib/mbuf/rte_mbuf.h
> > @@ -108,7 +108,7 @@
> >  static inline void
> >  rte_mbuf_prefetch_part1(struct rte_mbuf *m)
> >  {
> > -       rte_prefetch0(&m->cacheline0);
> > +       rte_prefetch0(m);
> >  }
> >
> >  /**
> > @@ -126,7 +126,7 @@
> >  rte_mbuf_prefetch_part2(struct rte_mbuf *m)
> >  {
> >  #if RTE_CACHE_LINE_SIZE == 64
> > -       rte_prefetch0(&m->cacheline1);
> > +       rte_prefetch0(RTE_PTR_ADD(m, RTE_CACHE_LINE_MIN_SIZE));
> >  #else
> >         RTE_SET_USED(m);
> >  #endif
> > diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
> > index 36551c2..4e06f15 100644
> > --- a/lib/mbuf/rte_mbuf_core.h
> > +++ b/lib/mbuf/rte_mbuf_core.h
> > @@ -18,6 +18,7 @@
> >
> >  #include <assert.h>
> >  #include <stddef.h>
> > +#include <stdalign.h>
> >  #include <stdint.h>
> >
> >  #include <rte_common.h>
> > @@ -467,8 +468,6 @@ enum {
> >   * The generic rte_mbuf, containing a packet mbuf.
> >   */
> >  struct rte_mbuf {
> > -       RTE_MARKER cacheline0;
> > -
> >         void *buf_addr;           /**< Virtual address of segment buffer. */
> >  #if RTE_IOVA_IN_MBUF
> >         /**
> > @@ -495,7 +494,6 @@ struct rte_mbuf {
> >          * To obtain a pointer to rearm_data use the rte_mbuf_rearm_data()
> >          * accessor instead of directly referencing through the data_off field.
> >          */
> > -       RTE_MARKER64 rearm_data;
> >         uint16_t data_off;
> 
> One subtile change of removing the marker is that fields may not be
> aligned as before.
> 
> #if RTE_IOVA_IN_MBUF
>         /**
>          * Physical address of segment buffer.
>          * This field is undefined if the build is configured to use only
>          * virtual address as IOVA (i.e. RTE_IOVA_IN_MBUF is 0).
>          * Force alignment to 8-bytes, so as to ensure we have the exact
>          * 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));
> #else
>         /**
>          * Next segment of scattered packet.
>          * This field is valid when physical address field is undefined.
>          * Otherwise next pointer in the second cache line will be used.
>          */
>         struct rte_mbuf *next;
> #endif
> 
> When building ! RTE_IOVA_IN_MBUF on a 32 bits arch, the next pointer
> is not force aligned to 64bits.
> Which has a cascade effect on data_off alignement.
> 
> In file included from ../lib/mbuf/rte_mbuf_core.h:19,
>                  from ../lib/mbuf/rte_mbuf.h:42,
>                  from ../lib/mbuf/rte_mbuf_dyn.c:18:
> ../lib/mbuf/rte_mbuf_core.h:676:1: error: static assertion failed: "data_off"
>   676 | static_assert(!(offsetof(struct rte_mbuf, data_off) !=
>       | ^~~~~~~~~~~~~
> 

it is the assert that is wrong here not the offset/alignment of the fields,
i failed to notice when i moved the assert from drivers to rte_mbuf_core.h
that it was only being evaluated for drivers that *require*
enable_iova_as_pa=true.

you can easily test by adding just this assert alone and building with
enable_iova_as_pa=false for -m32 you'll see the assert trigger.

sorry i should have been more careful about which asserts i consolidated
here. i can wrap this one up in conditional compile to only fire when
RTE_IOVA_IN_MBUF=1.

> I hope reviewers pay attention to the alignment changes when removing
> those markers.
> This is not trivial to catch in the CI.
> 
> 
> -- 
> David Marchand