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

Reply via email to