* [Nouveau] [PATCH v2 0/4] Add support for SVM atomics in Nouveau
@ 2021-02-19 2:07 Alistair Popple
2021-02-19 2:07 ` [Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access Alistair Popple
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Alistair Popple @ 2021-02-19 2:07 UTC (permalink / raw)
To: linux-mm, nouveau, bskeggs, akpm
Cc: rcampbell, daniel, linux-doc, Alistair Popple, linux-kernel,
dri-devel, hch, kvm-ppc, jgg
This is the second version of a series to add support to Nouveau for atomic
memory operations on OpenCL shared virtual memory (SVM) regions. This is
achieved using the atomic PTE bits on the GPU to only permit atomic
operations to system memory when a page is not mapped in userspace on the
CPU. The previous version of this series used an unmap and pin page
migration, however this resulted in problems with ZONE_MOVABLE and other
issues so this series uses a different approach.
Instead 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.
Patch 1 contains the bulk of the memory management changes required to
implement the new entry type.
Patch 2 contains some additions to the HMM selftests to ensure everything
works as expected.
Patch 3 was posted previously and has not changed.
Patch 4 is similar to what was posted previously but updated to use the new
swap entries instread of migration.
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 (4):
hmm: Device exclusive memory access
hmm: Selftests for exclusive device memory
nouveau/svm: Refactor nouveau_range_fault
nouveau/svm: Implement atomic SVM access
Documentation/vm/hmm.rst | 15 ++
drivers/gpu/drm/nouveau/include/nvif/if000c.h | 1 +
drivers/gpu/drm/nouveau/nouveau_svm.c | 118 +++++++---
drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h | 1 +
.../drm/nouveau/nvkm/subdev/mmu/vmmgp100.c | 6 +
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 +++
lib/test_hmm.c | 124 ++++++++++
lib/test_hmm_uapi.h | 2 +
mm/hmm.c | 206 ++++++++++++++++
mm/memory.c | 34 ++-
mm/mprotect.c | 7 +
mm/page_vma_mapped.c | 14 +-
mm/rmap.c | 29 ++-
tools/testing/selftests/vm/hmm-tests.c | 219 ++++++++++++++++++
18 files changed, 792 insertions(+), 38 deletions(-)
--
2.20.1
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access
2021-02-19 2:07 [Nouveau] [PATCH v2 0/4] Add support for SVM atomics in Nouveau Alistair Popple
@ 2021-02-19 2:07 ` Alistair Popple
2021-02-19 4:04 ` kernel test robot
` (2 more replies)
2021-02-19 2:07 ` [Nouveau] [PATCH v2 2/4] hmm: Selftests for exclusive device memory Alistair Popple
` (2 subsequent siblings)
3 siblings, 3 replies; 11+ messages in thread
From: Alistair Popple @ 2021-02-19 2:07 UTC (permalink / raw)
To: linux-mm, nouveau, bskeggs, akpm
Cc: rcampbell, daniel, linux-doc, Alistair Popple, linux-kernel,
dri-devel, hch, kvm-ppc, jgg
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@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
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Nouveau] [PATCH v2 2/4] hmm: Selftests for exclusive device memory
2021-02-19 2:07 [Nouveau] [PATCH v2 0/4] Add support for SVM atomics in Nouveau Alistair Popple
2021-02-19 2:07 ` [Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access Alistair Popple
@ 2021-02-19 2:07 ` Alistair Popple
2021-02-19 2:07 ` [Nouveau] [PATCH v2 3/4] nouveau/svm: Refactor nouveau_range_fault Alistair Popple
2021-02-19 2:07 ` [Nouveau] [PATCH v2 4/4] nouveau/svm: Implement atomic SVM access Alistair Popple
3 siblings, 0 replies; 11+ messages in thread
From: Alistair Popple @ 2021-02-19 2:07 UTC (permalink / raw)
To: linux-mm, nouveau, bskeggs, akpm
Cc: rcampbell, daniel, linux-doc, Alistair Popple, linux-kernel,
dri-devel, hch, kvm-ppc, jgg
Adds some selftests for exclusive device memory.
Signed-off-by: Alistair Popple <apopple@nvidia.com>
---
lib/test_hmm.c | 124 ++++++++++++++
lib/test_hmm_uapi.h | 2 +
tools/testing/selftests/vm/hmm-tests.c | 219 +++++++++++++++++++++++++
3 files changed, 345 insertions(+)
diff --git a/lib/test_hmm.c b/lib/test_hmm.c
index 80a78877bd93..d517d9d4c5aa 100644
--- a/lib/test_hmm.c
+++ b/lib/test_hmm.c
@@ -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 + (64 << PAGE_SHIFT))
+ next = end;
+ else
+ next = addr + (64 << PAGE_SHIFT);
+
+ ret = hmm_exclusive_range(mm, addr, next, pages);
+ 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;
diff --git a/lib/test_hmm_uapi.h b/lib/test_hmm_uapi.h
index 670b4ef2a5b6..f14dea5dcd06 100644
--- a/lib/test_hmm_uapi.h
+++ b/lib/test_hmm_uapi.h
@@ -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.
diff --git a/tools/testing/selftests/vm/hmm-tests.c b/tools/testing/selftests/vm/hmm-tests.c
index 5d1ac691b9f4..5d3c5db9ed3a 100644
--- a/tools/testing/selftests/vm/hmm-tests.c
+++ b/tools/testing/selftests/vm/hmm-tests.c
@@ -1485,4 +1485,223 @@ TEST_F(hmm2, double_map)
hmm_buffer_free(buffer);
}
+/*
+ * Basic check of exclusive faulting.
+ */
+TEST_F(hmm, exclusive)
+{
+ struct hmm_buffer *buffer;
+ unsigned long npages;
+ unsigned long size;
+ unsigned long i;
+ int *ptr;
+ int ret;
+
+ npages = ALIGN(HMM_BUFFER_SIZE, self->page_size) >> self->page_shift;
+ ASSERT_NE(npages, 0);
+ size = npages << self->page_shift;
+
+ buffer = malloc(sizeof(*buffer));
+ ASSERT_NE(buffer, NULL);
+
+ buffer->fd = -1;
+ buffer->size = size;
+ buffer->mirror = malloc(size);
+ ASSERT_NE(buffer->mirror, NULL);
+
+ buffer->ptr = mmap(NULL, size,
+ PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS,
+ buffer->fd, 0);
+ ASSERT_NE(buffer->ptr, MAP_FAILED);
+
+ /* Initialize buffer in system memory. */
+ for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
+ ptr[i] = i;
+
+ /* Map memory exclusively for device access. */
+ ret = hmm_dmirror_cmd(self->fd, HMM_DMIRROR_EXCLUSIVE, buffer, npages);
+ ASSERT_EQ(ret, 0);
+ ASSERT_EQ(buffer->cpages, npages);
+
+ /* Check what the device read. */
+ for (i = 0, ptr = buffer->mirror; i < size / sizeof(*ptr); ++i)
+ ASSERT_EQ(ptr[i], i);
+
+ /* Fault pages back to system memory and check them. */
+ for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
+ ASSERT_EQ(ptr[i]++, i);
+
+ for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
+ ASSERT_EQ(ptr[i], i+1);
+
+ /* Check atomic access revoked */
+ ret = hmm_dmirror_cmd(self->fd, HMM_DMIRROR_CHECK_EXCLUSIVE, buffer, npages);
+ ASSERT_EQ(ret, 0);
+
+ hmm_buffer_free(buffer);
+}
+
+TEST_F(hmm, exclusive_shared)
+{
+ struct hmm_buffer *buffer;
+ unsigned long npages;
+ unsigned long size;
+ int *ptr;
+ int ret, i;
+
+ npages = ALIGN(HMM_BUFFER_SIZE, self->page_size) >> self->page_shift;
+ ASSERT_NE(npages, 0);
+ size = npages << self->page_shift;
+
+ buffer = malloc(sizeof(*buffer));
+ ASSERT_NE(buffer, NULL);
+
+ buffer->fd = -1;
+ buffer->size = size;
+ buffer->mirror = malloc(size);
+ ASSERT_NE(buffer->mirror, NULL);
+
+ buffer->ptr = mmap(NULL, size,
+ PROT_READ | PROT_WRITE,
+ MAP_SHARED | MAP_ANONYMOUS,
+ buffer->fd, 0);
+ ASSERT_NE(buffer->ptr, MAP_FAILED);
+
+ /* Initialize buffer in system memory. */
+ for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
+ ptr[i] = i;
+
+ /* Map memory exclusively for device access. */
+ ret = hmm_dmirror_cmd(self->fd, HMM_DMIRROR_EXCLUSIVE, buffer, npages);
+ ASSERT_EQ(ret, 0);
+ ASSERT_EQ(buffer->cpages, npages);
+
+ /* Check what the device read. */
+ for (i = 0, ptr = buffer->mirror; i < size / sizeof(*ptr); ++i)
+ ASSERT_EQ(ptr[i], i);
+
+ /* Fault pages back to system memory and check them. */
+ for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
+ ASSERT_EQ(ptr[i]++, i);
+
+ for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
+ ASSERT_EQ(ptr[i], i+1);
+
+ /* Check atomic access revoked */
+ ret = hmm_dmirror_cmd(self->fd, HMM_DMIRROR_CHECK_EXCLUSIVE, buffer, npages);
+ ASSERT_FALSE(ret);
+
+ /* Map memory exclusively for device access again to check process tear down */
+ ret = hmm_dmirror_cmd(self->fd, HMM_DMIRROR_EXCLUSIVE, buffer, npages);
+ ASSERT_EQ(ret, 0);
+ ASSERT_EQ(buffer->cpages, npages);
+
+ hmm_buffer_free(buffer);
+}
+
+/*
+ * Same as above but for shared anonymous memory.
+ */
+TEST_F(hmm, exclusive_mprotect)
+{
+ struct hmm_buffer *buffer;
+ unsigned long npages;
+ unsigned long size;
+ unsigned long i;
+ int *ptr;
+ int ret;
+
+ npages = ALIGN(HMM_BUFFER_SIZE, self->page_size) >> self->page_shift;
+ ASSERT_NE(npages, 0);
+ size = npages << self->page_shift;
+
+ buffer = malloc(sizeof(*buffer));
+ ASSERT_NE(buffer, NULL);
+
+ buffer->fd = -1;
+ buffer->size = size;
+ buffer->mirror = malloc(size);
+ ASSERT_NE(buffer->mirror, NULL);
+
+ buffer->ptr = mmap(NULL, size,
+ PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS,
+ buffer->fd, 0);
+ ASSERT_NE(buffer->ptr, MAP_FAILED);
+
+ /* Initialize buffer in system memory. */
+ for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
+ ptr[i] = i;
+
+ /* Map memory exclusively for device access. */
+ ret = hmm_dmirror_cmd(self->fd, HMM_DMIRROR_EXCLUSIVE, buffer, npages);
+ ASSERT_EQ(ret, 0);
+ ASSERT_EQ(buffer->cpages, npages);
+
+ /* Check what the device read. */
+ for (i = 0, ptr = buffer->mirror; i < size / sizeof(*ptr); ++i)
+ ASSERT_EQ(ptr[i], i);
+
+ ret = mprotect(buffer->ptr, size, PROT_READ);
+ ASSERT_EQ(ret, 0);
+
+ /* Simulate a device writing system memory. */
+ ret = hmm_dmirror_cmd(self->fd, HMM_DMIRROR_WRITE, buffer, npages);
+ ASSERT_EQ(ret, -EPERM);
+
+ hmm_buffer_free(buffer);
+}
+
+/*
+ * Check copy-on-write works.
+ */
+TEST_F(hmm, exclusive_cow)
+{
+ struct hmm_buffer *buffer;
+ unsigned long npages;
+ unsigned long size;
+ unsigned long i;
+ int *ptr;
+ int ret;
+
+ npages = ALIGN(HMM_BUFFER_SIZE, self->page_size) >> self->page_shift;
+ ASSERT_NE(npages, 0);
+ size = npages << self->page_shift;
+
+ buffer = malloc(sizeof(*buffer));
+ ASSERT_NE(buffer, NULL);
+
+ buffer->fd = -1;
+ buffer->size = size;
+ buffer->mirror = malloc(size);
+ ASSERT_NE(buffer->mirror, NULL);
+
+ buffer->ptr = mmap(NULL, size,
+ PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS,
+ buffer->fd, 0);
+ ASSERT_NE(buffer->ptr, MAP_FAILED);
+
+ /* Initialize buffer in system memory. */
+ for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
+ ptr[i] = i;
+
+ /* Map memory exclusively for device access. */
+ ret = hmm_dmirror_cmd(self->fd, HMM_DMIRROR_EXCLUSIVE, buffer, npages);
+ ASSERT_EQ(ret, 0);
+ ASSERT_EQ(buffer->cpages, npages);
+
+ fork();
+
+ /* Fault pages back to system memory and check them. */
+ for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
+ ASSERT_EQ(ptr[i]++, i);
+
+ for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
+ ASSERT_EQ(ptr[i], i+1);
+
+ hmm_buffer_free(buffer);
+}
+
TEST_HARNESS_MAIN
--
2.20.1
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Nouveau] [PATCH v2 3/4] nouveau/svm: Refactor nouveau_range_fault
2021-02-19 2:07 [Nouveau] [PATCH v2 0/4] Add support for SVM atomics in Nouveau Alistair Popple
2021-02-19 2:07 ` [Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access Alistair Popple
2021-02-19 2:07 ` [Nouveau] [PATCH v2 2/4] hmm: Selftests for exclusive device memory Alistair Popple
@ 2021-02-19 2:07 ` Alistair Popple
2021-02-19 2:07 ` [Nouveau] [PATCH v2 4/4] nouveau/svm: Implement atomic SVM access Alistair Popple
3 siblings, 0 replies; 11+ messages in thread
From: Alistair Popple @ 2021-02-19 2:07 UTC (permalink / raw)
To: linux-mm, nouveau, bskeggs, akpm
Cc: rcampbell, daniel, linux-doc, Alistair Popple, linux-kernel,
dri-devel, hch, kvm-ppc, jgg
Call 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.
Signed-off-by: Alistair Popple <apopple@nvidia.com>
---
drivers/gpu/drm/nouveau/nouveau_svm.c | 34 ++++++++++++++++-----------
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
index f18bd53da052..cd7b47c946cf 100644
--- a/drivers/gpu/drm/nouveau/nouveau_svm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
@@ -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;
--
2.20.1
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Nouveau] [PATCH v2 4/4] nouveau/svm: Implement atomic SVM access
2021-02-19 2:07 [Nouveau] [PATCH v2 0/4] Add support for SVM atomics in Nouveau Alistair Popple
` (2 preceding siblings ...)
2021-02-19 2:07 ` [Nouveau] [PATCH v2 3/4] nouveau/svm: Refactor nouveau_range_fault Alistair Popple
@ 2021-02-19 2:07 ` Alistair Popple
3 siblings, 0 replies; 11+ messages in thread
From: Alistair Popple @ 2021-02-19 2:07 UTC (permalink / raw)
To: linux-mm, nouveau, bskeggs, akpm
Cc: rcampbell, daniel, linux-doc, Alistair Popple, linux-kernel,
dri-devel, hch, kvm-ppc, jgg
Some 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.
Signed-off-by: Alistair Popple <apopple@nvidia.com>
---
drivers/gpu/drm/nouveau/include/nvif/if000c.h | 1 +
drivers/gpu/drm/nouveau/nouveau_svm.c | 86 ++++++++++++++++---
drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h | 1 +
.../drm/nouveau/nvkm/subdev/mmu/vmmgp100.c | 6 ++
4 files changed, 81 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/include/nvif/if000c.h b/drivers/gpu/drm/nouveau/include/nvif/if000c.h
index d6dd40f21eed..9c7ff56831c5 100644
--- a/drivers/gpu/drm/nouveau/include/nvif/if000c.h
+++ b/drivers/gpu/drm/nouveau/include/nvif/if000c.h
@@ -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
diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
index cd7b47c946cf..d2ce4fb9c8ec 100644
--- a/drivers/gpu/drm/nouveau/nouveau_svm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
@@ -421,9 +421,9 @@ nouveau_svm_fault_cmp(const void *a, const void *b)
return ret;
if ((ret = (s64)fa->addr - fb->addr))
return ret;
- /*XXX: atomic? */
- return (fa->access == 0 || fa->access == 3) -
- (fb->access == 0 || fb->access == 3);
+ /* Atomic access (2) has highest priority */
+ return (-1*(fa->access == 2) + (fa->access == 0 || fa->access == 3)) -
+ (-1*(fb->access == 2) + (fb->access == 0 || fb->access == 3));
}
static void
@@ -555,10 +555,57 @@ static void nouveau_hmm_convert_pfn(struct nouveau_drm *drm,
args->p.phys[0] |= NVIF_VMM_PFNMAP_V0_W;
}
+static int nouveau_atomic_range_fault(struct nouveau_svmm *svmm,
+ struct nouveau_drm *drm,
+ struct nouveau_pfnmap_args *args, u32 size,
+ unsigned long hmm_flags, struct mm_struct *mm)
+{
+ struct page *page;
+ unsigned long start = args->p.addr;
+ struct vm_area_struct *vma;
+ int ret = 0;
+
+ mmap_read_lock(mm);
+ vma = find_vma_intersection(mm, start, start + size);
+ if (!vma || !(vma->vm_flags & VM_WRITE)) {
+ ret = -EPERM;
+ goto out;
+ }
+
+ hmm_exclusive_range(mm, start, start + PAGE_SIZE, &page);
+ if (!page) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ /* Map the page on the GPU. */
+ args->p.page = 12;
+ args->p.size = PAGE_SIZE;
+ args->p.addr = start;
+ args->p.phys[0] = page_to_phys(page) |
+ NVIF_VMM_PFNMAP_V0_V |
+ NVIF_VMM_PFNMAP_V0_W |
+ NVIF_VMM_PFNMAP_V0_A |
+ NVIF_VMM_PFNMAP_V0_HOST;
+
+ mutex_lock(&svmm->mutex);
+ svmm->vmm->vmm.object.client->super = true;
+ ret = nvif_object_ioctl(&svmm->vmm->vmm.object, args, size, NULL);
+ svmm->vmm->vmm.object.client->super = false;
+ mutex_unlock(&svmm->mutex);
+
+ unlock_page(page);
+ put_page(page);
+
+out:
+ mmap_read_unlock(mm);
+ return ret;
+}
+
static int nouveau_range_fault(struct nouveau_svmm *svmm,
struct nouveau_drm *drm,
struct nouveau_pfnmap_args *args, u32 size,
- unsigned long hmm_flags,
+ unsigned long hmm_flags, int atomic,
struct svm_notifier *notifier)
{
unsigned long timeout =
@@ -608,12 +655,18 @@ static int nouveau_range_fault(struct nouveau_svmm *svmm,
break;
}
- nouveau_hmm_convert_pfn(drm, &range, args);
+ if (atomic) {
+ mutex_unlock(&svmm->mutex);
+ ret = nouveau_atomic_range_fault(svmm, drm, args,
+ size, hmm_flags, mm);
+ } else {
+ nouveau_hmm_convert_pfn(drm, &range, args);
- svmm->vmm->vmm.object.client->super = true;
- ret = nvif_object_ioctl(&svmm->vmm->vmm.object, args, size, NULL);
- svmm->vmm->vmm.object.client->super = false;
- mutex_unlock(&svmm->mutex);
+ svmm->vmm->vmm.object.client->super = true;
+ ret = nvif_object_ioctl(&svmm->vmm->vmm.object, args, size, NULL);
+ svmm->vmm->vmm.object.client->super = false;
+ mutex_unlock(&svmm->mutex);
+ }
out:
mmu_interval_notifier_remove(¬ifier->notifier);
@@ -637,7 +690,7 @@ nouveau_svm_fault(struct nvif_notify *notify)
unsigned long hmm_flags;
u64 inst, start, limit;
int fi, fn;
- int replay = 0, ret;
+ int replay = 0, atomic = 0, ret;
/* Parse available fault buffer entries into a cache, and update
* the GET pointer so HW can reuse the entries.
@@ -718,12 +771,15 @@ nouveau_svm_fault(struct nvif_notify *notify)
/*
* Determine required permissions based on GPU fault
* access flags.
- * XXX: atomic?
*/
switch (buffer->fault[fi]->access) {
case 0: /* READ. */
hmm_flags = HMM_PFN_REQ_FAULT;
break;
+ case 2: /* ATOMIC. */
+ hmm_flags = HMM_PFN_REQ_FAULT | HMM_PFN_REQ_WRITE;
+ atomic = true;
+ break;
case 3: /* PREFETCH. */
hmm_flags = 0;
break;
@@ -740,7 +796,7 @@ nouveau_svm_fault(struct nvif_notify *notify)
notifier.svmm = svmm;
ret = nouveau_range_fault(svmm, svm->drm, &args.i,
- sizeof(args), hmm_flags, ¬ifier);
+ sizeof(args), hmm_flags, atomic, ¬ifier);
mmput(mm);
limit = args.i.p.addr + args.i.p.size;
@@ -760,7 +816,11 @@ nouveau_svm_fault(struct nvif_notify *notify)
!(args.phys[0] & NVIF_VMM_PFNMAP_V0_V)) ||
(buffer->fault[fi]->access != 0 /* READ. */ &&
buffer->fault[fi]->access != 3 /* PREFETCH. */ &&
- !(args.phys[0] & NVIF_VMM_PFNMAP_V0_W)))
+ !(args.phys[0] & NVIF_VMM_PFNMAP_V0_W)) ||
+ (buffer->fault[fi]->access != 0 /* READ. */ &&
+ buffer->fault[fi]->access != 1 /* WRITE. */ &&
+ buffer->fault[fi]->access != 3 /* PREFETCH. */ &&
+ !(args.phys[0] & NVIF_VMM_PFNMAP_V0_A)))
break;
}
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h
index a2b179568970..f6188aa9171c 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h
@@ -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
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp100.c b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp100.c
index 236db5570771..f02abd9cb4dd 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp100.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp100.c
@@ -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,
--
2.20.1
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access
2021-02-19 2:07 ` [Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access Alistair Popple
@ 2021-02-19 4:04 ` kernel test robot
2021-02-19 4:29 ` Alistair Popple
2021-02-19 5:09 ` kernel test robot
2021-02-19 9:47 ` Christoph Hellwig
2 siblings, 1 reply; 11+ messages in thread
From: kernel test robot @ 2021-02-19 4:04 UTC (permalink / raw)
To: Alistair Popple, linux-mm, nouveau, bskeggs, akpm
Cc: rcampbell, kbuild-all, linux-doc, linux-kernel, dri-devel,
clang-built-linux, kvm-ppc
[-- Attachment #1: Type: text/plain, Size: 5635 bytes --]
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@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@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29578 bytes --]
[-- Attachment #3: Type: text/plain, Size: 154 bytes --]
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access
2021-02-19 4:04 ` kernel test robot
@ 2021-02-19 4:29 ` Alistair Popple
0 siblings, 0 replies; 11+ messages in thread
From: Alistair Popple @ 2021-02-19 4:29 UTC (permalink / raw)
To: kernel test robot
Cc: rcampbell, kbuild-all, clang-built-linux, nouveau, dri-devel,
linux-doc, linux-kernel, kvm-ppc, linux-mm, bskeggs, akpm
Apologies for the noise, looks like I don't have a CONFIG_DEVICE_PRIVATE=n
build in my tests and missed creating definitions for the new static inline
functions for that configuration.
I'll wait for some feedback on the overall approach and fix this in a v3.
- Alistair
On Friday, 19 February 2021 3:04:07 PM AEDT kernel test robot wrote:
> External email: Use caution opening links or attachments
>
>
> 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@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@lists.01.org
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access
2021-02-19 2:07 ` [Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access Alistair Popple
2021-02-19 4:04 ` kernel test robot
@ 2021-02-19 5:09 ` kernel test robot
2021-02-19 9:47 ` Christoph Hellwig
2 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2021-02-19 5:09 UTC (permalink / raw)
To: Alistair Popple, linux-mm, nouveau, bskeggs, akpm
Cc: rcampbell, kbuild-all, linux-doc, linux-kernel, dri-devel, kvm-ppc
[-- Attachment #1: Type: text/plain, Size: 10125 bytes --]
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@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@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33708 bytes --]
[-- Attachment #3: Type: text/plain, Size: 154 bytes --]
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access
2021-02-19 2:07 ` [Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access Alistair Popple
2021-02-19 4:04 ` kernel test robot
2021-02-19 5:09 ` kernel test robot
@ 2021-02-19 9:47 ` Christoph Hellwig
2021-02-19 14:01 ` Jason Gunthorpe
2021-02-22 10:46 ` Alistair Popple
2 siblings, 2 replies; 11+ messages in thread
From: Christoph Hellwig @ 2021-02-19 9:47 UTC (permalink / raw)
To: Alistair Popple
Cc: rcampbell, daniel, linux-doc, nouveau, dri-devel, linux-kernel,
kvm-ppc, hch, linux-mm, bskeggs, jgg, akpm
> 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;
> + }
> +#endif
Doesn'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.
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access
2021-02-19 9:47 ` Christoph Hellwig
@ 2021-02-19 14:01 ` Jason Gunthorpe
2021-02-22 10:46 ` Alistair Popple
1 sibling, 0 replies; 11+ messages in thread
From: Jason Gunthorpe @ 2021-02-19 14:01 UTC (permalink / raw)
To: Christoph Hellwig
Cc: rcampbell, linux-doc, nouveau, dri-devel, Alistair Popple,
linux-kernel, kvm-ppc, linux-mm, bskeggs, daniel, akpm
On Fri, Feb 19, 2021 at 09:47:41AM +0000, Christoph Hellwig wrote:
> > diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> > index 866a0fa104c4..5d28ff6d4d80 100644
> > +++ 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)?
Yes please, I'd prefer it if hmm.c was just the special page walker
and not a grab bag of unrelated things
Is there is a more natural place to put this in the mm for this idea?
Jason
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access
2021-02-19 9:47 ` Christoph Hellwig
2021-02-19 14:01 ` Jason Gunthorpe
@ 2021-02-22 10:46 ` Alistair Popple
1 sibling, 0 replies; 11+ messages in thread
From: Alistair Popple @ 2021-02-22 10:46 UTC (permalink / raw)
To: Christoph Hellwig
Cc: rcampbell, daniel, linux-doc, nouveau, dri-devel, linux-kernel,
kvm-ppc, linux-mm, bskeggs, jgg, akpm
On Friday, 19 February 2021 8:47:41 PM AEDT Christoph Hellwig wrote:
> > 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)?
If I open code the entry_to_page() functions as suggested below then these
simplify down to single if statements like so:
if (is_migration_entry(entry) ||
is_device_private_entry(entry) ||
is_device_exclusive_entry(entry))
page = pfn_to_page(swp_offset(entry));
I could simplify further by hiding that in a single static inline like so:
static inline bool is_special_entry(swp_entry_t entry)
{
return is_migration_entry(entry) ||
is_device_private_entry(entry) ||
is_device_exclusive_entry(entry);
}
My only concern with doing that is these entries can't *always* be treated the
same so it might make it too easy to overlook the subtle differences.
> > 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)?
Sure. I ended up stuffing it in there for the moment because I didn't know of
any other "obvious" spot to put it. How about these for names:
int make_device_exclusive_range(struct mm_struct *mm, unsigned long start,
unsigned long end, struct page **pages);
vm_fault_t remove_device_exclusive_entry(struct vm_fault *vmf);
I am open to any alternative naming suggestions though.
> > +#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.
Ok, if I add a is_special_entry() function as suggested above these could just
use that.
> > +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/
> ??
See my next comment.
> > +
> > +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.
I assume you mean separate read and write functions instead of using the write
flag?
These are based on the existing device private functions which themselves
looked to be based on the migration entry functions. It would be good to keep
these consistent so when making the changes above I would also refactor the
existing make_device_private_entry() and make_migration_entry() functions to
match as well.
As a test I tried refactoring the migration entry functions into the below and
it seemed to make things a bit easier to follow if not a little verbose:
static inline int is_writable_migration_entry(swp_entry_t entry);
static inline swp_entry_t make_readable_migration_entry(pgoff_t offset);
static inline swp_entry_t make_writable_migration_entry(pgoff_t offset);
static inline int is_migration_entry(swp_entry_t entry);
So I can do that along with the same refactoring of device private accessor
functions as a prep patch if that seems reasonable?
> > +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.
Ok, I have just tried doing that and it ends up being more concise and
accurate so I'll add it to the series.
> > +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?
Good point. I had other code here but I didn't need it and should have just
removed what was left.
> > +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?
Good idea, you aren't missing anything although I don't think
get_user_pages_fast() will work as the driver needs to be able to operate on a
remote mm_struct.
But something like get_user_pages_remote() would work better than open-coding
it.
> > +#if defined(CONFIG_ARCH_ENABLE_THP_MIGRATION) || defined(CONFIG_HUGETLB)
> > + if (PageTransHuge(page)) {
> > + VM_BUG_ON_PAGE(1, page);
> > + continue;
> > + }
> > +#endif
>
> Doesn't PageTransHuge always return false for that case? If not
> shouldn't we make sure it does?
Right, this is probably an overly defensive check so I'll remove it.
[snip - will address code format/structure suggestions]
>
> try_to_unmap_one has turned into a monster. A little refactoring to
> split it into managable pieces would be nice.
>
Agreed, adding this it did seem a little unwieldy. I will see if I can do a
bit of refactoring for the next version.
- Alistair
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-02-22 10:47 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-19 2:07 [Nouveau] [PATCH v2 0/4] Add support for SVM atomics in Nouveau Alistair Popple
2021-02-19 2:07 ` [Nouveau] [PATCH v2 1/4] hmm: Device exclusive memory access Alistair Popple
2021-02-19 4:04 ` kernel test robot
2021-02-19 4:29 ` Alistair Popple
2021-02-19 5:09 ` kernel test robot
2021-02-19 9:47 ` Christoph Hellwig
2021-02-19 14:01 ` Jason Gunthorpe
2021-02-22 10:46 ` Alistair Popple
2021-02-19 2:07 ` [Nouveau] [PATCH v2 2/4] hmm: Selftests for exclusive device memory Alistair Popple
2021-02-19 2:07 ` [Nouveau] [PATCH v2 3/4] nouveau/svm: Refactor nouveau_range_fault Alistair Popple
2021-02-19 2:07 ` [Nouveau] [PATCH v2 4/4] nouveau/svm: Implement atomic SVM access Alistair Popple
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).