[v4,2/5] mbuf: detach mbuf with pinned external buffer

  Update detach routine to check the mbuf pool type.
Introduce the special internal version of detach routine to handle
the special case of pinned external bufferon mbuf freeing.

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
 lib/librte_mbuf/rte_mbuf.h | 95 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 88 insertions(+), 7 deletions(-)
  

Hi Slava,

Some comments inline.

On Thu, Jan 16, 2020 at 01:04:26PM +0000, Viacheslav Ovsiienko wrote:
> Update detach routine to check the mbuf pool type.
> Introduce the special internal version of detach routine to handle
> the special case of pinned external bufferon mbuf freeing.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
>  lib/librte_mbuf/rte_mbuf.h | 95 ++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 88 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index e9f6fa9..52d57d1 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -323,6 +323,24 @@ struct rte_pktmbuf_pool_private {
>  	return mbp_priv->flags;
>  }
>  
> +/**
> + * When set pktmbuf mempool will hold only mbufs with pinned external

few minor doc enhancements:

-When set pktmbuf...
+When set, pktmbuf...


> + * buffer. The external buffer will be attached on the mbuf at the

-attached on
+attached to

> + * memory pool creation and will never be detached by the mbuf free calls.
> + * mbuf should not contain any room for data after the mbuf structure.
> + */
> +#define RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF (1 << 0)
> +
> +/**
> + * Returns non zero if given mbuf has an pinned external buffer, or zero

-an
+a


> + * otherwise. The pinned external buffer is allocated at pool creation
> + * time and should not be freed on mbuf freeing.
> + *
> + * External buffer is a user-provided anonymous buffer.
> + */
> +#define RTE_MBUF_HAS_PINNED_EXTBUF(mb) \
> +	(rte_pktmbuf_priv_flags(mb->pool) & RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF)
> +
>  #ifdef RTE_LIBRTE_MBUF_DEBUG
>  
>  /**  check mbuf type in debug mode */
> @@ -588,7 +606,8 @@ static inline struct rte_mbuf *rte_mbuf_raw_alloc(struct rte_mempool *mp)
>  static __rte_always_inline void
>  rte_mbuf_raw_free(struct rte_mbuf *m)
>  {
> -	RTE_ASSERT(RTE_MBUF_DIRECT(m));
> +	RTE_ASSERT(!RTE_MBUF_CLONED(m) &&
> +		  (!RTE_MBUF_HAS_EXTBUF(m) || RTE_MBUF_HAS_PINNED_EXTBUF(m)));
>  	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);
>  	RTE_ASSERT(m->next == NULL);
>  	RTE_ASSERT(m->nb_segs == 1);
> @@ -794,7 +813,7 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
>  	m->nb_segs = 1;
>  	m->port = MBUF_INVALID_PORT;
>  
> -	m->ol_flags = 0;
> +	m->ol_flags &= EXT_ATTACHED_MBUF;
>  	m->packet_type = 0;
>  	rte_pktmbuf_reset_headroom(m);
>  
> @@ -1158,11 +1177,26 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>  	uint32_t mbuf_size, buf_len;
>  	uint16_t priv_size;
>  
> -	if (RTE_MBUF_HAS_EXTBUF(m))
> +	if (RTE_MBUF_HAS_EXTBUF(m)) {
> +		/*
> +		 * The mbuf has the external attached buffer,
> +		 * we should check the type of the memory pool where
> +		 * the mbuf was allocated from to detect the pinned
> +		 * external buffer.
> +		 */
> +		uint32_t flags = rte_pktmbuf_priv_flags(mp);
> +
> +		if (flags & RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF) {
> +			/*
> +			 * The pinned external buffer should not be
> +			 * detached from its backing mbuf, just exit.
> +			 */
> +			return;
> +		}
>  		__rte_pktmbuf_free_extbuf(m);
> -	else
> +	} else {
>  		__rte_pktmbuf_free_direct(m);
> -
> +	}

This new behavior could be documented in the API of detach(). I mean
something saying that an ext mem pinned mbuf cannot be detached, and
the function will do nothing.

>  	priv_size = rte_pktmbuf_priv_size(mp);
>  	mbuf_size = (uint32_t)(sizeof(struct rte_mbuf) + priv_size);
>  	buf_len = rte_pktmbuf_data_room_size(mp);
> @@ -1177,6 +1211,51 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>  }
>  
>  /**
> + * @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.

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

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

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
> >
  

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
> > >
>