[dpdk-dev] [PATCH v4 1/2] mbuf: support attaching external buffer to mbuf

[Yongseok Koh]

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