From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yongseok Koh Subject: Re: [PATCH v4 1/2] mbuf: support attaching external buffer to mbuf Date: Tue, 24 Apr 2018 16:34:43 -0700 Message-ID: <20180424233442.GC88208@yongseok-MBP.local> References: <20180310012532.15809-1-yskoh@mellanox.com> <20180424013854.33749-1-yskoh@mellanox.com> <934e714e-3cba-7f5d-9fcf-4f96611d758f@solarflare.com> <20180424160244.bggifhilvadxcjb2@neon> <20180424191538.exjgzoif4odhndew@neon> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: Andrew Rybchenko , wenzhuo.lu@intel.com, jingjing.wu@intel.com, dev@dpdk.org, konstantin.ananyev@intel.com, adrien.mazarguil@6wind.com, nelio.laranjeiro@6wind.com, Thomas Monjalon To: Olivier Matz Return-path: Received: from EUR03-VE1-obe.outbound.protection.outlook.com (mail-eopbgr50074.outbound.protection.outlook.com [40.107.5.74]) by dpdk.org (Postfix) with ESMTP id 0572437A6 for ; Wed, 25 Apr 2018 01:35:02 +0200 (CEST) Content-Disposition: inline In-Reply-To: <20180424191538.exjgzoif4odhndew@neon> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Apr 24, 2018 at 09:15:38PM +0200, Olivier Matz wrote: > On Tue, Apr 24, 2018 at 09:21:00PM +0300, Andrew Rybchenko wrote: > > On 04/24/2018 07:02 PM, Olivier Matz wrote: > > > Hi Andrew, Yongseok, > > > > > > On Tue, Apr 24, 2018 at 03:28:33PM +0300, Andrew Rybchenko wrote: > > > > On 04/24/2018 04:38 AM, Yongseok Koh wrote: [...] > > > > > + m->buf_addr = buf_addr; > > > > > + m->buf_iova = buf_iova; > > > > > + > > > > > + if (shinfo == NULL) { > > > > > + shinfo = RTE_PTR_ALIGN_FLOOR(RTE_PTR_SUB(buf_end, > > > > > + sizeof(*shinfo)), sizeof(uintptr_t)); > > > > > + if ((void *)shinfo <= buf_addr) > > > > > + return NULL; > > > > > + > > > > > + m->buf_len = RTE_PTR_DIFF(shinfo, buf_addr); > > > > > + } else { > > > > > + m->buf_len = buf_len; > > > > > + } > > > > > + > > > > > + m->data_len = 0; > > > > > + > > > > > + rte_pktmbuf_reset_headroom(m); > > > > I would suggest to make data_off one more parameter. > > > > If I have a buffer with data which I'd like to attach to an mbuf, I'd like > > > > to control data_off. > > > Another option is to set the headroom to 0. > > > Because the after attaching the mbuf to an external buffer, we will > > > still require to set the length. > > > > > > A user can do something like this: > > > > > > rte_pktmbuf_attach_extbuf(m, buf_va, buf_iova, buf_len, shinfo, > > > free_cb, free_cb_arg); > > > rte_pktmbuf_append(m, data_len + headroom); > > > rte_pktmbuf_adj(m, headroom); I'd take this option. Will make the change and document it. > > > > > + m->ol_flags |= EXT_ATTACHED_MBUF; > > > > > + m->shinfo = shinfo; > > > > > + > > > > > + rte_mbuf_ext_refcnt_set(shinfo, 1); > > > > Why is assignment used here? Cannot we attach extbuf already attached to > > > > other mbuf? > > > In rte_pktmbuf_attach(), this is true. That's not illogical to > > > keep the same approach here. Maybe an assert could be added? Like I described in the doc, intention is to attach external buffer by _attach_extbuf() for the first time and _attach() is just for additional mbuf attachment. Will add an assert. > > > > May be shinfo should be initialized only if it is not provided (shinfo == > > > > NULL on input)? > > > I don't get why, can you explain please? > > > > May be I misunderstand how it should look like when one huge buffer > > is partitioned. I thought that it should be only one shinfo per huge buffer > > to control when it is not used any more by any mbufs with extbuf. > > OK I got it. > > I think both approach could make sense: > - one shinfo per huge buffer > - or one shinfo per mbuf, and use the callback to manage another refcnt > (like what Yongseok described) > > So I agree with your proposal, shinfo should be initialized by > the caller if it is != NULL, else it can be initialized by > rte_pktmbuf_attach_extbuf(). Also agreed. Will change. > > Other option is to have shinfo per small buf plus reference counter > > per huge buf (which is decremented when small buf reference counter > > becomes zero and free callback is executed). I guess it is assumed above. > > My fear is that it is too much reference counters: > >  1. mbuf reference counter > >  2. small buf reference counter > >  3. huge buf reference counter > > May be it is possible use (1) for (2) as well? > > I would prefer to have only 2 reference counters, one in the mbuf > and one in the shinfo. Good discussion. It should be a design decision by user. In my use-case, it would be a good idea to make all the mbufs in a same chunk point to the same shared info in the head of the chunk and reset the refcnt of shinfo to the total number of slices in the chunk. +--+----+----+--------------+----+--------------+---+- - - |global |head|mbuf1 data |head|mbuf2 data | | | shinfo|room| |room| | | +--+----+----+--------------+----+--------------+---+- - - Thanks, Yongseok