Add support for SVM atomics in Nouveau
Subsystems (19)
Alistair Popple
reviewerv7 cover Add support for SVM atomics in Nouveau
This is the seventh version of a series to add support to Nouveau for atomic memory operations on OpenCL shared virtual memory (SVM) regions.
This version primarily improves readability of the Nouveau fault priority calculation code along with other minor functional and cosmetic improvements listed in the changelogs.
Exclusive device access is implemented by adding a new swap entry type (SWAP_DEVICE_EXCLUSIVE) which is similar to a migration entry. The main difference is that on fault the original entry is immediately restored by the fault handler instead of waiting.
Restoring the entry triggers calls to MMU notifers which allows a device driver to revoke the atomic access permission from the GPU prior to the CPU finalising the entry.
Patches 1 & 2 refactor existing migration and device private entry functions.
Patches 3 & 4 rework try_to_unmap_one() by splitting out unrelated functionality into separate functions - try_to_migrate_one() and try_to_munlock_one(). These should not change any functionality, but any help testing would be much appreciated as I have not been able to test every usage of try_to_unmap_one().
Patch 5 contains the bulk of the implementation for device exclusive memory.
Patch 6 contains some additions to the HMM selftests to ensure everything works as expected.
Patch 7 is a cleanup for the Nouveau SVM implementation.
Patch 8 contains the implementation of atomic access for the Nouveau driver.
This has been tested using the latest upstream Mesa userspace with a simple OpenCL test program which checks the results of atomic GPU operations on a SVM buffer whilst also writing to the same buffer from the CPU.
Alistair Popple (8):
mm: Remove special swap entry functions
mm/swapops: Rework swap entry manipulation code
mm/rmap: Split try_to_munlock from try_to_unmap
mm/rmap: Split migration into its own function
mm: Device exclusive memory access
mm: Selftests for exclusive device memory
nouveau/svm: Refactor nouveau_range_fault
nouveau/svm: Implement atomic SVM access
Documentation/vm/hmm.rst | 19 +- arch/s390/mm/pgtable.c | 2 +- drivers/gpu/drm/nouveau/include/nvif/if000c.h | 1 + drivers/gpu/drm/nouveau/nouveau_svm.c | 156 ++++- drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h | 1 + .../drm/nouveau/nvkm/subdev/mmu/vmmgp100.c | 6 + fs/proc/task_mmu.c | 23 +- include/linux/mmu_notifier.h | 26 +- include/linux/rmap.h | 9 +- include/linux/swap.h | 8 +- include/linux/swapops.h | 123 ++-- lib/test_hmm.c | 126 +++- lib/test_hmm_uapi.h | 2 + mm/debug_vm_pgtable.c | 12 +- mm/hmm.c | 12 +- mm/huge_memory.c | 45 +- mm/hugetlb.c | 10 +- mm/memcontrol.c | 2 +- mm/memory.c | 128 +++- mm/migrate.c | 51 +- mm/mprotect.c | 18 +- mm/page_vma_mapped.c | 15 +- mm/rmap.c | 604 +++++++++++++++--- tools/testing/selftests/vm/hmm-tests.c | 158 +++++ 24 files changed, 1282 insertions(+), 275 deletions(-)
Signature · 1 line
2.20.1
Alistair Popple
reviewerPATCH v7 1/8 mm: Remove special swap entry functions
Remove multiple similar inline functions for dealing with different types of special swap entries.
Both migration and device private swap entries use the swap offset to store a pfn. Instead of multiple inline functions to obtain a struct page for each swap entry type use a common function pfn_swap_entry_to_page(). Also open-code the various entry_to_pfn() functions as this results is shorter code that is easier to understand.
---
v7:
* Reworded commit message to include pfn_swap_entry_to_page()
* Added Christoph's Reviewed-by
v6:
* Removed redundant compound_page() call from inside PageLocked()
* Fixed a minor build issue for s390 reported by kernel test bot
v4:
* Added pfn_swap_entry_to_page()
* Reinstated check that migration entries point to locked pages
* Removed #define swapcache_prepare which isn't needed for CONFIG_SWAP=0 builds
--- arch/s390/mm/pgtable.c | 2 +- fs/proc/task_mmu.c | 23 +++++--------- include/linux/swap.h | 4 +-- include/linux/swapops.h | 69 ++++++++++++++--------------------------- mm/hmm.c | 5 ++- mm/huge_memory.c | 4 +-- mm/memcontrol.c | 2 +- mm/memory.c | 10 +++--- mm/migrate.c | 6 ++-- mm/page_vma_mapped.c | 6 ++-- 10 files changed, 50 insertions(+), 81 deletions(-)
arch/s390/mm/pgtable.c +1 -1
@@ -691,7 +691,7 @@ static void ptep_zap_swap_entry(struct mm_struct *mm, swp_entry_t entry)if (!non_swap_entry (entry))dec_mm_counter (mm,MM_SWAPENTS );else if (is_migration_entry (entry)) { -struct page *page =migration_entry_to_page (entry); +struct page *page =pfn_swap_entry_to_page (entry);dec_mm_counter (mm,mm_counter (page)); }
fs/proc/task_mmu.c +8 -15
@@ -514,10 +514,8 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr, }else { mss->swap_pss += (u64 )PAGE_SIZE <<PSS_SHIFT ; } - }else if (is_migration_entry (swpent)) - page =migration_entry_to_page (swpent); -else if (is_device_private_entry (swpent)) - page =device_private_entry_to_page (swpent); + }else if (is_pfn_swap_entry (swpent)) + page =pfn_swap_entry_to_page (swpent); }else if (unlikely (IS_ENABLED (CONFIG_SHMEM ) && mss->check_shmem_swap &&pte_none (*pte))) { page = xa_load(&vma->vm_file->f_mapping->i_pages,
@@ -549,7 +547,7 @@ static void smaps_pmd_entry(pmd_t *pmd, unsigned long addr,swp_entry_t entry =pmd_to_swp_entry (*pmd);if (is_migration_entry (entry)) - page =migration_entry_to_page (entry); + page =pfn_swap_entry_to_page (entry); }if (IS_ERR_OR_NULL (page))return ;
@@ -691,10 +689,8 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask, }else if (is_swap_pte (*pte)) {swp_entry_t swpent =pte_to_swp_entry (*pte); -if (is_migration_entry (swpent)) - page =migration_entry_to_page (swpent); -else if (is_device_private_entry (swpent)) - page =device_private_entry_to_page (swpent); +if (is_pfn_swap_entry (swpent)) + page =pfn_swap_entry_to_page (swpent); }if (page) {int mapcount =page_mapcount (page);
@@ -1383,11 +1379,8 @@ static pagemap_entry_t pte_to_pagemap_entry(struct pagemapread *pm, frame =swp_type (entry) | (swp_offset (entry) <<MAX_SWAPFILES_SHIFT ); flags |=PM_SWAP ; -if (is_migration_entry (entry)) - page =migration_entry_to_page (entry); - -if (is_device_private_entry (entry)) - page =device_private_entry_to_page (entry); +if (is_pfn_swap_entry (entry)) + page =pfn_swap_entry_to_page (entry); }if (page && !PageAnon (page))
@@ -1444,7 +1437,7 @@ static int pagemap_pmd_range(pmd_t *pmdp, unsigned long addr, unsigned long end,if (pmd_swp_soft_dirty (pmd)) flags |=PM_SOFT_DIRTY ;VM_BUG_ON (!is_pmd_migration_entry (pmd)); - page = migration_entry_to_page(entry); + page = pfn_swap_entry_to_page(entry); }#endif
include/linux/swap.h +2 -2
@@ -523,8 +523,8 @@ static inline void show_swap_cache_info(void) { } -#define free_swap_and_cache (e) ({(is_migration_entry(e) || is_device_private_entry(e));}) -#define swapcache_prepare (e) ({(is_migration_entry(e) || is_device_private_entry(e));}) +/* used to sanity check ptes in zap_pte_range when CONFIG_SWAP=0 */ +#define free_swap_and_cache (e) is_pfn_swap_entry(e)static inline int add_swap_count_continuation (swp_entry_t swp,gfp_t gfp_mask) {
include/linux/swapops.h +23 -46
99 lines not on this page · raw message
mm/hmm.c +2 -3
@@ -214,7 +214,7 @@ static inline bool hmm_is_device_private_entry(struct hmm_range *range, swp_entry_t entry) {return is_device_private_entry (entry) && -device_private_entry_to_page (entry)->pgmap->owner = +pfn_swap_entry_to_page (entry)->pgmap->owner = range->dev_private_owner; }
@@ -257,8 +257,7 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr, cpu_flags =HMM_PFN_VALID ;if (is_write_device_private_entry (entry)) cpu_flags |=HMM_PFN_WRITE ; - *hmm_pfn =device_private_entry_to_pfn (entry) | - cpu_flags; + *hmm_pfn =swp_offset (entry) | cpu_flags;return 0 ; }
mm/huge_memory.c +2 -2
@@ -1700,7 +1700,7 @@ int zap_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,VM_BUG_ON (!is_pmd_migration_entry (orig_pmd)); entry =pmd_to_swp_entry (orig_pmd); - page =pfn_to_page (swp_offset (entry)); + page =pfn_swap_entry_to_page (entry); flush_needed =0 ; }else WARN_ONCE (1 ,"Non present huge pmd without pmd migration enabled!" );
@@ -2108,7 +2108,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,swp_entry_t entry; entry =pmd_to_swp_entry (old_pmd); - page =pfn_to_page (swp_offset (entry)); + page =pfn_swap_entry_to_page (entry); write =is_write_migration_entry (entry); young = false; soft_dirty =pmd_swp_soft_dirty (old_pmd);
mm/memcontrol.c +1 -1
@@ -5523,7 +5523,7 @@ static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,* as special swap entry in the CPU page table. */ if (is_device_private_entry (ent)) { - page =device_private_entry_to_page (ent); + page =pfn_swap_entry_to_page (ent);/* * MEMORY_DEVICE_PRIVATE means ZONE_DEVICE page and which have * a refcount of 1 when free (unlike normal page)
mm/memory.c +5 -5
@@ -730,7 +730,7 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm, } rss[MM_SWAPENTS ]++; }else if (is_migration_entry (entry)) { - page =migration_entry_to_page (entry); + page =pfn_swap_entry_to_page (entry); rss[mm_counter (page)]++;
@@ -749,7 +749,7 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,set_pte_at (src_mm, addr, src_pte, pte); } }else if (is_device_private_entry (entry)) { - page =device_private_entry_to_page (entry); + page =pfn_swap_entry_to_page (entry);/* * Update rss count even for unaddressable pages, as
@@ -1286,7 +1286,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, entry =pte_to_swp_entry (ptent);if (is_device_private_entry (entry)) { -struct page *page =device_private_entry_to_page (entry); +struct page *page =pfn_swap_entry_to_page (entry);if (unlikely (details && details->check_mapping)) {/*
@@ -1315,7 +1315,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,else if (is_migration_entry (entry)) {struct page *page; - page =migration_entry_to_page (entry); + page =pfn_swap_entry_to_page (entry); rss[mm_counter (page)]--; }if (unlikely (!free_swap_and_cache (entry)))
@@ -3282,7 +3282,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)migration_entry_wait (vma->vm_mm, vmf->pmd, vmf->address); }else if (is_device_private_entry (entry)) { - vmf->page =device_private_entry_to_page (entry); + vmf->page =pfn_swap_entry_to_page (entry); ret = vmf->page->pgmap->ops->migrate_to_ram (vmf); }else if (is_hwpoison_entry (entry)) { ret =VM_FAULT_HWPOISON ;
mm/migrate.c +3 -3
@@ -321,7 +321,7 @@ void __migration_entry_wait(struct mm_struct *mm, pte_t *ptep,if (!is_migration_entry (entry)) goto out; - page =migration_entry_to_page (entry); + page =pfn_swap_entry_to_page (entry);/* * Once page cache replacement of page migration started, page_count
@@ -361,7 +361,7 @@ void pmd_migration_entry_wait(struct mm_struct *mm, pmd_t *pmd) ptl =pmd_lock (mm, pmd);if (!is_pmd_migration_entry (*pmd)) goto unlock; - page =migration_entry_to_page (pmd_to_swp_entry (*pmd)); + page =pfn_swap_entry_to_page (pmd_to_swp_entry (*pmd));if (!get_page_unless_zero (page)) goto unlock;spin_unlock (ptl);
@@ -2443,7 +2443,7 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,if (!is_device_private_entry (entry)) goto next; - page =device_private_entry_to_page (entry); + page =pfn_swap_entry_to_page (entry);if (!(migrate->flags &MIGRATE_VMA_SELECT_DEVICE_PRIVATE ) || page->pgmap->owner != migrate->pgmap_owner)
mm/page_vma_mapped.c +3 -3
@@ -96,7 +96,7 @@ static bool check_pte(struct page_vma_mapped_walk *pvmw)if (!is_migration_entry (entry))return false; - pfn =migration_entry_to_pfn (entry); + pfn =swp_offset (entry); }else if (is_swap_pte (*pvmw->pte)) {swp_entry_t entry;
@@ -105,7 +105,7 @@ static bool check_pte(struct page_vma_mapped_walk *pvmw)if (!is_device_private_entry (entry))return false; - pfn =device_private_entry_to_pfn (entry); + pfn =swp_offset (entry); }else {if (!pte_present (*pvmw->pte))return false;
@@ -200,7 +200,7 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)if (is_migration_entry (pmd_to_swp_entry (*pvmw->pmd))) {swp_entry_t entry =pmd_to_swp_entry (*pvmw->pmd); -if (migration_entry_to_page (entry) != page) +if (pfn_swap_entry_to_page (entry) != page)return not_found (pvmw);return true; }
Signature · 1 line
2.20.1
Re: [PATCH v7 1/8] mm: Remove special swap entry functions
Quoted 28 lines · Alistair Popple
Remove multiple similar inline functions for dealing with different types of special swap entries.
Both migration and device private swap entries use the swap offset to store a pfn. Instead of multiple inline functions to obtain a struct page for each swap entry type use a common function
pfn_swap_entry_to_page(). Also open-code the variousentry_to_pfn()functions as this results is shorter code that is easier to understand.Signed-off-by Alistair Popple <[email protected]>Reviewed-by Ralph Campbell <[email protected]>Reviewed-by Christoph Hellwig <[email protected]>---
v7:
* Reworded commit message to includepfn_swap_entry_to_page()
* Added Christoph's Reviewed-byv6:
* Removed redundantcompound_page()call from insidePageLocked()
* Fixed a minor build issue for s390 reported by kernel test botv4:
* Addedpfn_swap_entry_to_page()
* Reinstated check that migration entries point to locked pages
* Removed #defineswapcache_preparewhich isn't needed forCONFIG_SWAP=0builds
Quoted 12 lines
--- arch/s390/mm/pgtable.c | 2 +- fs/proc/task_mmu.c | 23 +++++--------- include/linux/swap.h | 4 +-- include/linux/swapops.h | 69 ++++++++++++++--------------------------- mm/hmm.c | 5 ++- mm/huge_memory.c | 4 +-- mm/memcontrol.c | 2 +- mm/memory.c | 10 +++--- mm/migrate.c | 6 ++-- mm/page_vma_mapped.c | 6 ++-- 10 files changed, 50 insertions(+), 81 deletions(-)
Looks good
Quoted 14 lines
mm/hmm.c +1
-1@@ -214,7 +214,7 @@ static inline bool hmm_is_device_private_entry(struct hmm_range *range, swp_entry_t entry) {return is_device_private_entry (entry) && -device_private_entry_to_page (entry)->pgmap->owner = +pfn_swap_entry_to_page (entry)->pgmap->owner = range->dev_private_owner; }
mm/hmm.c static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr, · line 260
Quoted 6 lines
mm/hmm.c +1
-2@@ -257,8 +257,7 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr, cpu_flags =HMM_PFN_VALID ;if (is_write_device_private_entry (entry)) cpu_flags |=HMM_PFN_WRITE ; - *hmm_pfn =device_private_entry_to_pfn (entry) | - cpu_flags; + *hmm_pfn =swp_offset (entry) | cpu_flags;
Though swp_offset() seems poor here
Something like this seems nicer, maybe as an additional patch in this series?
mm/hmm.c +27 -21
@@ -210,14 +210,6 @@ int hmm_vma_handle_pmd(struct mm_walk *walk, unsigned long addr, unsigned long end, unsigned long hmm_pfns[], pmd_t pmd);#endif /* CONFIG_TRANSPARENT_HUGEPAGE */ -static inline bool hmm_is_device_private_entry (struct hmm_range *range, -swp_entry_t entry) -{ -return is_device_private_entry (entry) && -device_private_entry_to_page (entry)->pgmap->owner = - range->dev_private_owner; -} -static inline unsigned long pte_to_hmm_pfn_flags (struct hmm_range *range,pte_t pte) {
@@ -226,6 +218,32 @@ static inline unsigned long pte_to_hmm_pfn_flags(struct hmm_range *range,return pte_write (pte) ? (HMM_PFN_VALID |HMM_PFN_WRITE ) :HMM_PFN_VALID ; } +static bool hmm_pte_handle_device_private (struct hmm_range *range,pte_t pte, +unsigned long *hmm_pfn) +{ +swp_entry_t entry =pte_to_swp_entry (pte); +struct page *device_page; +unsigned long cpu_flags; + +if (is_device_private_entry (entry)) +return false; + +/* +* If the device private page matches the device the caller understands +* then return the private pfn directly. The caller must know what to do +* with it. +*/ + device_page =pfn_swap_entry_to_page (entry); +if (device_page->pgmap->owner != range->dev_private_owner) +return false; + + cpu_flags =HMM_PFN_VALID ; +if (is_write_device_private_entry (entry)) + cpu_flags |=HMM_PFN_WRITE ; + *hmm_pfn =page_to_pfn (device_page) | cpu_flags; +return true; +} +static int hmm_vma_handle_pte (struct mm_walk *walk,unsigned long addr,unsigned long end,pmd_t *pmdp,pte_t *ptep,unsigned long *hmm_pfn)
@@ -247,20 +265,8 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr, }if (!pte_present (pte)) { -swp_entry_t entry =pte_to_swp_entry (pte); - -/* -* Never fault in device private pages, but just report -* the PFN even if not present. -*/ -if (hmm_is_device_private_entry (range, entry)) { - cpu_flags =HMM_PFN_VALID ; -if (is_write_device_private_entry (entry)) - cpu_flags |=HMM_PFN_WRITE ; - *hmm_pfn =device_private_entry_to_pfn (entry) | - cpu_flags; +if (hmm_pte_handle_device_private (range, pte, hmm_pfn))return 0 ; - } required_fault hmm_pte_need_fault(hmm_vma_walk, pfn_req_flags, 0);
Jason
Alistair Popple
reviewerPATCH v7 2/8 mm/swapops: Rework swap entry manipulation code
In reply to Alistair PoppleBoth migration and device private pages use special swap entries that are manipluated by a range of inline functions. The arguments to these are somewhat inconsitent so rework them to remove flag type arguments and to make the arguments similar for both read and write entry creation.
--- include/linux/swapops.h | 56 ++++++++++++++++++++++------------------- mm/debug_vm_pgtable.c | 12 ++++----- mm/hmm.c | 2 +- mm/huge_memory.c | 26 +++++++++++++------ mm/hugetlb.c | 10 +++++--- mm/memory.c | 10 +++++--- mm/migrate.c | 26 ++++++++++++++----- mm/mprotect.c | 10 +++++--- mm/rmap.c | 10 +++++--- 9 files changed, 100 insertions(+), 62 deletions(-)
include/linux/swapops.h +30 -26
@@ -100,35 +100,35 @@ static inline void *swp_to_radix_entry(swp_entry_t entry) }#if IS_ENABLED (CONFIG_DEVICE_PRIVATE ) -static inline swp_entry_t make_device_private_entry (struct page *page,bool write) +static inline swp_entry_t make_readable_device_private_entry (pgoff_t offset) { -return swp_entry (write ?SWP_DEVICE_WRITE :SWP_DEVICE_READ , -page_to_pfn (page)); +return swp_entry (SWP_DEVICE_READ , offset); } -static inline bool is_device_private_entry (swp_entry_t entry) +static inline swp_entry_t make_writable_device_private_entry (pgoff_t offset) { -int type =swp_type (entry); -return type =SWP_DEVICE_READ || type =SWP_DEVICE_WRITE ; +return swp_entry (SWP_DEVICE_WRITE , offset); } -static inline void make_device_private_entry_read (swp_entry_t *entry) +static inline bool is_device_private_entry (swp_entry_t entry) { - *entry =swp_entry (SWP_DEVICE_READ ,swp_offset (*entry)); +int type =swp_type (entry); +return type =SWP_DEVICE_READ || type =SWP_DEVICE_WRITE ; } -static inline bool is_write_device_private_entry (swp_entry_t entry) +static inline bool is_writable_device_private_entry (swp_entry_t entry) {return unlikely (swp_type (entry) =SWP_DEVICE_WRITE ); }#else /* CONFIG_DEVICE_PRIVATE */ -static inline swp_entry_t make_device_private_entry (struct page *page,bool write) +static inline swp_entry_t make_readable_device_private_entry (pgoff_t offset) {return swp_entry (0 ,0 ); } -static inline void make_device_private_entry_read (swp_entry_t *entry) +static inline swp_entry_t make_writable_device_private_entry (pgoff_t offset) { +return swp_entry (0 ,0 ); }static inline bool is_device_private_entry (swp_entry_t entry)
@@ -136,35 +136,32 @@ static inline bool is_device_private_entry(swp_entry_t entry)return false; } -static inline bool is_write_device_private_entry (swp_entry_t entry) +static inline bool is_writable_device_private_entry (swp_entry_t entry) {return false; }#endif /* CONFIG_DEVICE_PRIVATE */ #ifdef CONFIG_MIGRATION -static inline swp_entry_t make_migration_entry (struct page *page,int write) -{ -BUG_ON (!PageLocked (compound_head (page))); - -return swp_entry (write ?SWP_MIGRATION_WRITE :SWP_MIGRATION_READ , -page_to_pfn (page)); -} -static inline int is_migration_entry (swp_entry_t entry) {return unlikely (swp_type (entry) =SWP_MIGRATION_READ ||swp_type (entry) =SWP_MIGRATION_WRITE ); } -static inline int is_write_migration_entry (swp_entry_t entry) +static inline int is_writable_migration_entry (swp_entry_t entry) {return unlikely (swp_type (entry) =SWP_MIGRATION_WRITE ); } -static inline void make_migration_entry_read (swp_entry_t *entry) +static inline swp_entry_t make_readable_migration_entry (pgoff_t offset) { - *entry =swp_entry (SWP_MIGRATION_READ ,swp_offset (*entry)); +return swp_entry (SWP_MIGRATION_READ , offset); +} + +static inline swp_entry_t make_writable_migration_entry (pgoff_t offset) +{ +return swp_entry (SWP_MIGRATION_WRITE , offset); } extern void __migration_entry_wait(struct mm_struct *mm, pte_t *ptep,
@@ -174,21 +171,28 @@ extern void migration_entry_wait(struct mm_struct *mm, pmd_t *pmd,extern void migration_entry_wait_huge (struct vm_area_struct *vma,struct mm_struct *mm,pte_t *pte);#else +static inline swp_entry_t make_readable_migration_entry (pgoff_t offset) +{ +return swp_entry (0 ,0 ); +} + +static inline swp_entry_t make_writable_migration_entry (pgoff_t offset) +{ +return swp_entry (0 ,0 ); +} -#define make_migration_entry (page, write) swp_entry(0, 0)static inline int is_migration_entry (swp_entry_t swp) {return 0 ; } -static inline void make_migration_entry_read (swp_entry_t *entryp) { }static inline void __migration_entry_wait (struct mm_struct *mm,pte_t *ptep,spinlock_t *ptl) { }static inline void migration_entry_wait (struct mm_struct *mm,pmd_t *pmd,unsigned long address) { }static inline void migration_entry_wait_huge (struct vm_area_struct *vma,struct mm_struct *mm,pte_t *pte) { } -static inline int is_write_migration_entry (swp_entry_t entry) +static inline int is_writable_migration_entry (swp_entry_t entry) {return 0 ; }
mm/debug_vm_pgtable.c +6 -6
@@ -817,17 +817,17 @@ static void __init swap_migration_tests(void)* locked, otherwise it stumbles upon a BUG_ON(). */ __SetPageLocked (page); - swp =make_migration_entry (page,1 ); + swp =make_writable_migration_entry (page_to_pfn (page));WARN_ON (!is_migration_entry (swp)); -WARN_ON (!is_write_migration_entry (swp)); +WARN_ON (!is_writable_migration_entry (swp)); -make_migration_entry_read (&swp); + swp =make_readable_migration_entry (swp_offset (swp));WARN_ON (!is_migration_entry (swp)); -WARN_ON (is_write_migration_entry (swp)); +WARN_ON (is_writable_migration_entry (swp)); - swp =make_migration_entry (page,0 ); + swp =make_readable_migration_entry (page_to_pfn (page));WARN_ON (!is_migration_entry (swp)); -WARN_ON (is_write_migration_entry (swp)); +WARN_ON (is_writable_migration_entry (swp));__ClearPageLocked (page);__free_page (page); }
mm/hmm.c +1 -1
@@ -255,7 +255,7 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,*/ if (hmm_is_device_private_entry (range, entry)) { cpu_flags =HMM_PFN_VALID ; -if (is_write_device_private_entry (entry)) +if (is_writable_device_private_entry (entry)) cpu_flags |=HMM_PFN_WRITE ; *hmm_pfn =swp_offset (entry) | cpu_flags;return 0 ;
mm/huge_memory.c +18 -8
@@ -1051,8 +1051,9 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,swp_entry_t entry =pmd_to_swp_entry (pmd);VM_BUG_ON (!is_pmd_migration_entry (pmd)); -if (is_write_migration_entry (entry)) { -make_migration_entry_read (&entry); +if (is_writable_migration_entry (entry)) { + entry =make_readable_migration_entry ( +swp_offset (entry)); pmd =swp_entry_to_pmd (entry);if (pmd_swp_soft_dirty (*src_pmd)) pmd =pmd_swp_mksoft_dirty (pmd);
@@ -1825,13 +1826,14 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,swp_entry_t entry =pmd_to_swp_entry (*pmd);VM_BUG_ON (!is_pmd_migration_entry (*pmd)); -if (is_write_migration_entry (entry)) { +if (is_writable_migration_entry (entry)) {pmd_t newpmd;/* * A protection check is difficult so * just be safe and disable write */ -make_migration_entry_read (&entry); + entry =make_readable_migration_entry ( +swp_offset (entry)); newpmd =swp_entry_to_pmd (entry);if (pmd_swp_soft_dirty (*pmd)) newpmd =pmd_swp_mksoft_dirty (newpmd);
@@ -2109,7 +2111,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd, entry =pmd_to_swp_entry (old_pmd); page =pfn_swap_entry_to_page (entry); - write =is_write_migration_entry (entry); + write =is_writable_migration_entry (entry); young = false; soft_dirty =pmd_swp_soft_dirty (old_pmd); uffd_wp =pmd_swp_uffd_wp (old_pmd);
@@ -2141,7 +2143,12 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,*/ if (freeze || pmd_migration) {swp_entry_t swp_entry; - swp_entry =make_migration_entry (page + i, write); +if (write) + swp_entry =make_writable_migration_entry ( +page_to_pfn (page + i)); +else + swp_entry =make_readable_migration_entry ( +page_to_pfn (page + i)); entry =swp_entry_to_pte (swp_entry);if (soft_dirty) entry =pte_swp_mksoft_dirty (entry);
@@ -2998,7 +3005,10 @@ void set_pmd_migration_entry(struct page_vma_mapped_walk *pvmw, pmdval =pmdp_invalidate (vma, address, pvmw->pmd);if (pmd_dirty (pmdval))set_page_dirty (page); - entry =make_migration_entry (page,pmd_write (pmdval)); +if (pmd_write (pmdval)) + entry =make_writable_migration_entry (page_to_pfn (page)); +else + entry =make_readable_migration_entry (page_to_pfn (page)); pmdswp =swp_entry_to_pmd (entry);if (pmd_soft_dirty (pmdval)) pmdswp =pmd_swp_mksoft_dirty (pmdswp);
@@ -3024,7 +3034,7 @@ void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, struct page *new) pmde =pmd_mkold (mk_huge_pmd (new, vma->vm_page_prot));if (pmd_swp_soft_dirty (*pvmw->pmd)) pmde =pmd_mksoft_dirty (pmde); -if (is_write_migration_entry (entry)) +if (is_writable_migration_entry (entry)) pmde =maybe_pmd_mkwrite (pmde, vma);flush_cache_range (vma, mmun_start, mmun_start +HPAGE_PMD_SIZE );
mm/hugetlb.c +6 -4
@@ -3795,12 +3795,13 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src, is_hugetlb_entry_hwpoisoned(entry))) {swp_entry_t swp_entry =pte_to_swp_entry (entry); -if (is_write_migration_entry (swp_entry) && cow) { +if (is_writable_migration_entry (swp_entry) && cow) {/* * COW mappings require pages in both * parent and child to be set to read. */ -make_migration_entry_read (&swp_entry); + swp_entry =make_readable_migration_entry ( +swp_offset (swp_entry)); entry =swp_entry_to_pte (swp_entry);set_huge_swap_pte_at (src, addr, src_pte, entry, sz);
@@ -4970,10 +4971,11 @@ unsigned long hugetlb_change_protection(struct vm_area_struct *vma,if (unlikely (is_hugetlb_entry_migration (pte))) {swp_entry_t entry =pte_to_swp_entry (pte); -if (is_write_migration_entry (entry)) { +if (is_writable_migration_entry (entry)) {pte_t newpte; -make_migration_entry_read (&entry); + entry =make_readable_migration_entry ( +swp_offset (entry)); newpte =swp_entry_to_pte (entry);set_huge_swap_pte_at (mm, address, ptep, newpte,huge_page_size (h));
mm/memory.c +6 -4
@@ -734,13 +734,14 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm, rss[mm_counter (page)]++; -if (is_write_migration_entry (entry) && +if (is_writable_migration_entry (entry) &&is_cow_mapping (vm_flags)) {/* * COW mappings require pages in both * parent and child to be set to read. */ -make_migration_entry_read (&entry); + entry =make_readable_migration_entry ( +swp_offset (entry)); pte =swp_entry_to_pte (entry);if (pte_swp_soft_dirty (*src_pte)) pte =pte_swp_mksoft_dirty (pte);
@@ -771,9 +772,10 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,* when a device driver is involved (you cannot easily * save and restore device driver state). */ -if (is_write_device_private_entry (entry) && +if (is_writable_device_private_entry (entry) &&is_cow_mapping (vm_flags)) { -make_device_private_entry_read (&entry); + entry =make_readable_device_private_entry ( +swp_offset (entry)); pte =swp_entry_to_pte (entry);if (pte_swp_uffd_wp (*src_pte)) pte =pte_swp_mkuffd_wp (pte);
mm/migrate.c +20 -6
@@ -237,13 +237,18 @@ static bool remove_migration_pte(struct page *page, struct vm_area_struct *vma,* Recheck VMA as permissions can change since migration started */ entry =pte_to_swp_entry (*pvmw.pte); -if (is_write_migration_entry (entry)) +if (is_writable_migration_entry (entry)) pte =maybe_mkwrite (pte, vma);else if (pte_swp_uffd_wp (*pvmw.pte)) pte =pte_mkuffd_wp (pte);if (unlikely (is_device_private_page (new))) { - entry =make_device_private_entry (new,pte_write (pte)); +if (pte_write (pte)) + entry =make_writable_device_private_entry ( +page_to_pfn (new)); +else + entry =make_readable_device_private_entry ( +page_to_pfn (new)); pte =swp_entry_to_pte (entry);if (pte_swp_soft_dirty (*pvmw.pte)) pte =pte_swp_mksoft_dirty (pte);
@@ -2451,7 +2456,7 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp, mpfn =migrate_pfn (page_to_pfn (page)) |MIGRATE_PFN_MIGRATE ; -if (is_write_device_private_entry (entry)) +if (is_writable_device_private_entry (entry)) mpfn |=MIGRATE_PFN_WRITE ; }else {if (!(migrate->flags &MIGRATE_VMA_SELECT_SYSTEM ))
@@ -2497,8 +2502,12 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,ptep_get_and_clear (mm, addr, ptep);/* Setup special migration page table entry */ - entry =make_migration_entry (page, mpfn & -MIGRATE_PFN_WRITE ); +if (mpfn &MIGRATE_PFN_WRITE ) + entry =make_writable_migration_entry ( +page_to_pfn (page)); +else + entry =make_readable_migration_entry ( +page_to_pfn (page)); swp_pte =swp_entry_to_pte (entry);if (pte_present (pte)) {if (pte_soft_dirty (pte))
@@ -2971,7 +2980,12 @@ static void migrate_vma_insert_page(struct migrate_vma *migrate,if (is_device_private_page (page)) {swp_entry_t swp_entry; - swp_entry =make_device_private_entry (page, vma->vm_flags &VM_WRITE ); +if (vma->vm_flags &VM_WRITE ) + swp_entry =make_writable_device_private_entry ( +page_to_pfn (page)); +else + swp_entry =make_readable_device_private_entry ( +page_to_pfn (page)); entry =swp_entry_to_pte (swp_entry); } }else {
mm/mprotect.c +6 -4
@@ -143,23 +143,25 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd,swp_entry_t entry =pte_to_swp_entry (oldpte);pte_t newpte; -if (is_write_migration_entry (entry)) { +if (is_writable_migration_entry (entry)) {/* * A protection check is difficult so * just be safe and disable write */ -make_migration_entry_read (&entry); + entry =make_readable_migration_entry ( +swp_offset (entry)); newpte =swp_entry_to_pte (entry);if (pte_swp_soft_dirty (oldpte)) newpte =pte_swp_mksoft_dirty (newpte);if (pte_swp_uffd_wp (oldpte)) newpte =pte_swp_mkuffd_wp (newpte); - }else if (is_write_device_private_entry (entry)) { + }else if (is_writable_device_private_entry (entry)) {/* * We do not preserve soft-dirtiness. See * copy_one_pte() for explanation. */ -make_device_private_entry_read (&entry); + entry =make_readable_device_private_entry ( +swp_offset (entry)); newpte =swp_entry_to_pte (entry);if (pte_swp_uffd_wp (oldpte)) newpte =pte_swp_mkuffd_wp (newpte);
mm/rmap.c +7 -3
@@ -1526,7 +1526,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,* pte. do_swap_page() will wait until the migration * pte is removed and then restart fault handling. */ - entry =make_migration_entry (page,0 ); + entry =make_readable_migration_entry (page_to_pfn (page)); swp_pte =swp_entry_to_pte (entry);/*
@@ -1622,8 +1622,12 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,* pte. do_swap_page() will wait until the migration * pte is removed and then restart fault handling. */ - entry =make_migration_entry (subpage, -pte_write (pteval)); +if (pte_write (pteval)) + entry =make_writable_migration_entry ( +page_to_pfn (subpage)); +else + entry =make_readable_migration_entry ( +page_to_pfn (subpage)); swp_pte =swp_entry_to_pte (entry);if (pte_soft_dirty (pteval)) swp_pte =pte_swp_mksoft_dirty (swp_pte);
Signature · 1 line
2.20.1
Alistair Popple
reviewerPATCH v7 3/8 mm/rmap: Split try_to_munlock from try_to_unmap
In reply to Alistair PoppleThe behaviour of try_to_unmap_one() is difficult to follow because it performs different operations based on a fairly large set of flags used in different combinations.
TTU_MUNLOCK is one such flag. However it is exclusively used by try_to_munlock() which specifies no other flags. Therefore rather than overload try_to_unmap_one() with unrelated behaviour split this out into it's own function and remove the flag.
---
v7:
* Added Christoph's Reviewed-by
v4:
* Removed redundant check for VM_LOCKED
--- include/linux/rmap.h | 1 - mm/rmap.c | 40 ++++++++++++++++++++++++++++++++-------- 2 files changed, 32 insertions(+), 9 deletions(-)
include/linux/rmap.h +0 -1
@@ -87,7 +87,6 @@ struct anon_vma_chain {enum ttu_flags {TTU_MIGRATION =0x1 ,/* migration mode */ -TTU_MUNLOCK =0x2 ,/* munlock mode */ TTU_SPLIT_HUGE_PMD =0x4 ,/* split huge PMD if any */ TTU_IGNORE_MLOCK =0x8 ,/* ignore mlock */
mm/rmap.c +32 -8
@@ -1405,10 +1405,6 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,struct mmu_notifier_range range;enum ttu_flags flags = (enum ttu_flags )(long )arg; -/* munlock has nothing to gain from examining un-locked vmas */ -if ((flags &TTU_MUNLOCK ) && !(vma->vm_flags &VM_LOCKED )) -return true; -if (IS_ENABLED (CONFIG_MIGRATION ) && (flags &TTU_MIGRATION ) &&is_zone_device_page (page) && !is_device_private_page (page))return true;
@@ -1469,8 +1465,6 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,page_vma_mapped_walk_done (&pvmw);break ; } -if (flags &TTU_MUNLOCK ) -continue ; }/* Unexpected PMD-mapped THP? */
@@ -1784,6 +1778,37 @@ bool try_to_unmap(struct page *page, enum ttu_flags flags)return !page_mapcount (page) ? true : false; } +static bool try_to_munlock_one (struct page *page,struct vm_area_struct *vma, +unsigned long address,void *arg) +{ +struct page_vma_mapped_walk pvmw = { + .page = page, + .vma = vma, + .address = address, + }; + +/* munlock has nothing to gain from examining un-locked vmas */ +if (!(vma->vm_flags &VM_LOCKED )) +return true; + +while (page_vma_mapped_walk (&pvmw)) { +/* PTE-mapped THP are never mlocked */ +if (!PageTransCompound (page)) { +/* +* Holding pte lock, we do *not* need +* mmap_lock here +*/ +mlock_vma_page (page); + } +page_vma_mapped_walk_done (&pvmw); + +/* found a mlocked page, no point continuing munlock check */ +return false; + } + +return true; +} +/** * try_to_munlock - try to munlock a page * @page: the page to be munlocked
@@ -1796,8 +1821,7 @@ bool try_to_unmap(struct page *page, enum ttu_flags flags)void try_to_munlock (struct page *page) {struct rmap_walk_control rwc = { - .rmap_one = try_to_unmap_one, - .arg = (void *)TTU_MUNLOCK , + .rmap_one = try_to_munlock_one, .done = page_not_mapped, .anon_lock = page_lock_anon_vma_read,
Signature · 1 line
2.20.1
Jason Gunthorpe
maintainerRe: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap
mm/rmap.c bool try_to_unmap(struct page *page, enum ttu_flags flags) · line 1783
Quoted 3 lines · Alistair Popple
mm/rmap.c +3
-0+static bool try_to_munlock_one (struct page *page,struct vm_area_struct *vma, +unsigned long address,void *arg) +{
Is this function name right?
Quoted 8 lines
mm/rmap.c +8
-0+struct page_vma_mapped_walk pvmw = { + .page = page, + .vma = vma, + .address = address, + }; + +/* munlock has nothing to gain from examining un-locked vmas */ +if (!(vma->vm_flags &VM_LOCKED ))
mm/rmap.c bool try_to_unmap(struct page *page, enum ttu_flags flags) · line 1801
Quoted 10 lines
mm/rmap.c +10
-0+return true; + +while (page_vma_mapped_walk (&pvmw)) { +/* PTE-mapped THP are never mlocked */ +if (!PageTransCompound (page)) { +/* +* Holding pte lock, we do *not* need +* mmap_lock here +*/ +mlock_vma_page (page);
Because the only action this function seems to take is to call *mlock*_vma_page()
Quoted 14 lines
mm/rmap.c +10
-0+ } +page_vma_mapped_walk_done (&pvmw); + +/* found a mlocked page, no point continuing munlock check */ +return false; + } + +return true; +} +/** * try_to_munlock - try to munlock a page * @page: the page to be munlocked
mm/rmap.c bool try_to_unmap(struct page *page, enum ttu_flags flags) · line 1822
Quoted 2 lines
mm/rmap.c +0
-0@@ -1796,8 +1821,7 @@ bool try_to_unmap(struct page *page, enum ttu_flags flags)void try_to_munlock (struct page *page) {
But this is also called try_to_munlock ??
/**
* try_to_munlock - try to munlock a page
* @page: the page to be munlocked
*
* Called from munlock code. Checks all of the VMAs mapping the page
* to make sure nobody else has this page mlocked. The page will be
* returned with PG_mlocked cleared if no other vmas have it mlocked. */
So what clears PG_mlocked on this call path?
Something needs attention here..
Jason
Alistair Popple
reviewerRe: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap
Quoted 3 lines · AEDT Jason Gunthorpe
Quoted 1 line · Alistair Popple
+static bool try_to_munlock_one(struct page *page, struct
vm_area_struct
*vma,
Quoted 4 lines
mm/rmap.c bool try_to_unmap(struct page *page, enum ttu_flags flags) · line 1783
Quoted 2 lines
mm/rmap.c +2
-0+ unsigned long address, void *arg) +{Is this function name right?
Perhaps. This is called from try_to_munlock() hence the name, but see below for some commentary on that naming.
Quoted 22 lines
Quoted 8 lines
mm/rmap.c +8
-0+struct page_vma_mapped_walk pvmw = { + .page = page, + .vma = vma, + .address = address, + }; + +/* munlock has nothing to gain from examining un-locked vmas */ +if (!(vma->vm_flags &VM_LOCKED ))mm/rmap.c bool try_to_unmap(struct page *page, enum ttu_flags flags) · line 1801
Quoted 10 lines
mm/rmap.c +10
-0+return true; + +while (page_vma_mapped_walk (&pvmw)) { +/* PTE-mapped THP are never mlocked */ +if (!PageTransCompound (page)) { +/* +* Holding pte lock, we do *not* need +* mmap_lock here +*/ +mlock_vma_page (page);Because the only action this function seems to take is to call *mlock*_vma_page()
Quoted 14 lines
mm/rmap.c bool try_to_unmap(struct page *page, enum ttu_flags
Quoted 14 lines
mm/rmap.c +10
-0+ } +page_vma_mapped_walk_done (&pvmw); + +/* found a mlocked page, no point continuing munlock check */ +return false; + } + +return true; +} +/** * try_to_munlock - try to munlock a page * @page: the page to be munlocked @@ -1796,8 +1821,7 @@ bool try_to_unmap(struct page *page, enum ttu_flags
flags)
Quoted 4 lines
mm/rmap.c bool try_to_unmap(struct page *page, enum ttu_flags flags) · line 1822
Quoted 2 lines
mm/rmap.c +0
-0void try_to_munlock (struct page *page) {But this is also called
try_to_munlock??
As far as I can tell this has always been called try_to_munlock() even though it appears to do the opposite.
Quoted 8 lines
/**
*try_to_munlock- try to munlock a page
* @page: the page to be munlocked
*
* Called from munlock code. Checks all of the VMAs mapping the page
* to make sure nobody else has this page mlocked. The page will be
* returned withPG_mlockedcleared if no other vmas have it mlocked. */
In other words it sets PG_mlocked if one or more vmas has it mlocked. So try_to_mlock() might be a better name, except that seems to have the potential for confusion as well because it's only called from the munlock code path and never for mlock.
Quoted 1 line
So what clears
PG_mlockedon this call path?
See munlock_vma_page(). munlock works by clearing PG_mlocked, then calling try_to_munlock to check if any VMAs still need it locked in which case PG_mlocked gets set again. There are no other callers of try_to_munlock().
Quoted 1 line
Something needs attention here..
I think the code is correct, but perhaps the naming could be better. Would be interested hearing any thoughts on renaming try_to_munlock() to try_to_mlock() as the current name appears based on the context it is called from (munlock) rather than what it does (mlock).
- Alistair
Quoted 2 lines
Jason
Alistair Popple
reviewerRe: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap
Quoted 2 lines · AEDT Alistair Popple
Quoted 1 line · AEDT Jason Gunthorpe
On Fri, Mar 26, 2021 at 11:08:00AM +1100, Alistair Popple wrote:
<snip>
Quoted 6 lines
Quoted 1 line
So what clears
PG_mlockedon this call path?See
munlock_vma_page(). munlock works by clearingPG_mlocked, then callingtry_to_munlockto check if any VMAs still need it locked in which casePG_mlockedgets set again. There are no other callers oftry_to_munlock().
Quoted 6 lines
Quoted 1 line
Something needs attention here..
I think the code is correct, but perhaps the naming could be better. Would be interested hearing any thoughts on renaming
try_to_munlock()totry_to_mlock()as the current name appears based on the context it is called from (munlock) rather than what it does (mlock).
Actually Documentation/vm/unevictable-lru.rst contains a better suggestion:
try_to_munlock() Reverse Map Scan
---------------------------------
.. warning::
[!] TODO/FIXME: a better name might be page_mlocked() - analogous to the page_referenced() reverse map walker.
Thoughts on renaming try_to_unlock() -> page_mlocked() and try_to_munlock_one() -> page_mlock_one()?
Quoted 14 lines
- Alistair
Quoted 2 lines
Jason
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Jason Gunthorpe
maintainerRe: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap
In reply to Alistair PoppleQuoted 9 lines · Alistair Popple
Quoted 1 line
Review bool try_to_unmap(struct page *page, enum ttu_flags
Quoted 1 line
(quoted diff) +0
-0@@ -1796,8 +1821,7 @@ bool try_to_unmap(struct page *page, enum ttu_flagsflags)
Quoted 4 lines
mm/rmap.c bool try_to_unmap(struct page *page, enum ttu_flags flags) · line 1822
Quoted 2 lines
mm/rmap.c +0
-0void try_to_munlock (struct page *page) {But this is also called
try_to_munlock??As far as I can tell this has always been called
try_to_munlock()even though it appears to do the opposite.
Maybe we should change it then?
Quoted 9 lines
Quoted 8 lines
/**
*try_to_munlock- try to munlock a page
* @page: the page to be munlocked
*
* Called from munlock code. Checks all of the VMAs mapping the page
* to make sure nobody else has this page mlocked. The page will be
* returned withPG_mlockedcleared if no other vmas have it mlocked. */
Quoted 4 lines
In other words it sets
PG_mlockedif one or more vmas has it mlocked. Sotry_to_mlock()might be a better name, except that seems to have the potential for confusion as well because it's only called from the munlock code path and never for mlock.
That explanation makes more sense.. This function looks like it is 'set PG_mlocked of the page if any vm->flags has VM_LOCKED'
Maybe call it check_vm_locked or something then and reword the above comment?
(and why is it OK to read vm->flags for this without any locking?)
Quoted 6 lines
Quoted 1 line
Something needs attention here..
I think the code is correct, but perhaps the naming could be better. Would be interested hearing any thoughts on renaming
try_to_munlock()totry_to_mlock()as the current name appears based on the context it is called from (munlock) rather than what it does (mlock).
The point of this patch is to make it clearer, after all, so I'd change something and maybe slightly clarify the comment.
Jason
Re: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap
On 3/30/21 3:24 PM, Jason Gunthorpe wrote:
...
Quoted 29 lines
Quoted 2 lines
As far as I can tell this has always been called
try_to_munlock()even though it appears to do the opposite.Maybe we should change it then?
Quoted 9 lines
Quoted 8 lines
/**
*try_to_munlock- try to munlock a page
* @page: the page to be munlocked
*
* Called from munlock code. Checks all of the VMAs mapping the page
* to make sure nobody else has this page mlocked. The page will be
* returned withPG_mlockedcleared if no other vmas have it mlocked. */Quoted 4 lines
In other words it sets
PG_mlockedif one or more vmas has it mlocked. Sotry_to_mlock()might be a better name, except that seems to have the potential for confusion as well because it's only called from the munlock code path and never for mlock.That explanation makes more sense.. This function looks like it is 'set
PG_mlockedof the page if anyvm->flagshasVM_LOCKED'Maybe call it
check_vm_lockedor something then and reword the above comment?(and why is it OK to read
vm->flagsfor this without any locking?)Quoted 2 lines
Quoted 1 line
Something needs attention here..
Quoted 8 lines
Quoted 4 lines
I think the code is correct, but perhaps the naming could be better. Would be interested hearing any thoughts on renaming
try_to_munlock()totry_to_mlock()as the current name appears based on the context it is called from (munlock) rather than what it does (mlock).The point of this patch is to make it clearer, after all, so I'd change something and maybe slightly clarify the comment.
I'd add that, after looking around the calling code, this is a really unhappy pre-existing situation. Anyone reading this has to remember at which point in the call stack the naming transitions from "do the opposite of what the name says", to "do what the name says".
+1 for renaming "munlock*" items to "mlock*", where applicable. good grief.
Although, it seems reasonable to tack such renaming patches onto the tail end of this series. But whatever works.
thanks,
Signature · 2 lines
John Hubbard NVIDIA
Alistair Popple
reviewerRe: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap
Quoted 3 lines · AEDT John Hubbard
On 3/30/21 3:24 PM, Jason Gunthorpe wrote:
...Quoted 1 line
Quoted 1 line
As far as I can tell this has always been called
try_to_munlock()even
though
Quoted 13 lines
Quoted 13 lines
Quoted 1 line
it appears to do the opposite.
Maybe we should change it then?
Quoted 9 lines
Quoted 8 lines
/**
*try_to_munlock- try to munlock a page
* @page: the page to be munlocked
*
* Called from munlock code. Checks all of the VMAs mapping the page
* to make sure nobody else has this page mlocked. The page will be
* returned withPG_mlockedcleared if no other vmas have it mlocked. */
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
In other words it sets
PG_mlockedif one or more vmas has it mlocked. Sotry_to_mlock()might be a better name, except that seems to have the
potential
Quoted 1 line
Quoted 1 line
Quoted 1 line
for confusion as well because it's only called from the munlock code path
and
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
never for mlock.
That explanation makes more sense.. This function looks like it is 'set
PG_mlockedof the page if anyvm->flagshasVM_LOCKED'
Quoted 8 lines
Quoted 8 lines
Maybe call it
check_vm_lockedor something then and reword the above comment?(and why is it OK to read
vm->flagsfor this without any locking?)Quoted 3 lines
Quoted 1 line
Something needs attention here..
I think the code is correct, but perhaps the naming could be better.
Would be
Quoted 1 line
Quoted 1 line
Quoted 1 line
interested hearing any thoughts on renaming
try_to_munlock()to
try_to_mlock()
Quoted 1 line
Quoted 1 line
Quoted 1 line
as the current name appears based on the context it is called from
(munlock)
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
rather than what it does (mlock).
The point of this patch is to make it clearer, after all, so I'd change something and maybe slightly clarify the comment.
Yep, agree with that.
Quoted 1 line
I'd add that, after looking around the calling code, this is a really
unhappy
Quoted 1 line
pre-existing situation. Anyone reading this has to remember at which point
in the
Quoted 1 line
call stack the naming transitions from "do the opposite of what the name
says",
Quoted 3 lines
to "do what the name says".
+1 for renaming "munlock*" items to "mlock*", where applicable. good grief.
At least the situation was weird enough to prompt further investigation :)
Renaming to mlock* doesn't feel like the right solution to me either though. I am not sure if you saw me responding to myself earlier but I am thinking renaming try_to_munlock() -> page_mlocked() and try_to_munlock_one() -> page_mlock_one() might be better. Thoughts?
This is actually inspired from a suggestion in Documentation/vm/unevictable- lru.rst which warns about this problem:
try_to_munlock() Reverse Map Scan
---------------------------------
.. warning::
[!] TODO/FIXME: a better name might be page_mlocked() - analogous to the page_referenced() reverse map walker.
Quoted 1 line
Although, it seems reasonable to tack such renaming patches onto the tail
end
Quoted 1 line
of this series. But whatever works.
Unless anyone objects strongly I will roll the rename into this patch as there is only one caller of try_to_munlock.
- Alistair
Quoted 2 lines
thanks,
Re: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap
On 3/30/21 3:56 PM, Alistair Popple wrote:
...
Quoted 9 lines
Quoted 1 line
+1 for renaming "munlock*" items to "mlock*", where applicable. good grief.
At least the situation was weird enough to prompt further investigation :)
Renaming to mlock* doesn't feel like the right solution to me either though. I am not sure if you saw me responding to myself earlier but I am thinking renaming
try_to_munlock()->page_mlocked()andtry_to_munlock_one()->page_mlock_one()might be better. Thoughts?
Quite confused by this naming idea. Because: try_to_munlock() returns void, so a boolean-style name such as "page_mlocked()" is already not a good fit.
Even more important, though, is that try_to_munlock() is mlock-ing the page, right? Is there some subtle point I'm missing? It really is doing an mlock to the best of my knowledge here. Although the kerneldoc comment for try_to_munlock() seems questionable too:
/**
* try_to_munlock - try to munlock a page
* @page: the page to be munlocked
*
* Called from munlock code. Checks all of the VMAs mapping the page
* to make sure nobody else has this page mlocked. The page will be
* returned with PG_mlocked cleared if no other vmas have it mlocked. */
...because I don't see where, in *this* routine, it clears PG_mlocked!
Obviously we agree that a routine should be named based on what it does, rather than on who calls it. So I think that still leads to:
try_to_munlock() --> try_to_mlock()
try_to_munlock_one() --> try_to_mlock_one()Sorry if I'm missing something really obvious.
Quoted 10 lines
This is actually inspired from a suggestion in
Documentation/vm/unevictable-lru.rstwhich warns about this problem:
try_to_munlock()Reverse Map Scan
---------------------------------.. warning::
[!] TODO/FIXME: a better name might bepage_mlocked()- analogous to thepage_referenced()reverse map walker.
This is actually rather bad advice! page_referenced() returns an int-that-is-really-a-boolean, whereas try_to_munlock(), at least as it stands now, returns void. Usually when I'm writing a TODO item, I'm in a hurry, and I think that's what probably happened here, too. :)
Quoted 9 lines
Quoted 1 line
Although, it seems reasonable to tack such renaming patches onto the tail
end
Quoted 1 line
of this series. But whatever works.
Unless anyone objects strongly I will roll the rename into this patch as there is only one caller of
try_to_munlock.- Alistair
No objections here. :)
thanks,
Signature · 2 lines
John Hubbard NVIDIA
Re: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap
Quoted 32 lines · John Hubbard
On 3/30/21 3:56 PM, Alistair Popple wrote:
...Quoted 9 lines
Quoted 1 line
+1 for renaming "munlock*" items to "mlock*", where applicable. good grief.
At least the situation was weird enough to prompt further investigation :)
Renaming to mlock* doesn't feel like the right solution to me either though. I am not sure if you saw me responding to myself earlier but I am thinking renaming
try_to_munlock()->page_mlocked()andtry_to_munlock_one()->page_mlock_one()might be better. Thoughts?Quite confused by this naming idea. Because:
try_to_munlock()returns void, so a boolean-style name such as "page_mlocked()" is already not a good fit.Even more important, though, is that
try_to_munlock()is mlock-ing the page, right? Is there some subtle point I'm missing? It really is doing an mlock to the best of my knowledge here. Although the kerneldoc comment fortry_to_munlock()seems questionable too:/**
*try_to_munlock- try to munlock a page
* @page: the page to be munlocked
*
* Called from munlock code. Checks all of the VMAs mapping the page
* to make sure nobody else has this page mlocked. The page will be
* returned withPG_mlockedcleared if no other vmas have it mlocked. */...because I don't see where, in *this* routine, it clears
PG_mlocked!
Quoted 7 lines
Obviously we agree that a routine should be named based on what it does, rather than on who calls it. So I think that still leads to:
try_to_munlock()-->try_to_mlock()
try_to_munlock_one()-->try_to_mlock_one()Sorry if I'm missing something really obvious.
Actually, re-reading your and Jason's earlier points in the thread, I see that I'm *not* missing anything, and we are actually in agreement about how the code operates. OK, good!
Also, as you point out above, maybe the "try_" prefix is not really accurate either, given how this works. So maybe we have arrived at something like:
try_to_munlock() --> page_mlock() // or mlock_page()...
try_to_munlock_one() --> page_mlock_one()thanks,
Signature · 2 lines
John Hubbard NVIDIA
Alistair Popple
reviewerRe: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap
In reply to John HubbardQuoted 3 lines · AEDT John Hubbard
On 3/30/21 3:56 PM, Alistair Popple wrote:
...Quoted 1 line
Quoted 1 line
+1 for renaming "munlock*" items to "mlock*", where applicable. good
grief.
Quoted 4 lines
Quoted 4 lines
At least the situation was weird enough to prompt further investigation :)
Renaming to mlock* doesn't feel like the right solution to me either
though. I
Quoted 5 lines
Quoted 4 lines
am not sure if you saw me responding to myself earlier but I am thinking renaming
try_to_munlock()->page_mlocked()andtry_to_munlock_one()->page_mlock_one()might be better. Thoughts?
Quoted 8 lines
Quite confused by this naming idea. Because:
try_to_munlock()returns void, so a boolean-style name such as "page_mlocked()" is already not a good fit.Even more important, though, is that
try_to_munlock()is mlock-ing the page, right? Is there some subtle point I'm missing? It really is doing an mlock to the best of my knowledge here. Although the kerneldoc comment fortry_to_munlock()seems questionable too:
It's mlocking the page if it turns out it still needs to be locked after unlocking it. But I don't think you're missing anything.
Quoted 11 lines
/**
*try_to_munlock- try to munlock a page
* @page: the page to be munlocked
*
* Called from munlock code. Checks all of the VMAs mapping the page
* to make sure nobody else has this page mlocked. The page will be
* returned withPG_mlockedcleared if no other vmas have it mlocked. */...because I don't see where, in *this* routine, it clears
PG_mlocked!
Quoted 7 lines
Obviously we agree that a routine should be named based on what it does, rather than on who calls it. So I think that still leads to:
try_to_munlock() --> try_to_mlock() try_to_munlock_one() --> try_to_mlock_one()Sorry if I'm missing something really obvious.
Nope, I confused things somewhat by blindly quoting the documentation whilst forgetting that try_to_munlock() returns void rather than a bool.
Quoted 1 line
Quoted 1 line
This is actually inspired from a suggestion in
Documentation/vm/
unevictable-
Quoted 7 lines
Quoted 7 lines
lru.rstwhich warns about this problem:
try_to_munlock()Reverse Map Scan
---------------------------------.. warning::
[!] TODO/FIXME: a better name might bepage_mlocked()- analogous to
the
Quoted 7 lines
Quoted 2 lines
page_referenced()reverse map walker.This is actually rather bad advice!
page_referenced()returns an int-that-is-really-a-boolean, whereastry_to_munlock(), at least as it stands now, returns void. Usually when I'm writing a TODO item, I'm in a hurry, and I think that's what probably happened here, too. :)
So I think we're in agreement. The naming is bad and the advice in the documentation is also questionable :-)
Thanks for the thoughts, I will re-send this with naming and documentation updates.
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
Although, it seems reasonable to tack such renaming patches onto the tail
end
Quoted 1 line
of this series. But whatever works.
Unless anyone objects strongly I will roll the rename into this patch as
there
Quoted 9 lines
Quoted 4 lines
is only one caller of
try_to_munlock.- Alistair
No objections here. :)
thanks,
Jason Gunthorpe
maintainerRe: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap
Quoted 19 lines · Alistair Popple
Quoted 3 lines · AEDT John Hubbard
On 3/30/21 3:56 PM, Alistair Popple wrote:
...Quoted 1 line
Quoted 1 line
+1 for renaming "munlock*" items to "mlock*", where applicable. good
grief.
Quoted 4 lines
Quoted 4 lines
At least the situation was weird enough to prompt further investigation :)
Renaming to mlock* doesn't feel like the right solution to me either
though. I
Quoted 9 lines
Quoted 4 lines
am not sure if you saw me responding to myself earlier but I am thinking renaming
try_to_munlock()->page_mlocked()andtry_to_munlock_one()->page_mlock_one()might be better. Thoughts?Quite confused by this naming idea. Because:
try_to_munlock()returns void, so a boolean-style name such as "page_mlocked()" is already not a good fit.
Quoted 7 lines
Quoted 4 lines
Even more important, though, is that
try_to_munlock()is mlock-ing the page, right? Is there some subtle point I'm missing? It really is doing an mlock to the best of my knowledge here. Although the kerneldoc comment fortry_to_munlock()seems questionable too:It's mlocking the page if it turns out it still needs to be locked after unlocking it. But I don't think you're missing anything.
It is really searching all VMA's to see if the VMA flag is set and if any are found then it mlocks the page.
But presenting this rountine in its simplified form raises lots of questions:
- What locking is being used to read the VMA flag?
- Why do we need to manipulate global struct page flags under the page table locks of a single VMA?
- Why do we need to check for huge pages inside the VMA loop, not before going to the rmap? PageTransCompoundHead() is not sensitive to the PTEs. (and what happens if the huge page breaks up concurrently?)
- Why do we clear the mlock bit then run around to try and set it? Feels racey.
Jason
Alistair Popple
reviewerRe: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap
Quoted 8 lines · AEDT Jason Gunthorpe
Quoted 7 lines · Alistair Popple
Quoted 3 lines · AEDT John Hubbard
On 3/30/21 3:56 PM, Alistair Popple wrote:
...Quoted 1 line
Quoted 1 line
+1 for renaming "munlock*" items to "mlock*", where applicable. good
grief.
Quoted 2 lines
Quoted 2 lines
At least the situation was weird enough to prompt further
investigation :)
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
Renaming to mlock* doesn't feel like the right solution to me either
though. I
Quoted 1 line
Quoted 1 line
am not sure if you saw me responding to myself earlier but I am
thinking
Quoted 20 lines
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
renaming
try_to_munlock()->page_mlocked()andtry_to_munlock_one()-
page_mlock_one()might be better. Thoughts?Quite confused by this naming idea. Because:
try_to_munlock()returns void, so a boolean-style name such as "page_mlocked()" is already not a good fit.Quoted 7 lines
Quoted 4 lines
Even more important, though, is that
try_to_munlock()is mlock-ing the page, right? Is there some subtle point I'm missing? It really is doing an mlock to the best of my knowledge here. Although the kerneldoc comment fortry_to_munlock()seems questionable too:It's mlocking the page if it turns out it still needs to be locked after unlocking it. But I don't think you're missing anything.
It is really searching all VMA's to see if the VMA flag is set and if any are found then it mlocks the page.
Quoted 6 lines
But presenting this rountine in its simplified form raises lots of questions:
- What locking is being used to read the VMA flag?
- Why do we need to manipulate global struct page flags under the page table locks of a single VMA?
I was wondering that and questioned it in an earlier version of this series. I have done some digging and the commit log for b87537d9e2fe ("mm: rmap use pte lock not mmap_sem to set PageMlocked") provides the original justification.
It's fairly long so I won't quote it here but the summary seems to be that among other things the combination of page lock and ptl makes this safe. I have yet to verify if everything there still holds and is sensible, but the last paragraph certainly is :-)
"Stopped short of separating try_to_munlock_one() from try_to_munmap_one() on this occasion, but that's probably the sensible next step - with a rename, given that try_to_munlock()'s business is to try to set Mlocked."
Quoted 4 lines
- Why do we need to check for huge pages inside the VMA loop, not before going to the rmap?
PageTransCompoundHead()is not sensitive to the PTEs. (and what happens if the huge page breaks up concurrently?)
- Why do we clear the mlock bit then run around to try and set it?
I don't have an answer for that as I'm not (yet) across all the mlock code paths, but I'm hoping this patch at least won't change anything.
Quoted 4 lines
Feels racey.
Jason
Re: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap
Quoted 56 lines · Alistair Popple
Quoted 8 lines · AEDT Jason Gunthorpe
Quoted 7 lines · Alistair Popple
Quoted 3 lines · AEDT John Hubbard
On 3/30/21 3:56 PM, Alistair Popple wrote:
...Quoted 1 line
Quoted 1 line
+1 for renaming "munlock*" items to "mlock*", where applicable. good
grief.
Quoted 2 lines
Quoted 2 lines
At least the situation was weird enough to prompt further
investigation :)
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
Renaming to mlock* doesn't feel like the right solution to me either
though. I
Quoted 1 line
Quoted 1 line
am not sure if you saw me responding to myself earlier but I am
thinking
Quoted 20 lines
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
renaming
try_to_munlock()->page_mlocked()andtry_to_munlock_one()-
page_mlock_one()might be better. Thoughts?Quite confused by this naming idea. Because:
try_to_munlock()returns void, so a boolean-style name such as "page_mlocked()" is already not a good fit.Quoted 7 lines
Quoted 4 lines
Even more important, though, is that
try_to_munlock()is mlock-ing the page, right? Is there some subtle point I'm missing? It really is doing an mlock to the best of my knowledge here. Although the kerneldoc comment fortry_to_munlock()seems questionable too:It's mlocking the page if it turns out it still needs to be locked after unlocking it. But I don't think you're missing anything.
It is really searching all VMA's to see if the VMA flag is set and if any are found then it mlocks the page.
Quoted 6 lines
But presenting this rountine in its simplified form raises lots of questions:
- What locking is being used to read the VMA flag?
- Why do we need to manipulate global struct page flags under the page table locks of a single VMA?I was wondering that and questioned it in an earlier version of this series. I have done some digging and the commit log for
b87537d9e2fe("mm: rmap use pte lock notmmap_semto set PageMlocked") provides the original justification.It's fairly long so I won't quote it here but the summary seems to be that among other things the combination of page lock and ptl makes this safe. I have yet to verify if everything there still holds and is sensible, but the last paragraph certainly is :-)
"Stopped short of separating
try_to_munlock_one()fromtry_to_munmap_one()on this occasion, but that's probably the sensible next step - with a rename, given that try_to_munlock()'s business is to try to set Mlocked."
Quoted 8 lines
Quoted 4 lines
- Why do we need to check for huge pages inside the VMA loop, not before going to the rmap?
PageTransCompoundHead()is not sensitive to the PTEs. (and what happens if the huge page breaks up concurrently?)
- Why do we clear the mlock bit then run around to try and set it?I don't have an answer for that as I'm not (yet) across all the mlock code paths, but I'm hoping this patch at least won't change anything.
It would be good to ask the person who has the most answers?
Hugh, the thread started at
https://lore.kernel.org/dri-devel/[email protected]/
Alistair Popple
reviewerPATCH v7 4/8 mm/rmap: Split migration into its own function
In reply to Alistair PoppleMigration is currently implemented as a mode of operation for try_to_unmap_one() generally specified by passing the TTU_MIGRATION flag or in the case of splitting a huge anonymous page TTU_SPLIT_FREEZE.
However it does not have much in common with the rest of the unmap functionality of try_to_unmap_one() and thus splitting it into a separate function reduces the complexity of try_to_unmap_one() making it more readable.
Several simplifications can also be made in try_to_migrate_one() based on the following observations:
- All users of TTU_MIGRATION also set TTU_IGNORE_MLOCK.
- No users of TTU_MIGRATION ever set TTU_IGNORE_HWPOISON.
- No users of TTU_MIGRATION ever set TTU_BATCH_FLUSH.
TTU_SPLIT_FREEZE is a special case of migration used when splitting an anonymous page. This is most easily dealt with by calling the correct function from unmap_page() in mm/huge_memory.c - either try_to_migrate() for PageAnon or try_to_unmap().
---
v5:
* Added comments about how PMD splitting works for migration vs. unmapping
* Tightened up the flag check in try_to_migrate() to be explicit about which TTU_XXX flags are supported.
--- include/linux/rmap.h | 4 +- mm/huge_memory.c | 15 +- mm/migrate.c | 9 +- mm/rmap.c | 358 ++++++++++++++++++++++++++++++++----------- 4 files changed, 280 insertions(+), 106 deletions(-)
include/linux/rmap.h +1 -3
@@ -86,8 +86,6 @@ struct anon_vma_chain { };enum ttu_flags { -TTU_MIGRATION =0x1 ,/* migration mode */ -TTU_SPLIT_HUGE_PMD =0x4 ,/* split huge PMD if any */ TTU_IGNORE_MLOCK =0x8 ,/* ignore mlock */ TTU_IGNORE_HWPOISON =0x20 ,/* corrupted page is recoverable */
@@ -96,7 +94,6 @@ enum ttu_flags {* do a final flush if necessary */ TTU_RMAP_LOCKED = 0x80, /* do not grab rmap lock:* caller holds it */ - TTU_SPLIT_FREEZE = 0x100, /* freeze pte under splitting thp */ };#ifdef CONFIG_MMU
@@ -193,6 +190,7 @@ static inline void page_dup_rmap(struct page *page, bool compound)int page_referenced (struct page *,int is_locked,struct mem_cgroup *memcg,unsigned long *vm_flags); +bool try_to_migrate (struct page *page,enum ttu_flags flags);bool try_to_unmap (struct page *,enum ttu_flags flags);/* Avoid racy checks */
mm/huge_memory.c +10 -5
@@ -2357,16 +2357,21 @@ void vma_adjust_trans_huge(struct vm_area_struct *vma,static void unmap_page (struct page *page) { -enum ttu_flags ttu_flags =TTU_IGNORE_MLOCK | -TTU_RMAP_LOCKED |TTU_SPLIT_HUGE_PMD ; +enum ttu_flags ttu_flags =TTU_RMAP_LOCKED |TTU_SPLIT_HUGE_PMD ;bool unmap_success;VM_BUG_ON_PAGE (!PageHead (page), page);if (PageAnon (page)) - ttu_flags |=TTU_SPLIT_FREEZE ; - - unmap_success =try_to_unmap (page, ttu_flags); + unmap_success =try_to_migrate (page, ttu_flags); +else +/* +* Don't install migration entries for file backed pages. This +* helps handle cases when i_size is in the middle of the page +* as there is no need to unmap pages beyond i_size manually. +*/ + unmap_success =try_to_unmap (page, ttu_flags | +TTU_IGNORE_MLOCK );VM_BUG_ON_PAGE (!unmap_success, page); }
mm/migrate.c +4 -5
@@ -1130,7 +1130,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,/* Establish migration ptes */ VM_BUG_ON_PAGE (PageAnon (page) && !PageKsm (page) && !anon_vma, page); -try_to_unmap (page,TTU_MIGRATION |TTU_IGNORE_MLOCK ); +try_to_migrate (page,0 ); page_was_mapped =1 ; }
@@ -1332,7 +1332,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,if (page_mapped (hpage)) {bool mapping_locked = false; -enum ttu_flags ttu =TTU_MIGRATION |TTU_IGNORE_MLOCK ; +enum ttu_flags ttu =0 ;if (!PageAnon (hpage)) {/*
@@ -1349,7 +1349,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page, ttu |=TTU_RMAP_LOCKED ; } -try_to_unmap (hpage, ttu); +try_to_migrate (hpage, ttu); page_was_mapped =1 ;if (mapping_locked)
@@ -2756,7 +2756,6 @@ static void migrate_vma_prepare(struct migrate_vma *migrate)*/ static void migrate_vma_unmap (struct migrate_vma *migrate) { -int flags =TTU_MIGRATION |TTU_IGNORE_MLOCK ;const unsigned long npages = migrate->npages;const unsigned long start = migrate->start;unsigned long addr, i, restore =0 ;
@@ -2768,7 +2767,7 @@ static void migrate_vma_unmap(struct migrate_vma *migrate)continue ;if (page_mapped (page)) { -try_to_unmap (page, flags); +try_to_migrate (page,0 );if (page_mapped (page)) goto restore; }
mm/rmap.c +265 -93
394 lines not on this page · raw message
Signature · 1 line
2.20.1
Alistair Popple
reviewerPATCH v7 5/8 mm: Device exclusive memory access
In reply to Alistair PoppleSome devices require exclusive write access to shared virtual memory (SVM) ranges to perform atomic operations on that memory. This requires CPU page tables to be updated to deny access whilst atomic operations are occurring.
In order to do this introduce a new swap entry
type (SWP_DEVICE_EXCLUSIVE). When a SVM range needs to be marked for exclusive access by a device all page table mappings for the particular range are replaced with device exclusive swap entries. This causes any CPU access to the page to result in a fault.
Faults are resovled by replacing the faulting entry with the original mapping. This results in MMU notifiers being called which a driver uses to update access permissions such as revoking atomic access. After notifiers have been called the device will no longer have exclusive access to the region.
---
v7:
* Added Christoph's Reviewed-by.
* Minor cosmetic cleanups suggested by Christoph.
* Replace mmu_notifier_range_init_migrate/exclusive with mmu_notifier_range_init_owner as suggested by Christoph.
* Replaced lock_page() with lock_page_retry() when handling faults.
* Restrict to anonymous pages for now.
v6:
* Fixed a bisectablity issue due to incorrectly applying the rename of migrate_pgmap_owner to the wrong patches for Nouveau and hmm_test.
v5:
* Renamed range->migrate_pgmap_owner to range->owner.
* Added MMU_NOTIFY_EXCLUSIVE to allow passing of a driver cookie which allows notifiers called as a result of make_device_exclusive_range() to be ignored.
* Added a check to try_to_protect_one() to detect if the pages originally returned from get_user_pages() have been unmapped or not.
* Removed check_device_exclusive_range() as it is no longer required with the other changes.
* Documentation update.
v4:
* Add function to check that mappings are still valid and exclusive.
* s/long/unsigned long/ in make_device_exclusive_entry().
--- Documentation/vm/hmm.rst | 19 ++- drivers/gpu/drm/nouveau/nouveau_svm.c | 2 +- include/linux/mmu_notifier.h | 26 ++-- include/linux/rmap.h | 4 + include/linux/swap.h | 4 +- include/linux/swapops.h | 44 +++++- lib/test_hmm.c | 2 +- mm/hmm.c | 5 + mm/memory.c | 108 ++++++++++++- mm/migrate.c | 10 +- mm/mprotect.c | 8 + mm/page_vma_mapped.c | 9 +- mm/rmap.c | 210 ++++++++++++++++++++++++++ 13 files changed, 426 insertions(+), 25 deletions(-)
Documentation/vm/hmm.rst +17 -1
@@ -332,7 +332,7 @@ between device driver specific code and shared common code: walks to fill in the ``args->src`` array with PFNs to be migrated. The ``invalidate_range_start()`` callback is passed a ``struct mmu_notifier_range`` with the ``event`` field set to - ``MMU_NOTIFY_MIGRATE`` and the ``migrate_pgmap_owner`` field set to + ``MMU_NOTIFY_MIGRATE`` and the ``owner`` field set to the ``args->pgmap_owner`` field passed to migrate_vma_setup(). This is allows the device driver to skip the invalidation callback and only invalidate device private MMU mappings that are actually migrating.
@@ -405,6 +405,23 @@ between device driver specific code and shared common code: The lock can now be released. +Exclusive access memory +===========+ +Some devices have features such as atomic PTE bits that can be used to implement +atomic access to system memory. To support atomic operations to a shared virtual +memory page such a device needs access to that page which is exclusive of any +userspace access from the CPU. The ``make_device_exclusive_range()`` function +can be used to make a memory range inaccessible from userspace. + +This replaces all mappings for pages in the given range with special swap +entries. Any attempt to access the swap entry results in a fault which is +resovled by replacing the entry with the original mapping. A driver gets +notified that the mapping has been changed by MMU notifiers, after which point +it will no longer have exclusive access to the page. Exclusive access is +guranteed to last until the driver drops the page lock and page reference, at +which point any CPU faults on the page may proceed as described. + Memory cgroup (memcg) and rss accounting ====================
drivers/gpu/drm/nouveau/nouveau_svm.c +1 -1
@@ -265,7 +265,7 @@ nouveau_svmm_invalidate_range_start(struct mmu_notifier *mn,* the invalidation is handled as part of the migration process. */ if (update->event =MMU_NOTIFY_MIGRATE && - update->migrate_pgmap_owner = svmm->vmm->cli->drm->dev) + update->owner = svmm->vmm->cli->drm->dev) goto out;if (limit > svmm->unmanaged.start && start < svmm->unmanaged.limit) {
include/linux/mmu_notifier.h +16 -10
@@ -41,7 +41,12 @@ struct mmu_interval_notifier;* * @MMU_NOTIFY_MIGRATE: used during migrate_vma_collect() invalidate to signal * a device driver to possibly ignore the invalidation if the -* migrate_pgmap_owner field matches the driver's device private pgmap owner. +* owner field matches the driver's device private pgmap owner. +* +* @MMU_NOTIFY_EXCLUSIVE: to signal a device driver that the device will no +* longer have exclusive access to the page. May ignore the invalidation that's +* part of make_device_exclusive_range() if the owner field +* matches the value passed to make_device_exclusive_range(). */ enum mmu_notifier_event {MMU_NOTIFY_UNMAP =0 ,
@@ -51,6 +56,7 @@ enum mmu_notifier_event {MMU_NOTIFY_SOFT_DIRTY ,MMU_NOTIFY_RELEASE ,MMU_NOTIFY_MIGRATE , + MMU_NOTIFY_EXCLUSIVE, };#define MMU_NOTIFIER_RANGE_BLOCKABLE (1 << 0)
@@ -269,7 +275,7 @@ struct mmu_notifier_range {unsigned long end;unsigned flags;enum mmu_notifier_event event; -void *migrate_pgmap_owner; +void *owner; };static inline int mm_has_notifiers (struct mm_struct *mm)
@@ -521,14 +527,14 @@ static inline void mmu_notifier_range_init(struct mmu_notifier_range *range, range->flags = flags; } -static inline void mmu_notifier_range_init_migrate ( -struct mmu_notifier_range *range,unsigned int flags, +static inline void mmu_notifier_range_init_owner ( +struct mmu_notifier_range *range, +enum mmu_notifier_event event,unsigned int flags,struct vm_area_struct *vma,struct mm_struct *mm, -unsigned long start,unsigned long end,void *pgmap) +unsigned long start,unsigned long end,void *owner) { -mmu_notifier_range_init (range,MMU_NOTIFY_MIGRATE , flags, vma, mm, - start, end); - range->migrate_pgmap_owner = pgmap; +mmu_notifier_range_init (range, event, flags, vma, mm, start, end); + range->owner = owner; }#define ptep_clear_flush_young_notify (__vma, __address, __ptep) \
@@ -655,8 +661,8 @@ static inline void _mmu_notifier_range_init(struct mmu_notifier_range *range,#define mmu_notifier_range_init (range,event,flags,vma,mm,start,end) \ _mmu_notifier_range_init(range, start, end) -#define mmu_notifier_range_init_migrate (range, flags, vma, mm, start, end, \ - pgmap) \ +#define mmu_notifier_range_init_owner (range, event, flags, vma, mm, start, \ + end, owner) \ _mmu_notifier_range_init(range, start, end) static inline bool
include/linux/rmap.h +4 -0
@@ -193,6 +193,10 @@ int page_referenced(struct page *, int is_locked,bool try_to_migrate (struct page *page,enum ttu_flags flags);bool try_to_unmap (struct page *,enum ttu_flags flags); +int make_device_exclusive_range (struct mm_struct *mm,unsigned long start, +unsigned long end,struct page **pages, +void *arg); +/* Avoid racy checks */ #define PVMW_SYNC (1 << 0)/* Look for migarion entries rather than present PTEs */
include/linux/swap.h +3 -1
@@ -63,9 +63,11 @@ static inline int current_is_kswapd(void)* to a special SWP_DEVICE_* entry. */ #ifdef CONFIG_DEVICE_PRIVATE -#define SWP_DEVICE_NUM 2 +#define SWP_DEVICE_NUM 4#define SWP_DEVICE_WRITE (MAX_SWAPFILES+SWP_HWPOISON_NUM+SWP_MIGRATION_NUM)#define SWP_DEVICE_READ (MAX_SWAPFILES+SWP_HWPOISON_NUM+SWP_MIGRATION_NUM+1) +#define SWP_DEVICE_EXCLUSIVE_WRITE (MAX_SWAPFILES+SWP_HWPOISON_NUM+SWP_MIGRATION_NUM+2) +#define SWP_DEVICE_EXCLUSIVE_READ (MAX_SWAPFILES+SWP_HWPOISON_NUM+SWP_MIGRATION_NUM+3)#else #define SWP_DEVICE_NUM 0#endif
include/linux/swapops.h +43 -1
62 lines not on this page · raw message
lib/test_hmm.c +1 -1
@@ -218,7 +218,7 @@ static bool dmirror_interval_invalidate(struct mmu_interval_notifier *mni,* the invalidation is handled as part of the migration process. */ if (range->event =MMU_NOTIFY_MIGRATE && - range->migrate_pgmap_owner = dmirror->mdevice) + range->owner = dmirror->mdevice)return true;if (mmu_notifier_range_blockable (range))
mm/hmm.c +5 -0
@@ -26,6 +26,8 @@#include <linux/mmu_notifier.h> #include <linux/memory_hotplug.h> +#include "internal.h" +struct hmm_vma_walk {struct hmm_range *range;unsigned long last;
@@ -271,6 +273,9 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,if (!non_swap_entry (entry)) goto fault; +if (is_device_exclusive_entry (entry)) + goto fault; +if (is_migration_entry (entry)) {pte_unmap (ptep); hmm_vma_walk->last = addr;
mm/memory.c +106 -2
138 lines not on this page · raw message
mm/migrate.c +5 -5
@@ -2570,8 +2570,8 @@ static void migrate_vma_collect(struct migrate_vma *migrate)* that the registered device driver can skip invalidating device * private page mappings that won't be migrated. */ -mmu_notifier_range_init_migrate (&range,0 , migrate->vma, - migrate->vma->vm_mm, migrate->start, migrate->end, +mmu_notifier_range_init_owner (&range,MMU_NOTIFY_MIGRATE ,0 , + migrate->vma, migrate->vma->vm_mm, migrate->start, migrate->end, migrate->pgmap_owner);mmu_notifier_invalidate_range_start (&range);
@@ -3074,9 +3074,9 @@ void migrate_vma_pages(struct migrate_vma *migrate)if (!notified) { notified = true; -mmu_notifier_range_init_migrate (&range,0 , - migrate->vma, migrate->vma->vm_mm, - addr, migrate->end, +mmu_notifier_range_init_owner (&range, +MMU_NOTIFY_MIGRATE ,0 , migrate->vma, + migrate->vma->vm_mm, addr, migrate->end, migrate->pgmap_owner);mmu_notifier_invalidate_range_start (&range); }
mm/mprotect.c +8 -0
@@ -165,6 +165,14 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd, newpte =swp_entry_to_pte (entry);if (pte_swp_uffd_wp (oldpte)) newpte =pte_swp_mkuffd_wp (newpte); + }else if (is_writable_device_exclusive_entry (entry)) { + entry =make_readable_device_exclusive_entry ( +swp_offset (entry)); + newpte =swp_entry_to_pte (entry); +if (pte_swp_soft_dirty (oldpte)) + newpte =pte_swp_mksoft_dirty (newpte); +if (pte_swp_uffd_wp (oldpte)) + newpte =pte_swp_mkuffd_wp (newpte); }else { newpte = oldpte; }
mm/page_vma_mapped.c +6 -3
@@ -41,7 +41,8 @@ static bool map_pte(struct page_vma_mapped_walk *pvmw)/* Handle un-addressable ZONE_DEVICE memory */ entry =pte_to_swp_entry (*pvmw->pte); -if (!is_device_private_entry (entry)) +if (!is_device_private_entry (entry) && + !is_device_exclusive_entry (entry))return false; }else if (!pte_present (*pvmw->pte))return false;
@@ -93,7 +94,8 @@ static bool check_pte(struct page_vma_mapped_walk *pvmw)return false; entry =pte_to_swp_entry (*pvmw->pte); -if (!is_migration_entry (entry)) +if (!is_migration_entry (entry) && + !is_device_exclusive_entry (entry))return false; pfn =swp_offset (entry);
@@ -102,7 +104,8 @@ static bool check_pte(struct page_vma_mapped_walk *pvmw)/* Handle un-addressable ZONE_DEVICE memory */ entry =pte_to_swp_entry (*pvmw->pte); -if (!is_device_private_entry (entry)) +if (!is_device_private_entry (entry) && + !is_device_exclusive_entry (entry))return false; pfn =swp_offset (entry);
mm/rmap.c +210 -0
216 lines not on this page · raw message
Signature · 1 line
2.20.1
Jason Gunthorpe
maintainerRe: [PATCH v7 5/8] mm: Device exclusive memory access
Quoted 18 lines · Alistair Popple
mm/memory.c +12
-0+++ b/mm/memory.c@@ -781,6 +781,27 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm, pte =pte_swp_mkuffd_wp (pte);set_pte_at (src_mm, addr, src_pte, pte); } + }else if (is_device_exclusive_entry (entry)) { + page =pfn_swap_entry_to_page (entry); + +get_page (page); + rss[mm_counter (page)]++; + +if (is_writable_device_exclusive_entry (entry) && +is_cow_mapping (vm_flags)) { +/* +* COW mappings require pages in both +* parent and child to be set to read.
mm/memory.c copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm, · line 804
Quoted 10 lines
mm/memory.c +10
-0+*/ + entry =make_readable_device_exclusive_entry ( +swp_offset (entry)); + pte =swp_entry_to_pte (entry); +if (pte_swp_soft_dirty (*src_pte)) + pte =pte_swp_mksoft_dirty (pte); +if (pte_swp_uffd_wp (*src_pte)) + pte =pte_swp_mkuffd_wp (pte); +set_pte_at (src_mm, addr, src_pte, pte); + }
This needs to have the same logic as we now have in copy_present_page(). The page *is* present and we can't copy the PTE value hidden in a swap entry if we can't copy the PTE normally.
The code should be shared because nobody is going to remember about this corner case.
Jason
Alistair Popple
reviewerRe: [PATCH v7 5/8] mm: Device exclusive memory access
Quoted 5 lines · AEDT Jason Gunthorpe
mm/memory.c copy_nonpresent_pte(struct mm_struct *dst_mm, struct
Quoted 4 lines · Alistair Popple
mm/memory.c +1
-0+++ b/mm/memory.c@@ -781,6 +781,27 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct
mm_struct *src_mm,
Quoted 20 lines
Quoted 20 lines
mm/memory.c +17
-0pte =pte_swp_mkuffd_wp (pte);set_pte_at (src_mm, addr, src_pte, pte); } + }else if (is_device_exclusive_entry (entry)) { + page =pfn_swap_entry_to_page (entry); + +get_page (page); + rss[mm_counter (page)]++; + +if (is_writable_device_exclusive_entry (entry) && +is_cow_mapping (vm_flags)) { +/* +* COW mappings require pages in both +* parent and child to be set to read. +*/ + entry =make_readable_device_exclusive_entry ( +swp_offset (entry)); + pte =swp_entry_to_pte (entry); +if (pte_swp_soft_dirty (*src_pte)) + pte =pte_swp_mksoft_dirty (pte);
Quoted 8 lines
mm/memory.c copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm, · line 804
Quoted 4 lines
mm/memory.c +4
-0+if (pte_swp_uffd_wp (*src_pte)) + pte =pte_swp_mkuffd_wp (pte); +set_pte_at (src_mm, addr, src_pte, pte); + }This needs to have the same logic as we now have in
copy_present_page(). The page *is* present and we can't copy the PTE value hidden in a swap entry if we can't copy the PTE normally.
You're saying we need to use copy_present_page() to make sure the split goes the right way for pinned pages? I guess that makes sense as the split could go either way at the moment but I should add a check to make sure this isn't used with pinned pages anyway.
- Alistair
Quoted 5 lines
The code should be shared because nobody is going to remember about this corner case.
Jason
Jason Gunthorpe
maintainerRe: [PATCH v7 5/8] mm: Device exclusive memory access
Quoted 3 lines · Alistair Popple
I guess that makes sense as the split could go either way at the moment but I should add a check to make sure this isn't used with pinned pages anyway.
Is it possible to have a pinned page under one of these things? If I pin it before you migrate it then it remains pinned but hidden under the swap entry?
So the special logic is needed and the pinned page has to be copied and written as a normal pte, not dropped as a migration entry
Jason
Alistair Popple
reviewerRe: [PATCH v7 5/8] mm: Device exclusive memory access
Quoted 9 lines · AEDT Jason Gunthorpe
Quoted 3 lines · Alistair Popple
I guess that makes sense as the split could go either way at the moment but I should add a check to make sure this isn't used with pinned pages anyway.
Is it possible to have a pinned page under one of these things? If I pin it before you migrate it then it remains pinned but hidden under the swap entry?
At the moment yes. But I had planned (and this reminded me) to add a check to prevent marking pinned pages for exclusive access. This check was in the original migration based implementation as I don't think it makes much sense to allow exclusive access to pinned pages given it indicates another device is possibly using it.
Quoted 2 lines
So the special logic is needed and the pinned page has to be copied and written as a normal pte, not dropped as a migration entry
Yep, if we end up allowing pinned pages to exist under these then that makes sense. Thanks for the clarification.
- Alistair
Quoted 2 lines
Jason
Jason Gunthorpe
maintainerRe: [PATCH v7 5/8] mm: Device exclusive memory access
Quoted 7 lines · Alistair Popple
Quoted 6 lines · AEDT Jason Gunthorpe
Quoted 3 lines · Alistair Popple
I guess that makes sense as the split could go either way at the moment but I should add a check to make sure this isn't used with pinned pages anyway.
Quoted 6 lines
Quoted 3 lines
Is it possible to have a pinned page under one of these things? If I pin it before you migrate it then it remains pinned but hidden under the swap entry?
At the moment yes. But I had planned (and this reminded me) to add a check to prevent marking pinned pages for exclusive access.
How do you even do that without races with GUP fast?
Jason
Alistair Popple
reviewerRe: [PATCH v7 5/8] mm: Device exclusive memory access
Quoted 8 lines · AEDT Jason Gunthorpe
Quoted 7 lines · Alistair Popple
Quoted 6 lines · AEDT Jason Gunthorpe
Quoted 3 lines · Alistair Popple
I guess that makes sense as the split could go either way at the moment but I should add a check to make sure this isn't used with pinned pages anyway.
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Is it possible to have a pinned page under one of these things? If I pin it before you migrate it then it remains pinned but hidden under the swap entry?
At the moment yes. But I had planned (and this reminded me) to add a check
to
Quoted 3 lines
Quoted 1 line
prevent marking pinned pages for exclusive access.
How do you even do that without races with GUP fast?
Unless I've missed something I think I've convinced myself it should be safe to do the pin check after make_device_exclusive() has replaced all the PTEs with exclusive entries.
GUP fast sequence:
1. Read PTE
2. Pin page
3. Check PTE
4. if PTE changed -> unpin and fallback
If make_device_exclusive() runs after (1) it will either succeed or see the pin from (2) and fail (as desired). GUP should always see the PTE change and fallback which will revoke the exclusive access.
- Alistair
Quoted 2 lines
Jason
Jason Gunthorpe
maintainerRe: [PATCH v7 5/8] mm: Device exclusive memory access
Quoted 29 lines · Alistair Popple
Quoted 8 lines · AEDT Jason Gunthorpe
Quoted 7 lines · Alistair Popple
Quoted 6 lines · AEDT Jason Gunthorpe
Quoted 3 lines · Alistair Popple
I guess that makes sense as the split could go either way at the moment but I should add a check to make sure this isn't used with pinned pages anyway.
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Is it possible to have a pinned page under one of these things? If I pin it before you migrate it then it remains pinned but hidden under the swap entry?
At the moment yes. But I had planned (and this reminded me) to add a check
to
Quoted 3 lines
Quoted 1 line
prevent marking pinned pages for exclusive access.
How do you even do that without races with GUP fast?
Unless I've missed something I think I've convinced myself it should be safe to do the pin check after
make_device_exclusive()has replaced all the PTEs with exclusive entries.GUP fast sequence:
1. Read PTE
2. Pin page
3. Check PTE
4. if PTE changed -> unpin and fallback
Quoted 3 lines
If
make_device_exclusive()runs after (1) it will either succeed or see the pin from (2) and fail (as desired). GUP should always see the PTE change and fallback which will revoke the exclusive access.
AFAICT the user can trigger fork at that instant and fork will try to copy the desposited migration entry before it has been checked
Jason
Alistair Popple
reviewerRe: [PATCH v7 5/8] mm: Device exclusive memory access
Quoted 10 lines · AEDT Jason Gunthorpe
Quoted 9 lines · Alistair Popple
Quoted 8 lines · AEDT Jason Gunthorpe
Quoted 7 lines · Alistair Popple
Quoted 6 lines · AEDT Jason Gunthorpe
Quoted 3 lines · Alistair Popple
I guess that makes sense as the split could go either way at the moment but I should add a check to make sure this isn't used with pinned pages anyway.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Is it possible to have a pinned page under one of these things? If I pin it before you migrate it then it remains pinned but hidden under the swap entry?
At the moment yes. But I had planned (and this reminded me) to add a
check
Quoted 6 lines
Quoted 6 lines
to
Quoted 3 lines
Quoted 1 line
prevent marking pinned pages for exclusive access.
How do you even do that without races with GUP fast?
Unless I've missed something I think I've convinced myself it should be
safe
Quoted 1 line
Quoted 1 line
to do the pin check after
make_device_exclusive()has replaced all the
PTEs
Quoted 9 lines
Quoted 9 lines
with exclusive entries.
GUP fast sequence:
1. Read PTE
2. Pin page
3. Check PTE
4. if PTE changed -> unpin and fallbackIf
make_device_exclusive()runs after (1) it will either succeed or see
the
Quoted 1 line
Quoted 1 line
pin from (2) and fail (as desired). GUP should always see the PTE change
and
Quoted 4 lines
Quoted 1 line
fallback which will revoke the exclusive access.
AFAICT the user can trigger fork at that instant and fork will try to copy the desposited migration entry before it has been checked
In that case the child will get a read-only exclusive entry and eventually a page copy via do_wp_page() and GUP will fallback (or fail in the case of fast only) so the parent's exclusive entry will get removed before the page can be pinned and therefore shouldn't split the wrong way.
But that is sounding rather complex, and I am not convinced I haven't missed a corner case. It also seems like it shouldn't be necessary to copy exclusive entries anyway. I could just remove them and restore the original entry, which would be far simpler.
Quoted 2 lines
Jason
Jason Gunthorpe
maintainerRe: [PATCH v7 5/8] mm: Device exclusive memory access
Quoted 37 lines · Alistair Popple
Quoted 10 lines · AEDT Jason Gunthorpe
Quoted 9 lines · Alistair Popple
Quoted 8 lines · AEDT Jason Gunthorpe
Quoted 7 lines · Alistair Popple
Quoted 6 lines · AEDT Jason Gunthorpe
Quoted 3 lines · Alistair Popple
I guess that makes sense as the split could go either way at the moment but I should add a check to make sure this isn't used with pinned pages anyway.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Is it possible to have a pinned page under one of these things? If I pin it before you migrate it then it remains pinned but hidden under the swap entry?
At the moment yes. But I had planned (and this reminded me) to add a
check
Quoted 6 lines
Quoted 6 lines
to
Quoted 3 lines
Quoted 1 line
prevent marking pinned pages for exclusive access.
How do you even do that without races with GUP fast?
Unless I've missed something I think I've convinced myself it should be
safe
Quoted 1 line
Quoted 1 line
to do the pin check after
make_device_exclusive()has replaced all thePTEs
Quoted 9 lines
Quoted 9 lines
with exclusive entries.
GUP fast sequence:
1. Read PTE
2. Pin page
3. Check PTE
4. if PTE changed -> unpin and fallbackIf
make_device_exclusive()runs after (1) it will either succeed or seethe
Quoted 1 line
Quoted 1 line
pin from (2) and fail (as desired). GUP should always see the PTE change
Quoted 8 lines
and
Quoted 4 lines
Quoted 1 line
fallback which will revoke the exclusive access.
AFAICT the user can trigger fork at that instant and fork will try to copy the desposited migration entry before it has been checked
In that case the child will get a read-only exclusive entry and eventually a page copy via
do_wp_page()
Having do_wp_page() do a copy is a security bug. We closed it with the at-fork checks.
Jason
Alistair Popple
reviewerPATCH v7 6/8 mm: Selftests for exclusive device memory
In reply to Alistair PoppleAdds some selftests for exclusive device memory.
--- lib/test_hmm.c | 124 +++++++++++++++++++ lib/test_hmm_uapi.h | 2 + tools/testing/selftests/vm/hmm-tests.c | 158 +++++++++++++++++++++++++ 3 files changed, 284 insertions(+)
lib/test_hmm.c +124 -0
@@ -25,6 +25,7 @@#include <linux/swapops.h> #include <linux/sched/mm.h> #include <linux/platform_device.h> +#include <linux/rmap.h> #include "test_hmm_uapi.h"
@@ -46,6 +47,7 @@ struct dmirror_bounce {unsigned long cpages; }; +#define DPT_XA_TAG_ATOMIC 1UL#define DPT_XA_TAG_WRITE 3UL/*
@@ -619,6 +621,54 @@ static void dmirror_migrate_alloc_and_copy(struct migrate_vma *args, } } +static int dmirror_check_atomic (struct dmirror *dmirror,unsigned long start, +unsigned long end) +{ +unsigned long pfn; + +for (pfn = start >>PAGE_SHIFT ; pfn < (end >>PAGE_SHIFT ); pfn++) { +void *entry; +struct page *page; + + entry =xa_load (&dmirror->pt, pfn); + page =xa_untag_pointer (entry); +if (xa_pointer_tag (entry) =DPT_XA_TAG_ATOMIC ) +return -EPERM ; + } + +return 0 ; +} + +static int dmirror_atomic_map (unsigned long start,unsigned long end, +struct page **pages,struct dmirror *dmirror) +{ +unsigned long pfn, mapped =0 ; +int i; + +/* Map the migrated pages into the device's page tables. */ +mutex_lock (&dmirror->mutex); + +for (i =0 , pfn = start >>PAGE_SHIFT ; pfn < (end >>PAGE_SHIFT ); pfn++, i++) { +void *entry; + +if (!pages[i]) +continue ; + + entry = pages[i]; + entry =xa_tag_pointer (entry,DPT_XA_TAG_ATOMIC ); + entry =xa_store (&dmirror->pt, pfn, entry,GFP_ATOMIC ); +if (xa_is_err (entry)) { +mutex_unlock (&dmirror->mutex); +return xa_err (entry); + } + + mapped++; + } + +mutex_unlock (&dmirror->mutex); +return mapped; +} +static int dmirror_migrate_finalize_and_map (struct migrate_vma *args,struct dmirror *dmirror) {
@@ -661,6 +711,71 @@ static int dmirror_migrate_finalize_and_map(struct migrate_vma *args,return 0 ; } +static int dmirror_exclusive (struct dmirror *dmirror, +struct hmm_dmirror_cmd *cmd) +{ +unsigned long start, end, addr; +unsigned long size = cmd->npages <<PAGE_SHIFT ; +struct mm_struct *mm = dmirror->notifier.mm; +struct page *pages[64 ]; +struct dmirror_bounce bounce; +unsigned long next; +int ret; + + start = cmd->addr; + end = start + size; +if (end < start) +return -EINVAL ; + +/* Since the mm is for the mirrored process, get a reference first. */ +if (!mmget_not_zero (mm)) +return -EINVAL ; + +mmap_read_lock (mm); +for (addr = start; addr < end; addr = next) { +int i, mapped; + +if (end < addr + (ARRAY_SIZE (pages) <<PAGE_SHIFT )) + next = end; +else + next = addr + (ARRAY_SIZE (pages) <<PAGE_SHIFT ); + + ret =make_device_exclusive_range (mm, addr, next, pages,NULL ); + mapped =dmirror_atomic_map (addr, next, pages, dmirror); +for (i =0 ; i < ret; i++) { +if (pages[i]) { +unlock_page (pages[i]); +put_page (pages[i]); + } + } + +if (addr + (mapped <<PAGE_SHIFT ) < next) { +mmap_read_unlock (mm); +mmput (mm); +return -EBUSY ; + } + } +mmap_read_unlock (mm); +mmput (mm); + +/* Return the migrated data for verification. */ + ret =dmirror_bounce_init (&bounce, start, size); +if (ret) +return ret; +mutex_lock (&dmirror->mutex); + ret =dmirror_do_read (dmirror, start, end, &bounce); +mutex_unlock (&dmirror->mutex); +if (ret =0 ) { +if (copy_to_user (u64_to_user_ptr (cmd->ptr), bounce.ptr, + bounce.size)) + ret = -EFAULT ; + } + + cmd->cpages = bounce.cpages; +dmirror_bounce_fini (&bounce); +return ret; +} +static int dmirror_migrate (struct dmirror *dmirror,struct hmm_dmirror_cmd *cmd) {
@@ -949,6 +1064,15 @@ static long dmirror_fops_unlocked_ioctl(struct file *filp, ret =dmirror_migrate (dmirror, &cmd);break ; +case HMM_DMIRROR_EXCLUSIVE : + ret =dmirror_exclusive (dmirror, &cmd); +break ; + +case HMM_DMIRROR_CHECK_EXCLUSIVE : + ret =dmirror_check_atomic (dmirror, cmd.addr, + cmd.addr + (cmd.npages <<PAGE_SHIFT )); +break ; +case HMM_DMIRROR_SNAPSHOT : ret =dmirror_snapshot (dmirror, &cmd);break ;
lib/test_hmm_uapi.h +2 -0
@@ -33,6 +33,8 @@ struct hmm_dmirror_cmd {#define HMM_DMIRROR_WRITE _IOWR('H', 0x01, struct hmm_dmirror_cmd)#define HMM_DMIRROR_MIGRATE _IOWR('H', 0x02, struct hmm_dmirror_cmd)#define HMM_DMIRROR_SNAPSHOT _IOWR('H', 0x03, struct hmm_dmirror_cmd) +#define HMM_DMIRROR_EXCLUSIVE _IOWR('H', 0x04, struct hmm_dmirror_cmd) +#define HMM_DMIRROR_CHECK_EXCLUSIVE _IOWR('H', 0x05, struct hmm_dmirror_cmd)/* * Values returned in hmm_dmirror_cmd.ptr for HMM_DMIRROR_SNAPSHOT.
tools/testing/selftests/vm/hmm-tests.c +158 -0
162 lines not on this page · raw message
Signature · 1 line
2.20.1
Alistair Popple
reviewerPATCH v7 7/8 nouveau/svm: Refactor nouveau_range_fault
In reply to Alistair PoppleCall mmu_interval_notifier_insert() as part of nouveau_range_fault(). This doesn't introduce any functional change but makes it easier for a subsequent patch to alter the behaviour of nouveau_range_fault() to support GPU atomic operations.
--- drivers/gpu/drm/nouveau/nouveau_svm.c | 34 ++++++++++++++++----------- 1 file changed, 20 insertions(+), 14 deletions(-)
drivers/gpu/drm/nouveau/nouveau_svm.c +20 -14
@@ -567,18 +567,27 @@ static int nouveau_range_fault(struct nouveau_svmm *svmm,unsigned long hmm_pfns[1 ];struct hmm_range range = { .notifier = ¬ifier->notifier, - .start = notifier->notifier.interval_tree.start, - .end = notifier->notifier.interval_tree.last +1 , .default_flags = hmm_flags, .hmm_pfns = hmm_pfns, .dev_private_owner = drm->dev, }; -struct mm_struct *mm = notifier->notifier.mm; +struct mm_struct *mm = svmm->notifier.mm;int ret; + ret =mmu_interval_notifier_insert (¬ifier->notifier, mm, + args->p.addr, args->p.size, + &nouveau_svm_mni_ops); +if (ret) +return ret; + + range.start = notifier->notifier.interval_tree.start; + range.end = notifier->notifier.interval_tree.last +1 ; +while (true) { -if (time_after (jiffies, timeout)) -return -EBUSY ; +if (time_after (jiffies, timeout)) { + ret = -EBUSY ; + goto out; + } range.notifier_seq =mmu_interval_read_begin (range.notifier);mmap_read_lock (mm);
@@ -587,7 +596,7 @@ static int nouveau_range_fault(struct nouveau_svmm *svmm,if (ret) {if (ret = -EBUSY )continue ; -return ret; + goto out; }mutex_lock (&svmm->mutex);
@@ -606,6 +615,9 @@ static int nouveau_range_fault(struct nouveau_svmm *svmm, svmm->vmm->vmm.object.client->super = false;mutex_unlock (&svmm->mutex); +out: +mmu_interval_notifier_remove (¬ifier->notifier); +return ret; }
@@ -727,14 +739,8 @@ nouveau_svm_fault(struct nvif_notify *notify) } notifier.svmm = svmm; - ret =mmu_interval_notifier_insert (¬ifier.notifier, mm, - args.i.p.addr, args.i.p.size, - &nouveau_svm_mni_ops); -if (!ret) { - ret =nouveau_range_fault (svmm, svm->drm, &args.i, -sizeof (args), hmm_flags, ¬ifier); -mmu_interval_notifier_remove (¬ifier.notifier); - } + ret =nouveau_range_fault (svmm, svm->drm, &args.i, +sizeof (args), hmm_flags, ¬ifier);mmput (mm); limit = args.i.p.addr + args.i.p.size;
Signature · 1 line
2.20.1
Alistair Popple
reviewerPATCH v7 8/8 nouveau/svm: Implement atomic SVM access
In reply to Alistair PoppleSome NVIDIA GPUs do not support direct atomic access to system memory via PCIe. Instead this must be emulated by granting the GPU exclusive access to the memory. This is achieved by replacing CPU page table entries with special swap entries that fault on userspace access.
The driver then grants the GPU permission to update the page undergoing atomic access via the GPU page tables. When CPU access to the page is required a CPU fault is raised which calls into the device driver via MMU notifiers to revoke the atomic access. The original page table entries are then restored allowing CPU access to proceed.
---
v7:
* Removed magic values for fault access levels
* Improved readability of fault comparison code
v4:
* Check that page table entries haven't changed before mapping on the device
--- drivers/gpu/drm/nouveau/include/nvif/if000c.h | 1 + drivers/gpu/drm/nouveau/nouveau_svm.c | 126 ++++++++++++++++-- drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h | 1 + .../drm/nouveau/nvkm/subdev/mmu/vmmgp100.c | 6 + 4 files changed, 123 insertions(+), 11 deletions(-)
drivers/gpu/drm/nouveau/include/nvif/if000c.h +1 -0
@@ -77,6 +77,7 @@ struct nvif_vmm_pfnmap_v0 {#define NVIF_VMM_PFNMAP_V0_APER 0x00000000000000f0ULL#define NVIF_VMM_PFNMAP_V0_HOST 0x0000000000000000ULL#define NVIF_VMM_PFNMAP_V0_VRAM 0x0000000000000010ULL +#define NVIF_VMM_PFNMAP_V0_A 0x0000000000000004ULL#define NVIF_VMM_PFNMAP_V0_W 0x0000000000000002ULL#define NVIF_VMM_PFNMAP_V0_V 0x0000000000000001ULL#define NVIF_VMM_PFNMAP_V0_NONE 0x0000000000000000ULL
drivers/gpu/drm/nouveau/nouveau_svm.c +114 -11
190 lines not on this page · raw message
drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h +1 -0
@@ -178,6 +178,7 @@ void nvkm_vmm_unmap_region(struct nvkm_vmm *, struct nvkm_vma *);#define NVKM_VMM_PFN_APER 0x00000000000000f0ULL#define NVKM_VMM_PFN_HOST 0x0000000000000000ULL#define NVKM_VMM_PFN_VRAM 0x0000000000000010ULL +#define NVKM_VMM_PFN_A 0x0000000000000004ULL#define NVKM_VMM_PFN_W 0x0000000000000002ULL#define NVKM_VMM_PFN_V 0x0000000000000001ULL#define NVKM_VMM_PFN_NONE 0x0000000000000000ULL
drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp100.c +6 -0
@@ -88,6 +88,9 @@ gp100_vmm_pgt_pfn(struct nvkm_vmm *vmm, struct nvkm_mmu_pt *pt,if (!(*map->pfn &NVKM_VMM_PFN_W )) data |=BIT_ULL (6 );/* RO. */ +if (!(*map->pfn &NVKM_VMM_PFN_A )) + data |=BIT_ULL (7 );/* Atomic disable. */ +if (!(*map->pfn &NVKM_VMM_PFN_VRAM )) { addr = *map->pfn >>NVKM_VMM_PFN_ADDR_SHIFT ; addr = dma_map_page(dev, pfn_to_page(addr), 0,
@@ -322,6 +325,9 @@ gp100_vmm_pd0_pfn(struct nvkm_vmm *vmm, struct nvkm_mmu_pt *pt,if (!(*map->pfn &NVKM_VMM_PFN_W )) data |=BIT_ULL (6 );/* RO. */ +if (!(*map->pfn &NVKM_VMM_PFN_A )) + data |=BIT_ULL (7 );/* Atomic disable. */ +if (!(*map->pfn &NVKM_VMM_PFN_VRAM )) { addr = *map->pfn >>NVKM_VMM_PFN_ADDR_SHIFT ; addr = dma_map_page(dev, pfn_to_page(addr), 0,
Signature · 1 line
2.20.1