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 85C7043DF1; Thu, 4 Apr 2024 00:45:07 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0FB1C402D1; Thu, 4 Apr 2024 00:45:07 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id B53AA4029B for ; Thu, 4 Apr 2024 00:45:05 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id C0C0220E8FA5; Wed, 3 Apr 2024 15:45:04 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com C0C0220E8FA5 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1712184304; bh=RWcfKBYy1ZfqP5qEuI/7ycIaTrVz5EPMnKBfkIIMGBY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=QlPEeXhza5qrRc3ViOdkLTMfWVsfgEyesWu9ZvekvklSORg+r1+P/sa4sK2gjaHdf bWx3ca8N/cyI1ZWCBqJl0GoW3+8Bxir3aXBr4XK9teNbZp/u4PLe5QoZ9Op6uFEu4R tlS2Loq+1uKfMSYgT0IqwwNKgR0R30XPTShxbDIM= Date: Wed, 3 Apr 2024 15:45:04 -0700 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: dev@dpdk.org, 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 Subject: Re: [PATCH v10 2/4] mbuf: remove rte marker fields Message-ID: <20240403224504.GA30714@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> 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> <98CBD80474FA8B44BF855DF32C47DC35E9F355@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F355@smartserver.smartshare.dk> User-Agent: Mutt/1.5.21 (2010-09-15) 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 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 > > --- > > [...] > > > + /* 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 to the next version. ack, next rev.