Create 'old' ptes for faultaround mappings on arm64 with hardware access flag
Subsystems (8)
Will Deacon
maintainerv4 cover Create 'old' ptes for faultaround mappings on arm64 with hardware access flag
Hi all,
This is version four of the patches I previously posted here:
v1: https://lore.kernel.org/r/[email protected]
v2: https://lore.kernel.org/r/[email protected]
v3: https://lore.kernel.org/r/[email protected]
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].
The big change since v3 is that the immutable fields of 'struct vm_fault' now live in a 'const' anonymous struct. Although Clang will silently accept modifications to these fields [3], GCC emits an error. The resulting diffstat is _considerably_ more manageable with this approach.
As before, I've also updated this branch:
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"")
[3] https://bugs.llvm.org/show_bug.cgi?id=48755
--->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: Move immutable fields of 'struct vm_fault' into anonymous struct
mm: Pass 'address' to map to do_set_pte() and drop FAULT_FLAG_PREFAULT
mm: Avoid modifying vmf.address in __collapse_huge_page_swapin()
mm: Use static initialisers for immutable fields of 'struct vm_fault'
mm: Mark anonymous struct field of 'struct vm_fault' as 'const'
arch/arm64/include/asm/pgtable.h | 12 +- fs/xfs/xfs_file.c | 6 +- include/linux/mm.h | 25 ++-- include/linux/pgtable.h | 11 ++ mm/filemap.c | 178 ++++++++++++++++++------ mm/khugepaged.c | 37 +++-- mm/memory.c | 223 +++++++++++-------------------- mm/shmem.c | 6 +- mm/swapfile.c | 11 +- 9 files changed, 280 insertions(+), 229 deletions(-)
Signature · 1 line
2.30.0.284.gd98b1dd5eaa7-goog
Will Deacon
maintainerPATCH v4 1/8 mm: Cleanup faultaround and finish_fault() codepaths
From: "Kirill A. Shutemov" <[email protected]>
alloc_set_pte() has two users with different requirements: in the faultaround code, it called from an atomic context and PTE page table has to be preallocated. finish_fault() can sleep and allocate page table as needed.
PTL locking rules are also strange, hard to follow and overkill for finish_fault().
Let's untangle the mess. alloc_set_pte() has gone now. All locking is explicit.
The price is some code duplication to handle huge pages in faultaround path, but it should be fine, having overall improvement in readability.
--- fs/xfs/xfs_file.c | 6 +- include/linux/mm.h | 12 ++- include/linux/pgtable.h | 11 +++ mm/filemap.c | 177 ++++++++++++++++++++++++++--------- mm/memory.c | 199 ++++++++++++---------------------------- 5 files changed, 213 insertions(+), 192 deletions(-)
fs/xfs/xfs_file.c +4 -2
@@ -1319,17 +1319,19 @@ xfs_filemap_pfn_mkwrite(return __xfs_filemap_fault (vmf,PE_SIZE_PTE , true); } -static void +static vm_fault_t xfs_filemap_map_pages (struct vm_fault *vmf,pgoff_t start_pgoff,pgoff_t end_pgoff) {struct inode *inode =file_inode (vmf->vma->vm_file); +vm_fault_t ret;xfs_ilock (XFS_I (inode),XFS_MMAPLOCK_SHARED ); -filemap_map_pages (vmf, start_pgoff, end_pgoff); + ret =filemap_map_pages (vmf, start_pgoff, end_pgoff);xfs_iunlock (XFS_I (inode),XFS_MMAPLOCK_SHARED ); +return ret; }static const struct vm_operations_struct xfs_file_vm_ops = {
include/linux/mm.h +7 -5
@@ -542,8 +542,8 @@ struct vm_fault {* is not NULL, otherwise pmd. */ pgtable_t prealloc_pte;/* Pre-allocated pte page table. -* vm_ops->map_pages() calls -* alloc_set_pte() from atomic context. +* vm_ops->map_pages() sets up a page +* table from atomic context. * do_fault_around() pre-allocates * page table to avoid allocation from * atomic context.
@@ -578,7 +578,7 @@ struct vm_operations_struct {vm_fault_t (*fault)(struct vm_fault *vmf);vm_fault_t (*huge_fault)(struct vm_fault *vmf,enum page_entry_size pe_size); -void (*map_pages)(struct vm_fault *vmf, +vm_fault_t (*map_pages)(struct vm_fault *vmf,pgoff_t start_pgoff,pgoff_t end_pgoff);unsigned long (*pagesize)(struct vm_area_struct * area);
@@ -988,7 +988,9 @@ static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma) return pte; } -vm_fault_t alloc_set_pte (struct vm_fault *vmf,struct page *page); +vm_fault_t do_set_pmd (struct vm_fault *vmf,struct page *page); +void do_set_pte (struct vm_fault *vmf,struct page *page); +vm_fault_t finish_fault (struct vm_fault *vmf);vm_fault_t finish_mkwrite_fault (struct vm_fault *vmf);#endif
@@ -2622,7 +2624,7 @@ extern void truncate_inode_pages_final(struct address_space *);/* generic vm_area_ops exported for stackable file systems */ extern vm_fault_t filemap_fault (struct vm_fault *vmf); -extern void filemap_map_pages (struct vm_fault *vmf, +extern vm_fault_t filemap_map_pages (struct vm_fault *vmf,pgoff_t start_pgoff,pgoff_t end_pgoff);extern vm_fault_t filemap_page_mkwrite (struct vm_fault *vmf);
include/linux/pgtable.h +11 -0
@@ -1314,6 +1314,17 @@ static inline int pmd_trans_unstable(pmd_t *pmd)#endif } +/* +* the ordering of these checks is important for pmds with _page_devmap set. +* if we check pmd_trans_unstable() first we will trip the bad_pmd() check +* inside of pmd_none_or_trans_huge_or_clear_bad(). this will end up correctly +* returning 1 but not before it spams dmesg with the pmd_clear_bad() output. +*/ +static inline int pmd_devmap_trans_unstable (pmd_t *pmd) +{ +return pmd_devmap (*pmd) ||pmd_trans_unstable (pmd); +} +#ifndef CONFIG_NUMA_BALANCING /* * Technically a PTE can be PROTNONE even when not doing NUMA balancing but
mm/filemap.c +134 -43
214 lines not on this page · raw message
mm/memory.c +57 -142
285 lines not on this page · raw message
Signature · 1 line
2.30.0.284.gd98b1dd5eaa7-goog
Will Deacon
maintainerPATCH v4 2/8 mm: Allow architectures to request 'old' entries when prefaulting
In reply to Will DeaconCommit 5c0a85fad949 ("mm: make faultaround produce old ptes") changed the "faultaround" behaviour to initialise prefaulted PTEs as 'old', since this avoids vmscan wrongly assuming that they are hot, despite having never been explicitly accessed by userspace. The change has been shown to benefit numerous arm64 micro-architectures (with hardware access flag) running Android, where both application launch latency and direct reclaim time are significantly reduced (by 10%+ and ~80% respectively).
Unfortunately, commit 315d09bf30c2 ("Revert "mm: make faultaround produce old ptes"") reverted the change due to it being identified as the cause of a ~6% regression in unixbench on x86. Experiments on a variety of recent arm64 micro-architectures indicate that unixbench is not affected by the original commit, which appears to yield a 0-1% performance improvement.
Since one size does not fit all for the initial state of prefaulted PTEs, introduce arch_wants_old_prefaulted_pte(), which allows an architecture to opt-in to 'old' prefaulted PTEs at runtime based on whatever criteria it may have.
--- include/linux/mm.h | 5 ++++- mm/filemap.c | 14 ++++++++++---- mm/memory.c | 20 +++++++++++++++++++- 3 files changed, 33 insertions(+), 6 deletions(-)
include/linux/mm.h +4 -1
@@ -434,6 +434,7 @@ extern pgprot_t protection_map[16];* @FAULT_FLAG_REMOTE: The fault is not for current task/mm. * @FAULT_FLAG_INSTRUCTION: The fault was during an instruction fetch. * @FAULT_FLAG_INTERRUPTIBLE: The fault can be interrupted by non-fatal signals. +* @FAULT_FLAG_PREFAULT: Fault was a prefault. * * About @FAULT_FLAG_ALLOW_RETRY and @FAULT_FLAG_TRIED: we can specify * whether we would allow page faults to retry by specifying these two
@@ -464,6 +465,7 @@ extern pgprot_t protection_map[16];#define FAULT_FLAG_REMOTE 0x80#define FAULT_FLAG_INSTRUCTION 0x100#define FAULT_FLAG_INTERRUPTIBLE 0x200 +#define FAULT_FLAG_PREFAULT 0x400/* * The default fault flags that should be used by most of the
@@ -501,7 +503,8 @@ static inline bool fault_flag_allow_retry_first(unsigned int flags) { FAULT_FLAG_USER, "USER" }, \ { FAULT_FLAG_REMOTE, "REMOTE" }, \ { FAULT_FLAG_INSTRUCTION, "INSTRUCTION" }, \ - { FAULT_FLAG_INTERRUPTIBLE, "INTERRUPTIBLE" } + { FAULT_FLAG_INTERRUPTIBLE, "INTERRUPTIBLE" }, \ + { FAULT_FLAG_PREFAULT, "PREFAULT" }/* * vm_fault is filled by the pagefault handler and passed to the vma's
mm/filemap.c +10 -4
@@ -3019,6 +3019,7 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,struct address_space *mapping = file->f_mapping;pgoff_t last_pgoff = start_pgoff;unsigned long address = vmf->address; +unsigned long flags = vmf->flags;XA_STATE (xas, &mapping->i_pages, start_pgoff);struct page *head, *page;unsigned int mmap_miss =READ_ONCE (file->f_ra.mmap_miss);
@@ -3051,14 +3052,18 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,if (!pte_none (*vmf->pte)) goto unlock; +/* We're about to handle the fault */ +if (vmf->address == address) { + vmf->flags &= ~FAULT_FLAG_PREFAULT ; + ret =VM_FAULT_NOPAGE ; + }else { + vmf->flags |=FAULT_FLAG_PREFAULT ; + } +do_set_pte (vmf, page);/* no need to invalidate: a not-present page won't be cached */ update_mmu_cache (vma, vmf->address, vmf->pte);unlock_page (head); - -/* The fault is handled */ -if (vmf->address == address) - ret =VM_FAULT_NOPAGE ;continue ; unlock:unlock_page (head);
@@ -3067,6 +3072,7 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,pte_unmap_unlock (vmf->pte, vmf->ptl); out:rcu_read_unlock (); + vmf->flags = flags; vmf->address = address;WRITE_ONCE (file->f_ra.mmap_miss, mmap_miss);return ret;
mm/memory.c +19 -1
@@ -134,6 +134,18 @@ static inline bool arch_faults_on_old_pte(void) }#endif +#ifndef arch_wants_old_prefaulted_pte +static inline bool arch_wants_old_prefaulted_pte (void ) +{ +/* +* Transitioning a PTE from 'old' to 'young' can be expensive on +* some architectures, even if it's performed in hardware. By +* default, "false" means prefaulted entries will be 'young'. +*/ +return false; +} +#endif +static int __init disable_randmaps (char *s) { randomize_va_space =0 ;
@@ -3725,11 +3737,17 @@ void do_set_pte(struct vm_fault *vmf, struct page *page) {struct vm_area_struct *vma = vmf->vma;bool write = vmf->flags &FAULT_FLAG_WRITE ; +bool prefault = vmf->flags &FAULT_FLAG_PREFAULT ;pte_t entry;flush_icache_page (vma, page); entry =mk_pte (page, vma->vm_page_prot); - entry =pte_sw_mkyoung (entry); + +if (prefault &&arch_wants_old_prefaulted_pte ()) + entry =pte_mkold (entry); +else + entry =pte_sw_mkyoung (entry); +if (write) entry =maybe_mkwrite (pte_mkdirty (entry), vma);/* copy-on-write page */
Signature · 1 line
2.30.0.284.gd98b1dd5eaa7-goog
Will Deacon
maintainerPATCH v4 3/8 arm64: mm: Implement arch_wants_old_prefaulted_pte()
In reply to Will DeaconOn CPUs with hardware AF/DBM, initialising prefaulted PTEs as 'old' improves vmscan behaviour and does not appear to introduce any overhead elsewhere.
Implement arch_wants_old_prefaulted_pte() to return 'true' if we detect hardware access flag support at runtime. This can be extended in future based on MIDR matching if necessary.
--- arch/arm64/include/asm/pgtable.h | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
arch/arm64/include/asm/pgtable.h +11 -1
@@ -980,7 +980,17 @@ static inline bool arch_faults_on_old_pte(void) return !cpu_has_hw_af(); } -#define arch_faults_on_old_pte arch_faults_on_old_pte +#define arch_faults_on_old_pte arch_faults_on_old_pte + +/* +* Experimentally, it's cheap to set the access flag in hardware and we +* benefit from prefaulting mappings as 'old' to start with. +*/ +static inline bool arch_wants_old_prefaulted_pte (void ) +{ +return !arch_faults_on_old_pte (); +} +#define arch_wants_old_prefaulted_pte arch_wants_old_prefaulted_pte#endif /* !__ASSEMBLY__ */
Signature · 1 line
2.30.0.284.gd98b1dd5eaa7-goog
Will Deacon
maintainerPATCH v4 4/8 mm: Move immutable fields of 'struct vm_fault' into anonymous struct
In reply to Will Deacon'struct vm_fault' contains both information about the fault being serviced alongside mutable fields contributing to the state of the fault-handling logic. Unfortunately, the distinction between the two is not clear-cut, and a number of callers end up manipulating the structure temporarily before restoring it when returning.
Try to clean this up by moving the immutable fault information into an anonymous struct, which will later be marked as 'const'. GCC will then complain (with an error) about modification of these fields after they have been initialised, although LLVM currently allows them without even a warning:
https://bugs.llvm.org/show_bug.cgi?id=48755
Ideally, the 'flags' field would be part of the new structure too, but it seems as though the ->page_mkwrite() path is not ready for this yet.
--- include/linux/mm.h | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
include/linux/mm.h +8 -5
@@ -517,11 +517,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 */ -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 */ +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 +* XXX: should really be 'const' */ pmd_t *pmd;/* Pointer to pmd entry matching * the 'address' */ pud_t *pud;/* Pointer to pud entry matching
Signature · 1 line
2.30.0.284.gd98b1dd5eaa7-goog
Re: [PATCH v4 4/8] mm: Move immutable fields of 'struct vm_fault' into anonymous struct
Quoted 7 lines · Will Deacon
'struct
vm_fault' contains both information about the fault being serviced alongside mutable fields contributing to the state of the fault-handling logic. Unfortunately, the distinction between the two is not clear-cut, and a number of callers end up manipulating the structure temporarily before restoring it when returning.
Quoted 7 lines
Try to clean this up by moving the immutable fault information into an anonymous struct, which will later be marked as 'const'. GCC will then complain (with an error) about modification of these fields after they have been initialised, although LLVM currently allows them without even a warning:
I think this paragraph+link would be better on patch 8/8.
Quoted 39 lines
Ideally, the 'flags' field would be part of the new structure too, but it seems as though the ->page_mkwrite() path is not ready for this yet.
Cc Kirill A. Shutemov <[email protected]>Suggested-by Linus Torvalds <[email protected]>Link https://lore.kernel.org/r/CAHk-=whYs9XsO88iqJzN6NC=D-dp2m0oYXuOoZ=eWnvv=5OA+w@mail.gmail.com open hereSigned-off-by Will Deacon <[email protected]>--- include/linux/mm.h | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)include/linux/mm.h +8
-6@@ -517,11 +517,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 */ -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 */ +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 +* XXX: should really be 'const' */ pmd_t *pmd;/* Pointer to pmd entry matching * the 'address' */ pud_t *pud;/* Pointer to pud entry matching -- 2.30.0.284.gd98b1dd5eaa7-goog
Signature · 2 lines
Thanks, ~Nick Desaulniers
Will Deacon
maintainerRe: [PATCH v4 4/8] mm: Move immutable fields of 'struct vm_fault' into anonymous struct
Quoted 14 lines · Nick Desaulniers
Quoted 13 lines · Will Deacon
'struct
vm_fault' contains both information about the fault being serviced alongside mutable fields contributing to the state of the fault-handling logic. Unfortunately, the distinction between the two is not clear-cut, and a number of callers end up manipulating the structure temporarily before restoring it when returning.Try to clean this up by moving the immutable fault information into an anonymous struct, which will later be marked as 'const'. GCC will then complain (with an error) about modification of these fields after they have been initialised, although LLVM currently allows them without even a warning:
Quoted 3 lines
Quoted 1 line
I think this paragraph+link would be better on patch 8/8.
Agreed, I'll move it.
Will
Will Deacon
maintainerPATCH v4 5/8 mm: Pass 'address' to map to do_set_pte() and drop FAULT_FLAG_PREFAULT
In reply to Will DeaconRather than modifying the 'address' field of the 'struct vm_fault' passed to do_set_pte(), leave that to identify the real faulting address and pass in the virtual address to be mapped by the new pte as a separate argument.
This makes FAULT_FLAG_PREFAULT redundant, as a prefault entry can be identified simply by comparing the new address parameter with the faulting address, so remove the redundant flag at the same time.
--- include/linux/mm.h | 7 ++----- mm/filemap.c | 21 +++++++-------------- mm/memory.c | 10 +++++----- 3 files changed, 14 insertions(+), 24 deletions(-)
include/linux/mm.h +2 -5
@@ -434,7 +434,6 @@ extern pgprot_t protection_map[16];* @FAULT_FLAG_REMOTE: The fault is not for current task/mm. * @FAULT_FLAG_INSTRUCTION: The fault was during an instruction fetch. * @FAULT_FLAG_INTERRUPTIBLE: The fault can be interrupted by non-fatal signals. -* @FAULT_FLAG_PREFAULT: Fault was a prefault. * * About @FAULT_FLAG_ALLOW_RETRY and @FAULT_FLAG_TRIED: we can specify * whether we would allow page faults to retry by specifying these two
@@ -465,7 +464,6 @@ extern pgprot_t protection_map[16];#define FAULT_FLAG_REMOTE 0x80#define FAULT_FLAG_INSTRUCTION 0x100#define FAULT_FLAG_INTERRUPTIBLE 0x200 -#define FAULT_FLAG_PREFAULT 0x400/* * The default fault flags that should be used by most of the
@@ -503,8 +501,7 @@ static inline bool fault_flag_allow_retry_first(unsigned int flags) { FAULT_FLAG_USER, "USER" }, \ { FAULT_FLAG_REMOTE, "REMOTE" }, \ { FAULT_FLAG_INSTRUCTION, "INSTRUCTION" }, \ - { FAULT_FLAG_INTERRUPTIBLE, "INTERRUPTIBLE" }, \ - { FAULT_FLAG_PREFAULT, "PREFAULT" } + { FAULT_FLAG_INTERRUPTIBLE, "INTERRUPTIBLE" }/* * vm_fault is filled by the pagefault handler and passed to the vma's
@@ -995,7 +992,7 @@ static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma) }vm_fault_t do_set_pmd (struct vm_fault *vmf,struct page *page); -void do_set_pte (struct vm_fault *vmf,struct page *page); +void do_set_pte (struct vm_fault *vmf,struct page *page,unsigned long addr);vm_fault_t finish_fault (struct vm_fault *vmf);vm_fault_t finish_mkwrite_fault (struct vm_fault *vmf);
mm/filemap.c +7 -14
@@ -3018,8 +3018,7 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,struct file *file = vma->vm_file;struct address_space *mapping = file->f_mapping;pgoff_t last_pgoff = start_pgoff; -unsigned long address = vmf->address; -unsigned long flags = vmf->flags; +unsigned long addr;XA_STATE (xas, &mapping->i_pages, start_pgoff);struct page *head, *page;unsigned int mmap_miss =READ_ONCE (file->f_ra.mmap_miss);
@@ -3035,8 +3034,8 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf, goto out; } - vmf->address = vma->vm_start + ((start_pgoff - vma->vm_pgoff) <<PAGE_SHIFT ); - vmf->pte =pte_offset_map_lock (vma->vm_mm, vmf->pmd, vmf->address, &vmf->ptl); + addr = vma->vm_start + ((start_pgoff - vma->vm_pgoff) <<PAGE_SHIFT ); + vmf->pte =pte_offset_map_lock (vma->vm_mm, vmf->pmd, addr, &vmf->ptl); do { page = find_subpage(head, xas.xa_index); if (PageHWPoison(page))
@@ -3045,7 +3044,7 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,if (mmap_miss >0 ) mmap_miss--; - vmf->address += (xas.xa_index - last_pgoff) <<PAGE_SHIFT ; + addr += (xas.xa_index - last_pgoff) <<PAGE_SHIFT ; vmf->pte += xas.xa_index - last_pgoff; last_pgoff = xas.xa_index;
@@ -3053,16 +3052,12 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf, goto unlock;/* We're about to handle the fault */ -if (vmf->address == address) { - vmf->flags &= ~FAULT_FLAG_PREFAULT ; +if (vmf->address == addr) ret =VM_FAULT_NOPAGE ; - }else { - vmf->flags |=FAULT_FLAG_PREFAULT ; - } -do_set_pte (vmf, page); +do_set_pte (vmf, page, addr);/* no need to invalidate: a not-present page won't be cached */ -update_mmu_cache (vma, vmf->address, vmf->pte); +update_mmu_cache (vma, addr, vmf->pte);unlock_page (head);continue ; unlock:
@@ -3072,8 +3067,6 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,pte_unmap_unlock (vmf->pte, vmf->ptl); out:rcu_read_unlock (); - vmf->flags = flags; - vmf->address = address;WRITE_ONCE (file->f_ra.mmap_miss, mmap_miss);return ret; }
mm/memory.c +5 -5
@@ -3733,11 +3733,11 @@ vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page) }#endif -void do_set_pte (struct vm_fault *vmf,struct page *page) +void do_set_pte (struct vm_fault *vmf,struct page *page,unsigned long addr) {struct vm_area_struct *vma = vmf->vma;bool write = vmf->flags &FAULT_FLAG_WRITE ; -bool prefault = vmf->flags &FAULT_FLAG_PREFAULT ; +bool prefault = vmf->address != addr;pte_t entry;flush_icache_page (vma, page);
@@ -3753,13 +3753,13 @@ void do_set_pte(struct vm_fault *vmf, struct page *page)/* copy-on-write page */ if (write && !(vma->vm_flags &VM_SHARED )) {inc_mm_counter_fast (vma->vm_mm,MM_ANONPAGES ); -page_add_new_anon_rmap (page, vma, vmf->address, false); +page_add_new_anon_rmap (page, vma, addr, false);lru_cache_add_inactive_or_unevictable (page, vma); }else {inc_mm_counter_fast (vma->vm_mm,mm_counter_file (page));page_add_file_rmap (page, false); } -set_pte_at (vma->vm_mm, vmf->address, vmf->pte, entry); +set_pte_at (vma->vm_mm, addr, vmf->pte, entry); }/**
@@ -3819,7 +3819,7 @@ vm_fault_t finish_fault(struct vm_fault *vmf) ret =0 ;/* Re-check under ptl */ if (likely (pte_none (*vmf->pte))) -do_set_pte (vmf, page); +do_set_pte (vmf, page, vmf->address);else ret =VM_FAULT_NOPAGE ;
Signature · 1 line
2.30.0.284.gd98b1dd5eaa7-goog
Will Deacon
maintainerPATCH v4 6/8 mm: Avoid modifying vmf.address in __collapse_huge_page_swapin()
In reply to Will DeaconIn preparation for const-ifying the anonymous struct field of 'struct vm_fault', rework __collapse_huge_page_swapin() to avoid continuously updating vmf.address and instead populate a new 'struct vm_fault' on the stack for each page being processed.
--- mm/khugepaged.c | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-)
mm/khugepaged.c +18 -19
@@ -991,38 +991,41 @@ static int hugepage_vma_revalidate(struct mm_struct *mm, unsigned long address,static bool __collapse_huge_page_swapin (struct mm_struct *mm,struct vm_area_struct *vma, -unsigned long address,pmd_t *pmd, +unsigned long haddr,pmd_t *pmd,int referenced) {int swapped_in =0 ;vm_fault_t ret =0 ; -struct vm_fault vmf = { - .vma = vma, - .address = address, - .flags =FAULT_FLAG_ALLOW_RETRY , - .pmd = pmd, - .pgoff =linear_page_index (vma, address), - }; - - vmf.pte =pte_offset_map (pmd, address); -for (; vmf.address < address +HPAGE_PMD_NR *PAGE_SIZE ; - vmf.pte++, vmf.address +=PAGE_SIZE ) { +unsigned long address, end = haddr + (HPAGE_PMD_NR *PAGE_SIZE ); + +for (address = haddr; address < end; address +=PAGE_SIZE ) { +struct vm_fault vmf = { + .vma = vma, + .address = address, + .pgoff =linear_page_index (vma, haddr), + .flags =FAULT_FLAG_ALLOW_RETRY , + .pmd = pmd, + }; + + vmf.pte =pte_offset_map (pmd, address); vmf.orig_pte = *vmf.pte; -if (!is_swap_pte (vmf.orig_pte)) +if (!is_swap_pte (vmf.orig_pte)) { +pte_unmap (vmf.pte);continue ; + } swapped_in++; ret =do_swap_page (&vmf);/* do_swap_page returns VM_FAULT_RETRY with released mmap_lock */ if (ret &VM_FAULT_RETRY ) {mmap_read_lock (mm); -if (hugepage_vma_revalidate (mm, address, &vmf.vma)) { +if (hugepage_vma_revalidate (mm, haddr, &vma)) {/* vma is no longer available, don't continue to swapin */ trace_mm_collapse_huge_page_swapin (mm, swapped_in, referenced,0 );return false; }/* check if the pmd is still valid */ -if (mm_find_pmd (mm, address) != pmd) { +if (mm_find_pmd (mm, haddr) != pmd) {trace_mm_collapse_huge_page_swapin (mm, swapped_in, referenced,0 );return false; }
@@ -1031,11 +1034,7 @@ static bool __collapse_huge_page_swapin(struct mm_struct *mm,trace_mm_collapse_huge_page_swapin (mm, swapped_in, referenced,0 );return false; } -/* pte is unmapped now, we need to map it */ - vmf.pte =pte_offset_map (pmd, vmf.address); } - vmf.pte--; -pte_unmap (vmf.pte);/* Drain LRU add pagevec to remove extra pin on the swapped in pages */ if (swapped_in)
Signature · 1 line
2.30.0.284.gd98b1dd5eaa7-goog
Will Deacon
maintainerPATCH v4 7/8 mm: Use static initialisers for immutable fields of 'struct vm_fault'
In reply to Will DeaconIn preparation for const-ifying the anonymous struct field of 'struct vm_fault', ensure that it is initialised using static initialisers.
--- mm/shmem.c | 6 +++--- mm/swapfile.c | 11 ++++++----- 2 files changed, 9 insertions(+), 8 deletions(-)
mm/shmem.c +3 -3
@@ -1520,11 +1520,11 @@ static struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp, {struct vm_area_struct pvma;struct page *page; -struct vm_fault vmf; +struct vm_fault vmf = { + .vma = &pvma, + };shmem_pseudo_vma_init (&pvma, info, index); - vmf.vma = &pvma; - vmf.address =0 ; page =swap_cluster_readahead (swap, gfp, &vmf);shmem_pseudo_vma_destroy (&pvma);
mm/swapfile.c +6 -5
@@ -1951,8 +1951,6 @@ static int unuse_pte_range(struct vm_area_struct *vma, pmd_t *pmd, si = swap_info[type]; pte =pte_offset_map (pmd, addr); do { - struct vm_fault vmf; - if (!is_swap_pte(*pte)) continue;
@@ -1968,9 +1966,12 @@ static int unuse_pte_range(struct vm_area_struct *vma, pmd_t *pmd, swap_map = &si->swap_map[offset]; page =lookup_swap_cache (entry, vma, addr);if (!page) { - vmf.vma = vma; - vmf.address = addr; - vmf.pmd = pmd; +struct vm_fault vmf = { + .vma = vma, + .address = addr, + .pmd = pmd, + }; + page =swapin_readahead (entry,GFP_HIGHUSER_MOVABLE , &vmf); }
Signature · 1 line
2.30.0.284.gd98b1dd5eaa7-goog
Re: [PATCH v4 7/8] mm: Use static initialisers for immutable fields of 'struct vm_fault'
Quoted 4 lines · Will Deacon
In preparation for const-ifying the anonymous struct field of 'struct
vm_fault', ensure that it is initialised using static initialisers.
FWIW these are known as "designated initializers", ie.
struct foo my_foo = {
.bar = 42,
};
https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
Quoted 58 lines
Cc Kirill A. Shutemov <[email protected]>Cc Linus Torvalds <[email protected]>Signed-off-by Will Deacon <[email protected]>--- mm/shmem.c | 6 +++--- mm/swapfile.c | 11 ++++++----- 2 files changed, 9 insertions(+), 8 deletions(-)mm/shmem.c +3
-3@@ -1520,11 +1520,11 @@ static struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp, {struct vm_area_struct pvma;struct page *page; -struct vm_fault vmf; +struct vm_fault vmf = { + .vma = &pvma, + };shmem_pseudo_vma_init (&pvma, info, index); - vmf.vma = &pvma; - vmf.address =0 ; page =swap_cluster_readahead (swap, gfp, &vmf);shmem_pseudo_vma_destroy (&pvma);mm/swapfile.c +6
-6@@ -1951,8 +1951,6 @@ static int unuse_pte_range(struct vm_area_struct *vma, pmd_t *pmd, si = swap_info[type]; pte =pte_offset_map (pmd, addr); do { - struct vm_fault vmf; - if (!is_swap_pte(*pte)) continue;@@ -1968,9 +1966,12 @@ static int unuse_pte_range(struct vm_area_struct *vma, pmd_t *pmd, swap_map = &si->swap_map[offset]; page =lookup_swap_cache (entry, vma, addr);if (!page) { - vmf.vma = vma; - vmf.address = addr; - vmf.pmd = pmd; +struct vm_fault vmf = { + .vma = vma, + .address = addr, + .pmd = pmd, + }; + page =swapin_readahead (entry,GFP_HIGHUSER_MOVABLE , &vmf); } --
Quoted 2 lines
2.30.0.284.gd98b1dd5eaa7-goog
--
Thanks,
~Nick Desaulniers
Will Deacon
maintainerRe: [PATCH v4 7/8] mm: Use static initialisers for immutable fields of 'struct vm_fault'
Quoted 6 lines · Nick Desaulniers
Quoted 4 lines · Will Deacon
In preparation for const-ifying the anonymous struct field of 'struct
vm_fault', ensure that it is initialised using static initialisers.
Quoted 7 lines
FWIW these are known as "designated initializers", ie.
struct foo
my_foo= {
.bar = 42,
};
Thanks, I'm useless with terminology. Will update.
Will
Will Deacon
maintainerPATCH v4 8/8 mm: Mark anonymous struct field of 'struct vm_fault' as 'const'
In reply to Will DeaconThe fields of this struct are only ever read after being initialised, so mark it 'const' before somebody tries to modify it again.
--- include/linux/mm.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/mm.h +1 -1
@@ -514,7 +514,7 @@ 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 { +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 */
Signature · 1 line
2.30.0.284.gd98b1dd5eaa7-goog
Re: [PATCH v4 8/8] mm: Mark anonymous struct field of 'struct vm_fault' as 'const'
Quoted 16 lines · Will Deacon
The fields of this struct are only ever read after being initialised, so mark it 'const' before somebody tries to modify it again.
Cc Kirill A. Shutemov <[email protected]>Cc Linus Torvalds <[email protected]>Signed-off-by Will Deacon <[email protected]>--- include/linux/mm.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)include/linux/mm.h +0
-0
include/linux/mm.h static inline bool fault_flag_allow_retry_first(unsigned int flags) · line 520
Quoted 8 lines
include/linux/mm.h +1
-1@@ -514,7 +514,7 @@ 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 { +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 */
Is there a difference between:
+ 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 */
+ };
vs
+ struct {
+ struct vm_area_struct * const vma; /* Target VMA */
+ const gfp_t gfp_mask; /* gfp mask to
be used for allocations */
+ const pgoff_t pgoff; /* Logical page
offset based on vma */
+ const unsigned long address; /* Faulting
virtual address */
+ };
ie. marking the members const vs the anonymous struct? Does anything need to change in the use of these structures?
Quoted 3 lines
--
2.30.0.284.gd98b1dd5eaa7-goog
Signature · 2 lines
Thanks, ~Nick Desaulniers
Re: [PATCH v4 8/8] mm: Mark anonymous struct field of 'struct vm_fault' as 'const'
Quoted 2 lines · Nick Desaulniers
Is there a difference between: [ const unnamed struct and individual const members ]
Semantically? No.
Syntactically the "group the const members together" is a lot cleaner, imho. Not just from a "just a single const" standpoint, but from a "code as documentation" standpoint.
But I guess to avoid the clang issue, we could do the "mark individual fields" thing.
(It turns out that sparse gets this wrong too, so it's not just clang).
Linus
Will Deacon
maintainerRe: [PATCH v4 8/8] mm: Mark anonymous struct field of 'struct vm_fault' as 'const'
Quoted 5 lines · Linus Torvalds
Quoted 2 lines · Nick Desaulniers
Is there a difference between: [ const unnamed struct and individual const members ]
Quoted 8 lines
Semantically? No.
Syntactically the "group the const members together" is a lot cleaner, imho. Not just from a "just a single const" standpoint, but from a "code as documentation" standpoint.
But I guess to avoid the clang issue, we could do the "mark individual fields" thing.
I'd prefer to wait until the bug against LLVM has been resolved before we try to work around anything. Although I couldn't find any other examples like this in the kernel, requiring all of the member fields to be marked as 'const' still feels pretty fragile to me; it's only a matter of time before new non-const fields get added, at which point the temptation for developers to remove 'const' from other fields when it gets in the way is pretty high.
None of this is bullet-proof, of course, but if clang ends up emitting a warning (even if it's gated behind an option) then I think we're in a good place.
Quoted 1 line
(It turns out that sparse gets this wrong too, so it's not just clang).
Adding Luc, as hopefully that's fixable.
Will
Re: [PATCH v4 8/8] mm: Mark anonymous struct field of 'struct vm_fault' as 'const'
Quoted 16 lines · Will Deacon
Quoted 5 lines · Linus Torvalds
Quoted 2 lines · Nick Desaulniers
Is there a difference between: [ const unnamed struct and individual const members ]
Quoted 8 lines
Semantically? No.
Syntactically the "group the const members together" is a lot cleaner, imho. Not just from a "just a single const" standpoint, but from a "code as documentation" standpoint.
But I guess to avoid the clang issue, we could do the "mark individual fields" thing.
Quoted 6 lines
I'd prefer to wait until the bug against LLVM has been resolved before we try to work around anything. Although I couldn't find any other examples like this in the kernel, requiring all of the member fields to be marked as 'const' still feels pretty fragile to me; it's only a matter of time before new non-const fields get added, at which point the temptation for developers to remove 'const' from other fields when it gets in the way is pretty high.
What's to stop a new non-const field from getting added outside the const qualified anonymous struct?
What's to stop someone from removing const from the anonymous struct? What's to stop a number of callers from manipulating the structure temporarily before restoring it when returning by casting away the const?
Code review.
Using a non-toolchain-portable solution certainly could be considered more fragile.
It's always possible that the resolution is the C standards body goes the C++ route, at which point GCC would be forced to address this and potentially change behavior. Kind of like how people avoid going to court since things are never guaranteed to work out in their favor.
Quoted 10 lines
None of this is bullet-proof, of course, but if clang ends up emitting a warning (even if it's gated behind an option) then I think we're in a good place.
Quoted 1 line
(It turns out that sparse gets this wrong too, so it's not just clang).
Adding Luc, as hopefully that's fixable.
Will
--
Thanks,
~Nick Desaulniers
Will Deacon
maintainerRe: [PATCH v4 8/8] mm: Mark anonymous struct field of 'struct vm_fault' as 'const'
Quoted 24 lines · Nick Desaulniers
Quoted 16 lines · Will Deacon
Quoted 5 lines · Linus Torvalds
Quoted 2 lines · Nick Desaulniers
Is there a difference between: [ const unnamed struct and individual const members ]
Quoted 8 lines
Semantically? No.
Syntactically the "group the const members together" is a lot cleaner, imho. Not just from a "just a single const" standpoint, but from a "code as documentation" standpoint.
But I guess to avoid the clang issue, we could do the "mark individual fields" thing.
Quoted 6 lines
I'd prefer to wait until the bug against LLVM has been resolved before we try to work around anything. Although I couldn't find any other examples like this in the kernel, requiring all of the member fields to be marked as 'const' still feels pretty fragile to me; it's only a matter of time before new non-const fields get added, at which point the temptation for developers to remove 'const' from other fields when it gets in the way is pretty high.
Quoted 8 lines
What's to stop a new non-const field from getting added outside the const qualified anonymous struct?
What's to stop someone from removing const from the anonymous struct? What's to stop a number of callers from manipulating the structure temporarily before restoring it when returning by casting away the const?Code review.
Sure, but here we are cleaning up this stuff, so I think review only gets you so far. To me:
const struct {
int foo;
long bar;
};clearly says "don't modify fields of this struct", whereas:
struct {
const int foo;
const long bar;
};says "don't modify foo or bar, but add whatever you like on the end" and that's the slippery slope. So then we end up with the eye-sore of:
const struct {
const int foo;
const long bar;
};and maybe that's the right answer, but I'm just saying we should wait for clang to make up its mind first. It's not like this is a functional problem, and there are enough GCC users around that we're not exactly in a hurry.
Will
Re: [PATCH v4 8/8] mm: Mark anonymous struct field of 'struct vm_fault' as 'const'
Quoted 45 lines · Will Deacon
Quoted 24 lines · Nick Desaulniers
Quoted 16 lines · Will Deacon
Quoted 5 lines · Linus Torvalds
Quoted 2 lines · Nick Desaulniers
Is there a difference between: [ const unnamed struct and individual const members ]
Quoted 8 lines
Semantically? No.
Syntactically the "group the const members together" is a lot cleaner, imho. Not just from a "just a single const" standpoint, but from a "code as documentation" standpoint.
But I guess to avoid the clang issue, we could do the "mark individual fields" thing.
Quoted 6 lines
I'd prefer to wait until the bug against LLVM has been resolved before we try to work around anything. Although I couldn't find any other examples like this in the kernel, requiring all of the member fields to be marked as 'const' still feels pretty fragile to me; it's only a matter of time before new non-const fields get added, at which point the temptation for developers to remove 'const' from other fields when it gets in the way is pretty high.
Quoted 8 lines
What's to stop a new non-const field from getting added outside the const qualified anonymous struct?
What's to stop someone from removing const from the anonymous struct? What's to stop a number of callers from manipulating the structure temporarily before restoring it when returning by casting away the const?Code review.
Sure, but here we are cleaning up this stuff, so I think review only gets you so far. To me:
const struct { int foo; long bar; };clearly says "don't modify fields of this struct", whereas:
Quoted 7 lines
struct { const int foo; const long bar; };says "don't modify foo or bar, but add whatever you like on the end" and that's the slippery slope.
"but you could add additional non-const members on the end" for sure. Though going back to
Quoted 2 lines
Quoted 2 lines
What's to stop a new non-const field from getting added outside the const qualified anonymous struct?
my point with that is that the const anonymous struct is within a non-const anonymous struct.
struct vm_fault {
const {
struct vm_area_struct *vma;
gfp_t gfp_mask;
pgoff_t pgoff;
unsigned long address;
// Your point is about new member additions here, IIUC };
// My point: what's to stop someone from adding a new non-const member here? unsigned int flags;
pmd_t *pmd;
pud_t *pud;
...
// or here?
};
The const anonymous struct will help prevent additions of non-const members to the anonymous struct, sure; but if someone really wanted a new non-const member in a struct vm_fault instance they're just going to go around the const anonymous struct. Or is there something more I'm missing about the order of the members of struct vm_fault?
Quoted 10 lines
So then we end up with the eye-sore of:
const struct { const int foo; const long bar; };and maybe that's the right answer, but I'm just saying we should wait for clang to make up its mind first. It's not like this is a functional problem, and there are enough GCC users around that we're not exactly in a hurry.
Yeah, I mean I'm happy to whip something up for Clang, even though I suspect it will get shot down in code review until there's more guidance from standards bodies. It doesn't hurt to try, and at least have a patch "waiting in the wings" should we hear back from WG14 that favors GCC's behavior. Who knows, maybe the guidance will be that WG21 should revisit this behavior for C++ to avoid divergence with C (as g++ and gcc currently do).
Signature · 2 lines
Thanks, ~Nick Desaulniers
Will Deacon
maintainerRe: [PATCH v4 8/8] mm: Mark anonymous struct field of 'struct vm_fault' as 'const'
Hey Nick,
Quoted 46 lines · Nick Desaulniers
Quoted 12 lines · Will Deacon
Quoted 1 line · Nick Desaulniers
On Thu, Jan 21, 2021 at 5:11 AM Will Deacon <[email protected]> wrote:
Sure, but here we are cleaning up this stuff, so I think review only gets you so far. To me:
const struct { int foo; long bar; };clearly says "don't modify fields of this struct", whereas:
Quoted 7 lines
struct { const int foo; const long bar; };says "don't modify foo or bar, but add whatever you like on the end" and that's the slippery slope.
"but you could add additional non-const members on the end" for sure. Though going back to
Quoted 2 lines
Quoted 2 lines
What's to stop a new non-const field from getting added outside the const qualified anonymous struct?
my point with that is that the const anonymous struct is within a non-const anonymous struct.
struct
vm_fault{const { struct vm_area_struct *vma; gfp_t gfp_mask; pgoff_t pgoff; unsigned long address; // Your point is about new member additions here, IIUC};
// My point: what's to stop someone from adding a new non-const member here? unsigned int flags;
pmd_t*pmd;
pud_t*pud;
...
// or here?
};
Quoted 5 lines
The const anonymous struct will help prevent additions of non-const members to the anonymous struct, sure; but if someone really wanted a new non-const member in a
struct vm_faultinstance they're just going to go around the const anonymous struct. Or is there something more I'm missing about the order of the members of structvm_fault?
All I was trying to say is that, given a struct with a mixture of const and non-const members, I would be less hesitant to remove 'const' from one of the members if it helped me get something else done. Having the 'const' on the struct itself, however, would deter me because at that point its clear that you're not supposed to be modifying this stuff. That's the difference between the "Your point" and "My point" lines above.
But honestly, I can't say I particularly enjoy arguing about these idiosyncracies; I'd just rather wait for the dust to settle with clang before we add code to deal with that outcome.
Quoted 11 lines
Quoted 10 lines
So then we end up with the eye-sore of:
const struct { const int foo; const long bar; };and maybe that's the right answer, but I'm just saying we should wait for clang to make up its mind first. It's not like this is a functional problem, and there are enough GCC users around that we're not exactly in a hurry.
Quoted 7 lines
Yeah, I mean I'm happy to whip something up for Clang, even though I suspect it will get shot down in code review until there's more guidance from standards bodies. It doesn't hurt to try, and at least have a patch "waiting in the wings" should we hear back from WG14 that favors GCC's behavior. Who knows, maybe the guidance will be that WG21 should revisit this behavior for C++ to avoid divergence with C (as g++ and gcc currently do).
I mean, a warning doesn't seem controversial to me, especially as opinions certainly seem to be divided about what the right behaviour should be here.
Will
Re: [PATCH v4 8/8] mm: Mark anonymous struct field of 'struct vm_fault' as 'const'
In reply to Will DeaconQuoted 4 lines · Will Deacon
Quoted 2 lines
(It turns out that sparse gets this wrong too, so it's not just clang).
Adding Luc, as hopefully that's fixable.
I had talked to Luc about this earlier, and he just sent out a fix for sparse. It's not in the git repo yet, but it's coming.
Having looked at what sparse does, I suspect the clang behavior stems from a similar approach to looking up unnamed struct/union members.
And the sparse fix was fairly straightforward: changing the _lookup_ is painful, because that's late and trying to fix types after-the-fact is just not great. But just (recursively) modifying the type modifiers at type parsing time for anonymous struct/union members is quite straightforward, probably in clang too.
So honestly - I think the clang push-back was by somebody who thought it would be nasty to fix, and who was thus actively trying to mis-read the standards language.
I'm not willing to argue with compiler people who do standards language weasel-wording (I've seen it before, my life is too short to deal with those kinds of people), but maybe Nick is more used to deal with clang people.
Nick - I suspect that the sparse type system model is different enough from the clang one that the sparse patch is not really helpful as a "look, this is how it was done in sparse, maybe clang can do something similar", but I'll bounce it to you separately anyway just in case. Maybe your clang knowledge means that you go "yeah, in clang that function is called 'xyz', and we can do something very similar".
Linus
Will Deacon
maintainerQuoted 14 lines · Will Deacon
Hi all,
This is version four of the patches I previously posted here:
v1: https://lore.kernel.org/r/[email protected]
v2: https://lore.kernel.org/r/[email protected]
v3: https://lore.kernel.org/r/[email protected]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].
Quoted 4 lines
The big change since v3 is that the immutable fields of 'struct
vm_fault' now live in a 'const' anonymous struct. Although Clang will silently accept modifications to these fields [3], GCC emits an error. The resulting diffstat is _considerably_ more manageable with this approach.
The only changes I have pending against this series are cosmetic (commit logs). Can I go ahead and queue this in the arm64 tree so that it can sit in linux-next for a bit? (positive or negative feedback appreciated!).
Thanks,
Will
Hugh Dickins
maintainerQuoted 15 lines · Will Deacon
Quoted 14 lines · Will Deacon
Hi all,
This is version four of the patches I previously posted here:
v1: https://lore.kernel.org/r/[email protected]
v2: https://lore.kernel.org/r/[email protected]
v3: https://lore.kernel.org/r/[email protected]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].
Quoted 8 lines
Quoted 4 lines
The big change since v3 is that the immutable fields of 'struct
vm_fault' now live in a 'const' anonymous struct. Although Clang will silently accept modifications to these fields [3], GCC emits an error. The resulting diffstat is _considerably_ more manageable with this approach.The only changes I have pending against this series are cosmetic (commit logs). Can I go ahead and queue this in the arm64 tree so that it can sit in linux-next for a bit? (positive or negative feedback appreciated!).
That would be fine by me: I ran v3 on rc3, then the nicer smaller v4 on rc4, and saw no problems when running either of them (x86_64 only).
Hugh
Will Deacon
maintainerQuoted 21 lines · Hugh Dickins
Quoted 20 lines · Will Deacon
Quoted 18 lines · Will Deacon
Hi all,
This is version four of the patches I previously posted here:
v1: https://lore.kernel.org/r/[email protected]
v2: https://lore.kernel.org/r/[email protected]
v3: https://lore.kernel.org/r/[email protected]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].
The big change since v3 is that the immutable fields of 'struct
vm_fault' now live in a 'const' anonymous struct. Although Clang will silently accept modifications to these fields [3], GCC emits an error. The resulting diffstat is _considerably_ more manageable with this approach.
Quoted 6 lines
Quoted 3 lines
The only changes I have pending against this series are cosmetic (commit logs). Can I go ahead and queue this in the arm64 tree so that it can sit in linux-next for a bit? (positive or negative feedback appreciated!).
That would be fine by me: I ran v3 on rc3, then the nicer smaller v4 on rc4, and saw no problems when running either of them (
x86_64only).
Thanks, Hugh. I'll stick these into -next later today and we'll see how we get on.
Will