All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hugh Dickins <hughd@google.com>
To: Linus Torvalds <torvalds@linux-foundation.org>,
	Damian Tometzki <linux@tometzki.de>
Cc: Hugh Dickins <hughd@google.com>,
	"Kirill A. Shutemov" <kirill@shutemov.name>,
	Matthew Wilcox <willy@infradead.org>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	Will Deacon <will@kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Linux-MM <linux-mm@kvack.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Jan Kara <jack@suse.cz>, Minchan Kim <minchan@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Vinayak Menon <vinmenon@codeaurora.org>,
	Android Kernel Team <kernel-team@android.com>
Subject: Re: [PATCH 1/2] mm: Allow architectures to request 'old' entries when prefaulting
Date: Sun, 27 Dec 2020 14:35:32 -0800 (PST)	[thread overview]
Message-ID: <alpine.LSU.2.11.2012271418460.1091@eggly.anvils> (raw)
In-Reply-To: <X+jvZchuTptrxkCH@fedora.tometzki.de>

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3076 bytes --]

On Sun, 27 Dec 2020, Damian Tometzki wrote:
> On Sun, 27. Dec 11:38, Linus Torvalds wrote:
> > On Sat, Dec 26, 2020 at 6:38 PM Hugh Dickins <hughd@google.com> wrote:
> > >
> > > This patch (like its antecedents) moves the pte_unmap_unlock() from
> > > after do_fault_around()'s "check if the page fault is solved" into
> > > filemap_map_pages() itself (which apparently does not NULLify vmf->pte
> > > after unmapping it, which is poor, but good for revealing this issue).
> > > That looks cleaner, but of course there was a very good reason for its
> > > original positioning.
> > 
> > Good catch.
> > 
> > > Maybe you want to change the ->map_pages prototype, to pass down the
> > > requested address too, so that it can report whether the requested
> > > address was resolved or not.  Or it could be left to __do_fault(),
> > > or even to a repeated fault; but those would be less efficient.
> > 
> > Let's keep the old really odd "let's unlock in the caller" for now,
> > and minimize the changes.
> > 
> > Adding a big big comment at the end of filemap_map_pages() to note the
> > odd delayed page table unlocking.
> > 
> > Here's an updated patch that combines Kirill's original patch, his
> > additional incremental patch, and the fix for the pte lock oddity into
> > one thing.
> > 
> > Does this finally pass your testing?

Yes, this one passes my testing on x86_64 and on i386.  But...

> > 
> >                Linus
> Hello together,
> 
> when i try to build this patch, i got the following error:
> 
>  CC      arch/x86/kernel/cpu/mce/threshold.o
> mm/memory.c:3716:19: error: static declaration of ‘do_set_pmd’ follows non-static declaration
>  static vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page)
>                    ^~~~~~~~~~
> In file included from mm/memory.c:43:
> ./include/linux/mm.h:984:12: note: previous declaration of ‘do_set_pmd’ was here
>  vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page);
>             ^~~~~~~~~~
> make[3]: *** [scripts/Makefile.build:279: mm/memory.o] Error 1
> make[2]: *** [Makefile:1805: mm] Error 2
> make[2]: *** Waiting for unfinished jobs....
>   CC      arch/x86/kernel/cpu/mce/therm_throt.o

... Damian very helpfully reports that it does not build when
CONFIG_TRANSPARENT_HUGEPAGE is not set, since the "static " has
not been removed from the alternative definition of do_set_pmd().

And its BUILD_BUG() becomes invalid once it's globally available.
You don't like unnecessary BUG()s, and I don't like returning
success there: VM_FAULT_FALLBACK seems best.

--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3713,10 +3713,9 @@ out:
 	return ret;
 }
 #else
-static vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page)
+vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page)
 {
-	BUILD_BUG();
-	return 0;
+	return VM_FAULT_FALLBACK;
 }
 #endif
 

