[dpdk-dev,RFC,3/8] mbuf: set mbuf fields while in pool

  Set the value of m->refcnt to 1, m->nb_segs to 1 and m->next
to NULL when the mbuf is stored inside the mempool (unused).
This is done in rte_pktmbuf_prefree_seg(), before freeing or
recycling a mbuf.

Before this patch, the value of m->refcnt was expected to be 0
while in pool.

The objectives are:

- to avoid drivers to set m->next to NULL in the early Rx path, since
  this field is in the second 64B of the mbuf and its access could
  trigger a cache miss

- rationalize the behavior of raw_alloc/raw_free: one is now the
  symmetric of the other, and refcnt is never changed in these functions.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/mlx5/mlx5_rxtx.c     |  5 ++---
 drivers/net/mpipe/mpipe_tilegx.c |  1 +
 lib/librte_mbuf/rte_mbuf.c       |  2 ++
 lib/librte_mbuf/rte_mbuf.h       | 45 +++++++++++++++++++++++++++++-----------
 4 files changed, 38 insertions(+), 15 deletions(-)
  

On Tue, Jan 24, 2017 at 04:19:28PM +0100, Olivier Matz wrote:
> Set the value of m->refcnt to 1, m->nb_segs to 1 and m->next
> to NULL when the mbuf is stored inside the mempool (unused).
> This is done in rte_pktmbuf_prefree_seg(), before freeing or
> recycling a mbuf.
> 
> Before this patch, the value of m->refcnt was expected to be 0
> while in pool.
> 
> The objectives are:
> 
> - to avoid drivers to set m->next to NULL in the early Rx path, since
>   this field is in the second 64B of the mbuf and its access could
>   trigger a cache miss
> 
> - rationalize the behavior of raw_alloc/raw_free: one is now the
>   symmetric of the other, and refcnt is never changed in these functions.
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  drivers/net/mlx5/mlx5_rxtx.c     |  5 ++---
>  drivers/net/mpipe/mpipe_tilegx.c |  1 +
>  lib/librte_mbuf/rte_mbuf.c       |  2 ++
>  lib/librte_mbuf/rte_mbuf.h       | 45 +++++++++++++++++++++++++++++-----------
>  4 files changed, 38 insertions(+), 15 deletions(-)
> 
<snip>
> /* compat with older versions */
>  __rte_deprecated
> -static inline void __attribute__((always_inline))
> +static inline void
>  __rte_mbuf_raw_free(struct rte_mbuf *m)
>  {
>  	rte_mbuf_raw_free(m);
> @@ -1218,8 +1232,12 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
>  	m->data_len = 0;
>  	m->ol_flags = 0;
>  
> -	if (rte_mbuf_refcnt_update(md, -1) == 0)
> +	if (rte_mbuf_refcnt_update(md, -1) == 0) {

Minor nit, but in the case that we only have a single reference to the
mbufs, we are always setting that to zero just to re-increment it to 1
again.

> +		md->next = NULL;
> +		md->nb_segs = 1;
> +		rte_mbuf_refcnt_set(md, 1);
>  		rte_mbuf_raw_free(md);
> +	}
>  }
>  
>  /**
  

Hi Bruce,

On Tue, 24 Jan 2017 15:50:49 +0000, Bruce Richardson
<bruce.richardson@intel.com> wrote:
> On Tue, Jan 24, 2017 at 04:19:28PM +0100, Olivier Matz wrote:
> > Set the value of m->refcnt to 1, m->nb_segs to 1 and m->next
> > to NULL when the mbuf is stored inside the mempool (unused).
> > This is done in rte_pktmbuf_prefree_seg(), before freeing or
> > recycling a mbuf.
> > 
> > Before this patch, the value of m->refcnt was expected to be 0
> > while in pool.
> > 
> > The objectives are:
> > 
> > - to avoid drivers to set m->next to NULL in the early Rx path,
> > since this field is in the second 64B of the mbuf and its access
> > could trigger a cache miss
> > 
> > - rationalize the behavior of raw_alloc/raw_free: one is now the
> >   symmetric of the other, and refcnt is never changed in these
> > functions.
> > 
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > ---
> >  drivers/net/mlx5/mlx5_rxtx.c     |  5 ++---
> >  drivers/net/mpipe/mpipe_tilegx.c |  1 +
> >  lib/librte_mbuf/rte_mbuf.c       |  2 ++
> >  lib/librte_mbuf/rte_mbuf.h       | 45
> > +++++++++++++++++++++++++++++----------- 4 files changed, 38
> > insertions(+), 15 deletions(-) 
> <snip>
> > /* compat with older versions */
> >  __rte_deprecated
> > -static inline void __attribute__((always_inline))
> > +static inline void
> >  __rte_mbuf_raw_free(struct rte_mbuf *m)
> >  {
> >  	rte_mbuf_raw_free(m);
> > @@ -1218,8 +1232,12 @@ static inline void rte_pktmbuf_detach(struct
> > rte_mbuf *m) m->data_len = 0;
> >  	m->ol_flags = 0;
> >  
> > -	if (rte_mbuf_refcnt_update(md, -1) == 0)
> > +	if (rte_mbuf_refcnt_update(md, -1) == 0) {  
> 
> Minor nit, but in the case that we only have a single reference to the
> mbufs, we are always setting that to zero just to re-increment it to 1
> again.
> 
> > +		md->next = NULL;
> > +		md->nb_segs = 1;
> > +		rte_mbuf_refcnt_set(md, 1);
> >  		rte_mbuf_raw_free(md);
> > +	}
> >  }
> >  
> >  /**  


I'm trying to gather the comments that have been made on this patchset.
About this one, I think it would be more complex to change the code
to avoid to set the refcnt twice:

 - we would need to duplicate code from rte_mbuf_refcnt_update(), which
   I think is not a very good idea, due to the big comment
 - it would make the detach code less readable
 - it's even not sure that it will be more performant: since
   rte_mbuf_refcnt_update() is inline, the compiler is probably able to
   do the simplification by itself.



Olivier