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

[David Marchand]

On Wed, Feb 28, 2024 at 4:01 PM Morten Brørup <mb at smartsharesystems.com> wrote:
>
> > From: David Marchand [mailto:david.marchand at redhat.com]
> > Sent: Wednesday, 28 February 2024 15.19
> >
> > 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) !=
> >       | ^~~~~~~~~~~~~
> >
> >
> > I hope reviewers pay attention to the alignment changes when removing
> > those markers.
> > This is not trivial to catch in the CI.
>
> Good catch, David.
>
> I wonder about the reason for 64 bit aligning the rearm_data group of fields? Perhaps it's there for (64 bit arch) vector instruction purposes?
>
> Regardless, it's an ABI break, so padding or an alignment attribute must be added to avoid ABI breakage. If there is no valid reason for the 64 bit alignment, it could be noted that the padding (or alignment attribute) is there for 32 bit arch ABI compatibility reasons only.
>
> Please note that only RTE_MARKER64 is affected by this. The other marker types have arch bit-width (or smaller) alignment, i.e. RTE_MARKER is 8 byte aligned on 64 bit arch and 4 byte aligned on 32 bit arch.

Well, strictly speaking other RTE_MARKER users *may* be affected,
depending on the alignement for the following fields.
For example, I think removing the rxq_fastpath_data_end RTE_MARKER in
struct nicvf_rxq
(https://git.dpdk.org/dpdk/tree/drivers/net/thunderx/nicvf_struct.h#n72)
impacts rx_drop_en alignment and subsequent fields.

Now, in practice and focusing only on what this series touches, either
the markers were coupled with an explicit alignment constraint
(__rte_cache*_aligned) which is preserved by the series, or the
alignement constraint is stronger than that of the marker.
So there is probably only this ABI breakage I reported.


-- 
David Marchand