From: Peter Xu <peterx@redhat.com>
To: Ingo Molnar <mingo@kernel.org>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
Richard Henderson <rth@twiddle.net>,
David Hildenbrand <david@redhat.com>,
Matt Turner <mattst88@gmail.com>,
Albert Ou <aou@eecs.berkeley.edu>,
Michal Simek <monstr@monstr.eu>,
Russell King <linux@armlinux.org.uk>,
Ivan Kokshaysky <ink@jurassic.park.msu.ru>,
linux-riscv@lists.infradead.org,
Alexander Gordeev <agordeev@linux.ibm.com>,
Dave Hansen <dave.hansen@linux.intel.com>,
Jonas Bonn <jonas@southpole.se>, Will Deacon <will@kernel.org>,
"James E . J . Bottomley" <James.Bottomley@hansenpartnership.com>,
"H . Peter Anvin" <hpa@zytor.com>,
Andrea Arcangeli <aarcange@redhat.com>,
openrisc@lists.librecores.org, linux-s390@vger.kernel.org,
Ingo Molnar <mingo@redhat.com>,
linux-m68k@lists.linux-m68k.org
Subject: Re: [PATCH v3] mm: Avoid unnecessary page fault retires on shared memory types
Date: Fri, 27 May 2022 10:53:36 -0400 [thread overview]
Message-ID: <YpDl8IM0FF6GeJ4B@xz-m1.local> (raw)
In-Reply-To: <YpCsBwFArieTpvg2@gmail.com>
On Fri, May 27, 2022 at 12:46:31PM +0200, Ingo Molnar wrote:
>
> * Peter Xu <peterx@redhat.com> wrote:
>
> > This patch provides a ~12% perf boost on my aarch64 test VM with a simple
> > program sequentially dirtying 400MB shmem file being mmap()ed and these are
> > the time it needs:
> >
> > Before: 650.980 ms (+-1.94%)
> > After: 569.396 ms (+-1.38%)
>
> Nice!
>
> > arch/x86/mm/fault.c | 4 ++++
>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>
>
> Minor comment typo:
>
> > + /*
> > + * We should do the same as VM_FAULT_RETRY, but let's not
> > + * return -EBUSY since that's not reflecting the reality on
> > + * what has happened - we've just fully completed a page
> > + * fault, with the mmap lock released. Use -EAGAIN to show
> > + * that we want to take the mmap lock _again_.
> > + */
>
> s/reflecting the reality on what has happened
> /reflecting the reality of what has happened
Will fix.
>
> > ret = handle_mm_fault(vma, address, fault_flags, NULL);
> > +
> > + if (ret & VM_FAULT_COMPLETED) {
> > + /*
> > + * NOTE: it's a pity that we need to retake the lock here
> > + * to pair with the unlock() in the callers. Ideally we
> > + * could tell the callers so they do not need to unlock.
> > + */
> > + mmap_read_lock(mm);
> > + *unlocked = true;
> > + return 0;
>
> Indeed that's a pity - I guess more performance could be gained here,
> especially in highly parallel threaded workloads?
Yes I think so.
The patch avoids the page fault retry, including the mmap lock/unlock side.
Now if we retake the lock for fixup_user_fault() we still safe time for
pgtable walks but the lock overhead will be somehow kept, just with smaller
critical sections.
Some fixup_user_fault() callers won't be affected as long as unlocked==NULL
is passed - e.g. the futex code path (fault_in_user_writeable). After all
they never needed to retake the lock before/after this patch.
It's about the other callers, and they may need some more touch-ups case by
case. Examples are follow_fault_pfn() in vfio and hva_to_pfn_remapped() in
KVM: both of them returns -EAGAIN when *unlocked==true. We need to teach
them to know "*unlocked==true" does not necessarily mean a retry attempt.
I think I can look into them if this patch can be accepted as a follow up.
Thanks for taking a look!
--
Peter Xu
next prev parent reply other threads:[~2022-05-27 14:53 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-24 23:45 [PATCH v3] mm: Avoid unnecessary page fault retires on shared memory types Peter Xu
2022-05-25 8:03 ` Geert Uytterhoeven
2022-05-25 11:10 ` Peter Zijlstra
2022-05-25 12:44 ` Johannes Weiner
2022-05-26 3:40 ` Vineet Gupta
2022-05-27 2:54 ` Guo Ren
2022-05-27 5:39 ` Max Filippov
2022-05-27 8:21 ` Alistair Popple
2022-05-27 10:46 ` Ingo Molnar
2022-05-27 14:53 ` Peter Xu [this message]
2022-05-27 12:23 ` Heiko Carstens
2022-05-27 13:49 ` Peter Xu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YpDl8IM0FF6GeJ4B@xz-m1.local \
--to=peterx@redhat.com \
--cc=James.Bottomley@hansenpartnership.com \
--cc=aarcange@redhat.com \
--cc=agordeev@linux.ibm.com \
--cc=aou@eecs.berkeley.edu \
--cc=dave.hansen@linux.intel.com \
--cc=david@redhat.com \
--cc=hpa@zytor.com \
--cc=ink@jurassic.park.msu.ru \
--cc=jonas@southpole.se \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-m68k@lists.linux-m68k.org \
--cc=linux-mm@kvack.org \
--cc=linux-riscv@lists.infradead.org \
--cc=linux-s390@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=mattst88@gmail.com \
--cc=mingo@kernel.org \
--cc=mingo@redhat.com \
--cc=monstr@monstr.eu \
--cc=openrisc@lists.librecores.org \
--cc=rth@twiddle.net \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).