From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id DBD0F5933 for ; Mon, 12 May 2014 16:36:06 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga101.fm.intel.com with ESMTP; 12 May 2014 07:36:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.97,1035,1389772800"; d="scan'208";a="530479688" Received: from orsmsx103.amr.corp.intel.com ([10.22.225.130]) by fmsmga001.fm.intel.com with ESMTP; 12 May 2014 07:36:12 -0700 Received: from orsmsx153.amr.corp.intel.com (10.22.226.247) by ORSMSX103.amr.corp.intel.com (10.22.225.130) with Microsoft SMTP Server (TLS) id 14.3.123.3; Mon, 12 May 2014 07:36:12 -0700 Received: from orsmsx102.amr.corp.intel.com ([169.254.1.253]) by ORSMSX153.amr.corp.intel.com ([169.254.12.138]) with mapi id 14.03.0123.003; Mon, 12 May 2014 07:36:12 -0700 From: "Venkatesan, Venky" To: Thomas Monjalon , Olivier Matz Thread-Topic: [dpdk-dev] [PATCH RFC 06/11] mbuf: replace data pointer by an offset Thread-Index: AQHPbexb/1zIiZKJHk2fOvJbT9o8e5s9Aa7w Date: Mon, 12 May 2014 14:36:12 +0000 Message-ID: <1FD9B82B8BF2CF418D9A1000154491D9740A92B8@ORSMSX102.amr.corp.intel.com> References: <1399647038-15095-1-git-send-email-olivier.matz@6wind.com> <1399647038-15095-7-git-send-email-olivier.matz@6wind.com> <3144526.CGFdr4BbI8@xps13> In-Reply-To: <3144526.CGFdr4BbI8@xps13> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.22.254.139] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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:36:07 -0000 Olivier,=20 This is a hugely problematic change, and has a pretty large performance im= pact (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 als= o a repeated sequence - the driver will do it twice (Rx + Tx) and the next = level stack will do it twice (Rx + Tx) ...=20 My vote is to reject this change particular change to the mbuf.=20 Regards,=20 -Venky -----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=20 > buffer (m->buf_addr). It is not needed to use 8 bytes again to store=20 > another pointer to the beginning of the data. >=20 > Using a 16 bits unsigned integer is enough as we know that a mbuf is=20 > never longer than 64KB. We gain 6 bytes in the structure thanks to=20 > this modification. >=20 > 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=20 */ > + 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 >=20 > - uint16_t ol_flags; /**< Offload features. */ > - uint32_t reserved; /**< Unused field. Required for padding.= =20 */ > - > - /* 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. */ >=20 > /* 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