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); > > + } > > +} > > [...]