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

[Olivier Matz <olivier.matz@6wind.com>]

Hi Adrien, Konstantin,

I'm jumping in this (interesting) discussion. I already talked a
bit with Adrien in point to point, and I think its patch is valid.
Please see some comments below.


On 06/08/2016 03:57 PM, Adrien Mazarguil wrote:
> 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.

What is the purpose of this? Is there some code relying on this?

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

I agree this is somehow another discussion, but let's dive into :)

But since [1], it is allowed to call rte_mbuf_raw_alloc() in PMDs (all
PMDs were calling an internal function before). We could argue that
rte_mbuf_raw_free() should also be made public for PMDs.

As you said below, no-one promised that the free() reverts the malloc(),
but given the function names, one can legitimately expect that the
following code is valid:

m = __rte_mbuf_raw_alloc();
/* do nothing here */
__rte_mbuf_raw_free(m);

If no code relies on having the refcnt set to 0 when a mbuf is in
the pool, I suggest to relax this constraint as Adrien proposed.

Then, my opinion is that the refcount should be set to 1 in
rte_pktmbuf_reset(). And rte_pktmbuf_free() should not be allowed on
an uninitialized mbuf (yes, this would require some changes in PMDs).
This would open the door for bulk allocation/free in the mbuf api.

This could be done in several steps:

1/ remove these assert(), add introduce a public rte_mbuf_raw_free()
2/ announce that rte_pktmbuf_free() won't work on uninitialized mbufs
in a future version, and ensure that all PMD are inline with this
requirement
3/ later, move refcount initialization in rte_pktmbuf_reset()

What do you think?

Another option is to remove the rte_mbuf_raw_alloc()/rte_mbuf_raw_free()
and use mempool calls. But having a mbuf wrapper does not seem a bad
thing to me.

By the way, __rte_pktmbuf_prefree_seg() is also an internal function.
I think we should try to update the mbuf API so that the PMDs do not
need to call these internal functions.


[1]
http://dpdk.org/browse/dpdk/commit/?id=fbfd99551ca370266f4bfff58ce441cf5cb1203a


Regards,
Olivier


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