(I'm also a wee bit worried by filemap.c's +#include <asm/pgalloc.h>:
that's the kind of thing that might turn out not to work on some arch.)

Hugh

WARNING: multiple messages have this Message-ID
From: Hugh Dickins <hughd@google.com>
To: Linus Torvalds <torvalds@linux-foundation.org>,
	 Damian Tometzki <linux@tometzki.de>
Cc: Android Kernel Team <kernel-team@android.com>,
	Jan Kara <jack@suse.cz>, Minchan Kim <minchan@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Hugh Dickins <hughd@google.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Matthew Wilcox <willy@infradead.org>,
	Linux-MM <linux-mm@kvack.org>,
	Vinayak Menon <vinmenon@codeaurora.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	"Kirill A. Shutemov" <kirill@shutemov.name>,
	Andrew Morton <akpm@linux-foundation.org>,
	Will Deacon <will@kernel.org>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Subject: Re: [PATCH 1/2] mm: Allow architectures to request 'old' entries when prefaulting
Date: Sun, 27 Dec 2020 14:35:32 -0800 (PST)	[thread overview]
Message-ID: <alpine.LSU.2.11.2012271418460.1091@eggly.anvils> (raw)
In-Reply-To: <X+jvZchuTptrxkCH@fedora.tometzki.de>

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3076 bytes --]

On Sun, 27 Dec 2020, Damian Tometzki wrote:
> On Sun, 27. Dec 11:38, Linus Torvalds wrote:
> > On Sat, Dec 26, 2020 at 6:38 PM Hugh Dickins <hughd@google.com> wrote:
> > >
> > > This patch (like its antecedents) moves the pte_unmap_unlock() from
> > > after do_fault_around()'s "check if the page fault is solved" into
> > > filemap_map_pages() itself (which apparently does not NULLify vmf->pte
> > > after unmapping it, which is poor, but good for revealing this issue).
> > > That looks cleaner, but of course there was a very good reason for its
> > > original positioning.
> > 
> > Good catch.
> > 
> > > Maybe you want to change the ->map_pages prototype, to pass down the
> > > requested address too, so that it can report whether the requested
> > > address was resolved or not.  Or it could be left to __do_fault(),
> > > or even to a repeated fault; but those would be less efficient.
> > 
> > Let's keep the old really odd "let's unlock in the caller" for now,
> > and minimize the changes.
> > 
> > Adding a big big comment at the end of filemap_map_pages() to note the
> > odd delayed page table unlocking.
> > 
> > Here's an updated patch that combines Kirill's original patch, his
> > additional incremental patch, and the fix for the pte lock oddity into
> > one thing.
> > 
> > Does this finally pass your testing?

Yes, this one passes my testing on x86_64 and on i386.  But...

> > 
> >                Linus
> Hello together,
> 
> when i try to build this patch, i got the following error:
> 
>  CC      arch/x86/kernel/cpu/mce/threshold.o
> mm/memory.c:3716:19: error: static declaration of ‘do_set_pmd’ follows non-static declaration
>  static vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page)
>                    ^~~~~~~~~~
> In file included from mm/memory.c:43:
> ./include/linux/mm.h:984:12: note: previous declaration of ‘do_set_pmd’ was here
>  vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page);
>             ^~~~~~~~~~
> make[3]: *** [scripts/Makefile.build:279: mm/memory.o] Error 1
> make[2]: *** [Makefile:1805: mm] Error 2
> make[2]: *** Waiting for unfinished jobs....
>   CC      arch/x86/kernel/cpu/mce/therm_throt.o

... Damian very helpfully reports that it does not build when
CONFIG_TRANSPARENT_HUGEPAGE is not set, since the "static " has
not been removed from the alternative definition of do_set_pmd().

And its BUILD_BUG() becomes invalid once it's globally available.
You don't like unnecessary BUG()s, and I don't like returning
success there: VM_FAULT_FALLBACK seems best.

--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3713,10 +3713,9 @@ out:
 	return ret;
 }
 #else
-static vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page)
+vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page)
 {
-	BUILD_BUG();
-	return 0;
+	return VM_FAULT_FALLBACK;
 }
 #endif
 

(I'm also a wee bit worried by filemap.c's +#include <asm/pgalloc.h>:
that's the kind of thing that might turn out not to work on some arch.)

Hugh

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2020-12-27 22:36 UTC|newest]

