[v10,2/4] mbuf: remove 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).
Provide new rearm_data and rx_descriptor_fields1 fields in anonymous
unions as single element arrays of with types matching the original
markers to maintain API compatibility.
This change breaks the API for cacheline{0,1} fields that have been
removed from rte_mbuf but it does not break the ABI, to address the
false positives of the removed (but 0 size fields) provide the minimum
libabigail.abignore for type = rte_mbuf.
Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
devtools/libabigail.abignore | 6 +
doc/guides/rel_notes/release_24_07.rst | 3 +
lib/mbuf/rte_mbuf.h | 4 +-
lib/mbuf/rte_mbuf_core.h | 200 +++++++++++++++++----------------
4 files changed, 115 insertions(+), 98 deletions(-)
Comments
> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Wednesday, 3 April 2024 19.54
>
> 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).
>
> Provide new rearm_data and rx_descriptor_fields1 fields in anonymous
> unions as single element arrays of with types matching the original
> markers to maintain API compatibility.
>
> This change breaks the API for cacheline{0,1} fields that have been
> removed from rte_mbuf but it does not break the ABI, to address the
> false positives of the removed (but 0 size fields) provide the minimum
> libabigail.abignore for type = rte_mbuf.
>
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
[...]
> + /* remaining 24 bytes are set on RX when pulling packet from
> descriptor */
Good.
> union {
> + /* void * type of the array elements is retained for driver
> compatibility. */
> + void *rx_descriptor_fields1[24 / sizeof(void *)];
Good, also the description.
> __extension__
> struct {
> - uint8_t l2_type:4; /**< (Outer) L2 type. */
> - uint8_t l3_type:4; /**< (Outer) L3 type. */
> - uint8_t l4_type:4; /**< (Outer) L4 type. */
> - uint8_t tun_type:4; /**< Tunnel type. */
> + /*
> + * 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
> + * mbuf. Example: if vlan stripping is enabled, a
> received vlan packet
> + * would have RTE_PTYPE_L2_ETHER and not
> RTE_PTYPE_L2_VLAN because the
> + * vlan is stripped from the data.
> + */
> union {
> - uint8_t inner_esp_next_proto;
> - /**< ESP next protocol type, valid if
> - * RTE_PTYPE_TUNNEL_ESP tunnel type is set
> - * on both Tx and Rx.
> - */
[...]
> + /**< ESP next protocol type, valid
> if
> + * RTE_PTYPE_TUNNEL_ESP tunnel type
> is set
> + * on both Tx and Rx.
> + */
> + uint8_t inner_esp_next_proto;
Thank you for moving the comments up before the fields.
Please note that "/**<" means that the description is related to the field preceding the comment, so it should be replaced by "/**" when moving the description up above a field.
Maybe moving the descriptions as part of this patch was not a good idea after all; it doesn't improve the readability of the patch itself. I regret suggesting it.
If you leave the descriptions at their originals positions (relative to the fields), we can clean up the formatting of the descriptions in a later patch.
[...]
> /* second cache line - fields only used in slow path or on TX */
> - alignas(RTE_CACHE_LINE_MIN_SIZE) RTE_MARKER cacheline1;
> -
> #if RTE_IOVA_IN_MBUF
> /**
> * Next segment of scattered packet. Must be NULL in the last
> * segment or in case of non-segmented packet.
> */
> + alignas(RTE_CACHE_LINE_MIN_SIZE)
> struct rte_mbuf *next;
> #else
> /**
> * Reserved for dynamic fields
> * when the next pointer is in first cache line (i.e.
> RTE_IOVA_IN_MBUF is 0).
> */
> + alignas(RTE_CACHE_LINE_MIN_SIZE)
Good positioning of the alignas().
I like everything in this patch.
Please fix the descriptions preceding the fields "/**<" -> "/**" or move them back to their location after the fields; then you may add Reviewed-by: Morten Brørup <mb@smartsharesystems.com> to the next version.
On Wed, 3 Apr 2024 10:53:34 -0700
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).
>
> Provide new rearm_data and rx_descriptor_fields1 fields in anonymous
> unions as single element arrays of with types matching the original
> markers to maintain API compatibility.
>
> This change breaks the API for cacheline{0,1} fields that have been
> removed from rte_mbuf but it does not break the ABI, to address the
> false positives of the removed (but 0 size fields) provide the minimum
> libabigail.abignore for type = rte_mbuf.
>
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
On Wed, Apr 03, 2024 at 09:32:21PM +0200, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Wednesday, 3 April 2024 19.54
> >
> > 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).
> >
> > Provide new rearm_data and rx_descriptor_fields1 fields in anonymous
> > unions as single element arrays of with types matching the original
> > markers to maintain API compatibility.
> >
> > This change breaks the API for cacheline{0,1} fields that have been
> > removed from rte_mbuf but it does not break the ABI, to address the
> > false positives of the removed (but 0 size fields) provide the minimum
> > libabigail.abignore for type = rte_mbuf.
> >
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
>
> [...]
>
> > + /* remaining 24 bytes are set on RX when pulling packet from
> > descriptor */
>
> Good.
>
> > union {
> > + /* void * type of the array elements is retained for driver
> > compatibility. */
> > + void *rx_descriptor_fields1[24 / sizeof(void *)];
>
> Good, also the description.
>
> > __extension__
> > struct {
> > - uint8_t l2_type:4; /**< (Outer) L2 type. */
> > - uint8_t l3_type:4; /**< (Outer) L3 type. */
> > - uint8_t l4_type:4; /**< (Outer) L4 type. */
> > - uint8_t tun_type:4; /**< Tunnel type. */
> > + /*
> > + * 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
> > + * mbuf. Example: if vlan stripping is enabled, a
> > received vlan packet
> > + * would have RTE_PTYPE_L2_ETHER and not
> > RTE_PTYPE_L2_VLAN because the
> > + * vlan is stripped from the data.
> > + */
> > union {
> > - uint8_t inner_esp_next_proto;
> > - /**< ESP next protocol type, valid if
> > - * RTE_PTYPE_TUNNEL_ESP tunnel type is set
> > - * on both Tx and Rx.
> > - */
>
> [...]
>
> > + /**< ESP next protocol type, valid
> > if
> > + * RTE_PTYPE_TUNNEL_ESP tunnel type
> > is set
> > + * on both Tx and Rx.
> > + */
> > + uint8_t inner_esp_next_proto;
>
> Thank you for moving the comments up before the fields.
>
> Please note that "/**<" means that the description is related to the field preceding the comment, so it should be replaced by "/**" when moving the description up above a field.
ooh, i'll fix it i'm not well versed in doxygen documentation.
>
> Maybe moving the descriptions as part of this patch was not a good idea after all; it doesn't improve the readability of the patch itself. I regret suggesting it.
> If you leave the descriptions at their originals positions (relative to the fields), we can clean up the formatting of the descriptions in a later patch.
it's easy enough for me to fix the comments in place and bring in a new
version of the series, assuming other reviewers don't object i'll do that.
the diff is already kind of annoying to review in mail without -b
anyway.
>
> [...]
>
> > /* second cache line - fields only used in slow path or on TX */
> > - alignas(RTE_CACHE_LINE_MIN_SIZE) RTE_MARKER cacheline1;
> > -
> > #if RTE_IOVA_IN_MBUF
> > /**
> > * Next segment of scattered packet. Must be NULL in the last
> > * segment or in case of non-segmented packet.
> > */
> > + alignas(RTE_CACHE_LINE_MIN_SIZE)
> > struct rte_mbuf *next;
> > #else
> > /**
> > * Reserved for dynamic fields
> > * when the next pointer is in first cache line (i.e.
> > RTE_IOVA_IN_MBUF is 0).
> > */
> > + alignas(RTE_CACHE_LINE_MIN_SIZE)
>
> Good positioning of the alignas().
>
> I like everything in this patch.
>
> Please fix the descriptions preceding the fields "/**<" -> "/**" or move them back to their location after the fields; then you may add Reviewed-by: Morten Brørup <mb@smartsharesystems.com> to the next version.
ack, next rev.
@@ -37,3 +37,9 @@
[suppress_type]
name = rte_eth_fp_ops
has_data_member_inserted_between = {offset_of(reserved2), end}
+
+[suppress_type]
+ name = rte_mbuf
+ type_kind = struct
+ has_size_change = no
+ has_data_member = {cacheline0, rearm_data, rx_descriptor_fields1, cacheline1}
@@ -68,6 +68,9 @@ Removed Items
Also, make sure to start the actual text at the margin.
=======================================================
+* mbuf: ``RTE_MARKER`` fields ``cacheline0`` and ``cacheline1``
+ have been removed from ``struct rte_mbuf``.
+
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
@@ -465,8 +465,6 @@ enum {
* The generic rte_mbuf, containing a packet mbuf.
*/
struct __rte_cache_aligned rte_mbuf {
- RTE_MARKER cacheline0;
-
void *buf_addr; /**< Virtual address of segment buffer. */
#if RTE_IOVA_IN_MBUF
/**
@@ -488,127 +486,138 @@ struct __rte_cache_aligned rte_mbuf {
#endif
/* next 8 bytes are initialised on RX descriptor rearm */
- RTE_MARKER64 rearm_data;
- uint16_t data_off;
-
- /**
- * Reference counter. Its size should at least equal to the size
- * of port field (16 bits), to support zero-copy broadcast.
- * It should only be accessed using the following functions:
- * rte_mbuf_refcnt_update(), rte_mbuf_refcnt_read(), and
- * rte_mbuf_refcnt_set(). The functionality of these functions (atomic,
- * or non-atomic) is controlled by the RTE_MBUF_REFCNT_ATOMIC flag.
- */
- RTE_ATOMIC(uint16_t) refcnt;
+ union {
+ uint64_t rearm_data[1];
+ __extension__
+ struct {
+ uint16_t data_off;
+
+ /**
+ * Reference counter. Its size should at least equal to the size
+ * of port field (16 bits), to support zero-copy broadcast.
+ * It should only be accessed using the following functions:
+ * rte_mbuf_refcnt_update(), rte_mbuf_refcnt_read(), and
+ * rte_mbuf_refcnt_set(). The functionality of these functions (atomic,
+ * or non-atomic) is controlled by the RTE_MBUF_REFCNT_ATOMIC flag.
+ */
+ RTE_ATOMIC(uint16_t) refcnt;
- /**
- * Number of segments. Only valid for the first segment of an mbuf
- * chain.
- */
- uint16_t nb_segs;
+ /**
+ * Number of segments. Only valid for the first segment of an mbuf
+ * chain.
+ */
+ uint16_t nb_segs;
- /** Input port (16 bits to support more than 256 virtual ports).
- * The event eth Tx adapter uses this field to specify the output port.
- */
- uint16_t port;
+ /** Input port (16 bits to support more than 256 virtual ports).
+ * The event eth Tx adapter uses this field to specify the output port.
+ */
+ uint16_t port;
+ };
+ };
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
- * mbuf. Example: if vlan stripping is enabled, a received vlan packet
- * would have RTE_PTYPE_L2_ETHER and not RTE_PTYPE_L2_VLAN because the
- * vlan is stripped from the data.
- */
+ /* remaining 24 bytes are set on RX when pulling packet from descriptor */
union {
- uint32_t packet_type; /**< L2/L3/L4 and tunnel information. */
+ /* void * type of the array elements is retained for driver compatibility. */
+ void *rx_descriptor_fields1[24 / sizeof(void *)];
__extension__
struct {
- uint8_t l2_type:4; /**< (Outer) L2 type. */
- uint8_t l3_type:4; /**< (Outer) L3 type. */
- uint8_t l4_type:4; /**< (Outer) L4 type. */
- uint8_t tun_type:4; /**< Tunnel type. */
+ /*
+ * 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
+ * mbuf. Example: if vlan stripping is enabled, a received vlan packet
+ * would have RTE_PTYPE_L2_ETHER and not RTE_PTYPE_L2_VLAN because the
+ * vlan is stripped from the data.
+ */
union {
- uint8_t inner_esp_next_proto;
- /**< ESP next protocol type, valid if
- * RTE_PTYPE_TUNNEL_ESP tunnel type is set
- * on both Tx and Rx.
- */
+ uint32_t packet_type; /**< L2/L3/L4 and tunnel information. */
__extension__
struct {
- uint8_t inner_l2_type:4;
- /**< Inner L2 type. */
- uint8_t inner_l3_type:4;
- /**< Inner L3 type. */
+ uint8_t l2_type:4; /**< (Outer) L2 type. */
+ uint8_t l3_type:4; /**< (Outer) L3 type. */
+ uint8_t l4_type:4; /**< (Outer) L4 type. */
+ uint8_t tun_type:4; /**< Tunnel type. */
+ union {
+ /**< ESP next protocol type, valid if
+ * RTE_PTYPE_TUNNEL_ESP tunnel type is set
+ * on both Tx and Rx.
+ */
+ uint8_t inner_esp_next_proto;
+ __extension__
+ struct {
+ /**< Inner L2 type. */
+ uint8_t inner_l2_type:4;
+ /**< Inner L3 type. */
+ uint8_t inner_l3_type:4;
+ };
+ };
+ uint8_t inner_l4_type:4; /**< Inner L4 type. */
};
};
- uint8_t inner_l4_type:4; /**< Inner L4 type. */
- };
- };
- uint32_t pkt_len; /**< Total pkt len: sum of all segments. */
- uint16_t data_len; /**< Amount of data in segment buffer. */
- /** VLAN TCI (CPU order), valid if RTE_MBUF_F_RX_VLAN is set. */
- uint16_t vlan_tci;
+ uint32_t pkt_len; /**< Total pkt len: sum of all segments. */
- union {
- union {
- uint32_t rss; /**< RSS hash result if RSS enabled */
- struct {
+ uint16_t data_len; /**< Amount of data in segment buffer. */
+ /** VLAN TCI (CPU order), valid if RTE_MBUF_F_RX_VLAN is set. */
+ uint16_t vlan_tci;
+
+ union {
union {
+ uint32_t rss; /**< RSS hash result if RSS enabled */
struct {
- uint16_t hash;
- uint16_t id;
- };
- uint32_t lo;
- /**< Second 4 flexible bytes */
- };
- uint32_t hi;
- /**< First 4 flexible bytes or FD ID, dependent
- * on RTE_MBUF_F_RX_FDIR_* flag in ol_flags.
- */
- } fdir; /**< Filter identifier if FDIR enabled */
- struct rte_mbuf_sched sched;
- /**< Hierarchical scheduler : 8 bytes */
- struct {
- uint32_t reserved1;
- uint16_t reserved2;
- uint16_t txq;
- /**< The event eth Tx adapter uses this field
- * to store Tx queue id.
- * @see rte_event_eth_tx_adapter_txq_set()
- */
- } txadapter; /**< Eventdev ethdev Tx adapter */
- uint32_t usr;
- /**< User defined tags. See rte_distributor_process() */
- } hash; /**< hash information */
- };
+ union {
+ struct {
+ uint16_t hash;
+ uint16_t id;
+ };
+ /**< Second 4 flexible bytes */
+ uint32_t lo;
+ };
+ /**< First 4 flexible bytes or FD ID, dependent
+ * on RTE_MBUF_F_RX_FDIR_* flag in ol_flags.
+ */
+ uint32_t hi;
+ } fdir; /**< Filter identifier if FDIR enabled */
+ struct rte_mbuf_sched sched;
+ /**< Hierarchical scheduler : 8 bytes */
+ struct {
+ uint32_t reserved1;
+ uint16_t reserved2;
+ /**< The event eth Tx adapter uses this field
+ * to store Tx queue id.
+ * @see rte_event_eth_tx_adapter_txq_set()
+ */
+ uint16_t txq;
+ } txadapter; /**< Eventdev ethdev Tx adapter */
+ /**< User defined tags. See rte_distributor_process() */
+ uint32_t usr;
+ } hash; /**< hash information */
+ };
- /** Outer VLAN TCI (CPU order), valid if RTE_MBUF_F_RX_QINQ is set. */
- uint16_t vlan_tci_outer;
+ /** Outer VLAN TCI (CPU order), valid if RTE_MBUF_F_RX_QINQ is set. */
+ uint16_t vlan_tci_outer;
- uint16_t buf_len; /**< Length of segment buffer. */
+ uint16_t buf_len; /**< Length of segment buffer. */
+ };
+ };
struct rte_mempool *pool; /**< Pool from which mbuf was allocated. */
/* second cache line - fields only used in slow path or on TX */
- alignas(RTE_CACHE_LINE_MIN_SIZE) RTE_MARKER cacheline1;
-
#if RTE_IOVA_IN_MBUF
/**
* Next segment of scattered packet. Must be NULL in the last
* segment or in case of non-segmented packet.
*/
+ alignas(RTE_CACHE_LINE_MIN_SIZE)
struct rte_mbuf *next;
#else
/**
* Reserved for dynamic fields
* when the next pointer is in first cache line (i.e. RTE_IOVA_IN_MBUF is 0).
*/
+ alignas(RTE_CACHE_LINE_MIN_SIZE)
uint64_t dynfield2;
#endif
@@ -617,17 +626,16 @@ struct __rte_cache_aligned rte_mbuf {
uint64_t tx_offload; /**< combined for easy fetch */
__extension__
struct {
- uint64_t l2_len:RTE_MBUF_L2_LEN_BITS;
/**< L2 (MAC) Header Length for non-tunneling pkt.
* Outer_L4_len + ... + Inner_L2_len for tunneling pkt.
*/
- uint64_t l3_len:RTE_MBUF_L3_LEN_BITS;
+ uint64_t l2_len:RTE_MBUF_L2_LEN_BITS;
/**< L3 (IP) Header Length. */
- uint64_t l4_len:RTE_MBUF_L4_LEN_BITS;
+ uint64_t l3_len:RTE_MBUF_L3_LEN_BITS;
/**< L4 (TCP/UDP) Header Length. */
- uint64_t tso_segsz:RTE_MBUF_TSO_SEGSZ_BITS;
+ uint64_t l4_len:RTE_MBUF_L4_LEN_BITS;
/**< TCP TSO segment size */
-
+ uint64_t tso_segsz:RTE_MBUF_TSO_SEGSZ_BITS;
/*
* Fields for Tx offloading of tunnels.
* These are undefined for packets which don't request
@@ -640,10 +648,10 @@ struct __rte_cache_aligned rte_mbuf {
* Applications are expected to set appropriate tunnel
* offload flags when they fill in these fields.
*/
- uint64_t outer_l3_len:RTE_MBUF_OUTL3_LEN_BITS;
/**< Outer L3 (IP) Hdr Length. */
- uint64_t outer_l2_len:RTE_MBUF_OUTL2_LEN_BITS;
+ uint64_t outer_l3_len:RTE_MBUF_OUTL3_LEN_BITS;
/**< Outer L2 (MAC) Hdr Length. */
+ uint64_t outer_l2_len:RTE_MBUF_OUTL2_LEN_BITS;
/* uint64_t unused:RTE_MBUF_TXOFLD_UNUSED_BITS; */
};