RE: [PATCH 2/2 v2] sched/wait: Introduce lock breaker in wake_up_page_bit

[Liang, Kan]

> On Mon, Aug 28, 2017 at 7:51 AM, Liang, Kan <kan.liang@xxxxxxxxx> wrote:
> >
> > I tried this patch and https://lkml.org/lkml/2017/8/27/222 together.
> > But they don't fix the issue. I can still get the similar call stack.
> 
> So the main issue was that I *really* hated Tim's patch #2, and the patch to
> clean up the page wait queue should now make his patch series much more
> palatable.
> 
> Attached is an ALMOST COMPLETELY UNTESTED forward-port of those two
> patches, now without that nasty WQ_FLAG_ARRIVALS logic, because we now
> always put the new entries at the end of the waitqueue.
> 

The patches fix the long wait issue.

Tested-by: Kan Liang <kan.liang@xxxxxxxxx>

> The attached patches just apply directly on top of plain 4.13-rc7.
> 
> That makes patch #2 much more palatable, since it now doesn't need to play
> games and worry about new arrivals.
> 
> But note the lack of testing. I've actually booted this and am running these
> two patches right now, but honestly, you should consider them "untested"
> simply because I can't trigger the page waiters contention case to begin with.
> 
> But it's really just Tim's patches, modified for the page waitqueue cleanup
> which makes patch #2 become much simpler, and now it's
> palatable: it's just using the same bookmark thing that the normal wakeup
> uses, no extra hacks.
> 
> So Tim should look these over, and they should definitely be tested on that
> load-from-hell that you guys have, but if this set works, at least I'm ok with it
> now.
> 
> Tim - did I miss anything? I added a "cpu_relax()" in there between the
> release lock and irq and re-take it, I'm not convinced it makes any difference,
> but I wanted to mark that "take a breather" thing.
> 
> Oh, there's one more case I only realized after the patches: the stupid
> add_page_wait_queue() code still adds to the head of the list.
> So technically you need this too:
> 
>     diff --git a/mm/filemap.c b/mm/filemap.c
>     index 74123a298f53..598c3be57509 100644
>     --- a/mm/filemap.c
>     +++ b/mm/filemap.c
>     @@ -1061,7 +1061,7 @@ void add_page_wait_queue(struct page *page,
> wait_queue_entry_t *waiter)
>         unsigned long flags;
> 
>         spin_lock_irqsave(&q->lock, flags);
>     -   __add_wait_queue(q, waiter);
>     +   __add_wait_queue_entry_tail(q, waiter);
>         SetPageWaiters(page);
>         spin_unlock_irqrestore(&q->lock, flags);
>      }
> 
> but that only matters if you actually use the cachefiles thing, which I
> hope/assume you don't.
> 
>        Linus