From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id AB3608DA9 for ; Sat, 9 Jun 2018 11:23:31 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1-us1.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id 0FB35B4005E; Sat, 9 Jun 2018 09:23:30 +0000 (UTC) Received: from [192.168.38.17] (84.52.114.114) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Sat, 9 Jun 2018 10:23:25 +0100 To: Dan Gora , "Wiles, Keith" CC: "dev@dpdk.org" , Olivier Matz References: <20180607235454.27832-1-dg@adax.com> <99f224eb-0470-78d2-a062-8dca4d4b7b1a@solarflare.com> <5F1328F1-6991-45EA-8DC2-300600702210@intel.com> From: Andrew Rybchenko Message-ID: Date: Sat, 9 Jun 2018 12:23:21 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-Originating-IP: [84.52.114.114] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-11.0.0.1191-8.100.1062-23896.003 X-TM-AS-Result: No--16.879600-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-MDID: 1528536211-2MR8EPjvwgfB Subject: Re: [dpdk-dev] [PATCH 1/4] mbuf: add accessor function for private data area X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 09 Jun 2018 09:23:31 -0000 Hi Dan, On 06/09/2018 03:24 AM, Dan Gora wrote: >>>> +{ >>>> + if (md->priv_size == 0) >>>> + return NULL; >>>> + >>>> + return RTE_PTR_ADD(md, sizeof(struct rte_mbuf)); >>> Also a nit... >>> I'd use sizeof(*md) (or sizeof(*m) in fact as described above) here. >>> At least previous functions do it in such way. >> I believe the sizeof(struct rte_mbuf) is much more readable then sizeof(*m) it makes the reader have to look up what ‘m’ is defined to. I know this is a small function, but readability is still a good reason to not use sizeof(*m) IMO. > On one hand, using sizeof(*m) is useful in case the type of 'm' ever > changes, you don't have to remember to change the arguments to sizeof. > On the other hand, it does make it slightly harder to read, but not a > lot really. > > For me, it's six one way, half a dozen the other. I just cut-pasted > this from the ipsec-secgw code. I'm kind of inclined to leave it > sizeof(struct rte_mbuf) just to leave it clear. OK, I agree. > Any opinion on my question from the cover letter? Sorry, I was going to reply as I understand it, but forgot. > Specifically when should rte_mbuf_XXX be used vs rte_pktmbuf_XXX for > mbuf API functions? Why is there this inconsistency there? Are they > just historical names which couldn't get changed? I think that Olivier is best placed to answer it. As I understand it is mainly historical right now, since ctrlmbuf API was removed recently. For me, there is still a flavour of packet head in pktmbuf, but boundaries are so vague. > One more quick question: > > When sending a v2 of a patch series, should I resend the whole bundle, > even if there are no changes in the other patches or just send a v2 of > the patch which actually contains changes from the v1 version? All patches should be resent in v2. BTW, thinking about function I found out there is a trap in private area size related to the function. I think that the function description should highlight that rte_pktmbuf_priv_size(m->pool) should be used to find out the size of private area since indirect mbuf has size of the direct private are in its priv_size (but we return pointer to the indirect mbuf (the mbuf itself) private area here). Andrew.