Re: [dpdk-dev] [PATCH v4 2/5] mbuf: detach mbuf with pinned external buffer

[Slava Ovsiienko <viacheslavo@mellanox.com>]

Hi, Olivier

Thanks a lot for the thorough review.
There are some answers to comments, please, see below.

> >
> >  /**
> > + * @internal version of rte_pktmbuf_detach() to be used on mbuf freeing.
> 
> -version
> +Version
> 
> > + * For indirect and regular (not pinned) external mbufs the standard
> > + * rte_pktmbuf is involved, for pinned external buffer mbufs the
> > + special
> > + * handling is performed:
> 
> Sorry, it is not very clear to me, especially what "the standard rte_pktmbuf is
> involved" means.

Sorry, it is mistype, should be read as "rte_pktmbuf_detach is invoked".
> 
> > + *
> > + *  - return zero if reference counter in shinfo is one. It means
> > + there is
> > + *  no more references to this pinned buffer and mbuf can be returned
> > + to
> 
> -references
> +reference
> 
> > + *  the pool
> > + *
> > + *  - otherwise (if reference counter is not one), decrement
> > +reference
> > + *  counter and return non-zero value to prevent freeing the backing mbuf.
> > + *
> > + * Returns non zero if mbuf should not be freed.
> > + */
> > +static inline uint16_t __rte_pktmbuf_detach_on_free(struct rte_mbuf
> > +*m)
> 
> I think int would be better than uint16_t
> 
> > +{
> > +	if (RTE_MBUF_HAS_EXTBUF(m)) {
> > +		uint32_t flags = rte_pktmbuf_priv_flags(m->pool);
> > +
> > +		if (flags & RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF) {
> > +			struct rte_mbuf_ext_shared_info *shinfo;
> > +
> > +			/* Clear flags, mbuf is being freed. */
> > +			m->ol_flags = EXT_ATTACHED_MBUF;
> > +			shinfo = m->shinfo;
> > +			/* Optimize for performance - do not dec/reinit */
> > +			if (likely(rte_mbuf_ext_refcnt_read(shinfo) == 1))
> > +				return 0;
> > +			/*
> > +			 * Direct usage of add primitive to avoid
> > +			 * duplication of comparing with one.
> > +			 */
> > +			if (likely(rte_atomic16_add_return
> > +					(&shinfo->refcnt_atomic, -1)))
> > +				return 1;
> > +			/* Reinitialize counter before mbuf freeing. */
> > +			rte_mbuf_ext_refcnt_set(shinfo, 1);
> > +			return 0;
> > +		}
> > +	}
> > +	rte_pktmbuf_detach(m);
> > +	return 0;
> > +}
> 
> I don't think the API comment really reflects what is done in this function. In
> my understanding, the detach() operation does nothing on an extmem
> pinned mbuf. So detach() is probably not the proper name.
> 
> What about something like this instead:
> 
> /* [...].
>  *  assume m is pinned to external memory */ static inline int
> __rte_pktmbuf_pinned_ext_buf_decref(struct rte_mbuf *m) {
> 	struct rte_mbuf_ext_shared_info *shinfo;
> 
> 	/* Clear flags, mbuf is being freed. */
> 	m->ol_flags = EXT_ATTACHED_MBUF;
> 	shinfo = m->shinfo;
> 
> 	/* Optimize for performance - do not dec/reinit */
> 	if (likely(rte_mbuf_ext_refcnt_read(shinfo) == 1))
> 		return 0;
> 
> 	/*
> 	 * Direct usage of add primitive to avoid
> 	 * duplication of comparing with one.
> 	 */
> 	if (likely(rte_atomic16_add_return
> 			(&shinfo->refcnt_atomic, -1)))
> 		return 1;
> 
> 	/* Reinitialize counter before mbuf freeing. */
> 	rte_mbuf_ext_refcnt_set(shinfo, 1);
> 	return 0;
> }
> 
> static __rte_always_inline struct rte_mbuf * rte_pktmbuf_prefree_seg(struct
> rte_mbuf *m) {
> 	__rte_mbuf_sanity_check(m, 0);
> 
> 	if (likely(rte_mbuf_refcnt_read(m) == 1)) {
> 
> 		if (!RTE_MBUF_DIRECT(m))
> 			if (!RTE_MBUF_HAS_PINNED_EXTBUF(m))
> 				rte_pktmbuf_detach(m);
> 			else if (__rte_pktmbuf_pinned_ext_buf_decref(m))
> 				return NULL;
> 		}
> 		...
> 	... (and same below) ...
> 
> 
> (just quickly tested)
> 
> The other advantage is that we don't call rte_pktmbuf_detach() where not
> needed.
Your proposal fetches the private flags for all indirect packets, including the ones
with IND_ATTACHED_MBUF flags (not external), this extra fetch and check might affect
the performance for indirect packets (and it does not matter for packets with external
buffers). My approach updates the prefree routine for the packets with
external buffers only, keeping intact the handling for all other mbuf types. 

> 
> > +
> > +/**
> >   * Decrease reference counter and unlink a mbuf segment
> >   *
> >   * This function does the same than a free, except that it does not
> > @@ -1198,7 +1277,8 @@ static inline void rte_pktmbuf_detach(struct
> rte_mbuf *m)
> >  	if (likely(rte_mbuf_refcnt_read(m) == 1)) {
> >
> >  		if (!RTE_MBUF_DIRECT(m))
> > -			rte_pktmbuf_detach(m);
> > +			if (__rte_pktmbuf_detach_on_free(m))
> > +				return NULL;
> >
> >  		if (m->next != NULL) {
> >  			m->next = NULL;
> > @@ -1210,7 +1290,8 @@ static inline void rte_pktmbuf_detach(struct
> rte_mbuf *m)
> >  	} else if (__rte_mbuf_refcnt_update(m, -1) == 0) {
> >
> >  		if (!RTE_MBUF_DIRECT(m))
> > -			rte_pktmbuf_detach(m);
> > +			if (__rte_pktmbuf_detach_on_free(m))
> > +				return NULL;
> >
> >  		if (m->next != NULL) {
> >  			m->next = NULL;
> > --
> > 1.8.3.1
> >