On Mon, 19 Aug 2024, Linus Torvalds wrote:
> On Sun, 18 Aug 2024 at 22:36, NeilBrown <neilb@xxxxxxx> wrote:
> >
> > The main patches here are 7 and 8 which revise wake_up_bit and
> > wake_up_var respectively. They result in 3 interfaces:
> > wake_up_{bit,var} includes smp_mb__after_atomic()
>
> I actually think this is even worse than the current model, in that
> now it subtle only works after atomic ops, and it's not obvious from
> the name.
>
> At least the current model, correct code looks like
>
> do_some_atomic_op
> smp_mb__after_atomic()
> wake_up_{bit,var}
>
> and the smp_mb__after_atomic() makes sense and pairs with the atomic.
> So the current one may be complex, but at the same time it's also
> explicit. Your changed interface is still complex, but now it's even
> less obvious what is actually going on.
>
> With your suggested interface, a plain "wake_up_{bit,var}" only works
> after atomic ops, and other ops have to magically know that they
> should use the _mb() version or whatever. And somebody who doesn't
> understand that subtlety, and copies the code (but changes the op from
> an atomic one to something else) now introduces code that looks fine,
> but is really subtly wrong.
>
> The reason for the barrier is for the serialization with the
> waitqueue_active() check. Honestly, if you worry about correctness
> here, I think you should leave the existing wake_up_{bit,var}() alone,
> and concentrate on having helpers that do the whole "set and wake up".
>
> IOW, I do not think you should change existing semantics, but *this*
> kind of pattern:
>
> > [PATCH 2/9] Introduce atomic_dec_and_wake_up_var().
> > [PATCH 9/9] Use clear_and_wake_up_bit() where appropriate.
>
> sounds like a good idea.
>
> IOW, once you have a whole "atomic_dec_and_wake_up()" (skip the "_var"
> - it's implied by the fact that it's an atomic_dec), *then* that
> function makes for a simple-to-use model, and now the "atomic_dec(),
> the smp_mb__after_atomic(), and the wake_up_var()" are all together.
>
> For all the same reasons, it makes total sense to have
> "clear_bit_and_wake()" etc.
I can definitely get behind the idea has having a few more helpers and
using them more widely. But unless we get rid of wake_up_bit(), people
will still use and some will use it wrongly.
What would you think of changing wake_up_bit/var to always have a full
memory barrier, and adding wake_up_bit/var_relaxed() for use when a
different barrier, or not barrier, is wanted?
Thanks,
NeilBrown
>
> But exposing those "three different memory barrier scenarios" as three
> different helpers is the *opposite* of helpful. It keeps the current
> complexity, and makes it worse by making the barrier rules even more
> opaque, imho.
>
> Linus
>
