From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Ananyev, Konstantin" Subject: Re: [PATCH v3 1/2] mbuf: support attaching external buffer to mbuf Date: Wed, 25 Apr 2018 18:05:12 +0000 Message-ID: <2601191342CEEE43887BDE71AB977258AEBCFDCD@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> <2601191342CEEE43887BDE71AB977258AEBCF960@IRSMSX102.ger.corp.intel.com> <20180425164416.GA3268@yongseok-MBP.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "Lu, Wenzhuo" , "Wu, Jingjing" , "olivier.matz@6wind.com" , "dev@dpdk.org" , "adrien.mazarguil@6wind.com" , "nelio.laranjeiro@6wind.com" To: Yongseok Koh Return-path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 033E8A48F for ; Wed, 25 Apr 2018 20:05:16 +0200 (CEST) In-Reply-To: <20180425164416.GA3268@yongseok-MBP.local> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Yongseok Koh [mailto:yskoh@mellanox.com] > Sent: Wednesday, April 25, 2018 5:44 PM > To: Ananyev, Konstantin > Cc: Lu, Wenzhuo ; Wu, Jingjing ; olivier.matz@6wind.com; dev@dpdk.org; > adrien.mazarguil@6wind.com; nelio.laranjeiro@6wind.com > Subject: Re: [PATCH v3 1/2] mbuf: support attaching external buffer to mb= uf >=20 > On Wed, Apr 25, 2018 at 01:16:38PM +0000, Ananyev, Konstantin wrote: > > > > > > > > > > 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_M= BUF) > > > > > > > > > > /** > > > > > + * Returns TRUE if given mbuf has external buffer, or FALSE othe= rwise. > > > > > + */ > > > > > +#define RTE_MBUF_HAS_EXTBUF(mb) ((mb)->ol_flags & EXT_ATTACHED_M= BUF) > > > > > + > > > > > +/** > > > > > * 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_AT= TACHED_MBUF)) =3D=3D 0) > > > > > > 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 sur= e, > > so yes, I think better to do that explicitly. >=20 > Okay. >=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-regis= tered > > > > > + * 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 m= buf - seems quite expensive. > > > > Wonder is it possible to group them somehow and amortize the cost? > > > > > > Good point. I thought about it today. > > > > > > Comparing to the regular mbuf, maybe three differences. a) free funct= ion isn't > > > inlined but a real branch. b) no help from core local cache like memp= ool's c) no > > > free_bulk func like rte_mempool_put_bulk(). But these look quite cost= ly and > > > complicated for the external buffer attachment. > > > > > > For example, to free it in bulk, external buffers should be grouped a= s the > > > buffers would have different callback functions. To do that, I have t= o make 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 registe= red in > > > advance. If so, it would be better to use existing APIs, especially w= hen a user > > > wants high throughput... > > > > > > Let me know if you have better idea to implement it. Then, I'll gladl= y take > > > 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= with > > 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 buff= er to shinfo > > and call mbuf_attach_external(mbuf, shinfo). > > Though for free we can just call mempool_put(shinfo) and let particular= implementation > > decide when/how call free_cb(), etc. > I don't want to restrict external buffer to mempool object. Especially fo= r > storage users, they want to use **any** buffer, even coming outside of DP= DK. I am not talking about the case when external buffer can be allocated from = mempool. I am talking about the implementation where shinfo is a a mempool element. So to bring extrernal buffer into DPDK - users get a shinfo (from mempool) = and attach it to external buffer. When no one needs that external buffer any more (shinfo.refcnt =3D=3D 0)=20 mempool_put() is invoked for shinfo. Inside put() we can either call free_cb() or keep extrenal buffer for furth= er usage. Anyway just a thought. Konstantin >=20 > However, will open a follow-up discussion for this in the next release wi= ndow > probably with more measurement data. > Thank you for suggestions. >=20 > Yongseok