| Message ID | 20210114175934.13070-1-will@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Create 'old' ptes for faultaround mappings on arm64 with hardware access flag | expand |
On Thu, Jan 14, 2021 at 10:01 AM Will Deacon <will@kernel.org> wrote: > > Try to clean this up by splitting the immutable fault information out > into a new 'struct vm_fault_info' which is embedded in 'struct vm_fault' > and will later be made 'const'. The vast majority of this change was > performed with a coccinelle patch: You may have a reason for doing it this way, but my reaction to this was: "just make the new embedded struct unnamed". Then you wouldn't need to do all the automated coccinelle changes. Is there some reason you didn't do that, or just a "oh, I didn't think of it". Linus
On Thu, Jan 14, 2021 at 10:16:13AM -0800, Linus Torvalds wrote: > On Thu, Jan 14, 2021 at 10:01 AM Will Deacon <will@kernel.org> wrote: > > > > Try to clean this up by splitting the immutable fault information out > > into a new 'struct vm_fault_info' which is embedded in 'struct vm_fault' > > and will later be made 'const'. The vast majority of this change was > > performed with a coccinelle patch: > > You may have a reason for doing it this way, but my reaction to this > was: "just make the new embedded struct unnamed". > > Then you wouldn't need to do all the automated coccinelle changes. > > Is there some reason you didn't do that, or just a "oh, I didn't think of > it". I tried that initially, e.g. struct vm_fault { const struct { unsigned long address; ... }; }; 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? Will
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? Linus
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
On Thu, Jan 14, 2021 at 11:41 AM Will Deacon <will@kernel.org> wrote: > > 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. Weird, looks like a bug to me in Clang, filed https://bugs.llvm.org/show_bug.cgi?id=48755.
On Thu, Jan 14, 2021 at 11:41 AM Will Deacon <will@kernel.org> wrote: > > 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: Ok, looks like a clang bug, but a reasonably benign one. As long as we have sufficient coverage with gcc, we'll get error reporting in a timely manner for any new incorrect assignments, so I think we can do that constant anonymous struct even if it does mean that clang might let some bad cases through (I personally use gcc for build testing, and then clang for building my boot kernels, so I'd catch anything x86-64 allmodconfig in my build tests). And keeping it unnamed it would avoid a lot of noisy churn.. Linus
On Thu, Jan 14, 2021 at 01:11:12PM -0800, Linus Torvalds wrote: > On Thu, Jan 14, 2021 at 11:41 AM Will Deacon <will@kernel.org> wrote: > > > > 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: > > Ok, looks like a clang bug, but a reasonably benign one. > > As long as we have sufficient coverage with gcc, we'll get error > reporting in a timely manner for any new incorrect assignments, so I > think we can do that constant anonymous struct even if it does mean > that clang might let some bad cases through (I personally use gcc for > build testing, and then clang for building my boot kernels, so I'd > catch anything x86-64 allmodconfig in my build tests). > > And keeping it unnamed it would avoid a lot of noisy churn.. Hmm. The feedback on the clang bug suggests that GCC is the one in the wrong here (although the argument is based on C11 and I haven't trawled through the standards to see how this has evolved): https://bugs.llvm.org/show_bug.cgi?id=48755#c1 There is at least some sympathy to generating a warning, so that might be good enough. Otherwise, I suppose we can explicitly mark the fields as 'const' but I won't jump to that immediately. Will
On Fri, Jan 15, 2021 at 1:23 AM Will Deacon <will@kernel.org> wrote: > > Hmm. The feedback on the clang bug suggests that GCC is the one in the > wrong here (although the argument is based on C11 and I haven't trawled > through the standards to see how this has evolved): Oh well. That writing is absolutely the _worst_ kind of weaselwording standards language reading, trying to make excuses for bad behavior by basically depending on "this language is unclear", and trying to say that the buggy behavior is required by C11. What a disappointment. Absolutely nothing in the quoted C11 language says to me what that bug entry claims it says. The argument seems to hinge on "The members of an anonymous structure or union are considered to be members of the containing structure or union" and then it makes the completely uncalled-for leap that that means that because it was "int" in the const struct, it must be "int" in the containing structure too. Which is complete BS, and doesn't follow logically _or_ grammatically. It would be a "member of the containing structure" even with the "const" qualifier, so the argument they make is just inane. In fact, the _other_ sentence they quote clearly points to this being just a clang bug: "A modifiable lvalue is [...] if it is a structure or union, does not have any member (including, recursively, any member or element of all contained aggregates or unions) with a const- qualified type" and clearly this recursively is an element with a const-qualified recursive struct. Whatever. It's one of those "read the documentation squint-eyed to avoid doing the right thing" arguments. It's not worth arguing with people like that, and let's just ignore the clang bug here. Linus
On Fri, Jan 15, 2021 at 1:33 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Fri, Jan 15, 2021 at 1:23 AM Will Deacon <will@kernel.org> wrote: > > > > Hmm. The feedback on the clang bug suggests that GCC is the one in the > > wrong here (although the argument is based on C11 and I haven't trawled > > through the standards to see how this has evolved): > > Oh well. > > That writing is absolutely the _worst_ kind of weaselwording standards > language reading, trying to make excuses for bad behavior by basically > depending on "this language is unclear", and trying to say that the > buggy behavior is required by C11. > > What a disappointment. I don't really understand British humor either, but I assume that's how the language lawyers throw shade on one anothers' standards. Richard is both the WG21 spec editor (C++) and British, IIRC. Apparently, there's a long conversion (behind closed doors; it's the ISO way) going on in regards to the thread Richard has kicked off with them (WG14; C). Moreso on what should happen with the _Atomic qualifier, assignments, and memcpy. So it is still an important thing to nail down the language spec. Note there were also a lot of discussions lately on "where should the volatile qualifier be allowed, or not." http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1152r0.html https://www.youtube.com/watch?v=KJW_DLaVXIY (2018? ok, maybe not lately. Lately for C) I view this similarly as "where should the const qualifier be allowed, or not."
Hi again folks, This is the third version of the patches I previously posted here: v1: https://lore.kernel.org/r/20201209163950.8494-1-will@kernel.org v2: https://lore.kernel.org/r/20210108171517.5290-1-will@kernel.org The patches allow architectures to opt-in at runtime for faultaround mappings to be created as 'old' instead of 'young'. Although there have been previous attempts at this, they failed either because the decision was deferred to userspace [1] or because it was done unconditionally and shown to regress benchmarks for particular architectures [2]. Minor changes since v2 include: * Update commit messages * Remove repeated word 'from from' in a comment * Restore 'vmf->flags' in filemap_map_pages() The major additions are in the five RFC patches at the end of the series, which attempt to implement a suggestion from Linus to split up 'struct vm_fault', clearly separating the mutable and immutable fields in the data structure. I used Coccinelle to do most of the mechanical work, but I also ran into some tricky problems along the way: 1. 'vmf->flags' is modified on the '->page_mkwrite()' path so I couldn't find a satisfactory way to move it to the new const structure. I toyed with getting rid of FAULT_FLAG_[MK]WRITE completely and just tracking these as bools, but there's also a weird piece of code in vmw_bo_vm_mkwrite() which modifies FAULT_FLAG_ALLOW_RETRY, so I gave up and left the 'flags' field alone. 2. I had to perform terrifying surgery on __collapse_huge_page_swapin() and, in doing so, I'm a bit wary about the initialisation of 'pgoff', as it isn't updated along with the address (this matches the old code). 3. vmf_insert_pfn_pmd() and friends take both a 'struct vm_fault' _and_ a 'bool write'. I have left them alone, but that FAULT_FLAG_WRITE is causing trouble again. 4. Turns out 'struct vm_fault' is popular, so the diffstat is bloody massive. Anyway, be good to hear any thoughts on this lot, particular with regards to my comments above. I've also pushed the series here: https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=faultaround Cheers Will [1] https://www.spinics.net/lists/linux-mm/msg143831.html [2] 315d09bf30c2 ("Revert "mm: make faultaround produce old ptes"") Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Jan Kara <jack@suse.cz> Cc: Minchan Kim <minchan@kernel.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Vinayak Menon <vinmenon@codeaurora.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Hugh Dickins <hughd@google.com> Cc: <kernel-team@android.com> --->8 Kirill A. Shutemov (1): mm: Cleanup faultaround and finish_fault() codepaths Will Deacon (7): mm: Allow architectures to request 'old' entries when prefaulting arm64: mm: Implement arch_wants_old_prefaulted_pte() mm: Separate fault info out of 'struct vm_fault' mm: Pass 'address' to map to do_set_pte() and drop FAULT_FLAG_PREFAULT mm: Avoid modifying vmf.info.address in __collapse_huge_page_swapin() mm: Use static initialisers for 'info' field of 'struct vm_fault' mm: Mark 'info' field of 'struct vm_fault' as 'const' arch/arm64/include/asm/pgtable.h | 12 +- arch/arm64/kernel/vdso.c | 4 +- arch/powerpc/kvm/book3s_64_vio.c | 6 +- arch/powerpc/kvm/book3s_hv_uvmem.c | 4 +- arch/powerpc/kvm/book3s_xive_native.c | 13 +- arch/powerpc/platforms/cell/spufs/file.c | 16 +- arch/s390/kernel/vdso.c | 4 +- arch/s390/kvm/kvm-s390.c | 2 +- arch/x86/entry/vdso/vma.c | 22 +- arch/x86/kernel/cpu/sgx/encl.c | 4 +- drivers/char/agp/alpha-agp.c | 2 +- drivers/char/mspec.c | 6 +- drivers/dax/device.c | 37 +- drivers/dma-buf/heaps/cma_heap.c | 6 +- drivers/dma-buf/udmabuf.c | 4 +- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 +- drivers/gpu/drm/armada/armada_gem.c | 6 +- drivers/gpu/drm/drm_gem_shmem_helper.c | 8 +- drivers/gpu/drm/drm_vm.c | 18 +- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 10 +- drivers/gpu/drm/gma500/framebuffer.c | 4 +- drivers/gpu/drm/gma500/gem.c | 8 +- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 8 +- drivers/gpu/drm/msm/msm_gem.c | 11 +- drivers/gpu/drm/nouveau/nouveau_dmem.c | 8 +- drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 +- drivers/gpu/drm/omapdrm/omap_gem.c | 20 +- drivers/gpu/drm/radeon/radeon_ttm.c | 4 +- drivers/gpu/drm/tegra/gem.c | 6 +- drivers/gpu/drm/ttm/ttm_bo_vm.c | 10 +- drivers/gpu/drm/vc4/vc4_bo.c | 2 +- drivers/gpu/drm/vgem/vgem_drv.c | 6 +- drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c | 12 +- drivers/hsi/clients/cmt_speech.c | 2 +- drivers/hwtracing/intel_th/msu.c | 8 +- drivers/infiniband/core/uverbs_main.c | 10 +- drivers/infiniband/hw/hfi1/file_ops.c | 2 +- drivers/infiniband/hw/qib/qib_file_ops.c | 2 +- drivers/media/v4l2-core/videobuf-dma-sg.c | 6 +- drivers/misc/cxl/context.c | 9 +- drivers/misc/ocxl/context.c | 10 +- drivers/misc/ocxl/sysfs.c | 8 +- drivers/misc/sgi-gru/grumain.c | 4 +- drivers/scsi/cxlflash/ocxl_hw.c | 6 +- drivers/scsi/cxlflash/superpipe.c | 2 +- drivers/scsi/sg.c | 4 +- drivers/target/target_core_user.c | 6 +- drivers/uio/uio.c | 6 +- drivers/usb/mon/mon_bin.c | 4 +- drivers/vfio/pci/vfio_pci.c | 2 +- drivers/vfio/pci/vfio_pci_nvlink2.c | 8 +- drivers/vhost/vdpa.c | 6 +- drivers/video/fbdev/core/fb_defio.c | 14 +- drivers/xen/privcmd-buf.c | 5 +- drivers/xen/privcmd.c | 4 +- fs/9p/vfs_file.c | 2 +- fs/afs/write.c | 2 +- fs/btrfs/inode.c | 4 +- fs/ceph/addr.c | 6 +- fs/dax.c | 53 +-- fs/ext2/file.c | 6 +- fs/ext4/file.c | 6 +- fs/ext4/inode.c | 4 +- fs/f2fs/file.c | 8 +- fs/fuse/dax.c | 2 +- fs/fuse/file.c | 4 +- fs/gfs2/file.c | 8 +- fs/iomap/buffered-io.c | 2 +- fs/kernfs/file.c | 4 +- fs/nfs/file.c | 2 +- fs/nilfs2/file.c | 2 +- fs/ocfs2/mmap.c | 8 +- fs/orangefs/file.c | 2 +- fs/orangefs/inode.c | 4 +- fs/proc/vmcore.c | 4 +- fs/ubifs/file.c | 2 +- fs/userfaultfd.c | 17 +- fs/xfs/xfs_file.c | 18 +- fs/zonefs/super.c | 6 +- include/linux/huge_mm.h | 6 +- include/linux/mm.h | 21 +- include/linux/pgtable.h | 11 + include/trace/events/fs_dax.h | 28 +- ipc/shm.c | 2 +- kernel/events/core.c | 12 +- kernel/relay.c | 4 +- lib/test_hmm.c | 4 +- mm/filemap.c | 208 +++++++--- mm/huge_memory.c | 57 +-- mm/hugetlb.c | 6 +- mm/internal.h | 4 +- mm/khugepaged.c | 39 +- mm/memory.c | 452 +++++++++------------ mm/mmap.c | 6 +- mm/shmem.c | 16 +- mm/swap_state.c | 19 +- mm/swapfile.c | 13 +- samples/vfio-mdev/mbochs.c | 10 +- security/selinux/selinuxfs.c | 4 +- sound/core/pcm_native.c | 8 +- sound/usb/usx2y/us122l.c | 4 +- sound/usb/usx2y/usX2Yhwdep.c | 8 +- sound/usb/usx2y/usx2yhwdeppcm.c | 4 +- virt/kvm/kvm_main.c | 12 +- 104 files changed, 821 insertions(+), 730 deletions(-)