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@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>