Message-ID: <20230828101420.GA54787@dev-dsk-mheyne-1b-c1362c4d.eu-west-1.amazon.com>
Date:   Mon, 28 Aug 2023 10:14:20 +0000
From:   Maximilian Heyne <mheyne@...zon.de>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     <stable@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Michael Larabel <Michael@...haellarabel.com>,
        Matthieu Baerts <matthieu.baerts@...sares.net>,
        Dave Chinner <david@...morbit.com>,
        Matthew Wilcox <willy@...radead.org>, Chris Mason <clm@...com>,
        Jan Kara <jack@...e.cz>, Amir Goldstein <amir73il@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Kees Cook <keescook@...omium.org>, <linux-mm@...ck.org>,
        <linux-kernel@...r.kernel.org>, <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] mm: allow a controlled amount of unfairness in the page
 lock

On Sun, Aug 27, 2023 at 10:54:03AM +0200, Greg KH wrote:
> On Wed, Aug 23, 2023 at 06:16:42AM +0000, Maximilian Heyne wrote:
> > From: Linus Torvalds <torvalds@...ux-foundation.org>
> >
> > [ upstream commit 5ef64cc8987a9211d3f3667331ba3411a94ddc79 ]
> >
> > Commit 2a9127fcf229 ("mm: rewrite wait_on_page_bit_common() logic") made
> > the page locking entirely fair, in that if a waiter came in while the
> > lock was held, the lock would be transferred to the lockers strictly in
> > order.
> >
> > That was intended to finally get rid of the long-reported watchdog
> > failures that involved the page lock under extreme load, where a process
> > could end up waiting essentially forever, as other page lockers stole
> > the lock from under it.
> >
> > It also improved some benchmarks, but it ended up causing huge
> > performance regressions on others, simply because fair lock behavior
> > doesn't end up giving out the lock as aggressively, causing better
> > worst-case latency, but potentially much worse average latencies and
> > throughput.
> >
> > Instead of reverting that change entirely, this introduces a controlled
> > amount of unfairness, with a sysctl knob to tune it if somebody needs
> > to.  But the default value should hopefully be good for any normal load,
> > allowing a few rounds of lock stealing, but enforcing the strict
> > ordering before the lock has been stolen too many times.
> >
> > There is also a hint from Matthieu Baerts that the fair page coloring
> > may end up exposing an ABBA deadlock that is hidden by the usual
> > optimistic lock stealing, and while the unfairness doesn't fix the
> > fundamental issue (and I'm still looking at that), it avoids it in
> > practice.
> >
> > The amount of unfairness can be modified by writing a new value to the
> > 'sysctl_page_lock_unfairness' variable (default value of 5, exposed
> > through /proc/sys/vm/page_lock_unfairness), but that is hopefully
> > something we'd use mainly for debugging rather than being necessary for
> > any deep system tuning.
> >
> > This whole issue has exposed just how critical the page lock can be, and
> > how contended it gets under certain locks.  And the main contention
> > doesn't really seem to be anything related to IO (which was the origin
> > of this lock), but for things like just verifying that the page file
> > mapping is stable while faulting in the page into a page table.
> >
> > Link: https://lore.kernel.org/linux-fsdevel/ed8442fd-6f54-dd84-cd4a-941e8b7ee603@MichaelLarabel.com/
> > Link: https://www.phoronix.com/scan.php?page=article&item=linux-50-59&num=1
> > Link: https://lore.kernel.org/linux-fsdevel/c560a38d-8313-51fb-b1ec-e904bd8836bc@tessares.net/
> > Reported-and-tested-by: Michael Larabel <Michael@...haellarabel.com>
> > Tested-by: Matthieu Baerts <matthieu.baerts@...sares.net>
> > Cc: Dave Chinner <david@...morbit.com>
> > Cc: Matthew Wilcox <willy@...radead.org>
> > Cc: Chris Mason <clm@...com>
> > Cc: Jan Kara <jack@...e.cz>
> > Cc: Amir Goldstein <amir73il@...il.com>
> > Signed-off-by: Linus Torvalds <torvalds@...ux-foundation.org>
> > CC: <stable@...r.kernel.org> # 5.4
> > [ mheyne: fixed contextual conflict in mm/filemap.c due to missing
> >   commit c7510ab2cf5c ("mm: abstract out wake_page_match() from
> >   wake_page_function()"). Added WQ_FLAG_CUSTOM due to missing commit
> >   7f26482a872c ("locking/percpu-rwsem: Remove the embedded rwsem") ]
> > Signed-off-by: Maximilian Heyne <mheyne@...zon.de>
> > ---
> >  include/linux/mm.h   |   2 +
> >  include/linux/wait.h |   2 +
> >  kernel/sysctl.c      |   8 +++
> >  mm/filemap.c         | 160 ++++++++++++++++++++++++++++++++++---------
> >  4 files changed, 141 insertions(+), 31 deletions(-)
> 
> This was also backported here:
>         https://lore.kernel.org/r/20230821222547.483583-1-saeed.mirzamohammadi@oracle.com
> before yours.
> 
> I took that one, can you verify that it is identical to yours and works
> properly as well?

Yes it's identical and fixes the performance regression seen. Therefore,

  Tested-by: Maximilian Heyne <mheyne@...zon.de>

for the other patch.

Thanks,

Max



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Powered by blists - more mailing lists