Re: [PATCH 01/26] mm: Do page fault accounting in handle_mm_fault

[Peter Xu]

On Fri, Jun 26, 2020 at 09:54:24PM +0200, Gerald Schaefer wrote:
> On Wed, 24 Jun 2020 16:34:12 -0400
> Peter Xu <peterx@xxxxxxxxxx> wrote:
> 
> > On Wed, Jun 24, 2020 at 08:49:03PM +0200, Gerald Schaefer wrote:
> > > On Fri, 19 Jun 2020 12:05:13 -0400
> > > Peter Xu <peterx@xxxxxxxxxx> wrote:
> > > 
> > > [...]
> > > 
> > > > @@ -4393,6 +4425,38 @@ vm_fault_t handle_mm_fault(struct vm_area_struct *vma, unsigned long address,
> > > >  			mem_cgroup_oom_synchronize(false);
> > > >  	}
> > > > 
> > > > +	if (ret & VM_FAULT_RETRY)
> > > > +		return ret;
> > > 
> > > I'm wondering if this also needs a check and exit for VM_FAULT_ERROR.
> > > In arch code (s390 and all others I briefly checked), the accounting
> > > was skipped for VM_FAULT_ERROR case.
> > 
> > Yes. I didn't explicitly add the check because I thought it's still OK to count
> > the error cases, especially after we've discussed about
> > PERF_COUNT_SW_PAGE_FAULTS in v1.  So far, the major reason (iiuc) to have
> > PERF_COUNT_SW_PAGE_FAULTS still in per-arch handlers is to also cover these
> > corner cases like VM_FAULT_ERROR.  So to me it makes sense too to also count
> > them in here.  But I agree it changes the old counting on most archs.
> 
> Having PERF_COUNT_SW_PAGE_FAULTS count everything including VM_FAULT_ERROR
> is OK. Just major/minor accounting should be only about successes, IIRC from
> v1 discussion.
> 
> The "new rules" also say
> 
> +	 *  - faults that never even got here (because the address
> +	 *    wasn't valid). That includes arch_vma_access_permitted()
> +	 *    failing above.
> 
> VM_FAULT_ERROR, and also the arch-specific VM_FAULT_BADxxx, qualify
> as "address wasn't valid" I think, so they should not be counted as
> major/minor.
> 
> IIRC from v1, and we want to only count success as major/minor, maybe
> the rule could also be made more clear about that, e.g. like
> 
> +	 *  - unsuccessful faults (because the address wasn't valid)
> +	 *    do not count. That includes arch_vma_access_permitted()
> +	 *    failing above.

Sure.

> 
> > 
> > Again, I don't have strong opinion either on this, just like the same to
> > PERF_COUNT_SW_PAGE_FAULTS...  But if no one disagree, I will change this to:
> > 
> >   if (ret & (VM_FAULT_RETRY | VM_FAULT_ERROR))
> >       return ret;
> > 
> > So we try our best to follow the past.
> 
> Sounds good to me, and VM_FAULT_BADxxx should never show up here.
> 
> > 
> > Btw, note that there will still be some even more special corner cases. E.g.,
> > for ARM64 it's also not accounted for some ARM64 specific fault errors
> > (VM_FAULT_BADMAP, VM_FAULT_BADACCESS).  So even if we don't count
> > VM_FAULT_ERROR, we might still count these for ARM64.  We can try to redefine
> > VM_FAULT_ERROR in ARM64 to cover all the arch-specific errors, however that
> > seems an overkill to me sololy for fault accountings, so hopefully I can ignore
> > that difference.
> 
> Hmm, arm64 already does not count the VM_FAULT_BADxxx, but also does not
> call handle_mm_fault() for those, so no change with this patch. arm (and
> also unicore32) do count those, but also not call handle_mm_fault(), so
> there would be the change that they lose accounting, IIUC.

Oh you are right...  I just noticed that VM_FAULT_BADMAP and VM_FAULT_BADACCESS
can never returned in handle_mm_fault() itself.

> 
> I agree that this probably can be ignored. The code in arm64 also looks
> more recent, so it's probably just a left-over in arm/unicore32 code.

Anyway, glad to know that we've reached consensus so that we can accept these
differences.

Since this patch seems to be the only one that needs a new post so far, I'll
repost this patch only by replying to itself with v2.1.  Hopefully that can
avoid some unecessary mail bombs.

Thanks for the very detailed review!

-- 
Peter Xu