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 D41C9C5478C for ; Tue, 27 Feb 2024 17:17:22 +0000 (UTC) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8BA2740279; Tue, 27 Feb 2024 18:17:21 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 256D940150 for ; Tue, 27 Feb 2024 18:17:19 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 281E220B74C0; Tue, 27 Feb 2024 09:17:18 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 281E220B74C0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1709054238; bh=b9YAZpRSs6ndk80NdPgKavR4auvnO83/fWRaiLUtEwQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=aYWiIoHfBMIu3a0OCmykutZjf8y2Bs326njpU4/PxtbMZ8JCv6JsHtYSBYKBaEDro 2SinUBKRqZlxIttvxpCC46xTQ3+E27G6zm51zA66/Me9fkzG8KRj/NePTf3lcjvWM0 FVt2ki8XoKIaLb3xsFXHMnDMCGyj8TLDUdT3QAys= Date: Tue, 27 Feb 2024 09:17:18 -0800 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 v6 01/23] mbuf: add accessors for rearm and Rx descriptor fields Message-ID: <20240227171718.GA16014@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F26C@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 Tue, Feb 27, 2024 at 10:10:03AM +0100, Morten Brørup 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. if you mean just the cast i don't think it matters. arguably it could go void * first but we're already aliasing through a different type. this is one argument in favor of the union. > > > + > > +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. 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. > > > > > 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. 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). > > > + * 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. same as above. > > > + * referencing through the the anonymous union fields. > > */ > > union { > > uint32_t packet_type; /**< L2/L3/L4 and tunnel information. */ > > -- > > 1.8.3.1