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 15:30:49 -0700 Message-ID: <20180424223048.GB88208@yongseok-MBP.local> References: <20180310012532.15809-1-yskoh@mellanox.com> <20180424013854.33749-1-yskoh@mellanox.com> <934e714e-3cba-7f5d-9fcf-4f96611d758f@solarflare.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: wenzhuo.lu@intel.com, jingjing.wu@intel.com, olivier.matz@6wind.com, dev@dpdk.org, konstantin.ananyev@intel.com, adrien.mazarguil@6wind.com, nelio.laranjeiro@6wind.com To: Andrew Rybchenko Return-path: Received: from EUR03-DB5-obe.outbound.protection.outlook.com (mail-eopbgr40054.outbound.protection.outlook.com [40.107.4.54]) by dpdk.org (Postfix) with ESMTP id EBD5E2C12 for ; Wed, 25 Apr 2018 00:31:07 +0200 (CEST) Content-Disposition: inline In-Reply-To: <934e714e-3cba-7f5d-9fcf-4f96611d758f@solarflare.com> 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 03:28:33PM +0300, Andrew Rybchenko wrote: > On 04/24/2018 04:38 AM, Yongseok Koh wrote: [...] > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h > > index 06eceba37..7f6507a66 100644 > > --- a/lib/librte_mbuf/rte_mbuf.h > > +++ b/lib/librte_mbuf/rte_mbuf.h > > @@ -326,7 +326,7 @@ extern "C" { > > PKT_TX_MACSEC | \ > > PKT_TX_SEC_OFFLOAD) > > -#define __RESERVED (1ULL << 61) /**< reserved for future mbuf use */ > > +#define EXT_ATTACHED_MBUF (1ULL << 61) /**< Mbuf having external buffer */ > > May be it should mention that shinfo is filled in. Okay. > > #define IND_ATTACHED_MBUF (1ULL << 62) /**< Indirect attached mbuf */ > > @@ -566,8 +566,24 @@ struct rte_mbuf { > > /** Sequence number. See also rte_reorder_insert(). */ > > uint32_t seqn; > > + struct rte_mbuf_ext_shared_info *shinfo; > > I think it would be useful to add comment that it is used in the case of > RTE_MBUF_HAS_EXTBUF() only. Oops, I missed that. Thanks. [...] > > +static inline char * __rte_experimental > > +rte_pktmbuf_attach_extbuf(struct rte_mbuf *m, void *buf_addr, > > + rte_iova_t buf_iova, uint16_t buf_len, > > + struct rte_mbuf_ext_shared_info *shinfo, > > + rte_mbuf_extbuf_free_callback_t free_cb, void *fcb_opaque) > > +{ > > + void *buf_end = RTE_PTR_ADD(buf_addr, buf_len); > > May I suggest to move it inside if (shinfo == NULL) to make it clear that it > is not used if shinfo pointer is provided. Done. [...] > > static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m) > > { > > - struct rte_mbuf *md; > > - > > RTE_ASSERT(RTE_MBUF_DIRECT(mi) && > > rte_mbuf_refcnt_read(mi) == 1); > > - /* if m is not direct, get the mbuf that embeds the data */ > > - if (RTE_MBUF_DIRECT(m)) > > - md = m; > > - else > > - md = rte_mbuf_from_indirect(m); > > + if (RTE_MBUF_HAS_EXTBUF(m)) { > > + rte_mbuf_ext_refcnt_update(m->shinfo, 1); > > + mi->ol_flags = m->ol_flags; > > + } else { > > + rte_mbuf_refcnt_update(rte_mbuf_from_indirect(m), 1); > > It looks like handling of the direct mbuf is lost here. May be it is > intentional > to avoid branching since result will be the same for direct mbuf as well, > but looks confusing. Deserves at least a comment which explains why. > Ideally it should be proven by measurements. Right, that was intentional to avoid the branch. Sometimes branch is more expensive than arithmetic ops in core's pipeline. Will add a comment. [...] > > +static inline void > > +__rte_pktmbuf_free_direct(struct rte_mbuf *m) > > +{ > > + struct rte_mbuf *md = rte_mbuf_from_indirect(m); > > Shouldn't it be done after below assertion? Just to be less confusing. Right. Done. > > + > > + RTE_ASSERT(RTE_MBUF_INDIRECT(m)); > > + > > + if (rte_mbuf_refcnt_update(md, -1) == 0) { > > It is not directly related to the changeset, but rte_pktmbuf_prefree_seg() > has many optimizations which could be useful here: >  - do not update if refcnt is 1 >  - do not set next/nb_seq if next is already NULL Would be better to have a separate patch later. Thanks, Yongseok > > + md->next = NULL; > > + md->nb_segs = 1; > > + rte_mbuf_refcnt_set(md, 1); > > + rte_mbuf_raw_free(md); > > + } > > +} > > [...]