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

[Olivier Matz <olivier.matz@6wind.com>]

Hi,

On Mon, Jan 20, 2020 at 03:41:10PM +0000, Slava Ovsiienko wrote:
> 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. 

maybe just change the test to this?

	if (!RTE_MBUF_HAS_EXTBUF(m) || !RTE_MBUF_HAS_PINNED_EXTBUF(m))

if you prefer, test can be moved in __rte_pktmbuf_pinned_ext_buf_decref():

	if (!RTE_MBUF_HAS_EXTBUF(m) || !RTE_MBUF_HAS_PINNED_EXTBUF(m))
		return 0;

But my preference would go to the 1st one.

The root of my comment was more about the naming, I don't think the
function should be something_detach() because it would not detach
anything in case of ext mem pinned buffer.

> 
> > 
> > > +
> > > +/**
> > >   * 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
> > >
>