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 1CEECC48BEB for ; Wed, 14 Feb 2024 10:46:09 +0000 (UTC) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0CFBB40A7A; Wed, 14 Feb 2024 11:46:08 +0100 (CET) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 8456A40263 for ; Wed, 14 Feb 2024 11:46:06 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id 564AB2011D; Wed, 14 Feb 2024 11:46:06 +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 v3] mbuf: deprecate GCC marker in rte mbuf struct X-MimeOLE: Produced By Microsoft Exchange V6.5 Date: Wed, 14 Feb 2024 11:46:01 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9F216@smartserver.smartshare.dk> In-Reply-To: <1707867209-1901-2-git-send-email-roretzla@linux.microsoft.com> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v3] mbuf: deprecate GCC marker in rte mbuf struct Thread-Index: Adpe1Q/vKulOjJhhRrynvPHi+z/YWgAV/y5g References: <1706657173-26166-2-git-send-email-roretzla@linux.microsoft.com> <1707867209-1901-1-git-send-email-roretzla@linux.microsoft.com> <1707867209-1901-2-git-send-email-roretzla@linux.microsoft.com> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Tyler Retzlaff" , Cc: "Andrew Boyer" , "Andrew Rybchenko" , "Bruce Richardson" , "Chenbo Xia" , "Konstantin Ananyev" , "Maxime Coquelin" 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, 14 February 2024 00.33 >=20 > Provide a macro that allows conditional expansion of RTE_MARKER fields > to empty to allow rte_mbuf to be used with MSVC. It is proposed that > we announce the fields to be __rte_deprecated (currently disabled). >=20 > Introduce C11 anonymous unions to permit aliasing of well-known > offsets by name into the rte_mbuf structure by a *new* name and to > provide padding for cache alignment. >=20 > Signed-off-by: Tyler Retzlaff > --- I think you are nearly there now. Beautiful solution. :-) Only one comment inline below. > - /* remaining bytes are set on RX when pulling packet from > descriptor */ > - RTE_MARKER rx_descriptor_fields1; > + uint64_t ol_flags; /**< Offload features. */ >=20 > - /* > - * 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. > */ > - __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. */ > + /* remaining bytes are set on RX when pulling packet > from descriptor */ > + __rte_marker(RTE_MARKER, rx_descriptor_fields1); > 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. > - */ > - __extension__ > + char mbuf_rx_descriptor_fields1[sizeof(void > *)]; The size of the mbuf_rx_descriptor_fields1 array - and the union it = belongs to - may not be accurate. The existing rx_descriptor_fields1 effectively has no type/size, and the = associated comment "/* remaining bytes are set on RX" doesn't help; it = omits how many bytes it refers to. I'll call it "an already existing = bug" for the sake of discussion. I think the struct should go all the way to the pool pointer, to reflect = which "remaining bytes" are set on RX. You might consider taking the opportunity to fix this existing "bug", = i.e. update the comment and move more fields into the struct unionized = with the mbuf_rx_descriptor_fields1 array. (This suggestion is in the = category NICE TO HAVE, and certainly not a requirement!) Regardless if you fix this existing "bug" or not, the size of the = mbuf_rx_descriptor_fields1 array should match the size of the struct it = is unionized with, rather than using sizeof(void*) (which is either 4 or = 8 bytes, depending on CPU architecture). Instinctively, I think that it is a bad idea to carry over the = "sizeless" aspect of the rx_descriptor_fields1 marker into the = mbuf_rx_descriptor_fields1 array. However, you have been working more in = depth with this patch series, so I'm open for arguments for the way you = did it! > struct { > - uint8_t inner_l2_type:4; > - /**< Inner L2 type. */ > - uint8_t inner_l3_type:4; > - /**< Inner L3 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 { > + uint32_t packet_type; /**< L2/L3/L4 > and tunnel information. */ > + __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. */ > + 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. > + */ > + __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. */ > + }; > + }; > + uint32_t pkt_len; /**< Total pkt > len: sum of all segments. */ > }; > }; > - 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; > + 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; >=20 > - union { > - union { > - uint32_t rss; /**< RSS hash result if RSS enabled > */ > - struct { > + 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. */ > + struct rte_mempool *pool; /**< Pool from which mbuf > was allocated. */ > + }; > + }; >=20 > /* second cache line - fields only used in slow path or on TX */