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 2DBFB43C26; Wed, 28 Feb 2024 17:20:37 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 106C842670; Wed, 28 Feb 2024 17:20:37 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 68779402AE for ; Wed, 28 Feb 2024 17:20:35 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id A848D20B74C0; Wed, 28 Feb 2024 08:20:34 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com A848D20B74C0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1709137234; bh=P27yuSDW/MBa9tFxxhfNITlTGsx9bgvEppiJ6WJbc9A=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=VfKBVALkMRsf9rrYVmv4IWbwIH+J50p+BrrkOlGydEk+rPxvAp3zvK19dJYgeeHI+ Kewf8doK0FnJ+d44/bAEYgZFopCtbT3FM1xUXKrS+Ssvdj/fExDq9pm8dqOqgmMtFz t4HQItKiRYVlU5skZD6L06MgTRE4TceKysrcCnEI= Date: Wed, 28 Feb 2024 08:20:34 -0800 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: thomas@monjalon.net, 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: <20240228162034.GA27142@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> <20240227171718.GA16014@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35E9F27F@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: <98CBD80474FA8B44BF855DF32C47DC35E9F27F@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, Feb 28, 2024 at 09:28:22AM +0100, Morten Brørup wrote: > +To: Thomas, previously joined the discussion > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > Sent: Tuesday, 27 February 2024 18.17 > > > > 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. > > 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. ah, in many places the drivers want the uint64_t * keeping it here allows for some of the casts (and potential for mistakes) to be discarded. if i recall in some places they do math on the returned pointer. > > > 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. it's unfortunate the zero-size markers could be accessed slightly differently than the anonymous unions necessitating code referencing them to be changed. Thomas and i discussed rearm_data and rx_descriptor_fields1 at length and it was understood why i had to add new field names if using the union approach there was an expressed preference for accessors. one possibly important detail that came out of our conversation was that rearm_data rx_descriptor_fields1 are (I think) supposed to be internal and that being the case means updading the dpdk drivers means i updated *all* consumers i.e. i can break the api and adapt in the same commit if they are truly internal because no application should be accessing the fields. one approach i've enjoyed some success with is to present semi-opaque structures where the internal fields are by convention discouraged in their use but doing this requires duplicating parts of the whole to e.g. give the drivers the view of the struct they need and is an increased burden for maintainers. again this relies on the fields actually being internal only. both approaches have been put up in this series now, just looking for folks to look at the two and push for one or the other. thanks!