From: Will Deacon <will@kernel.org> To: Linus Torvalds <torvalds@linux-foundation.org>, ndesaulniers@google.com Cc: 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>, "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>, Vinayak Menon <vinmenon@codeaurora.org>, Hugh Dickins <hughd@google.com>, Android Kernel Team <kernel-team@android.com> Subject: Re: [RFC PATCH 4/8] mm: Separate fault info out of 'struct vm_fault' Date: Thu, 14 Jan 2021 19:41:29 +0000 [thread overview] Message-ID: <20210114194129.GA13314@willie-the-truck> (raw) In-Reply-To: <CAHk-=wjdJmL22+zk3_rWAfEJJCf=oDxiJ530qk-WNk_Ji0qhxw@mail.gmail.com> On Thu, Jan 14, 2021 at 11:09:01AM -0800, Linus Torvalds wrote: > On Thu, Jan 14, 2021 at 11:00 AM Will Deacon <will@kernel.org> wrote: > > > > I tried that initially, but I found that I had to make all of the > > members const to get it to work, at which point the anonymous struct > > wasn't really adding anything. Did I just botch the syntax? > > I'm not sure what you tried. But this stupid test-case sure works for me: > > struct hello { > const struct { > unsigned long address; > }; > unsigned int flags; > }; > > extern int fn(struct hello *); > > int test(void) > { > struct hello a = { > .address = 1, > }; > a.flags = 0; > return fn(&a); > } > > and because "address" is in that unnamed constant struct, you can only > set it within that initializer, and cannot do > > a.address = 0; > > without an error (the way you _can_ do "a.flags = 0"). > > I don't see naming the struct making a difference - apart from forcing > that big rename patch, of course. > > But maybe we're talking about different issues? Urgh... We _are_ both on the same page, and your reply above had me thinking I've lost the plot, so I went back to the start. Check out v5.11-rc3 and apply this patch: diff --git a/include/linux/mm.h b/include/linux/mm.h index ecdf8a8cd6ae..1eb950865450 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -514,11 +514,14 @@ static inline bool fault_flag_allow_retry_first(unsigned int flags) * pgoff should be used in favour of virtual_address, if possible. */ struct vm_fault { - struct vm_area_struct *vma; /* Target VMA */ + const struct { + struct vm_area_struct *vma; /* Target VMA */ + gfp_t gfp_mask; /* gfp mask to be used for allocations */ + pgoff_t pgoff; /* Logical page offset based on vma */ + unsigned long address; /* Faulting virtual address */ + }; + unsigned int flags; /* FAULT_FLAG_xxx flags */ - gfp_t gfp_mask; /* gfp mask to be used for allocations */ - pgoff_t pgoff; /* Logical page offset based on vma */ - unsigned long address; /* Faulting virtual address */ pmd_t *pmd; /* Pointer to pmd entry matching * the 'address' */ pud_t *pud; /* Pointer to pud entry matching Sure enough, an arm64 defconfig builds perfectly alright with that change, but it really shouldn't. I'm using clang 11.0.5, so I had another go with GCC 9.2.1 and bang: mm/filemap.c: In function ‘filemap_map_pages’: mm/filemap.c:2963:16: error: assignment of member ‘address’ in read-only object 2963 | vmf->address += (xas.xa_index - last_pgoff) << PAGE_SHIFT; | ^~ make[1]: *** [scripts/Makefile.build:279: mm/filemap.o] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1805: mm] Error 2 make: *** Waiting for unfinished jobs.... Nick -- any clue what's happening here? We would like that const anonymous struct to behave like a const struct member, as the alternative (naming the thing) results in a lot of refactoring churn. Cheers, Will
WARNING: multiple messages have this Message-ID (diff)
From: Will Deacon <will@kernel.org> To: Linus Torvalds <torvalds@linux-foundation.org>, ndesaulniers@google.com Cc: 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>, Linux-MM <linux-mm@kvack.org>, Vinayak Menon <vinmenon@codeaurora.org>, "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>, Andrew Morton <akpm@linux-foundation.org>, Android Kernel Team <kernel-team@android.com>, Linux ARM <linux-arm-kernel@lists.infradead.org> Subject: Re: [RFC PATCH 4/8] mm: Separate fault info out of 'struct vm_fault' Date: Thu, 14 Jan 2021 19:41:29 +0000 [thread overview] Message-ID: <20210114194129.GA13314@willie-the-truck> (raw) In-Reply-To: <CAHk-=wjdJmL22+zk3_rWAfEJJCf=oDxiJ530qk-WNk_Ji0qhxw@mail.gmail.com> On Thu, Jan 14, 2021 at 11:09:01AM -0800, Linus Torvalds wrote: > On Thu, Jan 14, 2021 at 11:00 AM Will Deacon <will@kernel.org> wrote: > > > > I tried that initially, but I found that I had to make all of the > > members const to get it to work, at which point the anonymous struct > > wasn't really adding anything. Did I just botch the syntax? > > I'm not sure what you tried. But this stupid test-case sure works for me: > > struct hello { > const struct { > unsigned long address; > }; > unsigned int flags; > }; > > extern int fn(struct hello *); > > int test(void) > { > struct hello a = { > .address = 1, > }; > a.flags = 0; > return fn(&a); > } > > and because "address" is in that unnamed constant struct, you can only > set it within that initializer, and cannot do > > a.address = 0; > > without an error (the way you _can_ do "a.flags = 0"). > > I don't see naming the struct making a difference - apart from forcing > that big rename patch, of course. > > But maybe we're talking about different issues? Urgh... We _are_ both on the same page, and your reply above had me thinking I've lost the plot, so I went back to the start. Check out v5.11-rc3 and apply this patch: diff --git a/include/linux/mm.h b/include/linux/mm.h index ecdf8a8cd6ae..1eb950865450 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -514,11 +514,14 @@ static inline bool fault_flag_allow_retry_first(unsigned int flags) * pgoff should be used in favour of virtual_address, if possible. */ struct vm_fault { - struct vm_area_struct *vma; /* Target VMA */ + const struct { + struct vm_area_struct *vma; /* Target VMA */ + gfp_t gfp_mask; /* gfp mask to be used for allocations */ + pgoff_t pgoff; /* Logical page offset based on vma */ + unsigned long address; /* Faulting virtual address */ + }; + unsigned int flags; /* FAULT_FLAG_xxx flags */ - gfp_t gfp_mask; /* gfp mask to be used for allocations */ - pgoff_t pgoff; /* Logical page offset based on vma */ - unsigned long address; /* Faulting virtual address */ pmd_t *pmd; /* Pointer to pmd entry matching * the 'address' */ pud_t *pud; /* Pointer to pud entry matching Sure enough, an arm64 defconfig builds perfectly alright with that change, but it really shouldn't. I'm using clang 11.0.5, so I had another go with GCC 9.2.1 and bang: mm/filemap.c: In function ‘filemap_map_pages’: mm/filemap.c:2963:16: error: assignment of member ‘address’ in read-only object 2963 | vmf->address += (xas.xa_index - last_pgoff) << PAGE_SHIFT; | ^~ make[1]: *** [scripts/Makefile.build:279: mm/filemap.o] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1805: mm] Error 2 make: *** Waiting for unfinished jobs.... Nick -- any clue what's happening here? We would like that const anonymous struct to behave like a const struct member, as the alternative (naming the thing) results in a lot of refactoring churn. Cheers, Will _______________________________________________ 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:[~2021-01-14 19:42 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-14 17:59 [PATCH v3 0/8] Create 'old' ptes for faultaround mappings on arm64 with hardware access flag Will Deacon 2021-01-14 17:59 ` Will Deacon 2021-01-14 17:59 ` [PATCH v3 1/8] mm: Cleanup faultaround and finish_fault() codepaths Will Deacon 2021-01-14 17:59 ` Will Deacon 2021-02-09 20:24 ` Guenter Roeck 2021-02-09 20:24 ` Guenter Roeck 2021-02-10 11:44 ` Will Deacon 2021-02-10 11:44 ` Will Deacon 2021-02-10 14:57 ` Guenter Roeck 2021-02-10 14:57 ` Guenter Roeck 2021-01-14 17:59 ` [PATCH v3 2/8] mm: Allow architectures to request 'old' entries when prefaulting Will Deacon 2021-01-14 17:59 ` Will Deacon 2021-01-14 17:59 ` [PATCH v3 3/8] arm64: mm: Implement arch_wants_old_prefaulted_pte() Will Deacon 2021-01-14 17:59 ` Will Deacon 2021-01-14 17:59 ` [RFC PATCH 4/8] mm: Separate fault info out of 'struct vm_fault' Will Deacon 2021-01-14 18:16 ` Linus Torvalds 2021-01-14 18:16 ` Linus Torvalds 2021-01-14 18:16 ` Linus Torvalds 2021-01-14 19:00 ` Will Deacon 2021-01-14 19:00 ` Will Deacon 2021-01-14 19:09 ` Linus Torvalds 2021-01-14 19:09 ` Linus Torvalds 2021-01-14 19:09 ` Linus Torvalds 2021-01-14 19:41 ` Will Deacon [this message] 2021-01-14 19:41 ` Will Deacon 2021-01-14 20:09 ` Nick Desaulniers 2021-01-14 20:09 ` Nick Desaulniers 2021-01-14 20:09 ` Nick Desaulniers 2021-01-14 21:11 ` Linus Torvalds 2021-01-14 21:11 ` Linus Torvalds 2021-01-14 21:11 ` Linus Torvalds 2021-01-15 9:23 ` Will Deacon 2021-01-15 9:23 ` Will Deacon 2021-01-15 21:32 ` Linus Torvalds 2021-01-15 21:32 ` Linus Torvalds 2021-01-15 21:32 ` Linus Torvalds 2021-01-20 0:00 ` Nick Desaulniers 2021-01-20 0:00 ` Nick Desaulniers 2021-01-20 0:00 ` Nick Desaulniers 2021-01-14 17:59 ` [RFC PATCH 5/8] mm: Pass 'address' to map to do_set_pte() and drop FAULT_FLAG_PREFAULT Will Deacon 2021-01-14 17:59 ` Will Deacon 2021-01-14 18:17 ` Linus Torvalds 2021-01-14 18:17 ` Linus Torvalds 2021-01-14 18:17 ` Linus Torvalds 2021-01-15 10:24 ` Will Deacon 2021-01-15 10:24 ` Will Deacon 2021-01-14 17:59 ` [RFC PATCH 6/8] mm: Avoid modifying vmf.info.address in __collapse_huge_page_swapin() Will Deacon 2021-01-14 17:59 ` Will Deacon 2021-01-14 17:59 ` [RFC PATCH 7/8] mm: Use static initialisers for 'info' field of 'struct vm_fault' Will Deacon 2021-01-14 17:59 ` Will Deacon 2021-01-14 17:59 ` [RFC PATCH 8/8] mm: Mark 'info' field of 'struct vm_fault' as 'const' Will Deacon 2021-01-14 17:59 ` 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=20210114194129.GA13314@willie-the-truck \ --to=will@kernel.org \ --cc=akpm@linux-foundation.org \ --cc=catalin.marinas@arm.com \ --cc=hughd@google.com \ --cc=jack@suse.cz \ --cc=kernel-team@android.com \ --cc=kirill.shutemov@linux.intel.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=minchan@kernel.org \ --cc=ndesaulniers@google.com \ --cc=torvalds@linux-foundation.org \ --cc=vinmenon@codeaurora.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.