Thread overview: 138+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-09 16:39 [PATCH 0/2] Create 'old' ptes for faultaround mappings on arm64 with hardware access flag Will Deacon
2020-12-09 16:39 ` Will Deacon
2020-12-09 16:39 ` [PATCH 1/2] mm: Allow architectures to request 'old' entries when prefaulting Will Deacon
2020-12-09 16:39   ` Will Deacon
2020-12-09 17:58   ` Linus Torvalds
2020-12-09 17:58     ` Linus Torvalds
2020-12-09 17:58     ` Linus Torvalds
2020-12-09 18:40     ` Will Deacon
2020-12-09 18:40       ` Will Deacon
2020-12-09 19:04       ` Linus Torvalds
2020-12-09 19:04         ` Linus Torvalds
2020-12-09 19:04         ` Linus Torvalds
2020-12-09 20:32         ` Matthew Wilcox
2020-12-09 20:32           ` Matthew Wilcox
2020-12-09 21:04           ` Linus Torvalds
2020-12-09 21:04             ` Linus Torvalds
2020-12-09 21:04             ` Linus Torvalds
2020-12-10 15:08         ` Kirill A. Shutemov
2020-12-10 15:08           ` Kirill A. Shutemov
2020-12-10 17:23           ` Linus Torvalds
2020-12-10 17:23             ` Linus Torvalds
2020-12-10 17:23             ` Linus Torvalds
2020-12-14 16:07             ` Kirill A. Shutemov
2020-12-14 16:07               ` Kirill A. Shutemov
2020-12-14 17:54               ` Linus Torvalds
2020-12-14 17:54                 ` Linus Torvalds
2020-12-14 17:54                 ` Linus Torvalds
2020-12-14 18:56                 ` Matthew Wilcox
2020-12-14 18:56                   ` Matthew Wilcox
2020-12-16 17:07                 ` Kirill A. Shutemov
2020-12-16 17:07                   ` Kirill A. Shutemov
2020-12-16 18:41                   ` Linus Torvalds
2020-12-16 18:41                     ` Linus Torvalds
2020-12-16 18:41                     ` Linus Torvalds
2020-12-17 10:54                     ` Kirill A. Shutemov
2020-12-17 10:54                       ` Kirill A. Shutemov
2020-12-17 18:22                       ` Linus Torvalds
2020-12-17 18:22                         ` Linus Torvalds
2020-12-17 18:22                         ` Linus Torvalds
2020-12-18 11:04                         ` Kirill A. Shutemov
2020-12-18 11:04                           ` Kirill A. Shutemov
2020-12-18 18:56                           ` Linus Torvalds
2020-12-18 18:56                             ` Linus Torvalds
2020-12-18 18:56                             ` Linus Torvalds
2020-12-19 12:41                             ` Kirill A. Shutemov
2020-12-19 12:41                               ` Kirill A. Shutemov
2020-12-19 20:08                               ` Linus Torvalds
2020-12-19 20:08                                 ` Linus Torvalds
2020-12-19 20:08                                 ` Linus Torvalds
2020-12-19 20:34                               ` Linus Torvalds
2020-12-19 20:34                                 ` Linus Torvalds
2020-12-19 20:34                                 ` Linus Torvalds
2020-12-22 10:00                                 ` Kirill A. Shutemov
2020-12-22 10:00                                   ` Kirill A. Shutemov
2020-12-24  4:04                                   ` Hugh Dickins
2020-12-24  4:04                                     ` Hugh Dickins
2020-12-24  4:04                                     ` Hugh Dickins
2020-12-25 11:31                                     ` Kirill A. Shutemov
2020-12-25 11:31                                       ` Kirill A. Shutemov
2020-12-26 17:57                                       ` Linus Torvalds
2020-12-26 17:57                                         ` Linus Torvalds
2020-12-26 17:57                                         ` Linus Torvalds
2020-12-26 20:43                                         ` Kirill A. Shutemov
2020-12-26 20:43                                           ` Kirill A. Shutemov
2020-12-26 21:03                                           ` Hugh Dickins
2020-12-26 21:03                                             ` Hugh Dickins
2020-12-26 21:03                                             ` Hugh Dickins
2020-12-26 21:16                                             ` Linus Torvalds
2020-12-26 21:16                                               ` Linus Torvalds
2020-12-26 21:16                                               ` Linus Torvalds
2020-12-26 22:40                                               ` Kirill A. Shutemov
2020-12-26 22:40                                                 ` Kirill A. Shutemov
2020-12-27  0:45                                                 ` Hugh Dickins
2020-12-27  0:45                                                   ` Hugh Dickins
2020-12-27  0:45                                                   ` Hugh Dickins
2020-12-27  2:38                                                   ` Hugh Dickins
2020-12-27  2:38                                                     ` Hugh Dickins
2020-12-27  2:38                                                     ` Hugh Dickins
2020-12-27 19:38                                                     ` Linus Torvalds
2020-12-27 19:38                                                       ` Linus Torvalds
2020-12-27 19:38                                                       ` Linus Torvalds
2020-12-27 20:32                                                       ` Damian Tometzki
2020-12-27 20:32                                                         ` Damian Tometzki
2020-12-27 22:35                                                         ` Hugh Dickins [this message]
2020-12-27 22:35                                                           ` Hugh Dickins
2020-12-27 22:35                                                           ` Hugh Dickins
2020-12-27 23:12                                                           ` Linus Torvalds
2020-12-27 23:12                                                             ` Linus Torvalds
2020-12-27 23:12                                                             ` Linus Torvalds
2020-12-27 23:40                                                             ` Linus Torvalds
2020-12-27 23:40                                                               ` Linus Torvalds
2020-12-27 23:40                                                               ` Linus Torvalds
2020-12-27 23:55                                                               ` Kirill A. Shutemov
2020-12-27 23:55                                                                 ` Kirill A. Shutemov
2020-12-27 23:48                                                       ` Kirill A. Shutemov
2020-12-27 23:48                                                         ` Kirill A. Shutemov
2020-12-28  1:54                                                         ` Linus Torvalds
2020-12-28  1:54                                                           ` Linus Torvalds
2020-12-28  1:54                                                           ` Linus Torvalds
2020-12-28  6:43                                                           ` Hugh Dickins
2020-12-28  6:43                                                             ` Hugh Dickins
2020-12-28  6:43                                                             ` Hugh Dickins
2020-12-28 12:53                                                             ` Kirill A. Shutemov
2020-12-28 12:53                                                               ` Kirill A. Shutemov
2020-12-28 18:47                                                               ` Linus Torvalds
2020-12-28 18:47                                                                 ` Linus Torvalds
2020-12-28 18:47                                                                 ` Linus Torvalds
2020-12-28 21:58                                                                 ` Linus Torvalds
2020-12-28 21:58                                                                   ` Linus Torvalds
2020-12-28 21:58                                                                   ` Linus Torvalds
2020-12-29 13:28                                                                   ` Kirill A. Shutemov
2020-12-29 13:28                                                                     ` Kirill A. Shutemov
2020-12-29 15:19                                                                     ` Matthew Wilcox
2020-12-29 15:19                                                                       ` Matthew Wilcox
2020-12-29 20:52                                                                     ` Linus Torvalds
2020-12-29 20:52                                                                       ` Linus Torvalds
2020-12-29 20:52                                                                       ` Linus Torvalds
2020-12-28 22:05                                                                 ` Kirill A. Shutemov
2020-12-28 22:05                                                                   ` Kirill A. Shutemov
2020-12-28 22:12                                                                   ` Kirill A. Shutemov
2020-12-28 22:12                                                                     ` Kirill A. Shutemov
2020-12-29  4:35                                                                     ` Hugh Dickins
2020-12-29  4:35                                                                       ` Hugh Dickins
2020-12-29  4:35                                                                       ` Hugh Dickins
2020-12-28 23:28                                                                   ` Linus Torvalds
2020-12-28 23:28                                                                     ` Linus Torvalds
2020-12-28 23:28                                                                     ` Linus Torvalds
2020-12-26 21:07                                           ` Linus Torvalds
2020-12-26 21:07                                             ` Linus Torvalds
2020-12-26 21:07                                             ` Linus Torvalds
2020-12-26 21:41                                           ` Matthew Wilcox
2020-12-26 21:41                                             ` Matthew Wilcox
2020-12-09 16:39 ` [PATCH 2/2] arm64: mm: Implement arch_wants_old_faultaround_pte() Will Deacon
2020-12-09 16:39   ` Will Deacon
2020-12-09 18:35   ` Catalin Marinas
2020-12-09 18:35     ` Catalin Marinas
2020-12-09 18:46     ` Will Deacon
2020-12-09 18:46       ` Will Deacon

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=alpine.LSU.2.11.2012271418460.1091@eggly.anvils \
    --to=hughd@google.com \
    --cc=akpm@linux-foundation.org \
    --cc=catalin.marinas@arm.com \
    --cc=jack@suse.cz \
    --cc=kernel-team@android.com \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=kirill@shutemov.name \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux@tometzki.de \
    --cc=minchan@kernel.org \
    --cc=torvalds@linux-foundation.org \
    --cc=vinmenon@codeaurora.org \
    --cc=will@kernel.org \
    --cc=willy@infradead.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.