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

["Ananyev, Konstantin" <konstantin.ananyev@intel.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.

Honestly, I don't understand why.
Right now the rule of thumb is - when mbuf is in the pool, it's refcnt should be equal zero.
Yes, as you pointed below - that rule probably can be changed to: 
when mbuf is in the pool, it's refcnt should equal one, and that would probably allow us
to speedup things a bit, but I suppose that's the matter of another aptch/discussion.

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

That's' just an assert() enabled when MBUF_DEBUG  is on.
Its sole purpose is to help troubleshoot the bugs and help to catch situations
when someone silently updates mbufs supposed to be free.  

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

Then it probably is a bug in these PMDs that need to be fixed.


> 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().

These are internal functions.
Comments in mbuf clearly state that library users shouldn't call them directly.
They are written to fit internal librte_mbuf needs, and no-one ever promised
malloc/free() compatibility here. 

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

That just doesn't seem correct to me.
The proper way to do free fo mbuf segment is:

static inline void __attribute__((always_inline))
rte_pktmbuf_free_seg(struct rte_mbuf *m)
{
        if (likely(NULL != (m = __rte_pktmbuf_prefree_seg(m)))) {
                m->next = NULL;
                __rte_mbuf_raw_free(m);
        }
}

If by some reason you choose not to use this function, then it is your
responsibility to perform similar actions on your own before pushing mbuf into the pool.
That's what some TX functions for some Intel NICs do to improve performance:
they call _prefree_seg() manually and try to put mbufs into the pool in groups.

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

I can hardly imagine that __rte_pktmbuf_prefree_seg() impact would be that severe...
But ok, probably  you do have some very specific case, but then why you PMD just doesn't call:
rte_mempool_put(m->pool, m); 
directly?
Why instead you choose to change common functions and compromise
librte_mbuf debug ability?

> 
> What remains is the consistency issue, I think these statements were only
> added to catch multiple frees,

Yes these asserts() here to help catch bugs,
and I think it is a good thing to have them here. 

> and those should be caught at a higher
> level, where other consistency checks are also performed.

Like where?

Konstantin


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