linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Peter Xu <peterx@redhat.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Gerald Schaefer <gerald.schaefer@de.ibm.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Andrea Arcangeli <aarcange@redhat.com>,
	openrisc@lists.librecores.org,
	linux-arch <linux-arch@vger.kernel.org>,
	Alexander Gordeev <agordeev@linux.ibm.com>,
	linux-s390 <linux-s390@vger.kernel.org>,
	Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 00/25] mm: Page fault accounting cleanups
Date: Tue, 16 Jun 2020 11:55:17 -0700	[thread overview]
Message-ID: <CAHk-=wiTjaXHu+uxMi0xCZQOm4KVr0MucECAK=Zm4p4YZZ1XEg@mail.gmail.com> (raw)
In-Reply-To: <20200615221607.7764-1-peterx@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 2788 bytes --]

On Mon, Jun 15, 2020 at 3:16 PM Peter Xu <peterx@redhat.com> wrote:
>
> This series tries to address all of them by introducing mm_fault_accounting()
> first, so that we move all the page fault accounting into the common code base,
> then call it properly from arch pf handlers just like handle_mm_fault().

Hmm.

So having looked at this a bit more, I'd actually like to go even
further, and just get rid of the per-architecture code _entirely_.

Here's a straw-man patch to the generic code - the idea is mostly laid
out in the comment that I'm just quoting here directly too:

        /*
         * Do accounting in the common code, to avoid unnecessary
         * architecture differences or duplicated code.
         *
         * We arbitrarily make the rules be:
         *
         *  - faults that never even got here (because the address
         *    wasn't valid). That includes arch_vma_access_permitted()
         *    failing above.
         *
         *    So this is expressly not a "this many hardware page
         *    faults" counter. Use the hw profiling for that.
         *
         *  - incomplete faults (ie RETRY) do not count (see above).
         *    They will only count once completed.
         *
         *  - the fault counts as a "major" fault when the final
         *    successful fault is VM_FAULT_MAJOR, or if it was a
         *    retry (which implies that we couldn't handle it
         *    immediately previously).
         *
         *  - if the fault is done for GUP, regs wil be NULL and
         *    no accounting will be done (but you _could_ pass in
         *    your own regs and it would be accounted to the thread
         *    doing the fault, not to the target!)
         */

the code itself in the patch is

 (a) pretty trivial and self-evident

 (b) INCOMPLETE

that (b) is worth noting: this patch won't compile on its own. It
intentionally leaves all the users without the new 'regs' argument,
because you obviously simply need to remove all the code that
currently tries to do any accounting.

Comments?

This is a bigger change, but I think it might be worth it to _really_
consolidate the major/minor logic.

One detail worth noting: I do wonder if we should put the

    perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, addr);

just in the arch code at the top of the fault handling, and consider
it entirely unrelated to the major/minor fault handling. The
major/minor faults fundamnetally are about successes. But the plain
PERF_COUNT_SW_PAGE_FAULTS could be about things that fail, including
things that never even get to this point at all.

I'm not convinced it's useful to have three SW events that are defined
to be A=B+C.

But this does *not* do that part. It' sreally just an RFC patch.

                    Linus

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 2918 bytes --]

 include/linux/mm.h |  3 ++-
 mm/memory.c        | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index dc7b87310c10..e82a604339c0 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1648,7 +1648,8 @@ int invalidate_inode_page(struct page *page);
 
 #ifdef CONFIG_MMU
 extern vm_fault_t handle_mm_fault(struct vm_area_struct *vma,
-			unsigned long address, unsigned int flags);
+			unsigned long address, unsigned int flags,
+			struct pt_regs *);
 extern int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm,
 			    unsigned long address, unsigned int fault_flags,
 			    bool *unlocked);
diff --git a/mm/memory.c b/mm/memory.c
index dc7f3543b1fd..f15056151fb1 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -72,6 +72,7 @@
 #include <linux/oom.h>
 #include <linux/numa.h>
 
+#include <linux/perf_event.h>
 #include <trace/events/kmem.h>
 
 #include <asm/io.h>
