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 A574F43C12; Wed, 28 Feb 2024 09:28:48 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 94C6F402BC; Wed, 28 Feb 2024 09:28:48 +0100 (CET) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 6E27540295 for ; Wed, 28 Feb 2024 09:28:24 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id 4995B211AB; Wed, 28 Feb 2024 09:28:24 +0100 (CET) Content-class: urn:content-classes:message Subject: RE: [PATCH v6 01/23] mbuf: add accessors for rearm and Rx descriptor fields MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Date: Wed, 28 Feb 2024 09:28:22 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9F27F@smartserver.smartshare.dk> X-MimeOLE: Produced By Microsoft Exchange V6.5 In-Reply-To: <20240227171718.GA16014@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v6 01/23] mbuf: add accessors for rearm and Rx descriptor fields Thread-Index: AdppoNIp/GafmACSQZqUJbZIOiuFbwAd2NEw References: <1706657173-26166-1-git-send-email-roretzla@linux.microsoft.com> <1709012499-12813-1-git-send-email-roretzla@linux.microsoft.com> <1709012499-12813-2-git-send-email-roretzla@linux.microsoft.com> <98CBD80474FA8B44BF855DF32C47DC35E9F26C@smartserver.smartshare.dk> <20240227171718.GA16014@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> 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 +To: Thomas, previously joined the discussion > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > Sent: Tuesday, 27 February 2024 18.17 >=20 > On Tue, Feb 27, 2024 at 10:10:03AM +0100, Morten Br=F8rup wrote: > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > > Sent: Tuesday, 27 February 2024 06.41 > > > > > > RTE_MARKER typedefs are a GCC extension unsupported by MSVC. = Provide > > > inline functions to access compatible type pointer to rearm_data > > > and rx_descriptor_fields1 which will allow direct references on = the > > > rte marker fields to be removed. > > > > > > Signed-off-by: Tyler Retzlaff > > > --- > > > lib/mbuf/rte_mbuf.h | 13 +++++++++++++ > > > lib/mbuf/rte_mbuf_core.h | 11 ++++++++++- > > > 2 files changed, 23 insertions(+), 1 deletion(-) > > > > > > diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h > > > index 286b32b..aa7495b 100644 > > > --- a/lib/mbuf/rte_mbuf.h > > > +++ b/lib/mbuf/rte_mbuf.h > > > @@ -132,6 +132,19 @@ > > > #endif > > > } > > > > > > +static inline > > > +uint64_t * > > > +rte_mbuf_rearm_data(struct rte_mbuf *m) > > > +{ > > > + return (uint64_t *)&m->data_off; > > > +} > > > > Consider returning (void *) instead of (uint64_t *). > > Just a suggestion; I don't know which is better. >=20 > if you mean just the cast i don't think it matters. No, I meant the return type. The type is opaque; only its size is fixed at 8 byte. Maybe uint64_t * is the best type, after all. > arguably it could go > void * first but we're already aliasing through a different type. this > is one argument in favor of the union. The zero-size markers (rearm_data and rx_descriptor_fields1) were = intended to be used like unions, which the drivers effectively do. Previous C standards didn't allow anonymous structures, so using = union+struct in pre-C11 DPDK would clutter the code with subfield names. = But now that we moved on to C11, we don't have this problem anymore. IMHO, replacing the zero-size markers - which are directly visible in = the structure's source code - with access functions located in some = other header file degrades source code readability. I am in favor of union+struct in the mbuf structure over access = functions. I think the explicit grouping of the fields improves the = readability of the mbuf structure, also compared to the zero-size = markers. >=20 > > > > > + > > > +static inline > > > +void * > > > +rte_mbuf_rx_descriptor_fields1(struct rte_mbuf *m) > > > +{ > > > + return &m->packet_type; > > > +} > > > > The mbuf_rx_descriptor_fields1 field is disappearing in a later = patch; > > consider taking the opportunity to use a better name here. >=20 > oh boy. you're asking for a new name but not suggesting a new name. > naming is hard (no one is ever happy). for this and rearm_data if you > or anyone suggests a name i'll make the change ~everywhere including > comments across all drivers. As you mention below, keeping the current names is an advantage for = familiarity. Changing the name brings no real benefit, and although the current name = is somewhat silly (perhaps anticipating a second group of rx descriptor = fields), we should probably just keep it. >=20 > > > > > > > > static inline uint16_t rte_pktmbuf_priv_size(struct rte_mempool = *mp); > > > > > > diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h > > > index 5688683..7000c04 100644 > > > --- a/lib/mbuf/rte_mbuf_core.h > > > +++ b/lib/mbuf/rte_mbuf_core.h > > > @@ -486,7 +486,12 @@ struct rte_mbuf { > > > struct rte_mbuf *next; > > > #endif > > > > > > - /* next 8 bytes are initialised on RX descriptor rearm */ > > > + /** > > > + * next 8 bytes are initialised on RX descriptor rearm > > > + * > > > + * To obtain a pointer to rearm_data use the = rte_mbuf_rearm_data() > > > > The "rearm_data" field is disappearing in a later patch; > > don't refer to it by that name in the comments here. >=20 > without having renamed the block of fields cheerfully known as > "rearm_data" i kept all documentation/comments here and in drivers > using rearm_data for familiarity. (unless someone suggests a new name = for the > group > of fields). OK, you already thought about the use of the names in the documentation. = I don't object to your conclusion. >=20 > > > > > + * accessor instead of directly referencing through the data_off = field. > > > + */ > > > RTE_MARKER64 rearm_data; > > > uint16_t data_off; > > > > > > @@ -522,6 +527,10 @@ struct rte_mbuf { > > > * 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. > > > + * > > > + * To obtain a pointer to rx_descriptor_fields1 use the > > > + * rte_mbuf_rx_descriptor_fields1() accessor instead of directly > > > > The "rx_descriptor_fields1" field is disappearing in a later patch; > > don't refer to it by that name in the comments here. > > And if you update the access function's name, remember to update it = in the > comment here too. >=20 > same as above. >=20 > > > > > + * referencing through the the anonymous union fields. > > > */ > > > union { > > > uint32_t packet_type; /**< L2/L3/L4 and tunnel information. */ > > > -- > > > 1.8.3.1