From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 3FEF75933 for ; Mon, 12 May 2014 16:41:09 +0200 (CEST) Received: from hmsreliant.think-freely.org ([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1WjrPh-0004Qr-4y; Mon, 12 May 2014 10:41:11 -0400 Date: Mon, 12 May 2014 10:41:08 -0400 From: Neil Horman To: "Venkatesan, Venky" Message-ID: <20140512144108.GB21298@hmsreliant.think-freely.org> References: <1399647038-15095-1-git-send-email-olivier.matz@6wind.com> <1399647038-15095-7-git-send-email-olivier.matz@6wind.com> <3144526.CGFdr4BbI8@xps13> <1FD9B82B8BF2CF418D9A1000154491D9740A92B8@ORSMSX102.amr.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1FD9B82B8BF2CF418D9A1000154491D9740A92B8@ORSMSX102.amr.corp.intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Spam-Score: -2.9 (--) X-Spam-Status: No Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH RFC 06/11] mbuf: replace data pointer by an offset 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: Mon, 12 May 2014 14:41:09 -0000 On Mon, May 12, 2014 at 02:36:12PM +0000, Venkatesan, Venky wrote: > Olivier, > > This is a hugely problematic change, and has a pretty large performance impact (because the dependency to compute and access). We debated this for a long time during the early days of DPDK and decided against it. This is also a repeated sequence - the driver will do it twice (Rx + Tx) and the next level stack will do it twice (Rx + Tx) ... > > My vote is to reject this change particular change to the mbuf. > > Regards, > -Venky > Do you have perforamance numbers to compare throughput with and without this change? I always feel suspcious when I see the spectre of performane used to support or deny a change without supporting reasoning or metrics. Neil > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon > Sent: Monday, May 12, 2014 7:13 AM > To: Olivier Matz > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH RFC 06/11] mbuf: replace data pointer by an offset > > Hi Olivier, > > 2014-05-09 16:50, Olivier Matz: > > The mbuf structure already contains a pointer to the beginning of the > > buffer (m->buf_addr). It is not needed to use 8 bytes again to store > > another pointer to the beginning of the data. > > > > Using a 16 bits unsigned integer is enough as we know that a mbuf is > > never longer than 64KB. We gain 6 bytes in the structure thanks to > > this modification. > > > > Signed-off-by: Olivier Matz > [...] > > --- a/lib/librte_mbuf/rte_mbuf.h > > +++ b/lib/librte_mbuf/rte_mbuf.h > > @@ -132,6 +132,13 @@ struct rte_mbuf { > > void *buf_addr; /**< Virtual address of segment buffer. */ > > uint64_t buf_physaddr:48; /**< Physical address of segment buffer. */ > > uint64_t buf_len:16; /**< Length of segment buffer. */ > > + > > + /* valid for any segment */ > > + struct rte_mbuf *next; /**< Next segment of scattered packet. */ > > + uint16_t data_off; > > + uint16_t data_len; /**< Amount of data in segment buffer. */ > > + uint32_t pkt_len; /**< Total pkt len: sum of all segments. */ > > + > > #ifdef RTE_MBUF_REFCNT > > /** > > * 16-bit Reference counter. > > @@ -142,36 +149,30 @@ struct rte_mbuf { > > * config option. > > */ > > union { > > - rte_atomic16_t refcnt_atomic; /**< Atomically accessed refcnt */ > > - uint16_t refcnt; /**< Non-atomically accessed refcnt > */ > > + rte_atomic16_t refcnt_atomic; /**< Atomically accessed refcnt */ > > + uint16_t refcnt; /**< Non-atomically accessed refcnt */ > > }; > > #else > > - uint16_t refcnt_reserved; /**< Do not use this field */ > > + uint16_t refcnt_reserved; /**< Do not use this field */ > > #endif > > > > - uint16_t ol_flags; /**< Offload features. */ > > - uint32_t reserved; /**< Unused field. Required for padding. > */ > > - > > - /* valid for any segment */ > > - struct rte_mbuf *next; /**< Next segment of scattered packet. */ > > - void* data; /**< Start address of data in segment buffer. */ > > - uint16_t data_len; /**< Amount of data in segment buffer. */ > > - > > /* these fields are valid for first segment only */ > > - uint8_t nb_segs; /**< Number of segments. */ > > - uint8_t in_port; /**< Input port. */ > > - uint32_t pkt_len; /**< Total pkt len: sum of all segment data_len. > > */ + uint8_t nb_segs; /**< Number of segments. */ > > + uint8_t in_port; /**< Input port. */ > > + uint16_t ol_flags; /**< Offload features. */ > > + uint16_t reserved; /**< Unused field. Required for padding. */ > > > > /* offload features, valid for first segment only */ > > union rte_vlan_macip vlan_macip; > > union { > > - uint32_t rss; /**< RSS hash result if RSS enabled */ > > + uint32_t rss; /**< RSS hash result if RSS enabled */ > > struct { > > uint16_t hash; > > uint16_t id; > > - } fdir; /**< Filter identifier if FDIR enabled */ > > - uint32_t sched; /**< Hierarchical scheduler */ > > - } hash; /**< hash information */ > > + } fdir; /**< Filter identifier if FDIR enabled */ > > + uint32_t sched; /**< Hierarchical scheduler */ > > + } hash; /**< hash information */ > > + uint64_t reserved2; /**< Unused field. Required for padding. */ > > } __rte_cache_aligned; > > There are some cosmetic changes mixed with real changes. > It make hard to read them. > Please split this patch. > > -- > Thomas >