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

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

On Wed, Jun 08, 2016 at 01:09:18PM +0000, Ananyev, Konstantin wrote:
> > 
> > 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.

Agreed.

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

I perfectly understand and I cannot agree more with this explanation,
however the fact these functions are not symmetrical remains an issue that
needs to be addressed somehow in my opinion.

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

So it cannot be provided for the sake of not providing it or is there a good
reason?

What I meant is that since PMDs already made the mistake of using these
functions to benefit from the improved performance, DPDK being all about
performance and stuff, let them use it as intended. Perhaps we should drop
those "__" like for rte_mbuf_raw_alloc().

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

Not anymore it seems, but in the current code base both ena and mpipe PMDs
(soon mlx5 as well) seem to get this wrong.

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

To survive the upcoming transition to mbufs backed by libc malloc() without
having to fix them? Joke aside, I guess the reason is to use functions with
"mbuf" in their names when dealing with mbufs.

> Why instead you choose to change common functions and compromise
> librte_mbuf debug ability?

No, I'm fine with keeping the debug ability, however I did not find a way to
both keep it and fix the consistency issue without breaking something
(performance or refcount assumptions I'm not familiar with elsewhere).

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

Possibly rte_pktmbuf_free_seg().

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

-- 
Adrien Mazarguil
6WIND