Alistair Popple
2021-Feb-19 02:07 UTC
[Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access
Some 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. Signed-off-by: Alistair Popple <apopple at nvidia.com> --- Documentation/vm/hmm.rst | 15 +++ fs/proc/task_mmu.c | 7 ++ include/linux/hmm.h | 4 + include/linux/rmap.h | 1 + include/linux/swap.h | 10 +- include/linux/swapops.h | 32 ++++++ mm/hmm.c | 206 +++++++++++++++++++++++++++++++++++++++ mm/memory.c | 34 ++++++- mm/mprotect.c | 7 ++ mm/page_vma_mapped.c | 14 ++- mm/rmap.c | 29 +++++- 11 files changed, 347 insertions(+), 12 deletions(-) diff --git a/Documentation/vm/hmm.rst b/Documentation/vm/hmm.rst index 09e28507f5b2..ffdc58cb2e7c 100644 --- a/Documentation/vm/hmm.rst +++ b/Documentation/vm/hmm.rst @@ -405,6 +405,21 @@ between device driver specific code and shared common code: The lock can now be released. +Exclusive access memory +======================+ +Not all devices support 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 +``hmm_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. + Memory cgroup (memcg) and rss accounting ======================================= diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index 602e3a52884d..79aaf8768be3 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -518,6 +518,8 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr, page = migration_entry_to_page(swpent); else if (is_device_private_entry(swpent)) page = device_private_entry_to_page(swpent); + else if (is_device_exclusive_entry(swpent)) + page = device_exclusive_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, @@ -695,6 +697,8 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask, page = migration_entry_to_page(swpent); else if (is_device_private_entry(swpent)) page = device_private_entry_to_page(swpent); + else if (is_device_exclusive_entry(swpent)) + page = device_exclusive_entry_to_page(swpent); } if (page) { int mapcount = page_mapcount(page); @@ -1387,6 +1391,9 @@ static pagemap_entry_t pte_to_pagemap_entry(struct pagemapread *pm, if (is_device_private_entry(entry)) page = device_private_entry_to_page(entry); + + if (is_device_exclusive_entry(entry)) + page = device_exclusive_entry_to_page(entry); } if (page && !PageAnon(page)) diff --git a/include/linux/hmm.h b/include/linux/hmm.h index 866a0fa104c4..5d28ff6d4d80 100644 --- a/include/linux/hmm.h +++ b/include/linux/hmm.h @@ -109,6 +109,10 @@ struct hmm_range { */ int hmm_range_fault(struct hmm_range *range); +int hmm_exclusive_range(struct mm_struct *mm, unsigned long start, + unsigned long end, struct page **pages); +vm_fault_t hmm_remove_exclusive_entry(struct vm_fault *vmf); + /* * HMM_RANGE_DEFAULT_TIMEOUT - default timeout (ms) when waiting for a range * diff --git a/include/linux/rmap.h b/include/linux/rmap.h index 70085ca1a3fc..5503fc4d1138 100644 --- a/include/linux/rmap.h +++ b/include/linux/rmap.h @@ -98,6 +98,7 @@ enum ttu_flags { TTU_RMAP_LOCKED = 0x80, /* do not grab rmap lock: * caller holds it */ TTU_SPLIT_FREEZE = 0x100, /* freeze pte under splitting thp */ + TTU_EXCLUSIVE = 0x200, /* replace with exclusive access */ }; #ifdef CONFIG_MMU diff --git a/include/linux/swap.h b/include/linux/swap.h index 596bc2f4d9b0..9803753b0302 100644 --- a/include/linux/swap.h +++ b/include/linux/swap.h @@ -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 @@ -519,8 +521,10 @@ 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));}) +#define free_swap_and_cache(e) ({(is_migration_entry(e) || is_device_private_entry(e) \ + || is_device_exclusive_entry(e)); }) +#define swapcache_prepare(e) ({(is_migration_entry(e) || is_device_private_entry(e) \ + || is_device_exclusive_entry(e)); }) static inline int add_swap_count_continuation(swp_entry_t swp, gfp_t gfp_mask) { diff --git a/include/linux/swapops.h b/include/linux/swapops.h index d9b7c9132c2f..a307134bfee5 100644 --- a/include/linux/swapops.h +++ b/include/linux/swapops.h @@ -122,6 +122,38 @@ static inline bool is_write_device_private_entry(swp_entry_t entry) return unlikely(swp_type(entry) == SWP_DEVICE_WRITE); } +static inline void make_device_exclusive_entry_read(swp_entry_t *entry) +{ + *entry = swp_entry(SWP_DEVICE_EXCLUSIVE_READ, swp_offset(*entry)); +} + +static inline swp_entry_t make_device_exclusive_entry(struct page *page, bool write) +{ + return swp_entry(write ? SWP_DEVICE_EXCLUSIVE_WRITE : SWP_DEVICE_EXCLUSIVE_READ, + page_to_pfn(page)); +} + +static inline bool is_device_exclusive_entry(swp_entry_t entry) +{ + int type = swp_type(entry); + return type == SWP_DEVICE_EXCLUSIVE_READ || type == SWP_DEVICE_EXCLUSIVE_WRITE; +} + +static inline bool is_write_device_exclusive_entry(swp_entry_t entry) +{ + return swp_type(entry) == SWP_DEVICE_EXCLUSIVE_WRITE; +} + +static inline unsigned long device_exclusive_entry_to_pfn(swp_entry_t entry) +{ + return swp_offset(entry); +} + +static inline struct page *device_exclusive_entry_to_page(swp_entry_t entry) +{ + return pfn_to_page(swp_offset(entry)); +} + static inline unsigned long device_private_entry_to_pfn(swp_entry_t entry) { return swp_offset(entry); diff --git a/mm/hmm.c b/mm/hmm.c index 943cb2ba4442..90cdf99bee52 100644 --- a/mm/hmm.c +++ b/mm/hmm.c @@ -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; @@ -272,6 +274,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; @@ -593,3 +598,204 @@ int hmm_range_fault(struct hmm_range *range) return ret; } EXPORT_SYMBOL(hmm_range_fault); + +struct hmm_exclusive_walk { + struct page **pages; + int npages; +}; + +static int hmm_exclusive_skip(unsigned long start, + unsigned long end, + __always_unused int depth, + struct mm_walk *walk) +{ + struct hmm_exclusive_walk *hmm_exclusive_walk = walk->private; + unsigned long addr; + + for (addr = start; addr < end; addr += PAGE_SIZE) + hmm_exclusive_walk->pages[hmm_exclusive_walk->npages++] = NULL; + + return 0; +} + +static int hmm_exclusive_pmd(pmd_t *pmdp, unsigned long addr, unsigned long end, + struct mm_walk *walk) +{ + struct hmm_exclusive_walk *hmm_exclusive_walk = walk->private; + struct vm_area_struct *vma = walk->vma; + struct mm_struct *mm = vma->vm_mm; + spinlock_t *ptl; + pte_t *ptep; + + if (pmd_trans_huge(*pmdp)) { + int ret; + struct page *page; + + ptl = pmd_lock(mm, pmdp); + if (unlikely(!pmd_trans_huge(*pmdp))) { + spin_unlock(ptl); + walk->action = ACTION_AGAIN; + return 0; + } + + page = pmd_page(*pmdp); + if (is_huge_zero_page(page)) { + spin_unlock(ptl); + return hmm_exclusive_skip(addr, end, -1, walk); + } + + get_page(page); + spin_unlock(ptl); + if (unlikely(!trylock_page(page))) + return hmm_exclusive_skip(addr, end, -1, walk); + + ret = split_huge_page(page); + unlock_page(page); + put_page(page); + if (ret || pmd_none(*pmdp)) + return hmm_exclusive_skip(addr, end, -1, walk); + } + + if (unlikely(pmd_bad(*pmdp))) + return hmm_exclusive_skip(addr, end, -1, walk); + + ptep = pte_offset_map_lock(mm, pmdp, addr, &ptl); + for (; addr < end; addr += PAGE_SIZE, ptep++) { + struct page *page; + pte_t pte = *ptep; + + if (pte_none(pte) || !pte_present(pte) || + is_zero_pfn(pte_pfn(pte))) + continue; + + page = vm_normal_page(vma, addr, pte); + if (!page) + continue; + + get_page(page); + if (!trylock_page(page)) { + put_page(page); + continue; + } + + hmm_exclusive_walk->pages[hmm_exclusive_walk->npages++] = page; + } + + pte_unmap_unlock(ptep - 1, ptl); + return 0; +} + +static const struct mm_walk_ops hmm_exclusive_walk_ops = { + .pmd_entry = hmm_exclusive_pmd, + .pte_hole = hmm_exclusive_skip, +}; + +/** + * hmm_exclusive_range() - Mark a range for exclusive use by a device + * @mm: mm_struct of assoicated target process + * @start: start of the region to mark for exclusive device access + * @end: end address of region + * @pages: returns the pages which were successfully mark for exclusive acces + * + * Returns: number of pages successfully marked for exclusive access + * + * This function finds the ptes mapping page(s) to the given address range and + * replaces them with special swap entries preventing userspace CPU access. On + * fault these entries are replaced with the original mapping after calling MMU + * notifiers. + */ +int hmm_exclusive_range(struct mm_struct *mm, unsigned long start, + unsigned long end, struct page **pages) +{ + struct hmm_exclusive_walk hmm_exclusive_walk = { .pages = pages, .npages = 0 }; + int i; + + /* Collect and lock candidate pages */ + walk_page_range(mm, start, end, &hmm_exclusive_walk_ops, &hmm_exclusive_walk); + + for (i = 0; i < hmm_exclusive_walk.npages; i++) + if (pages[i]) + if (!try_to_unmap(pages[i], TTU_EXCLUSIVE | TTU_IGNORE_MLOCK)) { + unlock_page(pages[i]); + put_page(pages[i]); + pages[i] = NULL; + } + + return hmm_exclusive_walk.npages; +} +EXPORT_SYMBOL_GPL(hmm_exclusive_range); + +/* + * Restore a potential migration pte to a working pte entry + */ +vm_fault_t hmm_remove_exclusive_entry(struct vm_fault *vmf) +{ + struct page *page = vmf->page; + struct vm_area_struct *vma = vmf->vma; + struct page_vma_mapped_walk pvmw = { + .page = page, + .vma = vma, + .address = vmf->address, + .flags = PVMW_SYNC, + }; + pte_t pte; + vm_fault_t ret = 0; + struct mmu_notifier_range range; + + lock_page(page); + mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, vma->vm_mm, + vmf->address & PAGE_MASK, + (vmf->address & PAGE_MASK) + PAGE_SIZE); + mmu_notifier_invalidate_range_start(&range); + + while (page_vma_mapped_walk(&pvmw)) { + swp_entry_t entry; + + if (unlikely(!pte_same(*pvmw.pte, vmf->orig_pte))) { + page_vma_mapped_walk_done(&pvmw); + break; + } + +#if defined(CONFIG_ARCH_ENABLE_THP_MIGRATION) || defined(CONFIG_HUGETLB) + if (PageTransHuge(page)) { + VM_BUG_ON_PAGE(1, page); + continue; + } +#endif + + pte = pte_mkold(mk_pte(page, READ_ONCE(vma->vm_page_prot))); + if (pte_swp_soft_dirty(*pvmw.pte)) + pte = pte_mksoft_dirty(pte); + + entry = pte_to_swp_entry(*pvmw.pte); + if (pte_swp_uffd_wp(*pvmw.pte)) + pte = pte_mkuffd_wp(pte); + else if (is_write_device_exclusive_entry(entry)) + pte = maybe_mkwrite(pte_mkdirty(pte), vma); + + set_pte_at(vma->vm_mm, pvmw.address, pvmw.pte, pte); + + /* + * No need to take a page reference as one was already + * created when the swap entry was made. + */ + if (PageAnon(page)) + page_add_anon_rmap(page, vma, pvmw.address, false); + else + page_add_file_rmap(page, false); + + if (vma->vm_flags & VM_LOCKED) + mlock_vma_page(page); + + /* + * No need to invalidate - it was non-present before. However + * secondary CPUs may have mappings that need invalidating. + */ + update_mmu_cache(vma, pvmw.address, pvmw.pte); + } + + unlock_page(page); + + mmu_notifier_invalidate_range_end(&range); + return ret; +} diff --git a/mm/memory.c b/mm/memory.c index feff48e1465a..ed03a37f25cb 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -73,6 +73,7 @@ #include <linux/perf_event.h> #include <linux/ptrace.h> #include <linux/vmalloc.h> +#include <linux/hmm.h> #include <trace/events/kmem.h> @@ -736,9 +737,29 @@ 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_private_entry(entry)) { + } else if (is_device_exclusive_entry(entry)) { page = device_private_entry_to_page(entry); + get_page(page); + rss[mm_counter(page)]++; + + if (is_write_device_exclusive_entry(entry) && + is_cow_mapping(vm_flags)) { + /* + * COW mappings require pages in both + * parent and child to be set to read. + */ + make_device_exclusive_entry_read(&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); + } + } else if (is_device_private_entry(entry)) { + page = device_exclusive_entry_to_page(entry); + /* * Update rss count even for unaddressable pages, as * they should treated just like normal pages in this @@ -1273,7 +1294,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, } entry = pte_to_swp_entry(ptent); - if (is_device_private_entry(entry)) { + if (is_device_private_entry(entry) || + is_device_exclusive_entry(entry)) { struct page *page = device_private_entry_to_page(entry); if (unlikely(details && details->check_mapping)) { @@ -1289,7 +1311,10 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, pte_clear_not_present_full(mm, addr, pte, tlb->fullmm); rss[mm_counter(page)]--; - page_remove_rmap(page, false); + + if (is_device_private_entry(entry)) + page_remove_rmap(page, false); + put_page(page); continue; } @@ -3270,6 +3295,9 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) if (is_migration_entry(entry)) { migration_entry_wait(vma->vm_mm, vmf->pmd, vmf->address); + } else if (is_device_exclusive_entry(entry)) { + vmf->page = device_exclusive_entry_to_page(entry); + ret = hmm_remove_exclusive_entry(vmf); } else if (is_device_private_entry(entry)) { vmf->page = device_private_entry_to_page(entry); ret = vmf->page->pgmap->ops->migrate_to_ram(vmf); diff --git a/mm/mprotect.c b/mm/mprotect.c index ab709023e9aa..f93967211e52 100644 --- a/mm/mprotect.c +++ b/mm/mprotect.c @@ -163,6 +163,13 @@ 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_write_device_exclusive_entry(entry)) { + make_device_exclusive_entry_read(&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; } diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c index 86e3a3688d59..ff65a2c35996 100644 --- a/mm/page_vma_mapped.c +++ b/mm/page_vma_mapped.c @@ -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,16 +94,18 @@ 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)) + pfn = migration_entry_to_pfn(entry); + else if (is_device_exclusive_entry(entry)) + pfn = device_exclusive_entry_to_pfn(entry); + else return false; - - pfn = migration_entry_to_pfn(entry); } else if (is_swap_pte(*pvmw->pte)) { swp_entry_t entry; /* 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 = device_private_entry_to_pfn(entry); @@ -216,6 +219,7 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw) } if (!map_pte(pvmw)) goto next_pte; + while (1) { if (check_pte(pvmw)) return true; diff --git a/mm/rmap.c b/mm/rmap.c index 08c56aaf72eb..e4d03f7a8c22 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -1395,7 +1395,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, if ((flags & TTU_MUNLOCK) && !(vma->vm_flags & VM_LOCKED)) return true; - if (IS_ENABLED(CONFIG_MIGRATION) && (flags & TTU_MIGRATION) && + if (IS_ENABLED(CONFIG_MIGRATION) && (flags & (TTU_MIGRATION | TTU_EXCLUSIVE)) && is_zone_device_page(page) && !is_device_private_page(page)) return true; @@ -1591,6 +1591,33 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, /* We have to invalidate as we cleared the pte */ mmu_notifier_invalidate_range(mm, address, address + PAGE_SIZE); + } else if (flags & TTU_EXCLUSIVE) { + swp_entry_t entry; + pte_t swp_pte; + + if (arch_unmap_one(mm, vma, address, pteval) < 0) { + set_pte_at(mm, address, pvmw.pte, pteval); + ret = false; + page_vma_mapped_walk_done(&pvmw); + break; + } + + /* + * Store the pfn of the page in a special migration + * pte. do_swap_page() will wait until the migration + * pte is removed and then restart fault handling. + */ + entry = make_device_exclusive_entry(subpage, + pte_write(pteval)); + swp_pte = swp_entry_to_pte(entry); + if (pte_soft_dirty(pteval)) + swp_pte = pte_swp_mksoft_dirty(swp_pte); + if (pte_uffd_wp(pteval)) + swp_pte = pte_swp_mkuffd_wp(swp_pte); + + /* Take a reference for the swap entry */ + get_page(page); + set_pte_at(mm, address, pvmw.pte, swp_pte); } else if (IS_ENABLED(CONFIG_MIGRATION) && (flags & (TTU_MIGRATION|TTU_SPLIT_FREEZE))) { swp_entry_t entry; -- 2.20.1
kernel test robot
2021-Feb-19 04:04 UTC
[Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access
Hi Alistair, Thank you for the patch! Yet something to improve: [auto build test ERROR on kselftest/next] [also build test ERROR on linus/master v5.11 next-20210218] [cannot apply to hnaz-linux-mm/master] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Alistair-Popple/Add-support-for-SVM-atomics-in-Nouveau/20210219-100858 base: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git next config: mips-randconfig-r036-20210218 (attached as .config) compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c9439ca36342fb6013187d0a69aef92736951476) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install mips cross compiling tool for clang build # apt-get install binutils-mips-linux-gnu # https://github.com/0day-ci/linux/commit/bb5444811772d30b2e3bbaa44baeb8a4b3f03cec git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Alistair-Popple/Add-support-for-SVM-atomics-in-Nouveau/20210219-100858 git checkout bb5444811772d30b2e3bbaa44baeb8a4b3f03cec # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp at intel.com> All error/warnings (new ones prefixed by >>):>> fs/proc/task_mmu.c:521:12: error: implicit declaration of function 'is_device_exclusive_entry' [-Werror,-Wimplicit-function-declaration]else if (is_device_exclusive_entry(swpent)) ^ fs/proc/task_mmu.c:521:12: note: did you mean 'is_device_private_entry'? include/linux/swapops.h:176:20: note: 'is_device_private_entry' declared here static inline bool is_device_private_entry(swp_entry_t entry) ^>> fs/proc/task_mmu.c:522:11: error: implicit declaration of function 'device_exclusive_entry_to_page' [-Werror,-Wimplicit-function-declaration]page = device_exclusive_entry_to_page(swpent); ^ fs/proc/task_mmu.c:522:11: note: did you mean 'device_private_entry_to_page'? include/linux/swapops.h:191:28: note: 'device_private_entry_to_page' declared here static inline struct page *device_private_entry_to_page(swp_entry_t entry) ^>> fs/proc/task_mmu.c:522:9: warning: incompatible integer to pointer conversion assigning to 'struct page *' from 'int' [-Wint-conversion]page = device_exclusive_entry_to_page(swpent); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ fs/proc/task_mmu.c:1395:7: error: implicit declaration of function 'is_device_exclusive_entry' [-Werror,-Wimplicit-function-declaration] if (is_device_exclusive_entry(entry)) ^ fs/proc/task_mmu.c:1396:11: error: implicit declaration of function 'device_exclusive_entry_to_page' [-Werror,-Wimplicit-function-declaration] page = device_exclusive_entry_to_page(entry); ^ fs/proc/task_mmu.c:1396:9: warning: incompatible integer to pointer conversion assigning to 'struct page *' from 'int' [-Wint-conversion] page = device_exclusive_entry_to_page(entry); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2 warnings and 4 errors generated. vim +/is_device_exclusive_entry +521 fs/proc/task_mmu.c 490 491 static void smaps_pte_entry(pte_t *pte, unsigned long addr, 492 struct mm_walk *walk) 493 { 494 struct mem_size_stats *mss = walk->private; 495 struct vm_area_struct *vma = walk->vma; 496 bool locked = !!(vma->vm_flags & VM_LOCKED); 497 struct page *page = NULL; 498 499 if (pte_present(*pte)) { 500 page = vm_normal_page(vma, addr, *pte); 501 } else if (is_swap_pte(*pte)) { 502 swp_entry_t swpent = pte_to_swp_entry(*pte); 503 504 if (!non_swap_entry(swpent)) { 505 int mapcount; 506 507 mss->swap += PAGE_SIZE; 508 mapcount = swp_swapcount(swpent); 509 if (mapcount >= 2) { 510 u64 pss_delta = (u64)PAGE_SIZE << PSS_SHIFT; 511 512 do_div(pss_delta, mapcount); 513 mss->swap_pss += pss_delta; 514 } else { 515 mss->swap_pss += (u64)PAGE_SIZE << PSS_SHIFT; 516 } 517 } else if (is_migration_entry(swpent)) 518 page = migration_entry_to_page(swpent); 519 else if (is_device_private_entry(swpent)) 520 page = device_private_entry_to_page(swpent); > 521 else if (is_device_exclusive_entry(swpent)) > 522 page = device_exclusive_entry_to_page(swpent); 523 } else if (unlikely(IS_ENABLED(CONFIG_SHMEM) && mss->check_shmem_swap 524 && pte_none(*pte))) { 525 page = xa_load(&vma->vm_file->f_mapping->i_pages, 526 linear_page_index(vma, addr)); 527 if (xa_is_value(page)) 528 mss->swap += PAGE_SIZE; 529 return; 530 } 531 532 if (!page) 533 return; 534 535 smaps_account(mss, page, false, pte_young(*pte), pte_dirty(*pte), locked); 536 } 537 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all at lists.01.org -------------- next part -------------- A non-text attachment was scrubbed... Name: .config.gz Type: application/gzip Size: 29578 bytes Desc: not available URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20210219/0909472c/attachment-0001.gz>
kernel test robot
2021-Feb-19 05:09 UTC
[Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access
Hi Alistair, Thank you for the patch! Yet something to improve: [auto build test ERROR on kselftest/next] [also build test ERROR on linus/master v5.11 next-20210218] [cannot apply to hnaz-linux-mm/master] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Alistair-Popple/Add-support-for-SVM-atomics-in-Nouveau/20210219-100858 base: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git next config: x86_64-randconfig-s021-20210217 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.3-215-g0fb77bb6-dirty # https://github.com/0day-ci/linux/commit/bb5444811772d30b2e3bbaa44baeb8a4b3f03cec git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Alistair-Popple/Add-support-for-SVM-atomics-in-Nouveau/20210219-100858 git checkout bb5444811772d30b2e3bbaa44baeb8a4b3f03cec # save the attached .config to linux build tree make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp at intel.com> All errors (new ones prefixed by >>): ld: warning: orphan section `.data..decrypted' from `arch/x86/kernel/cpu/vmware.o' being placed in section `.data..decrypted' ld: warning: orphan section `.data..decrypted' from `arch/x86/kernel/kvm.o' being placed in section `.data..decrypted' ld: mm/memory.o: in function `do_swap_page':>> mm/memory.c:3300: undefined reference to `hmm_remove_exclusive_entry'vim +3300 mm/memory.c 3270 3271 /* 3272 * We enter with non-exclusive mmap_lock (to exclude vma changes, 3273 * but allow concurrent faults), and pte mapped but not yet locked. 3274 * We return with pte unmapped and unlocked. 3275 * 3276 * We return with the mmap_lock locked or unlocked in the same cases 3277 * as does filemap_fault(). 3278 */ 3279 vm_fault_t do_swap_page(struct vm_fault *vmf) 3280 { 3281 struct vm_area_struct *vma = vmf->vma; 3282 struct page *page = NULL, *swapcache; 3283 swp_entry_t entry; 3284 pte_t pte; 3285 int locked; 3286 int exclusive = 0; 3287 vm_fault_t ret = 0; 3288 void *shadow = NULL; 3289 3290 if (!pte_unmap_same(vma->vm_mm, vmf->pmd, vmf->pte, vmf->orig_pte)) 3291 goto out; 3292 3293 entry = pte_to_swp_entry(vmf->orig_pte); 3294 if (unlikely(non_swap_entry(entry))) { 3295 if (is_migration_entry(entry)) { 3296 migration_entry_wait(vma->vm_mm, vmf->pmd, 3297 vmf->address); 3298 } else if (is_device_exclusive_entry(entry)) { 3299 vmf->page = device_exclusive_entry_to_page(entry);> 3300 ret = hmm_remove_exclusive_entry(vmf);3301 } else if (is_device_private_entry(entry)) { 3302 vmf->page = device_private_entry_to_page(entry); 3303 ret = vmf->page->pgmap->ops->migrate_to_ram(vmf); 3304 } else if (is_hwpoison_entry(entry)) { 3305 ret = VM_FAULT_HWPOISON; 3306 } else { 3307 print_bad_pte(vma, vmf->address, vmf->orig_pte, NULL); 3308 ret = VM_FAULT_SIGBUS; 3309 } 3310 goto out; 3311 } 3312 3313 3314 delayacct_set_flag(DELAYACCT_PF_SWAPIN); 3315 page = lookup_swap_cache(entry, vma, vmf->address); 3316 swapcache = page; 3317 3318 if (!page) { 3319 struct swap_info_struct *si = swp_swap_info(entry); 3320 3321 if (data_race(si->flags & SWP_SYNCHRONOUS_IO) && 3322 __swap_count(entry) == 1) { 3323 /* skip swapcache */ 3324 page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma, 3325 vmf->address); 3326 if (page) { 3327 int err; 3328 3329 __SetPageLocked(page); 3330 __SetPageSwapBacked(page); 3331 set_page_private(page, entry.val); 3332 3333 /* Tell memcg to use swap ownership records */ 3334 SetPageSwapCache(page); 3335 err = mem_cgroup_charge(page, vma->vm_mm, 3336 GFP_KERNEL); 3337 ClearPageSwapCache(page); 3338 if (err) { 3339 ret = VM_FAULT_OOM; 3340 goto out_page; 3341 } 3342 3343 shadow = get_shadow_from_swap_cache(entry); 3344 if (shadow) 3345 workingset_refault(page, shadow); 3346 3347 lru_cache_add(page); 3348 swap_readpage(page, true); 3349 } 3350 } else { 3351 page = swapin_readahead(entry, GFP_HIGHUSER_MOVABLE, 3352 vmf); 3353 swapcache = page; 3354 } 3355 3356 if (!page) { 3357 /* 3358 * Back out if somebody else faulted in this pte 3359 * while we released the pte lock. 3360 */ 3361 vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, 3362 vmf->address, &vmf->ptl); 3363 if (likely(pte_same(*vmf->pte, vmf->orig_pte))) 3364 ret = VM_FAULT_OOM; 3365 delayacct_clear_flag(DELAYACCT_PF_SWAPIN); 3366 goto unlock; 3367 } 3368 3369 /* Had to read the page from swap area: Major fault */ 3370 ret = VM_FAULT_MAJOR; 3371 count_vm_event(PGMAJFAULT); 3372 count_memcg_event_mm(vma->vm_mm, PGMAJFAULT); 3373 } else if (PageHWPoison(page)) { 3374 /* 3375 * hwpoisoned dirty swapcache pages are kept for killing 3376 * owner processes (which may be unknown at hwpoison time) 3377 */ 3378 ret = VM_FAULT_HWPOISON; 3379 delayacct_clear_flag(DELAYACCT_PF_SWAPIN); 3380 goto out_release; 3381 } 3382 3383 locked = lock_page_or_retry(page, vma->vm_mm, vmf->flags); 3384 3385 delayacct_clear_flag(DELAYACCT_PF_SWAPIN); 3386 if (!locked) { 3387 ret |= VM_FAULT_RETRY; 3388 goto out_release; 3389 } 3390 3391 /* 3392 * Make sure try_to_free_swap or reuse_swap_page or swapoff did not 3393 * release the swapcache from under us. The page pin, and pte_same 3394 * test below, are not enough to exclude that. Even if it is still 3395 * swapcache, we need to check that the page's swap has not changed. 3396 */ 3397 if (unlikely((!PageSwapCache(page) || 3398 page_private(page) != entry.val)) && swapcache) 3399 goto out_page; 3400 3401 page = ksm_might_need_to_copy(page, vma, vmf->address); 3402 if (unlikely(!page)) { 3403 ret = VM_FAULT_OOM; 3404 page = swapcache; 3405 goto out_page; 3406 } 3407 3408 cgroup_throttle_swaprate(page, GFP_KERNEL); 3409 3410 /* 3411 * Back out if somebody else already faulted in this pte. 3412 */ 3413 vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address, 3414 &vmf->ptl); 3415 if (unlikely(!pte_same(*vmf->pte, vmf->orig_pte))) 3416 goto out_nomap; 3417 3418 if (unlikely(!PageUptodate(page))) { 3419 ret = VM_FAULT_SIGBUS; 3420 goto out_nomap; 3421 } 3422 3423 /* 3424 * The page isn't present yet, go ahead with the fault. 3425 * 3426 * Be careful about the sequence of operations here. 3427 * To get its accounting right, reuse_swap_page() must be called 3428 * while the page is counted on swap but not yet in mapcount i.e. 3429 * before page_add_anon_rmap() and swap_free(); try_to_free_swap() 3430 * must be called after the swap_free(), or it will never succeed. 3431 */ 3432 3433 inc_mm_counter_fast(vma->vm_mm, MM_ANONPAGES); 3434 dec_mm_counter_fast(vma->vm_mm, MM_SWAPENTS); 3435 pte = mk_pte(page, vma->vm_page_prot); 3436 if ((vmf->flags & FAULT_FLAG_WRITE) && reuse_swap_page(page, NULL)) { 3437 pte = maybe_mkwrite(pte_mkdirty(pte), vma); 3438 vmf->flags &= ~FAULT_FLAG_WRITE; 3439 ret |= VM_FAULT_WRITE; 3440 exclusive = RMAP_EXCLUSIVE; 3441 } 3442 flush_icache_page(vma, page); 3443 if (pte_swp_soft_dirty(vmf->orig_pte)) 3444 pte = pte_mksoft_dirty(pte); 3445 if (pte_swp_uffd_wp(vmf->orig_pte)) { 3446 pte = pte_mkuffd_wp(pte); 3447 pte = pte_wrprotect(pte); 3448 } 3449 set_pte_at(vma->vm_mm, vmf->address, vmf->pte, pte); 3450 arch_do_swap_page(vma->vm_mm, vma, vmf->address, pte, vmf->orig_pte); 3451 vmf->orig_pte = pte; 3452 3453 /* ksm created a completely new copy */ 3454 if (unlikely(page != swapcache && swapcache)) { 3455 page_add_new_anon_rmap(page, vma, vmf->address, false); 3456 lru_cache_add_inactive_or_unevictable(page, vma); 3457 } else { 3458 do_page_add_anon_rmap(page, vma, vmf->address, exclusive); 3459 } 3460 3461 swap_free(entry); 3462 if (mem_cgroup_swap_full(page) || 3463 (vma->vm_flags & VM_LOCKED) || PageMlocked(page)) 3464 try_to_free_swap(page); 3465 unlock_page(page); 3466 if (page != swapcache && swapcache) { 3467 /* 3468 * Hold the lock to avoid the swap entry to be reused 3469 * until we take the PT lock for the pte_same() check 3470 * (to avoid false positives from pte_same). For 3471 * further safety release the lock after the swap_free 3472 * so that the swap count won't change under a 3473 * parallel locked swapcache. 3474 */ 3475 unlock_page(swapcache); 3476 put_page(swapcache); 3477 } 3478 3479 if (vmf->flags & FAULT_FLAG_WRITE) { 3480 ret |= do_wp_page(vmf); 3481 if (ret & VM_FAULT_ERROR) 3482 ret &= VM_FAULT_ERROR; 3483 goto out; 3484 } 3485 3486 /* No need to invalidate - it was non-present before */ 3487 update_mmu_cache(vma, vmf->address, vmf->pte); 3488 unlock: 3489 pte_unmap_unlock(vmf->pte, vmf->ptl); 3490 out: 3491 return ret; 3492 out_nomap: 3493 pte_unmap_unlock(vmf->pte, vmf->ptl); 3494 out_page: 3495 unlock_page(page); 3496 out_release: 3497 put_page(page); 3498 if (page != swapcache && swapcache) { 3499 unlock_page(swapcache); 3500 put_page(swapcache); 3501 } 3502 return ret; 3503 } 3504 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all at lists.01.org -------------- next part -------------- A non-text attachment was scrubbed... Name: .config.gz Type: application/gzip Size: 33708 bytes Desc: not available URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20210219/a77d2675/attachment-0001.gz>
Christoph Hellwig
2021-Feb-19 09:47 UTC
[Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access
> page = migration_entry_to_page(swpent); > else if (is_device_private_entry(swpent)) > page = device_private_entry_to_page(swpent); > + else if (is_device_exclusive_entry(swpent)) > + page = device_exclusive_entry_to_page(swpent);> page = migration_entry_to_page(swpent); > else if (is_device_private_entry(swpent)) > page = device_private_entry_to_page(swpent); > + else if (is_device_exclusive_entry(swpent)) > + page = device_exclusive_entry_to_page(swpent);> if (is_device_private_entry(entry)) > page = device_private_entry_to_page(entry); > + > + if (is_device_exclusive_entry(entry)) > + page = device_exclusive_entry_to_page(entry);Any chance we can come up with a clever scheme to avoid all this boilerplate code (and maybe also what it gets compiled to)?> diff --git a/include/linux/hmm.h b/include/linux/hmm.h > index 866a0fa104c4..5d28ff6d4d80 100644 > --- a/include/linux/hmm.h > +++ b/include/linux/hmm.h > @@ -109,6 +109,10 @@ struct hmm_range { > */ > int hmm_range_fault(struct hmm_range *range); > > +int hmm_exclusive_range(struct mm_struct *mm, unsigned long start, > + unsigned long end, struct page **pages); > +vm_fault_t hmm_remove_exclusive_entry(struct vm_fault *vmf);Can we avoid the hmm naming for new code (we should probably also kill it off for the existing code)?> +#define free_swap_and_cache(e) ({(is_migration_entry(e) || is_device_private_entry(e) \ > + || is_device_exclusive_entry(e)); }) > +#define swapcache_prepare(e) ({(is_migration_entry(e) || is_device_private_entry(e) \ > + || is_device_exclusive_entry(e)); })Can you turn these into properly formatted inline functions? As-is this becomes pretty unreadable.> +static inline void make_device_exclusive_entry_read(swp_entry_t *entry) > +{ > + *entry = swp_entry(SWP_DEVICE_EXCLUSIVE_READ, swp_offset(*entry)); > +}s/make_device_exclusive_entry_read/mark_device_exclusive_entry_readable/ ??> + > +static inline swp_entry_t make_device_exclusive_entry(struct page *page, bool write) > +{ > + return swp_entry(write ? SWP_DEVICE_EXCLUSIVE_WRITE : SWP_DEVICE_EXCLUSIVE_READ, > + page_to_pfn(page)); > +}I'd split this into two helpers, which is easier to follow and avoids the pointlessly overlong lines.> +static inline bool is_device_exclusive_entry(swp_entry_t entry) > +{ > + int type = swp_type(entry); > + return type == SWP_DEVICE_EXCLUSIVE_READ || type == SWP_DEVICE_EXCLUSIVE_WRITE; > +}Another overly long line. I also wouldn't bother with the local variable: return swp_type(entry) == SWP_DEVICE_EXCLUSIVE_READ || swp_type(entry) == SWP_DEVICE_EXCLUSIVE_WRITE;> +static inline bool is_write_device_exclusive_entry(swp_entry_t entry) > +{ > + return swp_type(entry) == SWP_DEVICE_EXCLUSIVE_WRITE; > +}Or reuse these kind of helpers..> + > +static inline unsigned long device_exclusive_entry_to_pfn(swp_entry_t entry) > +{ > + return swp_offset(entry); > +} > + > +static inline struct page *device_exclusive_entry_to_page(swp_entry_t entry) > +{ > + return pfn_to_page(swp_offset(entry)); > +}I'd rather open code these two, and as a prep patch also kill off the equivalents for the migration and device private entries, which would actually clean up a lot of the mess mentioned in my first comment above.> +static int hmm_exclusive_skip(unsigned long start, > + unsigned long end, > + __always_unused int depth, > + struct mm_walk *walk) > +{ > + struct hmm_exclusive_walk *hmm_exclusive_walk = walk->private; > + unsigned long addr; > + > + for (addr = start; addr < end; addr += PAGE_SIZE) > + hmm_exclusive_walk->pages[hmm_exclusive_walk->npages++] = NULL; > + > + return 0; > +}Wouldn't pre-zeroing the array be simpler and more efficient?> +int hmm_exclusive_range(struct mm_struct *mm, unsigned long start, > + unsigned long end, struct page **pages) > +{ > + struct hmm_exclusive_walk hmm_exclusive_walk = { .pages = pages, .npages = 0 }; > + int i; > + > + /* Collect and lock candidate pages */ > + walk_page_range(mm, start, end, &hmm_exclusive_walk_ops, &hmm_exclusive_walk);Please avoid the overly long lines. But more importantly: Unless I'm missing something obvious this walk_page_range call just open codes get_user_pages_fast, why can't you use that?> +#if defined(CONFIG_ARCH_ENABLE_THP_MIGRATION) || defined(CONFIG_HUGETLB) > + if (PageTransHuge(page)) { > + VM_BUG_ON_PAGE(1, page); > + continue; > + } > +#endifDoesn't PageTransHuge always return false for that case? If not shouldn't we make sure it does?> + > + pte = pte_mkold(mk_pte(page, READ_ONCE(vma->vm_page_prot))); > + if (pte_swp_soft_dirty(*pvmw.pte)) > + pte = pte_mksoft_dirty(pte); > + > + entry = pte_to_swp_entry(*pvmw.pte); > + if (pte_swp_uffd_wp(*pvmw.pte)) > + pte = pte_mkuffd_wp(pte); > + else if (is_write_device_exclusive_entry(entry)) > + pte = maybe_mkwrite(pte_mkdirty(pte), vma); > + > + set_pte_at(vma->vm_mm, pvmw.address, pvmw.pte, pte); > + > + /* > + * No need to take a page reference as one was already > + * created when the swap entry was made. > + */ > + if (PageAnon(page)) > + page_add_anon_rmap(page, vma, pvmw.address, false); > + else > + page_add_file_rmap(page, false); > + > + if (vma->vm_flags & VM_LOCKED) > + mlock_vma_page(page); > + > + /* > + * No need to invalidate - it was non-present before. However > + * secondary CPUs may have mappings that need invalidating. > + */ > + update_mmu_cache(vma, pvmw.address, pvmw.pte);It would be nice to split out the body of this loop into a helper.> + if (!is_device_private_entry(entry) && > + !is_device_exclusive_entry(entry))The normal style for this would be: if (!is_device_private_entry(entry) && !is_device_exclusive_entry(entry))> - if (!is_device_private_entry(entry)) > + if (!is_device_private_entry(entry) && !is_device_exclusive_entry(entry))Plase split this into two lines.> @@ -216,6 +219,7 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw) > } > if (!map_pte(pvmw)) > goto next_pte; > + > while (1) { > if (check_pte(pvmw)) > return true;Spurious whitespace change.> - if (IS_ENABLED(CONFIG_MIGRATION) && (flags & TTU_MIGRATION) && > + if (IS_ENABLED(CONFIG_MIGRATION) && (flags & (TTU_MIGRATION | TTU_EXCLUSIVE)) &&Please split this into two lines.> is_zone_device_page(page) && !is_device_private_page(page)) > return true; > > @@ -1591,6 +1591,33 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, > /* We have to invalidate as we cleared the pte */ > mmu_notifier_invalidate_range(mm, address, > address + PAGE_SIZE); > + } else if (flags & TTU_EXCLUSIVE) {try_to_unmap_one has turned into a monster. A little refactoring to split it into managable pieces would be nice.