From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 4C5CD32A5 for ; Wed, 10 Sep 2014 17:07:58 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP; 10 Sep 2014 08:03:11 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,499,1406617200"; d="scan'208";a="600889981" Received: from bricha3-mobl.ger.corp.intel.com (HELO bricha3-mobl.ir.intel.com) ([10.243.20.24]) by orsmga002.jf.intel.com with SMTP; 10 Sep 2014 08:09:15 -0700 Received: by bricha3-mobl.ir.intel.com (sSMTP sendmail emulation); Wed, 10 Sep 2014 16:09:14 +0001 Date: Wed, 10 Sep 2014 16:09:14 +0100 From: Bruce Richardson To: Olivier MATZ Message-ID: <20140910150914.GA9656@BRICHA3-MOBL> References: <1409759378-10113-1-git-send-email-bruce.richardson@intel.com> <1409759378-10113-8-git-send-email-bruce.richardson@intel.com> <540D9B95.3020504@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <540D9B95.3020504@6wind.com> Organization: Intel Shannon Limited. Registered in Ireland. Registered Office: Collinstown Industrial Park, Leixlip, County Kildare. Registered Number: 308263. Business address: Dromore House, East Park, Shannon, Co. Clare. User-Agent: Mutt/1.5.22 (2013-10-16) Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH 07/13] mbuf: use macros only to access the mbuf metadata X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Sep 2014 15:07:59 -0000 On Mon, Sep 08, 2014 at 02:05:41PM +0200, Olivier MATZ wrote: > Hi Bruce, > > On 09/03/2014 05:49 PM, Bruce Richardson wrote: > > Removed the explicit zero-sized metadata definition at the end of the > > mbuf data structure. Updated the metadata macros to take account of this > > change so that all existing code which uses those macros still works. > > > > Signed-off-by: Bruce Richardson > > --- > > lib/librte_mbuf/rte_mbuf.h | 22 ++++++++-------------- > > 1 file changed, 8 insertions(+), 14 deletions(-) > > > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h > > index 5260001..ca66d9a 100644 > > --- a/lib/librte_mbuf/rte_mbuf.h > > +++ b/lib/librte_mbuf/rte_mbuf.h > > @@ -166,31 +166,25 @@ struct rte_mbuf { > > struct rte_mempool *pool; /**< Pool from which mbuf was allocated. */ > > struct rte_mbuf *next; /**< Next segment of scattered packet. */ > > > > - union { > > - uint8_t metadata[0]; > > - uint16_t metadata16[0]; > > - uint32_t metadata32[0]; > > - uint64_t metadata64[0]; > > - } __rte_cache_aligned; > > } __rte_cache_aligned; > > > > #define RTE_MBUF_METADATA_UINT8(mbuf, offset) \ > > - (mbuf->metadata[offset]) > > + (((uint8_t *)&(mbuf)[1])[offset]) > > #define RTE_MBUF_METADATA_UINT16(mbuf, offset) \ > > - (mbuf->metadata16[offset/sizeof(uint16_t)]) > > + (((uint16_t *)&(mbuf)[1])[offset/sizeof(uint16_t)]) > > #define RTE_MBUF_METADATA_UINT32(mbuf, offset) \ > > - (mbuf->metadata32[offset/sizeof(uint32_t)]) > > + (((uint32_t *)&(mbuf)[1])[offset/sizeof(uint32_t)]) > > #define RTE_MBUF_METADATA_UINT64(mbuf, offset) \ > > - (mbuf->metadata64[offset/sizeof(uint64_t)]) > > + (((uint64_t *)&(mbuf)[1])[offset/sizeof(uint64_t)]) > > > > #define RTE_MBUF_METADATA_UINT8_PTR(mbuf, offset) \ > > - (&mbuf->metadata[offset]) > > + (&RTE_MBUF_METADATA_UINT8(mbuf, offset)) > > #define RTE_MBUF_METADATA_UINT16_PTR(mbuf, offset) \ > > - (&mbuf->metadata16[offset/sizeof(uint16_t)]) > > + (&RTE_MBUF_METADATA_UINT16(mbuf, offset)) > > #define RTE_MBUF_METADATA_UINT32_PTR(mbuf, offset) \ > > - (&mbuf->metadata32[offset/sizeof(uint32_t)]) > > + (&RTE_MBUF_METADATA_UINT32(mbuf, offset)) > > #define RTE_MBUF_METADATA_UINT64_PTR(mbuf, offset) \ > > - (&mbuf->metadata64[offset/sizeof(uint64_t)]) > > + (&RTE_MBUF_METADATA_UINT64(mbuf, offset)) > > > > /** > > * Given the buf_addr returns the pointer to corresponding mbuf. > > > > I think it goes in the good direction. So: > Acked-by: Olivier Matz > > Just one question: why not removing RTE_MBUF_METADATA*() macros? > I'd just provide one macro that gives a (void*) to the first byte > after the mbuf structure. > > The format of the metadata is up to the application, that usually > casts (m + 1) into a private structure, making the macros not very > useful. I suggest to move these macros outside rte_mbuf.h, in the > application-specific or library-specific header, what do you think? > Things look to work if I just move the definitions en-masse into rte_port.h. Is that the sort of change you would be thinking of? I was wondering about replacing the typed macros here with a generic one to access just beyond the definition of the mbuf structure, but on further thought, I believe that using the buf_addr pointer in the mbuf data structure is probably enough for most applications. [An alternative to moving the definitions into rte_port.h is to move them into rte_table.h and having port_frag.c use the buf_addr pointer instead of a macro to get at the metadata. All other references to the macros apart from two in that port file, are in the tables or in apps that use the tables lib.] What do you think? /Bruce