Re: [dpdk-dev] [PATCH] mbuf: optimize refcnt handling during free

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

Hi Neil,

On 03/27/2015 11:25 AM, Neil Horman wrote:
> On Thu, Mar 26, 2015 at 09:00:33PM +0000, Wiles, Keith wrote:
>>
>>
>> On 3/26/15, 1:10 PM, "Zoltan Kiss" <zoltan.kiss@linaro.org> wrote:
>>
>>> The current way is not the most efficient: if m->refcnt is 1, the second
>>> condition never evaluates, and we set it to 0. If refcnt > 1, the 2nd
>>> condition fails again, although the code suggest otherwise to branch
>>> prediction. Instead we should keep the second condition only, and remove
>>> the
>>> duplicate set to zero.
>>>
>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
>>> ---
>>> lib/librte_mbuf/rte_mbuf.h | 5 +----
>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>>> index 17ba791..3ec4024 100644
>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>> @@ -764,10 +764,7 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>>> {
>>> 	__rte_mbuf_sanity_check(m, 0);
>>>
>>> -	if (likely (rte_mbuf_refcnt_read(m) == 1) ||
>>> -			likely (rte_mbuf_refcnt_update(m, -1) == 0)) {
>>> -
>>> -		rte_mbuf_refcnt_set(m, 0);
>>> +	if (likely (rte_mbuf_refcnt_update(m, -1) == 0)) {
>>>
>>> 		/* if this is an indirect mbuf, then
>>> 		 *  - detach mbuf
>>
>> I fell for this one too, but read Bruce¹s email
>> http://dpdk.org/ml/archives/dev/2015-March/014481.html
> 
> This is still the right thing to do though, Bruce's reasoning is erroneous.
> Just because the return from rte_mbuf_refcnt_read returns 1, doesn't mean you
> are the last user of the mbuf, you are only guaranteed that if the update
> operation returns zero.
> 
> In other words:
> rte_mbuf_refcnt_update(m, -1)
> 
> is an atomic operation
> 
> if (likely (rte_mbuf_refcnt_read(m) == 1) ||
>                     likely (rte_mbuf_refcnt_update(m, -1) == 0)) {
> 
> 
> is not.
> 
> To illustrate, on two cpus, this might occur:
> 
> CPU0					CPU1
> rte_mbuf_refcnt_read			...
>    returns 1				rte_mbuf_refcnt_read
> ...					   returns 1
> execute if clause			execute if clause
> 
> In the above scenario both cpus fell into the if clause because they both held a
> pointer to the same buffer and both got a return value of one, so they skipped
> the update portion of the if clause and both executed the internal block of the
> conditional expression.  you might be tempted to think thats ok, since that
> block just sets the refcnt to zero, and doing so twice isn't harmful, but the
> entire purpose of that if conditional above was to ensure that only one
> execution context ever executed the conditional for a given buffer.  Look at
> what else happens in that conditional:

I disagree, I also spent some time to think about this code, and I think
Bruce is right here. If you read rte_mbuf_refcnt and it returns 1, it
means you are the last user, so no other core references the mbuf
anymore.

Your scenario is not possible, because 2 CPUs do not have the right to
access to a mbuf pointer at the same time. It's like writing data in the
mbuf while reading it on another core. If you think your scenario can
happen, could you give an example of code that would led to such case?

If you want to use a mbuf on 2 CPUs at the same time, you have to clone
it first, and in this case the reference counter would be at least 2,
preventing your case to happen


Olivier



> 
> static inline struct rte_mbuf* __attribute__((always_inline))
> __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> {
>         __rte_mbuf_sanity_check(m, 0);
> 
>         if (likely (rte_mbuf_refcnt_read(m) == 1) ||
>                         likely (rte_mbuf_refcnt_update(m, -1) == 0)) {
> 
>                 rte_mbuf_refcnt_set(m, 0);
> 
>                 /* if this is an indirect mbuf, then
>                  *  - detach mbuf
>                  *  - free attached mbuf segment
>                  */
>                 if (RTE_MBUF_INDIRECT(m)) {
>                         struct rte_mbuf *md = RTE_MBUF_FROM_BADDR(m->buf_addr);
>                         rte_pktmbuf_detach(m);
>                         if (rte_mbuf_refcnt_update(md, -1) == 0)
>                                 __rte_mbuf_raw_free(md);
>                 }
>                 return(m);
>         }
>         return (NULL);
> }
> 
> If the buffer is indirect, another refcnt update occurs to the buf_addr mbuf,
> and in the scenario I outlined above, that refcnt will underflow, likely causing
> a buffer leak.  Additionally, the return code of this function is designed to
> indicate to the caller if they were the last user of the buffer.  In the above
> scenario, two execution contexts will be told that they were, which is wrong.
> 
> Zoltans patch is a good fix
> 
> Acked-by: Neil Horman <nhorman@tuxdriver.com>
>