From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id A968743DEF; Wed, 3 Apr 2024 21:32:25 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D459B4025D; Wed, 3 Apr 2024 21:32:23 +0200 (CEST) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 5547C4025C for ; Wed, 3 Apr 2024 21:32:22 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id 1DF6A20CEE; Wed, 3 Apr 2024 21:32:22 +0200 (CEST) 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 v10 2/4] mbuf: remove rte marker fields Date: Wed, 3 Apr 2024 21:32:21 +0200 X-MimeOLE: Produced By Microsoft Exchange V6.5 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9F355@smartserver.smartshare.dk> In-Reply-To: <1712166816-10624-3-git-send-email-roretzla@linux.microsoft.com> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v10 2/4] mbuf: remove rte marker fields Thread-Index: AdqF798YuzFr2DLnS4apMhqTelrO+QACLiKw References: <1706657173-26166-1-git-send-email-roretzla@linux.microsoft.com> <1712166816-10624-1-git-send-email-roretzla@linux.microsoft.com> <1712166816-10624-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, 3 April 2024 19.54 >=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 > 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 =3D rte_mbuf. >=20 > Signed-off-by: Tyler Retzlaff > --- [...] > + /* 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=F8rup to the next = version.