@@ -4353,7 +4354,7 @@ static vm_fault_t __handle_mm_fault(struct vm_area_struct *vma,
  * return value.  See filemap_fault() and __lock_page_or_retry().
  */
 vm_fault_t handle_mm_fault(struct vm_area_struct *vma, unsigned long address,
-		unsigned int flags)
+		unsigned int flags, struct pt_regs *regs)
 {
 	vm_fault_t ret;
 
@@ -4394,6 +4395,49 @@ 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;
+
+	/*
+	 * Do accounting in the common code, to avoid unnecessary
+	 * architecture differences or duplicated code.
+	 *
+	 * We arbitrarily make the rules be:
+	 *
+	 *  - faults that never even got here (because the address
+	 *    wasn't valid). That includes arch_vma_access_permitted()
+	 *    failing above.
+	 *
+	 *    So this is expressly not a "this many hardware page
+	 *    faults" counter. Use the hw profiling for that.
+	 *
+	 *  - incomplete faults (ie RETRY) do not count (see above).
+	 *    They will only count once completed.
+	 *
+	 *  - the fault counts as a "major" fault when the final
+	 *    successful fault is VM_FAULT_MAJOR, or if it was a
+	 *    retry (which implies that we couldn't handle it
+	 *    immediately previously).
+	 *
+	 *  - if the fault is done for GUP, regs wil be NULL and
+	 *    no accounting will be done (but you _could_ pass in
+	 *    your own regs and it would be accounted to the thread
+	 *    doing the fault, not to the target!)
+	 */
+
+	if (!regs)
+		return ret;
+
+	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
+
+	if ((ret & VM_FAULT_MAJOR) || (flags & FAULT_FLAG_TRIED)) {
+		current->maj_flt++;
+		perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MAJ, 1, regs, address);
+	} else {
+		current->min_flt++;
+		perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MIN, 1, regs, address);
+	}
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(handle_mm_fault);

  parent reply	other threads:[~2020-06-16 18:55 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-15 22:15 Peter Xu
2020-06-15 22:15 ` [PATCH 01/25] mm/um: Fix extra accounting for page fault retries Peter Xu
2020-06-15 22:15 ` [PATCH 02/25] mm: Introduce mm_fault_accounting() Peter Xu
2020-06-15 22:32   ` Linus Torvalds
2020-06-15 23:19     ` Peter Xu
2020-06-16 19:00       ` Andrew Morton
2020-06-17 16:26         ` Peter Xu
2020-06-15 22:15 ` [PATCH 03/25] mm/alpha: Use mm_fault_accounting() Peter Xu
2020-06-15 22:15 ` [PATCH 04/25] mm/arc: " Peter Xu
2020-06-15 22:15 ` [PATCH 05/25] mm/arm: " Peter Xu
2020-06-15 22:15 ` [PATCH 06/25] mm/arm64: " Peter Xu
2020-06-16  7:43   ` Will Deacon
2020-06-16 15:59     ` Peter Xu
2020-06-15 22:15 ` [PATCH 07/25] mm/csky: " Peter Xu
2020-06-17  7:04   ` Guo Ren
2020-06-17 15:49     ` Peter Xu
2020-06-17 17:53       ` Linus Torvalds
2020-06-17 19:58         ` Peter Xu
2020-06-17 20:15           ` Linus Torvalds
2020-06-18 14:38             ` Peter Xu
2020-06-18 17:15               ` Linus Torvalds
2020-06-18 21:24                 ` Peter Xu
2020-06-18 22:28                   ` Peter Xu
2020-06-18 22:59                     ` Linus Torvalds
2020-06-15 22:15 ` [PATCH 08/25] mm/hexagon: " Peter Xu
2020-06-15 22:15 ` [PATCH 09/25] mm/ia64: " Peter Xu
2020-06-15 22:15 ` [PATCH 10/25] mm/m68k: " Peter Xu
2020-06-15 22:15 ` [PATCH 11/25] mm/microblaze: " Peter Xu
2020-06-15 22:15 ` [PATCH 12/25] mm/mips: " Peter Xu
2020-06-15 22:15 ` [PATCH 13/25] mm/nds32: " Peter Xu
2020-06-17  1:05   ` Greentime Hu
2020-06-15 22:15 ` [PATCH 14/25] mm/nios2: " Peter Xu
2020-06-15 22:15 ` [PATCH 15/25] mm/openrisc: " Peter Xu
2020-06-16 18:11   ` Stafford Horne
2020-06-15 22:15 ` [PATCH 16/25] mm/parisc: " Peter Xu
2020-06-15 22:15 ` [PATCH 17/25] mm/powerpc: " Peter Xu
2020-06-15 22:16 ` [PATCH 18/25] mm/riscv: " Peter Xu
2020-06-18 23:49   ` Palmer Dabbelt
2020-06-19  0:12     ` Peter Xu
2020-06-15 22:23 ` [PATCH 19/25] mm/s390: " Peter Xu
2020-06-16 15:59   ` Alexander Gordeev
2020-06-16 16:35     ` Peter Xu
2020-06-17  6:19       ` Christian Borntraeger
2020-06-17 16:06         ` Peter Xu
2020-06-17 16:14           ` Christian Borntraeger
2020-06-17 16:44             ` Peter Xu
2020-06-15 22:23 ` [PATCH 20/25] mm/sh: " Peter Xu
2020-07-20 21:25   ` Rich Felker
2020-07-20 22:05     ` Peter Xu
2020-06-15 22:23 ` [PATCH 21/25] mm/sparc32: " Peter Xu
2020-06-15 22:23 ` [PATCH 22/25] mm/sparc64: " Peter Xu
2020-06-15 22:23 ` [PATCH 23/25] mm/unicore32: " Peter Xu
2020-06-15 22:23 ` [PATCH 24/25] mm/x86: " Peter Xu
2020-06-15 22:23 ` [PATCH 25/25] mm/xtensa: " Peter Xu
2020-06-15 23:13   ` Max Filippov
2020-06-16 18:55 ` Linus Torvalds [this message]
2020-06-16 21:03   ` [PATCH 00/25] mm: Page fault accounting cleanups Peter Xu
2020-06-17  0:55   ` Michael Ellerman
2020-06-17  8:04     ` Will Deacon
2020-06-17 16:10       ` Peter Xu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHk-=wiTjaXHu+uxMi0xCZQOm4KVr0MucECAK=Zm4p4YZZ1XEg@mail.gmail.com' \
    --to=torvalds@linux-foundation.org \
    --cc=aarcange@redhat.com \
    --cc=agordeev@linux.ibm.com \
    --cc=akpm@linux-foundation.org \
    --cc=catalin.marinas@arm.com \
    --cc=gerald.schaefer@de.ibm.com \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=openrisc@lists.librecores.org \
    --cc=peterx@redhat.com \
    --cc=will@kernel.org \
    --subject='Re: [PATCH 00/25] mm: Page fault accounting cleanups' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox