From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id A36AA6CCF for ; Wed, 25 Apr 2018 15:16:42 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Apr 2018 06:16:41 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,326,1520924400"; d="scan'208";a="223236613" Received: from irsmsx110.ger.corp.intel.com ([163.33.3.25]) by fmsmga005.fm.intel.com with ESMTP; 25 Apr 2018 06:16:39 -0700 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.83]) by irsmsx110.ger.corp.intel.com ([169.254.15.211]) with mapi id 14.03.0319.002; Wed, 25 Apr 2018 14:16:39 +0100 From: "Ananyev, Konstantin" To: Yongseok Koh CC: "Lu, Wenzhuo" , "Wu, Jingjing" , "olivier.matz@6wind.com" , "dev@dpdk.org" , "adrien.mazarguil@6wind.com" , "nelio.laranjeiro@6wind.com" Thread-Topic: [PATCH v3 1/2] mbuf: support attaching external buffer to mbuf Thread-Index: AQHT13tb7EdOoqZeqEi/dR5l7YKFH6QOO30QgADl7oCAAlyUkA== Date: Wed, 25 Apr 2018 13:16:38 +0000 Message-ID: <2601191342CEEE43887BDE71AB977258AEBCF960@IRSMSX102.ger.corp.intel.com> References: <20180310012532.15809-1-yskoh@mellanox.com> <20180419011105.9694-1-yskoh@mellanox.com> <2601191342CEEE43887BDE71AB977258AE91994F@IRSMSX102.ger.corp.intel.com> <20180424020427.GA83470@yongseok-MBP.local> In-Reply-To: <20180424020427.GA83470@yongseok-MBP.local> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOTJlODA1MjctNzJiMy00YjE5LWFjOWQtMmI5YTg5OWY3MGY5IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6Ilpub2ZlSDI1ZzZUK1JITkVYaHYrMUlWVEFhYXhndjVXT0c3eTJXNFJcL1djPSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3 1/2] mbuf: support attaching external buffer to mbuf 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: Wed, 25 Apr 2018 13:16:43 -0000 >=20 > On Mon, Apr 23, 2018 at 11:53:04AM +0000, Ananyev, Konstantin wrote: > [...] > > > @@ -693,9 +711,14 @@ rte_mbuf_to_baddr(struct rte_mbuf *md) > > > #define RTE_MBUF_INDIRECT(mb) ((mb)->ol_flags & IND_ATTACHED_MBUF) > > > > > > /** > > > + * Returns TRUE if given mbuf has external buffer, or FALSE otherwis= e. > > > + */ > > > +#define RTE_MBUF_HAS_EXTBUF(mb) ((mb)->ol_flags & EXT_ATTACHED_MBUF) > > > + > > > +/** > > > * Returns TRUE if given mbuf is direct, or FALSE otherwise. > > > */ > > > -#define RTE_MBUF_DIRECT(mb) (!RTE_MBUF_INDIRECT(mb)) > > > +#define RTE_MBUF_DIRECT(mb) (!RTE_MBUF_INDIRECT(mb) && !RTE_MBUF_HAS= _EXTBUF(mb)) > > > > As a nit: > > RTE_MBUF_DIRECT(mb) (((mb)->ol_flags & (IND_ATTACHED_MBUF | EXT_ATTACH= ED_MBUF)) =3D=3D 0) >=20 > It was for better readability and I expected compiler did the same. > But, if you still want this way, I can change it. I know compilers are quite smart these days, but you never know for sure, so yes, I think better to do that explicitly. >=20 > [...] > > > /** > > > - * Detach an indirect packet mbuf. > > > + * @internal used by rte_pktmbuf_detach(). > > > + * > > > + * Decrement the reference counter of the external buffer. When the > > > + * reference counter becomes 0, the buffer is freed by pre-registere= d > > > + * callback. > > > + */ > > > +static inline void > > > +__rte_pktmbuf_free_extbuf(struct rte_mbuf *m) > > > +{ > > > + struct rte_mbuf_ext_shared_info *shinfo; > > > + > > > + RTE_ASSERT(RTE_MBUF_HAS_EXTBUF(m)); > > > + > > > + shinfo =3D rte_mbuf_ext_shinfo(m); > > > + > > > + if (rte_extbuf_refcnt_update(shinfo, -1) =3D=3D 0) > > > + shinfo->free_cb(m->buf_addr, shinfo->fcb_opaque); > > > > > > I understand the reason but extra function call for each external mbuf = - seems quite expensive. > > Wonder is it possible to group them somehow and amortize the cost? >=20 > Good point. I thought about it today. >=20 > Comparing to the regular mbuf, maybe three differences. a) free function = isn't > inlined but a real branch. b) no help from core local cache like mempool'= s c) no > free_bulk func like rte_mempool_put_bulk(). But these look quite costly a= nd > complicated for the external buffer attachment. >=20 > For example, to free it in bulk, external buffers should be grouped as th= e > buffers would have different callback functions. To do that, I have to ma= ke an > API to pre-register an external buffer group to prepare resources for the= bulk > free. Then, buffers can't be anonymous anymore but have to be registered = in > advance. If so, it would be better to use existing APIs, especially when = a user > wants high throughput... >=20 > Let me know if you have better idea to implement it. Then, I'll gladly ta= ke > that. Or, we can push any improvement patch in the next releases. I don't have any extra-smart thoughts here. One option I thought about - was to introduce group of external buffers wit= h common free routine (I think o mentioned it already). Second - hide all that external buffer management inside mempool, i.e. if user wants to use external buffers he create a mempool (with rte_mbuf_ext_shared_info as elements?), then attach external buffer t= o shinfo and call mbuf_attach_external(mbuf, shinfo). Though for free we can just call mempool_put(shinfo) and let particular imp= lementation decide when/how call free_cb(), etc.=20 Konstantin