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