Re: [dpdk-dev] [PATCH] mbuf: remove inconsistent assert statements

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

Hi Konstantin,

On Wed, Jun 08, 2016 at 10:34:17AM +0000, Ananyev, Konstantin wrote:
> Hi Adrien,
> 
> > 
> > An assertion failure occurs in __rte_mbuf_raw_free() (called by a few PMDs)
> > when compiling DPDK with CONFIG_RTE_LOG_LEVEL=RTE_LOG_DEBUG and starting
> > applications with a log level high enough to trigger it.
> > 
> > While rte_mbuf_raw_alloc() sets refcount to 1, __rte_mbuf_raw_free()
> > expects it to be 0. 
> >Considering users are not expected to reset the
> > reference count to satisfy assert() and that raw functions are designed on
> > purpose without safety belts, remove these checks.
> 
> Yes, it refcnt supposed to be set to 0 by __rte_pktmbuf_prefree_seg().
> Wright now, it is a user responsibility to make sure refcnt==0 before pushing
> mbuf back to the pool.
> Not sure why do you consider that wrong?

I do not consider this wrong and I'm all for using assert() to catch
programming errors, however in this specific case, I think they are
inconsistent and misleading.

> If the user calls __rte_mbuf_raw_free() manualy it is his responsibility to make
> sure mbuf's refcn==0.

Sure, however what harm does it cause (besides assert() to fail), since the
allocation function sets refcount to 1?

Why having the allocation function set the refcount if we are sure it is
already 0 (assert() proves it). Removing rte_mbuf_refcnt_set(m, 1) can
surely improve performance.

> BTW, why are you doing it?
> The comment clearly states that the function is for internal use:
> /**
>  * @internal Put mbuf back into its original mempool.
>  * The use of that function is reserved for RTE internal needs.
>  * Please use rte_pktmbuf_free().
>  *
>  * @param m
>  *   The mbuf to be freed.
>  */
> static inline void __attribute__((always_inline))
> __rte_mbuf_raw_free(struct rte_mbuf *m)

Several PMDs are using it anyway (won't name names, but I know one of them
quite well). I chose to modify this code instead of its users for the
following reasons:

- Considering their names, these functions should be opposites and able to
  work together like malloc()/free().

- PMDs are relying on these functions for performance reasons, we can assume
  they took the extra care necessary to make sure it would work properly.

- Preventing it would make these PMDs slower and is not acceptable either.

What remains is the consistency issue, I think these statements were only
added to catch multiple frees, and those should be caught at a higher
level, where other consistency checks are also performed.

> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > ---
> >  lib/librte_mbuf/rte_mbuf.h | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index 11fa06d..7070bb8 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -1108,7 +1108,6 @@ static inline struct rte_mbuf *rte_mbuf_raw_alloc(struct rte_mempool *mp)
> >  	if (rte_mempool_get(mp, &mb) < 0)
> >  		return NULL;
> >  	m = (struct rte_mbuf *)mb;
> > -	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 0);
> >  	rte_mbuf_refcnt_set(m, 1);
> >  	__rte_mbuf_sanity_check(m, 0);
> > 
> > @@ -1133,7 +1132,6 @@ __rte_mbuf_raw_alloc(struct rte_mempool *mp)
> >  static inline void __attribute__((always_inline))
> >  __rte_mbuf_raw_free(struct rte_mbuf *m)
> >  {
> > -	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 0);
> >  	rte_mempool_put(m->pool, m);
> >  }
> > 
> > --
> > 2.1.4
> 

-- 
Adrien Mazarguil
6WIND