Re: [PATCH v4] mm: Avoid unnecessary page fault retires on shared memory types
Am 30.05.22 um 18:00 schrieb Peter Xu:
On Mon, May 30, 2022 at 11:52:54AM -0400, Peter Xu wrote:
On Mon, May 30, 2022 at 11:35:10AM +0200, Christian Borntraeger wrote:
Am 29.05.22 um 22:33 schrieb Heiko Carstens:
[...]
Guess the patch below on top of your patch is what we want.
Just for clarification: if gmap is not NULL then the process is a kvm
process. So, depending on the workload, this optimization makes sense.
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 4608cc962ecf..e1d40ca341b7 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -436,12 +436,11 @@ static inline vm_fault_t do_exception(struct pt_regs
*regs, int access)
/* The fault is fully completed (including releasing mmap lock) */
if (fault & VM_FAULT_COMPLETED) {
- /*
-* Gmap will need the mmap lock again, so retake it. TODO:
-* only conditionally take the lock when CONFIG_PGSTE set.
-*/
- mmap_read_lock(mm);
- goto out_gmap;
+ if (gmap) {
+ mmap_read_lock(mm);
+ goto out_gmap;
+ }
+ goto out;
Hmm, right after I replied I found "goto out" could be problematic, since
all s390 callers of do_exception() will assume it an error condition (side
note: "goto out_gmap" contains one step to clear "fault" to 0). I'll
replace this with "return 0" instead if it looks good to both of you.
I'll wait for a confirmation before reposting. Thanks,
Yes, that sounds good and thank you for double checking.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v4] mm: Avoid unnecessary page fault retires on shared memory types
On Mon, May 30, 2022 at 07:03:31PM +0200, Heiko Carstens wrote:
> On Mon, May 30, 2022 at 12:00:52PM -0400, Peter Xu wrote:
> > On Mon, May 30, 2022 at 11:52:54AM -0400, Peter Xu wrote:
> > > On Mon, May 30, 2022 at 11:35:10AM +0200, Christian Borntraeger wrote:
> > > > > diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> > > > > index 4608cc962ecf..e1d40ca341b7 100644
> > > > > --- a/arch/s390/mm/fault.c
> > > > > +++ b/arch/s390/mm/fault.c
> > > > > @@ -436,12 +436,11 @@ static inline vm_fault_t do_exception(struct
> > > > > pt_regs *regs, int access)
> > > > > /* The fault is fully completed (including releasing mmap lock)
> > > > > */
> > > > > if (fault & VM_FAULT_COMPLETED) {
> > > > > - /*
> > > > > - * Gmap will need the mmap lock again, so retake it.
> > > > > TODO:
> > > > > - * only conditionally take the lock when CONFIG_PGSTE
> > > > > set.
> > > > > - */
> > > > > - mmap_read_lock(mm);
> > > > > - goto out_gmap;
> > > > > + if (gmap) {
> > > > > + mmap_read_lock(mm);
> > > > > + goto out_gmap;
> > > > > + }
> fault = 0; <
> > > > > + goto out;
> >
> > Hmm, right after I replied I found "goto out" could be problematic, since
> > all s390 callers of do_exception() will assume it an error condition (side
> > note: "goto out_gmap" contains one step to clear "fault" to 0). I'll
> > replace this with "return 0" instead if it looks good to both of you.
> >
> > I'll wait for a confirmation before reposting. Thanks,
>
> Right, that was stupid. Thanks for double checking!
>
> However could you please add "fault = 0" just in front of the goto out
> like above? I'd like to avoid having returns and gotos mixed.
Sure thing.
--
Peter Xu
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v4] mm: Avoid unnecessary page fault retires on shared memory types
On Mon, May 30, 2022 at 12:00:52PM -0400, Peter Xu wrote:
> On Mon, May 30, 2022 at 11:52:54AM -0400, Peter Xu wrote:
> > On Mon, May 30, 2022 at 11:35:10AM +0200, Christian Borntraeger wrote:
> > > > diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> > > > index 4608cc962ecf..e1d40ca341b7 100644
> > > > --- a/arch/s390/mm/fault.c
> > > > +++ b/arch/s390/mm/fault.c
> > > > @@ -436,12 +436,11 @@ static inline vm_fault_t do_exception(struct
> > > > pt_regs *regs, int access)
> > > > /* The fault is fully completed (including releasing mmap lock)
> > > > */
> > > > if (fault & VM_FAULT_COMPLETED) {
> > > > - /*
> > > > -* Gmap will need the mmap lock again, so retake it.
> > > > TODO:
> > > > -* only conditionally take the lock when CONFIG_PGSTE
> > > > set.
> > > > -*/
> > > > - mmap_read_lock(mm);
> > > > - goto out_gmap;
> > > > + if (gmap) {
> > > > + mmap_read_lock(mm);
> > > > + goto out_gmap;
> > > > + }
fault = 0; <
> > > > + goto out;
>
> Hmm, right after I replied I found "goto out" could be problematic, since
> all s390 callers of do_exception() will assume it an error condition (side
> note: "goto out_gmap" contains one step to clear "fault" to 0). I'll
> replace this with "return 0" instead if it looks good to both of you.
>
> I'll wait for a confirmation before reposting. Thanks,
Right, that was stupid. Thanks for double checking!
However could you please add "fault = 0" just in front of the goto out
like above? I'd like to avoid having returns and gotos mixed.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v4] mm: Avoid unnecessary page fault retires on shared memory types
On Mon, May 30, 2022 at 11:52:54AM -0400, Peter Xu wrote:
> On Mon, May 30, 2022 at 11:35:10AM +0200, Christian Borntraeger wrote:
> >
> >
> > Am 29.05.22 um 22:33 schrieb Heiko Carstens:
> > [...]
> > >
> > > Guess the patch below on top of your patch is what we want.
> > > Just for clarification: if gmap is not NULL then the process is a kvm
> > > process. So, depending on the workload, this optimization makes sense.
> > >
> > > diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> > > index 4608cc962ecf..e1d40ca341b7 100644
> > > --- a/arch/s390/mm/fault.c
> > > +++ b/arch/s390/mm/fault.c
> > > @@ -436,12 +436,11 @@ static inline vm_fault_t do_exception(struct
> > > pt_regs *regs, int access)
> > > /* The fault is fully completed (including releasing mmap lock)
> > > */
> > > if (fault & VM_FAULT_COMPLETED) {
> > > - /*
> > > - * Gmap will need the mmap lock again, so retake it. TODO:
> > > - * only conditionally take the lock when CONFIG_PGSTE set.
> > > - */
> > > - mmap_read_lock(mm);
> > > - goto out_gmap;
> > > + if (gmap) {
> > > + mmap_read_lock(mm);
> > > + goto out_gmap;
> > > + }
> > > + goto out;
Hmm, right after I replied I found "goto out" could be problematic, since
all s390 callers of do_exception() will assume it an error condition (side
note: "goto out_gmap" contains one step to clear "fault" to 0). I'll
replace this with "return 0" instead if it looks good to both of you.
I'll wait for a confirmation before reposting. Thanks,
--
Peter Xu
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v4] mm: Avoid unnecessary page fault retires on shared memory types
On Mon, May 30, 2022 at 11:35:10AM +0200, Christian Borntraeger wrote:
>
>
> Am 29.05.22 um 22:33 schrieb Heiko Carstens:
> [...]
> >
> > Guess the patch below on top of your patch is what we want.
> > Just for clarification: if gmap is not NULL then the process is a kvm
> > process. So, depending on the workload, this optimization makes sense.
> >
> > diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> > index 4608cc962ecf..e1d40ca341b7 100644
> > --- a/arch/s390/mm/fault.c
> > +++ b/arch/s390/mm/fault.c
> > @@ -436,12 +436,11 @@ static inline vm_fault_t do_exception(struct pt_regs
> > *regs, int access)
> > /* The fault is fully completed (including releasing mmap lock) */
> > if (fault & VM_FAULT_COMPLETED) {
> > - /*
> > -* Gmap will need the mmap lock again, so retake it. TODO:
> > -* only conditionally take the lock when CONFIG_PGSTE set.
> > -*/
> > - mmap_read_lock(mm);
> > - goto out_gmap;
> > + if (gmap) {
> > + mmap_read_lock(mm);
> > + goto out_gmap;
> > + }
> > + goto out;
>
> Yes, that makes sense. With that
>
> Acked-by: Christian Borntraeger
Looks sane, thanks Heiko, Christian. I'll cook another one.
--
Peter Xu
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v4] mm: Avoid unnecessary page fault retires on shared memory types
On Fri, May 27, 2022 at 03:39:36PM -0400, Peter Xu wrote:
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 77341b160aca..e401d416bbd6 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -607,6 +607,10 @@ static int __kprobes do_page_fault(unsigned long far,
> unsigned int esr,
> return 0;
> }
>
> + /* The fault is fully completed (including releasing mmap lock) */
> + if (fault & VM_FAULT_COMPLETED)
> + return 0;
> +
> if (fault & VM_FAULT_RETRY) {
> mm_flags |= FAULT_FLAG_TRIED;
> goto retry;
For arm64:
Acked-by: Catalin Marinas
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v4] mm: Avoid unnecessary page fault retires on shared memory types
Am 29.05.22 um 22:33 schrieb Heiko Carstens:
[...]
Guess the patch below on top of your patch is what we want.
Just for clarification: if gmap is not NULL then the process is a kvm
process. So, depending on the workload, this optimization makes sense.
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 4608cc962ecf..e1d40ca341b7 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -436,12 +436,11 @@ static inline vm_fault_t do_exception(struct pt_regs
*regs, int access)
/* The fault is fully completed (including releasing mmap lock) */
if (fault & VM_FAULT_COMPLETED) {
- /*
-* Gmap will need the mmap lock again, so retake it. TODO:
-* only conditionally take the lock when CONFIG_PGSTE set.
-*/
- mmap_read_lock(mm);
- goto out_gmap;
+ if (gmap) {
+ mmap_read_lock(mm);
+ goto out_gmap;
+ }
+ goto out;
Yes, that makes sense. With that
Acked-by: Christian Borntraeger
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v4] mm: Avoid unnecessary page fault retires on shared memory types
On Fri, May 27, 2022 at 03:39:36PM -0400, Peter Xu wrote:
> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index e173b6187ad5..4608cc962ecf 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -433,6 +433,17 @@ static inline vm_fault_t do_exception(struct pt_regs
> *regs, int access)
> goto out_up;
> goto out;
> }
> +
> + /* The fault is fully completed (including releasing mmap lock) */
> + if (fault & VM_FAULT_COMPLETED) {
> + /*
> + * Gmap will need the mmap lock again, so retake it. TODO:
> + * only conditionally take the lock when CONFIG_PGSTE set.
> + */
> + mmap_read_lock(mm);
> + goto out_gmap;
> + }
> +
> if (unlikely(fault & VM_FAULT_ERROR))
> goto out_up;
>
Guess the patch below on top of your patch is what we want.
Just for clarification: if gmap is not NULL then the process is a kvm
process. So, depending on the workload, this optimization makes sense.
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 4608cc962ecf..e1d40ca341b7 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -436,12 +436,11 @@ static inline vm_fault_t do_exception(struct pt_regs
*regs, int access)
/* The fault is fully completed (including releasing mmap lock) */
if (fault & VM_FAULT_COMPLETED) {
- /*
-* Gmap will need the mmap lock again, so retake it. TODO:
-* only conditionally take the lock when CONFIG_PGSTE set.
-*/
- mmap_read_lock(mm);
- goto out_gmap;
+ if (gmap) {
+ mmap_read_lock(mm);
+ goto out_gmap;
+ }
+ goto out;
}
if (unlikely(fault & VM_FAULT_ERROR))
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH v4] mm: Avoid unnecessary page fault retires on shared memory types
I observed that for each of the shared file-backed page faults, we're very
likely to retry one more time for the 1st write fault upon no page. It's
because we'll need to release the mmap lock for dirty rate limit purpose
with balance_dirty_pages_ratelimited() (in fault_dirty_shared_page()).
Then after that throttling we return VM_FAULT_RETRY.
We did that probably because VM_FAULT_RETRY is the only way we can return
to the fault handler at that time telling it we've released the mmap lock.
However that's not ideal because it's very likely the fault does not need
to be retried at all since the pgtable was well installed before the
throttling, so the next continuous fault (including taking mmap read lock,
walk the pgtable, etc.) could be in most cases unnecessary.
It's not only slowing down page faults for shared file-backed, but also add
more mmap lock contention which is in most cases not needed at all.
To observe this, one could try to write to some shmem page and look at
"pgfault" value in /proc/vmstat, then we should expect 2 counts for each
shmem write simply because we retried, and vm event "pgfault" will capture
that.
To make it more efficient, add a new VM_FAULT_COMPLETED return code just to
show that we've completed the whole fault and released the lock. It's also
a hint that we should very possibly not need another fault immediately on
this page because we've just completed it.
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%)
I believe it could help more than that.
We need some special care on GUP and the s390 pgfault handler (for gmap
code before returning from pgfault), the rest changes in the page fault
handlers should be relatively straightforward.
Another thing to mention is that mm_account_fault() does take this new
fault as a generic fault to be accounted, unlike VM_FAULT_RETRY.
I explicitly didn't touch hmm_vma_fault() and break_ksm() because they do
not handle VM_FAULT_RETRY even with existing code, so I'm literally keeping
them as-is.
Acked-by: Geert Uytterhoeven
Acked-by: Peter Zijlstra (Intel)
Acked-by: Johannes Weiner
Acked-by: Vineet Gupta
Acked-by: Guo Ren
Acked-by: Max Filippov
Reviewed-by: Alistair Popple
Reviewed-by: Ingo Molnar
Signed-off-by: Peter Xu
---
v4:
- Picked up a-bs and r-bs
- Fix grammar in the comment of faultin_page() [Ingo]
- Fix s390 for gmap since gmap needs the mmap lock [Heiko]
v3:
- Rebase to akpm/mm-unstable
- Copy arch maintainers
---
arch/alpha/mm/fault.c | 4
arch/arc/mm/fault.c | 4
arch/arm/mm/fault.c | 4
arch/arm64/mm/fault.c | 4
arch/csky/mm/fault.c | 4
arch/hexagon/mm/vm_fault.c| 4
arch/ia64/mm/fault.c | 4
arch/m68k/mm/fault.c | 4
arch/microblaze/mm/fault.c| 4
arch/mips/mm/fault.c | 4
arch/nios2/mm/fault.c | 4
arch/openrisc/mm/fault.c | 4
arch/parisc/mm/fault.c| 4
arch/powerpc/mm/copro_fault.c | 5 +
arch/powerpc/mm/fault.c | 5 +
arch/riscv/mm/fault.c | 4
arch/s390/mm/fault.c | 12
arch/sh/mm/fault.c| 4
arch/sparc/mm/fault_32.c | 4
arch/sparc/mm/fault_64.c | 5 +
arch/um/kernel/trap.c | 4
arch/x86/mm/fault.c | 4
arch/xtensa/mm/fault.c| 4
include/linux/mm_types.h | 2 ++
mm/gup.c | 34 +-
mm/memory.c | 2 +-
26 files changed, 139 insertions(+), 2 deletions(-)
diff --git a/arch/alpha/mm/fault.c b/arch/alpha/mm/fault.c
index ec20c1004abf..ef427a6bdd1a 100644
--- a/arch/alpha/mm/fault.c
+++ b/arch/alpha/mm/fault.c
@@ -155,6 +155,10 @@ do_page_fault(unsigned long address, unsigned long mmcsr,
if (fault_signal_pending(fault, regs))
return;
+ /* The fault is fully completed (including releasing mmap lock) */
+ if (fault & VM_FAULT_COMPLETED)
+ return;
+
if (unlikely(fault & VM_FAULT_ERROR)) {
if (fault & VM_FAULT_OOM)
goto out_of_memory;
diff --git a/arch/arc/mm/fault.c b/arch/arc/mm/fault.c
index dad27e4d69ff..5ca59a482632 100644
--- a/arch/arc/mm/fault.c
+++ b/arch/arc/mm/fault.c
@@ -146,6 +146,10 @@ void do_page_fault(unsigned long address, struct pt_regs
*regs)
return;
}
+ /* The fault is fully completed (including releasing mmap lock) */
+ if (fault & VM_FAULT_COMPLETED)
+ return;
+
/*
* Fault retry nuances, mmap_lock already relinquished by core mm
*/
diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index a062e07516dd..46cccd6bf705 100644
--- a/arch/arm/mm/fault.c
