On Thu, Jul 23, 2020 at 5:47 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> I still can't convince myself thatI fully understand this patch but I see
> nothing really wrong after a quick glance...

I guess my comments should be extended further then.

Is there anything in particular you think is unclear?

> > +     wake_up_state(target, mode);
>
> We can no longer use 'target'. If it was already woken up it can notice
> list_empty_careful(), return without taking q->lock, and exit.

Good point.

And yes, I think using WQ_FLAG_WOKEN is the right thing to do, and I
wouldn't call it "abuse". It's exactly what it's for.

And that also allows us to just use finish_wait(), since we no longer
care as deeply about the waitlist state, we can just test that
WQ_FLAG_WOKEN at the end instead.

So that actually makes the patch much more straightforward too. I
really disliked my open-coding there. Your suggestion fixes
everything.

> do we need SetPageWaiters() if trylock() succeeds ?

We need to set it before the final page flag test, because otherwise
we might miss somebody just about to wake us up (ie we see the bit
set, but it's getting cleared on another CPU, and if PageWaiters isn't
set then that other CPU won't do the wakeup).

So here's a v2, now as a "real" commit with a commit message and everything.

Is there anything in particular you would like clarified, or something
else you find in this?

Hugh - this should make your "patch 2" redundant. Is there any way to
test that in your environment that triggered it?

This v2 isn't tested, but the core of it is the same, just with nice
cleanups from Oleg's suggestion, and an added comment about that
SetPageWaiters() thing.

            Linus