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