From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5B155CD11DD for ; Tue, 26 Mar 2024 11:12:21 +0000 (UTC) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0B48540265; Tue, 26 Mar 2024 12:12:20 +0100 (CET) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id E04D340261 for ; Tue, 26 Mar 2024 12:12:17 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id B37E920CEF; Tue, 26 Mar 2024 12:12:17 +0100 (CET) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH v7 2/4] mbuf: remove rte marker fields Date: Tue, 26 Mar 2024 12:12:16 +0100 X-MimeOLE: Produced By Microsoft Exchange V6.5 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9F335@smartserver.smartshare.dk> In-Reply-To: <1710972098-2209-3-git-send-email-roretzla@linux.microsoft.com> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v7 2/4] mbuf: remove rte marker fields Thread-Index: Adp7EjBDo4eBTwdeQyWR/A3anmPz6wEV9B/g References: <1706657173-26166-1-git-send-email-roretzla@linux.microsoft.com> <1710972098-2209-1-git-send-email-roretzla@linux.microsoft.com> <1710972098-2209-3-git-send-email-roretzla@linux.microsoft.com> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Tyler Retzlaff" , Cc: "Ajit Khaparde" , "Andrew Boyer" , "Andrew Rybchenko" , "Bruce Richardson" , "Chenbo Xia" , "Chengwen Feng" , "Dariusz Sosnowski" , "David Christensen" , "Hyong Youb Kim" , "Jerin Jacob" , "Jie Hai" , "Jingjing Wu" , "John Daley" , "Kevin Laatz" , "Kiran Kumar K" , "Konstantin Ananyev" , "Maciej Czekaj" , "Matan Azrad" , "Maxime Coquelin" , "Nithin Dabilpuram" , "Ori Kam" , "Ruifeng Wang" , "Satha Rao" , "Somnath Kotur" , "Suanming Mou" , "Sunil Kumar Kori" , "Viacheslav Ovsiienko" , "Yisen Zhuang" , "Yuying Zhang" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > Sent: Wednesday, 20 March 2024 23.02 >=20 > RTE_MARKER typedefs are a GCC extension unsupported by MSVC. Remove > RTE_MARKER fields from rte_mbuf struct. >=20 > Maintain alignment of fields after removed cacheline1 marker by = placing > C11 alignas(RTE_CACHE_LINE_MIN_SIZE). >=20 > 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. >=20 > Signed-off-by: Tyler Retzlaff I agree with the concepts in this patch. A few comments inline below. > --- > doc/guides/rel_notes/release_24_03.rst | 2 + > lib/mbuf/rte_mbuf.h | 4 +- > lib/mbuf/rte_mbuf_core.h | 188 = ++++++++++++++++++-------- > ------- > 3 files changed, 104 insertions(+), 90 deletions(-) >=20 > diff --git a/doc/guides/rel_notes/release_24_03.rst > b/doc/guides/rel_notes/release_24_03.rst > index 14826ea..4f18cca 100644 > --- a/doc/guides/rel_notes/release_24_03.rst > +++ b/doc/guides/rel_notes/release_24_03.rst > @@ -216,6 +216,8 @@ Removed Items >=20 > * acc101: Removed obsolete code for non productized HW variant. >=20 > +* mbuf: ``RTE_MARKER`` fields ``cacheline0`` and ``cacheline1`` > + have been removed from ``struct rte_mbuf``. >=20 > API Changes > ----------- > diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h > index 286b32b..4c4722e 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); > } >=20 > /** > @@ -126,7 +126,7 @@ > rte_mbuf_prefetch_part2(struct rte_mbuf *m) > { > #if RTE_CACHE_LINE_SIZE =3D=3D 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 9f58076..665213c 100644 > --- a/lib/mbuf/rte_mbuf_core.h > +++ b/lib/mbuf/rte_mbuf_core.h > @@ -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,116 +486,130 @@ struct __rte_cache_aligned rte_mbuf { > #endif >=20 > /* 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; >=20 > - /** > - * 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; >=20 > - /** 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; > + }; > + }; >=20 > uint64_t ol_flags; /**< Offload features. */ >=20 > /* 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. > - */ > union { > - uint32_t packet_type; /**< L2/L3/L4 and tunnel information. > */ > + void *rx_descriptor_fields1[1]; (As suggested by Bruce, and already discussed, please make this array = 3/6 elements.) > __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 { > + 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. > + */ If the comment describing the field doesn't fit on the same line as the = field, please put the description before the field, not after it. This applies to many more field descriptions in the rte_mbuf structure. Please note that multiple field's descriptions were also "after" before = this patch, so perhaps the descriptions should be moved around in a = preceding patch. Or not at all, if minimal changes are preferred. I wouldn't mind moving them with this patch, but some people prefer = multiple smaller patches for separate changes. > + __extension__ > + struct { > + uint8_t inner_l2_type:4; > + /**< Inner L2 type. */ > + uint8_t inner_l3_type:4; > + /**< Inner L3 type. */ > + }; > + }; > + uint8_t inner_l4_type:4; /**< Inner L4 > type. */ > }; > }; > - uint8_t inner_l4_type:4; /**< Inner L4 type. */ > - }; > - }; >=20 > - 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; > + /* > + * pkt_len is not actually part of > rx_descriptor_fields1 > + * but is included here to account for changes in > alignment > + * and offset for void * on 32-bit vs 64-bit targets. > + */ I disagree with this comment. For non-segmented packets, it is part of rx_descriptor_fields1. Also, the high-level comment says "/* remaining bytes are set on RX when = pulling packet from descriptor */", which also covers the pkt_len field. I think you should not add the comment. > + uint32_t pkt_len; /**< Total pkt len: sum of > all segments. */ >=20 > - 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; > + }; > + 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 > */ > + }; >=20 > - /** 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; >=20 > - uint16_t buf_len; /**< Length of segment buffer. */ > + uint16_t buf_len; /**< Length of segment > buffer. */ > + }; > + }; >=20 > struct rte_mempool *pool; /**< Pool from which mbuf was allocated. > */ >=20 > /* second cache line - fields only used in slow path or on TX */ > - alignas(RTE_CACHE_LINE_MIN_SIZE) RTE_MARKER cacheline1; > - > + alignas(RTE_CACHE_LINE_MIN_SIZE) I strongly prefer having this alignas() immediately preceding the field = it applies to; otherwise it might be overlooked. I.e. both after #if RTE_IOVA_IN_MBUF and #else, instead of only before = #if RTE_IOVA_IN_MBUF. > #if RTE_IOVA_IN_MBUF > /** > * Next segment of scattered packet. Must be NULL in the last > -- > 1.8.3.1