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 (diff)
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
next prev parent 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: linkBe 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.