On Wed, Feb 28, 2024 at 03:18:40PM +0100, David Marchand wrote:
> On Tue, Feb 27, 2024 at 6:44 AM Tyler Retzlaff
> <roret...@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 <roret...@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
