| Message ID | 20200619161335.9664-1-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: Page fault accounting cleanups | expand |
On Fri, 19 Jun 2020 12:13:35 -0400 Peter Xu <peterx@redhat.com> wrote: > Use the general page fault accounting by passing regs into handle_mm_fault(). > It naturally solve the issue of multiple page fault accounting when page fault > retry happened. > > CC: Heiko Carstens <heiko.carstens@de.ibm.com> > CC: Vasily Gorbik <gor@linux.ibm.com> > CC: Christian Borntraeger <borntraeger@de.ibm.com> > CC: linux-s390@vger.kernel.org > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > arch/s390/mm/fault.c | 16 +--------------- > 1 file changed, 1 insertion(+), 15 deletions(-) > > diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c > index ab6d7eedcfab..4d62ca7d3e09 100644 > --- a/arch/s390/mm/fault.c > +++ b/arch/s390/mm/fault.c > @@ -479,7 +479,7 @@ static inline vm_fault_t do_exception(struct pt_regs *regs, int access) > * make sure we exit gracefully rather than endlessly redo > * the fault. > */ > - fault = handle_mm_fault(vma, address, flags, NULL); > + fault = handle_mm_fault(vma, address, flags, regs); > if (fault_signal_pending(fault, regs)) { > fault = VM_FAULT_SIGNAL; > if (flags & FAULT_FLAG_RETRY_NOWAIT) > @@ -489,21 +489,7 @@ static inline vm_fault_t do_exception(struct pt_regs *regs, int access) > if (unlikely(fault & VM_FAULT_ERROR)) > goto out_up; There are two cases here where we skipped the accounting, fault_signal_pending() and VM_FAULT_ERROR, similar to other archs. fault_signal_pending() should be ok, because that only seems to be true for fault & VM_FAULT_RETRY, in which case the new approach also skips the accounting. But for VM_FAULT_ERROR, the new approach would do accounting, IIUC. Is that changed on purpose? See also my reply on [PATCH 01/26]. Regards, Gerald
On Wed, Jun 24, 2020 at 08:49:30PM +0200, Gerald Schaefer wrote: > On Fri, 19 Jun 2020 12:13:35 -0400 > Peter Xu <peterx@redhat.com> wrote: > > > Use the general page fault accounting by passing regs into handle_mm_fault(). > > It naturally solve the issue of multiple page fault accounting when page fault > > retry happened. > > > > CC: Heiko Carstens <heiko.carstens@de.ibm.com> > > CC: Vasily Gorbik <gor@linux.ibm.com> > > CC: Christian Borntraeger <borntraeger@de.ibm.com> > > CC: linux-s390@vger.kernel.org > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > arch/s390/mm/fault.c | 16 +--------------- > > 1 file changed, 1 insertion(+), 15 deletions(-) > > > > diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c > > index ab6d7eedcfab..4d62ca7d3e09 100644 > > --- a/arch/s390/mm/fault.c > > +++ b/arch/s390/mm/fault.c > > @@ -479,7 +479,7 @@ static inline vm_fault_t do_exception(struct pt_regs *regs, int access) > > * make sure we exit gracefully rather than endlessly redo > > * the fault. > > */ > > - fault = handle_mm_fault(vma, address, flags, NULL); > > + fault = handle_mm_fault(vma, address, flags, regs); > > if (fault_signal_pending(fault, regs)) { > > fault = VM_FAULT_SIGNAL; > > if (flags & FAULT_FLAG_RETRY_NOWAIT) > > @@ -489,21 +489,7 @@ static inline vm_fault_t do_exception(struct pt_regs *regs, int access) > > if (unlikely(fault & VM_FAULT_ERROR)) > > goto out_up; > > There are two cases here where we skipped the accounting, > fault_signal_pending() and VM_FAULT_ERROR, similar to other archs. > > fault_signal_pending() should be ok, because that only seems to be true > for fault & VM_FAULT_RETRY, in which case the new approach also skips > the accounting. IMHO it's still possible to have fault_signal_pending() return true even if the fault is not with VM_FAULT_RETRY, e.g., when the signal is delivered right after the fault is correctly handled for the thread. However I hope we can avoid considering that too even if so... > > But for VM_FAULT_ERROR, the new approach would do accounting, IIUC. Is > that changed on purpose? See also my reply on [PATCH 01/26]. (replied in the other thread) Thanks,
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c index ab6d7eedcfab..4d62ca7d3e09 100644 --- a/arch/s390/mm/fault.c +++ b/arch/s390/mm/fault.c @@ -479,7 +479,7 @@ static inline vm_fault_t do_exception(struct pt_regs *regs, int access) * make sure we exit gracefully rather than endlessly redo * the fault. */ - fault = handle_mm_fault(vma, address, flags, NULL); + fault = handle_mm_fault(vma, address, flags, regs); if (fault_signal_pending(fault, regs)) { fault = VM_FAULT_SIGNAL; if (flags & FAULT_FLAG_RETRY_NOWAIT) @@ -489,21 +489,7 @@ static inline vm_fault_t do_exception(struct pt_regs *regs, int access) if (unlikely(fault & VM_FAULT_ERROR)) goto out_up; - /* - * Major/minor page fault accounting is only done on the - * initial attempt. If we go through a retry, it is extremely - * likely that the page will be found in page cache at that point. - */ if (flags & FAULT_FLAG_ALLOW_RETRY) { - if (fault & VM_FAULT_MAJOR) { - tsk->maj_flt++; - perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MAJ, 1, - regs, address); - } else { - tsk->min_flt++; - perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MIN, 1, - regs, address); - } if (fault & VM_FAULT_RETRY) { if (IS_ENABLED(CONFIG_PGSTE) && gmap && (flags & FAULT_FLAG_RETRY_NOWAIT)) {
Use the general page fault accounting by passing regs into handle_mm_fault(). It naturally solve the issue of multiple page fault accounting when page fault retry happened. CC: Heiko Carstens <heiko.carstens@de.ibm.com> CC: Vasily Gorbik <gor@linux.ibm.com> CC: Christian Borntraeger <borntraeger@de.ibm.com> CC: linux-s390@vger.kernel.org Signed-off-by: Peter Xu <peterx@redhat.com> --- arch/s390/mm/fault.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-)