[v6,20/23] mbuf: remove and stop using rte marker fields
Checks
Commit Message
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@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(-)
Comments
> @@ -607,8 +603,7 @@ struct rte_mbuf {
> struct rte_mempool *pool; /**< Pool from which mbuf was allocated. */
>
> /* second cache line - fields only used in slow path or on TX */
> - RTE_MARKER cacheline1 __rte_cache_min_aligned;
> -
> + alignas(RTE_CACHE_LINE_MIN_SIZE)
This comment is a matter of taste...
The alignas() is too far away from the field it applies to, and might get overlooked.
I would prefer it immediately before "struct rte_mbuf *next;" and "uint64_t dynfield2;".
> #if RTE_IOVA_IN_MBUF
> /**
> * Next segment of scattered packet. Must be NULL in the last
> Subject: [PATCH v6 20/23] mbuf: remove and stop using rte marker fields
>
> 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@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;
>
> /**
> @@ -522,8 +520,6 @@ struct rte_mbuf {
> uint64_t ol_flags; /**< Offload features. */
>
> /* remaining bytes are set on RX when pulling packet from descriptor */
> - RTE_MARKER rx_descriptor_fields1;
> -
> /*
> * The packet type, which is the combination of outer/inner L2, L3, L4
> * and tunnel types. The packet_type is about data really present in the
> @@ -607,8 +603,7 @@ struct rte_mbuf {
> struct rte_mempool *pool; /**< Pool from which mbuf was allocated. */
>
> /* second cache line - fields only used in slow path or on TX */
> - RTE_MARKER cacheline1 __rte_cache_min_aligned;
> -
> + alignas(RTE_CACHE_LINE_MIN_SIZE)
> #if RTE_IOVA_IN_MBUF
> /**
> * Next segment of scattered packet. Must be NULL in the last
> @@ -677,35 +672,31 @@ struct rte_mbuf {
> } __rte_cache_aligned;
>
> static_assert(!(offsetof(struct rte_mbuf, ol_flags) !=
> - offsetof(struct rte_mbuf, rearm_data) + 8), "ol_flags");
> -static_assert(!(offsetof(struct rte_mbuf, rearm_data) !=
> - RTE_ALIGN(offsetof(struct rte_mbuf, rearm_data), 16)), "rearm_data");
> + offsetof(struct rte_mbuf, data_off) + 8), "ol_flags");
> static_assert(!(offsetof(struct rte_mbuf, data_off) !=
> - offsetof(struct rte_mbuf, rearm_data)), "data_off");
> -static_assert(!(offsetof(struct rte_mbuf, data_off) <
> - offsetof(struct rte_mbuf, rearm_data)), "data_off");
> + RTE_ALIGN(offsetof(struct rte_mbuf, data_off), 16)), "data_off");
> static_assert(!(offsetof(struct rte_mbuf, refcnt) <
> - offsetof(struct rte_mbuf, rearm_data)), "refcnt");
> + offsetof(struct rte_mbuf, data_off)), "refcnt");
> static_assert(!(offsetof(struct rte_mbuf, nb_segs) <
> - offsetof(struct rte_mbuf, rearm_data)), "nb_segs");
> + offsetof(struct rte_mbuf, data_off)), "nb_segs");
> static_assert(!(offsetof(struct rte_mbuf, port) <
> - offsetof(struct rte_mbuf, rearm_data)), "port");
> + offsetof(struct rte_mbuf, data_off)), "port");
> static_assert(!(offsetof(struct rte_mbuf, data_off) -
> - offsetof(struct rte_mbuf, rearm_data) > 6), "data_off");
> + offsetof(struct rte_mbuf, data_off) > 6), "data_off");
> static_assert(!(offsetof(struct rte_mbuf, refcnt) -
> - offsetof(struct rte_mbuf, rearm_data) > 6), "refcnt");
> + offsetof(struct rte_mbuf, data_off) > 6), "refcnt");
> static_assert(!(offsetof(struct rte_mbuf, nb_segs) -
> - offsetof(struct rte_mbuf, rearm_data) > 6), "nb_segs");
> + offsetof(struct rte_mbuf, data_off) > 6), "nb_segs");
> static_assert(!(offsetof(struct rte_mbuf, port) -
> - offsetof(struct rte_mbuf, rearm_data) > 6), "port");
> + offsetof(struct rte_mbuf, data_off) > 6), "port");
> static_assert(!(offsetof(struct rte_mbuf, pkt_len) !=
> - offsetof(struct rte_mbuf, rx_descriptor_fields1) + 4), "pkt_len");
> + offsetof(struct rte_mbuf, packet_type) + 4), "pkt_len");
> static_assert(!(offsetof(struct rte_mbuf, data_len) !=
> - offsetof(struct rte_mbuf, rx_descriptor_fields1) + 8), "data_len");
> + offsetof(struct rte_mbuf, packet_type) + 8), "data_len");
> static_assert(!(offsetof(struct rte_mbuf, vlan_tci) !=
> - offsetof(struct rte_mbuf, rx_descriptor_fields1) + 10), "vlan_tci");
> + offsetof(struct rte_mbuf, packet_type) + 10), "vlan_tci");
> static_assert(!(offsetof(struct rte_mbuf, hash) !=
> - offsetof(struct rte_mbuf, rx_descriptor_fields1) + 12), "hash");
> + offsetof(struct rte_mbuf, packet_type) + 12), "hash");
>
> /**
> * Function typedef of callback to free externally attached buffer.
> --
Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> 1.8.3.1
Hello Dodji,
On Tue, Feb 27, 2024 at 6:44 AM Tyler Retzlaff
<roretzla@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@linux.microsoft.com>
This change is reported as a potential ABI change.
For the context, this patch
https://patchwork.dpdk.org/project/dpdk/patch/1709012499-12813-21-git-send-email-roretzla@linux.microsoft.com/
removes null-sized markers (those fields were using RTE_MARKER, see
https://git.dpdk.org/dpdk/tree/lib/eal/include/rte_common.h#n583) from
the rte_mbuf struct.
I would argue this change do not impact ABI as the layout of the mbuf
object is not impacted.
As reported by the CI:
[C] 'function const rte_eth_rxtx_callback*
rte_eth_add_first_rx_callback(uint16_t, uint16_t, rte_rx_callback_fn,
void*)' at rte_ethdev.c:5768:1 has some indirect sub-type changes:
parameter 3 of type 'typedef rte_rx_callback_fn' has sub-type changes:
underlying type 'typedef uint16_t (typedef uint16_t, typedef
uint16_t, rte_mbuf**, typedef uint16_t, typedef uint16_t, void*)*'
changed:
in pointed to type 'function type typedef uint16_t (typedef
uint16_t, typedef uint16_t, rte_mbuf**, typedef uint16_t, typedef
uint16_t, void*)':
parameter 3 of type 'rte_mbuf**' has sub-type changes:
in pointed to type 'rte_mbuf*':
in pointed to type 'struct rte_mbuf' at rte_mbuf_core.h:470:1:
type size hasn't changed
4 data member deletions:
'RTE_MARKER cacheline0', at offset 0 (in bits) at
rte_mbuf_core.h:467:1
'RTE_MARKER64 rearm_data', at offset 128 (in bits)
at rte_mbuf_core.h:490:1
'RTE_MARKER rx_descriptor_fields1', at offset 256
(in bits) at rte_mbuf_core.h:517:1
'RTE_MARKER cacheline1', at offset 512 (in bits) at
rte_mbuf_core.h:598:1
no data member change (1 filtered);
Error: ABI issue reported for abidiff --suppr
/home/runner/work/dpdk/dpdk/devtools/libabigail.abignore
--no-added-syms --headers-dir1 reference/usr/local/include
--headers-dir2 install/usr/local/include
reference/usr/local/lib/librte_ethdev.so.24.0
install/usr/local/lib/librte_ethdev.so.24.1
ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged
this as a potential issue).
Opinions?
Btw, I see no way to suppress this (except a global [suppress_type]
name = rte_mbuf)...
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Tuesday, 27 February 2024 16.18
>
> Hello Dodji,
>
> On Tue, Feb 27, 2024 at 6:44 AM Tyler Retzlaff
> <roretzla@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@linux.microsoft.com>
>
> This change is reported as a potential ABI change.
>
> For the context, this patch
> https://patchwork.dpdk.org/project/dpdk/patch/1709012499-12813-21-git-
> send-email-roretzla@linux.microsoft.com/
> removes null-sized markers (those fields were using RTE_MARKER, see
> https://git.dpdk.org/dpdk/tree/lib/eal/include/rte_common.h#n583) from
> the rte_mbuf struct.
> I would argue this change do not impact ABI as the layout of the mbuf
> object is not impacted.
>
> As reported by the CI:
>
> [C] 'function const rte_eth_rxtx_callback*
> rte_eth_add_first_rx_callback(uint16_t, uint16_t, rte_rx_callback_fn,
> void*)' at rte_ethdev.c:5768:1 has some indirect sub-type changes:
> parameter 3 of type 'typedef rte_rx_callback_fn' has sub-type
> changes:
> underlying type 'typedef uint16_t (typedef uint16_t, typedef
> uint16_t, rte_mbuf**, typedef uint16_t, typedef uint16_t, void*)*'
> changed:
> in pointed to type 'function type typedef uint16_t (typedef
> uint16_t, typedef uint16_t, rte_mbuf**, typedef uint16_t, typedef
> uint16_t, void*)':
> parameter 3 of type 'rte_mbuf**' has sub-type changes:
> in pointed to type 'rte_mbuf*':
> in pointed to type 'struct rte_mbuf' at
> rte_mbuf_core.h:470:1:
> type size hasn't changed
> 4 data member deletions:
> 'RTE_MARKER cacheline0', at offset 0 (in bits) at
> rte_mbuf_core.h:467:1
> 'RTE_MARKER64 rearm_data', at offset 128 (in bits)
> at rte_mbuf_core.h:490:1
> 'RTE_MARKER rx_descriptor_fields1', at offset 256
> (in bits) at rte_mbuf_core.h:517:1
> 'RTE_MARKER cacheline1', at offset 512 (in bits) at
> rte_mbuf_core.h:598:1
> no data member change (1 filtered);
>
> Error: ABI issue reported for abidiff --suppr
> /home/runner/work/dpdk/dpdk/devtools/libabigail.abignore
> --no-added-syms --headers-dir1 reference/usr/local/include
> --headers-dir2 install/usr/local/include
> reference/usr/local/lib/librte_ethdev.so.24.0
> install/usr/local/lib/librte_ethdev.so.24.1
> ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged
> this as a potential issue).
>
> Opinions?
Agree: Not an ABI change, only API change.
>
> Btw, I see no way to suppress this (except a global [suppress_type]
> name = rte_mbuf)...
>
>
> --
> David Marchand
On Tue, Feb 27, 2024 at 04:18:10PM +0100, David Marchand wrote:
> Hello Dodji,
>
> On Tue, Feb 27, 2024 at 6:44 AM Tyler Retzlaff
> <roretzla@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@linux.microsoft.com>
>
> This change is reported as a potential ABI change.
>
> For the context, this patch
> https://patchwork.dpdk.org/project/dpdk/patch/1709012499-12813-21-git-send-email-roretzla@linux.microsoft.com/
> removes null-sized markers (those fields were using RTE_MARKER, see
> https://git.dpdk.org/dpdk/tree/lib/eal/include/rte_common.h#n583) from
> the rte_mbuf struct.
> I would argue this change do not impact ABI as the layout of the mbuf
> object is not impacted.
It isn't a surprise that the change got flagged because the 0 sized
fields being removed probably not something the checker understands.
So no ABI change just API break (as was requested).
> As reported by the CI:
>
> [C] 'function const rte_eth_rxtx_callback*
> rte_eth_add_first_rx_callback(uint16_t, uint16_t, rte_rx_callback_fn,
> void*)' at rte_ethdev.c:5768:1 has some indirect sub-type changes:
> parameter 3 of type 'typedef rte_rx_callback_fn' has sub-type changes:
> underlying type 'typedef uint16_t (typedef uint16_t, typedef
> uint16_t, rte_mbuf**, typedef uint16_t, typedef uint16_t, void*)*'
> changed:
> in pointed to type 'function type typedef uint16_t (typedef
> uint16_t, typedef uint16_t, rte_mbuf**, typedef uint16_t, typedef
> uint16_t, void*)':
> parameter 3 of type 'rte_mbuf**' has sub-type changes:
> in pointed to type 'rte_mbuf*':
> in pointed to type 'struct rte_mbuf' at rte_mbuf_core.h:470:1:
> type size hasn't changed
> 4 data member deletions:
> 'RTE_MARKER cacheline0', at offset 0 (in bits) at
> rte_mbuf_core.h:467:1
> 'RTE_MARKER64 rearm_data', at offset 128 (in bits)
> at rte_mbuf_core.h:490:1
> 'RTE_MARKER rx_descriptor_fields1', at offset 256
> (in bits) at rte_mbuf_core.h:517:1
> 'RTE_MARKER cacheline1', at offset 512 (in bits) at
> rte_mbuf_core.h:598:1
> no data member change (1 filtered);
>
> Error: ABI issue reported for abidiff --suppr
> /home/runner/work/dpdk/dpdk/devtools/libabigail.abignore
> --no-added-syms --headers-dir1 reference/usr/local/include
> --headers-dir2 install/usr/local/include
> reference/usr/local/lib/librte_ethdev.so.24.0
> install/usr/local/lib/librte_ethdev.so.24.1
> ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged
> this as a potential issue).
>
> Opinions?
>
> Btw, I see no way to suppress this (except a global [suppress_type]
> name = rte_mbuf)...
I am unfamiliar with the ABI checker I'm afraid i have no suggestion to
offer. Maybe we can just ignore the failure for this one series when we
decide it is ready to be merged and don't suppress the checker?
>
>
> --
> David Marchand
On Tue, Feb 27, 2024 at 6:23 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> On Tue, Feb 27, 2024 at 04:18:10PM +0100, David Marchand wrote:
> > Hello Dodji,
> >
> > On Tue, Feb 27, 2024 at 6:44 AM Tyler Retzlaff
> > <roretzla@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@linux.microsoft.com>
> >
> > This change is reported as a potential ABI change.
> >
> > For the context, this patch
> > https://patchwork.dpdk.org/project/dpdk/patch/1709012499-12813-21-git-send-email-roretzla@linux.microsoft.com/
> > removes null-sized markers (those fields were using RTE_MARKER, see
> > https://git.dpdk.org/dpdk/tree/lib/eal/include/rte_common.h#n583) from
> > the rte_mbuf struct.
> > I would argue this change do not impact ABI as the layout of the mbuf
> > object is not impacted.
>
> It isn't a surprise that the change got flagged because the 0 sized
> fields being removed probably not something the checker understands.
> So no ABI change just API break (as was requested).
>
> > As reported by the CI:
> >
> > [C] 'function const rte_eth_rxtx_callback*
> > rte_eth_add_first_rx_callback(uint16_t, uint16_t, rte_rx_callback_fn,
> > void*)' at rte_ethdev.c:5768:1 has some indirect sub-type changes:
> > parameter 3 of type 'typedef rte_rx_callback_fn' has sub-type changes:
> > underlying type 'typedef uint16_t (typedef uint16_t, typedef
> > uint16_t, rte_mbuf**, typedef uint16_t, typedef uint16_t, void*)*'
> > changed:
> > in pointed to type 'function type typedef uint16_t (typedef
> > uint16_t, typedef uint16_t, rte_mbuf**, typedef uint16_t, typedef
> > uint16_t, void*)':
> > parameter 3 of type 'rte_mbuf**' has sub-type changes:
> > in pointed to type 'rte_mbuf*':
> > in pointed to type 'struct rte_mbuf' at rte_mbuf_core.h:470:1:
> > type size hasn't changed
> > 4 data member deletions:
> > 'RTE_MARKER cacheline0', at offset 0 (in bits) at
> > rte_mbuf_core.h:467:1
> > 'RTE_MARKER64 rearm_data', at offset 128 (in bits)
> > at rte_mbuf_core.h:490:1
> > 'RTE_MARKER rx_descriptor_fields1', at offset 256
> > (in bits) at rte_mbuf_core.h:517:1
> > 'RTE_MARKER cacheline1', at offset 512 (in bits) at
> > rte_mbuf_core.h:598:1
> > no data member change (1 filtered);
> >
> > Error: ABI issue reported for abidiff --suppr
> > /home/runner/work/dpdk/dpdk/devtools/libabigail.abignore
> > --no-added-syms --headers-dir1 reference/usr/local/include
> > --headers-dir2 install/usr/local/include
> > reference/usr/local/lib/librte_ethdev.so.24.0
> > install/usr/local/lib/librte_ethdev.so.24.1
> > ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged
> > this as a potential issue).
> >
> > Opinions?
> >
> > Btw, I see no way to suppress this (except a global [suppress_type]
> > name = rte_mbuf)...
>
> I am unfamiliar with the ABI checker I'm afraid i have no suggestion to
> offer. Maybe we can just ignore the failure for this one series when we
> decide it is ready to be merged and don't suppress the checker?
The ABI check compares a current build with a (cached) reference build.
There is no "let's ignore this specific error" mechanism at the moment.
And I suspect it would be non-trivial to add (parsing abidiff text
output... brrr).
Changing the check so that it compares against origin/main (for
example) every time is doable *on paper*, but it would consume a lot
of cpu for maintainers (like Thomas, Ferruh or me) and the CI.
CI scripts would have to be updated too.
Fow now, one thing we can do is to change the reference to point at
the exact commit that introduces a change we know safe.
This requires a little sync between people (maintainers / users of
test-meson-builds.h) and UNH CI, but this is doable.
On the other hand, by the time we merge this series, libabigail may
have fixed this for us already? ;-)
Hello,
David Marchand <david.marchand@redhat.com> writes:
> Hello Dodji,
o/
[...]
> This change is reported as a potential ABI change.
>
> For the context, this patch
> https://patchwork.dpdk.org/project/dpdk/patch/1709012499-12813-21-git-send-email-roretzla@linux.microsoft.com/
> removes null-sized markers (those fields were using RTE_MARKER, see
> https://git.dpdk.org/dpdk/tree/lib/eal/include/rte_common.h#n583) from
> the rte_mbuf struct.
Thank you for the context.
[...]
> As reported by the CI:
[...]
> [C] 'function const rte_eth_rxtx_callback*
> rte_eth_add_first_rx_callback(uint16_t, uint16_t, rte_rx_callback_fn,
> void*)' at rte_ethdev.c:5768:1 has some indirect sub-type changes:
> parameter 3 of type 'typedef rte_rx_callback_fn' has sub-type changes:
[...]
> in pointed to type 'struct rte_mbuf' at rte_mbuf_core.h:470:1:
> type size hasn't changed
> 4 data member deletions:
> 'RTE_MARKER cacheline0', at offset 0 (in bits) at
> rte_mbuf_core.h:467:1
> 'RTE_MARKER64 rearm_data', at offset 128 (in bits)
> at rte_mbuf_core.h:490:1
> 'RTE_MARKER rx_descriptor_fields1', at offset 256
> (in bits) at rte_mbuf_core.h:517:1
> 'RTE_MARKER cacheline1', at offset 512 (in bits) at
> rte_mbuf_core.h:598:1
> no data member change (1 filtered);
[...]
> I would argue this change do not impact ABI as the layout of the mbuf
> object is not impacted.
I agree that on the /particular platform/ that the checker runs on,
there is no incompatible ABI change because no data member offset from
the 'struct rte_mbuf' type got modified and the size of the type hasn't
changed either.
>
> Error: ABI issue reported for abidiff --suppr
> /home/runner/work/dpdk/dpdk/devtools/libabigail.abignore
> --no-added-syms --headers-dir1 reference/usr/local/include
> --headers-dir2 install/usr/local/include
> reference/usr/local/lib/librte_ethdev.so.24.0
> install/usr/local/lib/librte_ethdev.so.24.1
> ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged
> this as a potential issue).
>
> Opinions?
>
> Btw, I see no way to suppress this (except a global [suppress_type]
> name = rte_mbuf)...
Right.
To avoid having subsequent changes to that type from being "overly"
suppressed, maybe do something like:
[suppress_type]
name = rte_mbuf
has_size_change = no
has_data_member = {cacheline0, rearm_data, rx_descriptor_fields1, cacheline1}
That way, only size-impacting changes to struct rte_mbuf in its form
that predates this patch would be suppressed, hopefully.
[...]
Cheers,
On Tue, Feb 27, 2024 at 6:44 AM Tyler Retzlaff
<roretzla@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@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.
On Wed, Feb 28, 2024 at 3:04 PM Dodji Seketeli <dodji@redhat.com> wrote:
> > Btw, I see no way to suppress this (except a global [suppress_type]
> > name = rte_mbuf)...
>
> Right.
>
> To avoid having subsequent changes to that type from being "overly"
> suppressed, maybe do something like:
>
> [suppress_type]
> name = rte_mbuf
> has_size_change = no
> has_data_member = {cacheline0, rearm_data, rx_descriptor_fields1, cacheline1}
>
> That way, only size-impacting changes to struct rte_mbuf in its form
> that predates this patch would be suppressed, hopefully.
Do you mean, only changes *not* size-impacting would be suppressed?
This is slightly better than the suppression on the whole rte_mbuf
object, but it won't catch field reordering iiuc.
On the other hand, now that I try reordering fields (to test this
suggestion of yours), I get build failures all over the DPDK tree
because we have many build checks to ensure those fields are at known
locations...
So maybe we can relax and just go with the full suppression.
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Wednesday, 28 February 2024 15.19
>
> On Tue, Feb 27, 2024 at 6:44 AM Tyler Retzlaff
> <roretzla@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@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.
And RTE_MARKER64 is only used in the rte_mbuf structure.
On Wed, Feb 28, 2024 at 4:01 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Wednesday, 28 February 2024 15.19
> >
> > On Tue, Feb 27, 2024 at 6:44 AM Tyler Retzlaff
> > <roretzla@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@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.
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@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@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
David Marchand <david.marchand@redhat.com> writes:
> On Wed, Feb 28, 2024 at 3:04 PM Dodji Seketeli <dodji@redhat.com> wrote:
>> > Btw, I see no way to suppress this (except a global [suppress_type]
>> > name = rte_mbuf)...
>>
>> Right.
>>
>> To avoid having subsequent changes to that type from being "overly"
>> suppressed, maybe do something like:
>>
>> [suppress_type]
>> name = rte_mbuf
>> has_size_change = no
>> has_data_member = {cacheline0, rearm_data, rx_descriptor_fields1, cacheline1}
>>
>> That way, only size-impacting changes to struct rte_mbuf in its form
>> that predates this patch would be suppressed, hopefully.
>
> Do you mean, only changes *not* size-impacting would be suppressed?
Yes, of course. Sorry for the typo. You are right.
> This is slightly better than the suppression on the whole rte_mbuf
> object, but it won't catch field reordering iiuc.
Indeed.
>
> On the other hand, now that I try reordering fields (to test this
> suggestion of yours), I get build failures all over the DPDK tree
> because we have many build checks to ensure those fields are at known
> locations...
> So maybe we can relax and just go with the full suppression.
Yes, that would make sense.
Thanks!
@@ -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
-----------
@@ -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
@@ -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;
/**
@@ -522,8 +520,6 @@ struct rte_mbuf {
uint64_t ol_flags; /**< Offload features. */
/* remaining bytes are set on RX when pulling packet from descriptor */
- RTE_MARKER rx_descriptor_fields1;
-
/*
* The packet type, which is the combination of outer/inner L2, L3, L4
* and tunnel types. The packet_type is about data really present in the
@@ -607,8 +603,7 @@ struct rte_mbuf {
struct rte_mempool *pool; /**< Pool from which mbuf was allocated. */
/* second cache line - fields only used in slow path or on TX */
- RTE_MARKER cacheline1 __rte_cache_min_aligned;
-
+ alignas(RTE_CACHE_LINE_MIN_SIZE)
#if RTE_IOVA_IN_MBUF
/**
* Next segment of scattered packet. Must be NULL in the last
@@ -677,35 +672,31 @@ struct rte_mbuf {
} __rte_cache_aligned;
static_assert(!(offsetof(struct rte_mbuf, ol_flags) !=
- offsetof(struct rte_mbuf, rearm_data) + 8), "ol_flags");
-static_assert(!(offsetof(struct rte_mbuf, rearm_data) !=
- RTE_ALIGN(offsetof(struct rte_mbuf, rearm_data), 16)), "rearm_data");
+ offsetof(struct rte_mbuf, data_off) + 8), "ol_flags");
static_assert(!(offsetof(struct rte_mbuf, data_off) !=
- offsetof(struct rte_mbuf, rearm_data)), "data_off");
-static_assert(!(offsetof(struct rte_mbuf, data_off) <
- offsetof(struct rte_mbuf, rearm_data)), "data_off");
+ RTE_ALIGN(offsetof(struct rte_mbuf, data_off), 16)), "data_off");
static_assert(!(offsetof(struct rte_mbuf, refcnt) <
- offsetof(struct rte_mbuf, rearm_data)), "refcnt");
+ offsetof(struct rte_mbuf, data_off)), "refcnt");
static_assert(!(offsetof(struct rte_mbuf, nb_segs) <
- offsetof(struct rte_mbuf, rearm_data)), "nb_segs");
+ offsetof(struct rte_mbuf, data_off)), "nb_segs");
static_assert(!(offsetof(struct rte_mbuf, port) <
- offsetof(struct rte_mbuf, rearm_data)), "port");
+ offsetof(struct rte_mbuf, data_off)), "port");
static_assert(!(offsetof(struct rte_mbuf, data_off) -
- offsetof(struct rte_mbuf, rearm_data) > 6), "data_off");
+ offsetof(struct rte_mbuf, data_off) > 6), "data_off");
static_assert(!(offsetof(struct rte_mbuf, refcnt) -
- offsetof(struct rte_mbuf, rearm_data) > 6), "refcnt");
+ offsetof(struct rte_mbuf, data_off) > 6), "refcnt");
static_assert(!(offsetof(struct rte_mbuf, nb_segs) -
- offsetof(struct rte_mbuf, rearm_data) > 6), "nb_segs");
+ offsetof(struct rte_mbuf, data_off) > 6), "nb_segs");
static_assert(!(offsetof(struct rte_mbuf, port) -
- offsetof(struct rte_mbuf, rearm_data) > 6), "port");
+ offsetof(struct rte_mbuf, data_off) > 6), "port");
static_assert(!(offsetof(struct rte_mbuf, pkt_len) !=
- offsetof(struct rte_mbuf, rx_descriptor_fields1) + 4), "pkt_len");
+ offsetof(struct rte_mbuf, packet_type) + 4), "pkt_len");
static_assert(!(offsetof(struct rte_mbuf, data_len) !=
- offsetof(struct rte_mbuf, rx_descriptor_fields1) + 8), "data_len");
+ offsetof(struct rte_mbuf, packet_type) + 8), "data_len");
static_assert(!(offsetof(struct rte_mbuf, vlan_tci) !=
- offsetof(struct rte_mbuf, rx_descriptor_fields1) + 10), "vlan_tci");
+ offsetof(struct rte_mbuf, packet_type) + 10), "vlan_tci");
static_assert(!(offsetof(struct rte_mbuf, hash) !=
- offsetof(struct rte_mbuf, rx_descriptor_fields1) + 12), "hash");
+ offsetof(struct rte_mbuf, packet_type) + 12), "hash");
/**
* Function typedef of callback to free externally attached buffer.