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

Gerald Schaefer
2020-06-26T19:54:39Z

On Wed, 24 Jun 2020 16:34:12 -0400
Peter Xu <peterx@redhat.com> 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@redhat.com> 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.

>
> 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.

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.

Regards,
Gerald