Re: [PATCH v5 18/25] mm/s390: Use general page fault accounting

[Peter Xu <peterx@redhat.com>]

On Wed, Jul 08, 2020 at 07:49:47AM +0200, Alexander Gordeev wrote:
> On Tue, Jul 07, 2020 at 06:50:14PM -0400, Peter Xu 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
> > Reviewed-by: Gerald Schaefer <gerald.schaefer@de.ibm.com>
> > Acked-by: Gerald Schaefer <gerald.schaefer@de.ibm.com>
> > 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 fc14df0b4d6e..9aa201df2e94 100644
> > --- a/arch/s390/mm/fault.c
> > +++ b/arch/s390/mm/fault.c
> > @@ -478,7 +478,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)
> > @@ -488,21 +488,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) {
> 
> Shouldn't this check ^^^ be dropped as well?
> 
> Since commit 4064b9827063 ("mm: allow VM_FAULT_RETRY for multiple times")
> FAULT_FLAG_ALLOW_RETRY never gets unset, so no need to check..

I agree, but it should be out of the scope of the accounting changes that this
patch wants to address.  Maybe more suitable for a work on top?

This should also exist for most of the archs too, and I'm also not sure whether
compiler could be smart enough to optimize this directly since it seems to have
all the knowledge.

Thanks,

-- 
Peter Xu