Re: Is it OK to pass non-acquired objects to kfree?

[Eric Dumazet <eric.dumazet@gmail.com>]

On Thu, 2015-09-10 at 14:08 +0200, Dmitry Vyukov wrote:
> On Thu, Sep 10, 2015 at 12:42 PM, Jesper Dangaard Brouer

> > This reminds me of some code in the network stack[1] in kfree_skb()
> > where we have a smp_rmb().  Should we have used smp_load_acquire() ?
> >
> >  void kfree_skb(struct sk_buff *skb)
> >  {
> >         if (unlikely(!skb))
> >                 return;
> >         if (likely(atomic_read(&skb->users) == 1))
> >                 smp_rmb();
> >         else if (likely(!atomic_dec_and_test(&skb->users)))
> >                 return;
> >         trace_kfree_skb(skb, __builtin_return_address(0));
> >         __kfree_skb(skb);
> >  }
> >  EXPORT_SYMBOL(kfree_skb);
> 
> rmb is much better than nothing :)
> I generally prefer to use smp_load_acquire just because it's more
> explicit (you see what memory access the barrier relates to), fewer
> lines of code, agrees with modern atomic APIs in C, C++, Java, etc,
> and FWIW is much better for dynamic race detectors.
> As for semantic difference between rmb and smp_load_acquire, rmb does
> not order stores, so stores from __kfree_skb can hoist above the
> atomic_read(&skb->users) == 1 check. The only architecture that can do
> that is Alpha, I don't know enough about Alpha and barrier
> implementation on Alpha (maybe rmb and smp_load_acquire do the same
> hardware barrier on Alpha) to say whether it can break in real life or
> not. But I would still consider smp_load_acquire as safer and cleaner
> alternative.

smp_load_acquire() is a kid compared to kfree_skb() code written decades
ago.

Sure, new code has plenty of ways to implement all this stuff, and now
we can discuss days about choosing right variant in a single spot.

In the old days, Alexei was writing thousand of lines of code per day,
and he got it mostly right, even for the Alpha ;)

Another discussion is whether or not reading the value before attempting
the lock atomic dec is a win on modern cpus, because it might incur an
additional bus transaction in the case skbs are allocated/freed on
different cpus. I believe I made tests ~4 years ago and it was worth
keeping it.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>