Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On Thu, Mar 19, 2020 at 09:03:45PM -0300, Jason Gunthorpe wrote: > > Should tests enable the feature or the feature enable the test? > > IMHO, if the feature is being compiled into the kernel, that should > > enable the menu item for the test. If the feature isn't selected, > > no need to test it :-) > > I ment if DEVICE_PRIVATE should be a user selectable option at all, or > should it be turned on when a driver like nouveau is selected. I don't think it should be user selectable. This is an implementation detail users can't know about. > Is there some downside to enabling DEVICE_PRIVATE? The option itself adds a little more code to the core kernel, and introduces a few additional branches in core mm code. But more importantly it pulls in the whole pgmap infrastructure. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On Thu, Mar 19, 2020 at 06:33:04PM -0700, Ralph Campbell wrote:
> > > + .default_flags = dmirror_hmm_flags[HMM_PFN_VALID] |
> > > + (write ? dmirror_hmm_flags[HMM_PFN_WRITE] : 0),
> > > + .dev_private_owner = dmirror->mdevice,
> > > + };
> > > + int ret = 0;
> >
> > > +static int dmirror_snapshot(struct dmirror *dmirror,
> > > + struct hmm_dmirror_cmd *cmd)
> > > +{
> > > + struct mm_struct *mm = dmirror->mm;
> > > + unsigned long start, end;
> > > + unsigned long size = cmd->npages << PAGE_SHIFT;
> > > + unsigned long addr;
> > > + unsigned long next;
> > > + uint64_t pfns[64];
> > > + unsigned char perm[64];
> > > + char __user *uptr;
> > > + struct hmm_range range = {
> > > + .pfns = pfns,
> > > + .flags = dmirror_hmm_flags,
> > > + .values = dmirror_hmm_values,
> > > + .pfn_shift = DPT_SHIFT,
> > > + .pfn_flags_mask = ~0ULL,
> >
> > Same here, especially since this is snapshot
> >
> > Jason
>
> Actually, snapshot ignores pfn_flags_mask and default_flags.
Yes, so no reason to set them to not 0..
Jason
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On Tue, Mar 17, 2020 at 04:14:31PM -0700, Ralph Campbell wrote:
> +static int dmirror_fault(struct dmirror *dmirror, unsigned long start,
> + unsigned long end, bool write)
> +{
> + struct mm_struct *mm = dmirror->mm;
> + unsigned long addr;
> + uint64_t pfns[64];
> + struct hmm_range range = {
> + .notifier = >notifier,
> + .pfns = pfns,
> + .flags = dmirror_hmm_flags,
> + .values = dmirror_hmm_values,
> + .pfn_shift = DPT_SHIFT,
> + .pfn_flags_mask = ~(dmirror_hmm_flags[HMM_PFN_VALID] |
> + dmirror_hmm_flags[HMM_PFN_WRITE]),
Since pfns is not initialized pfn_flags_mask should be 0.
> + .default_flags = dmirror_hmm_flags[HMM_PFN_VALID] |
> + (write ? dmirror_hmm_flags[HMM_PFN_WRITE] : 0),
> + .dev_private_owner = dmirror->mdevice,
> + };
> + int ret = 0;
> +static int dmirror_snapshot(struct dmirror *dmirror,
> + struct hmm_dmirror_cmd *cmd)
> +{
> + struct mm_struct *mm = dmirror->mm;
> + unsigned long start, end;
> + unsigned long size = cmd->npages << PAGE_SHIFT;
> + unsigned long addr;
> + unsigned long next;
> + uint64_t pfns[64];
> + unsigned char perm[64];
> + char __user *uptr;
> + struct hmm_range range = {
> + .pfns = pfns,
> + .flags = dmirror_hmm_flags,
> + .values = dmirror_hmm_values,
> + .pfn_shift = DPT_SHIFT,
> + .pfn_flags_mask = ~0ULL,
Same here, especially since this is snapshot
Jason
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On Thu, Mar 19, 2020 at 03:56:50PM -0700, Ralph Campbell wrote:
> Adding linux-kselft...@vger.kernel.org for the test config question.
>
> On 3/19/20 11:17 AM, Jason Gunthorpe wrote:
> > On Tue, Mar 17, 2020 at 04:14:31PM -0700, Ralph Campbell wrote:
> > >
> > > On 3/17/20 5:59 AM, Christoph Hellwig wrote:
> > > > On Tue, Mar 17, 2020 at 09:47:55AM -0300, Jason Gunthorpe wrote:
> > > > > I've been using v7 of Ralph's tester and it is working well - it has
> > > > > DEVICE_PRIVATE support so I think it can test this flow too. Ralph are
> > > > > you able?
> > > > >
> > > > > This hunk seems trivial enough to me, can we include it now?
> > > >
> > > > I can send a separate patch for it once the tester covers it. I don't
> > > > want to add it to the original patch as it is a significant behavior
> > > > change compared to the existing code.
> > > >
> > >
> > > Attached is an updated version of my HMM tests based on linux-5.6.0-rc6.
> > > I ran this OK with Jason's 8+1 HMM patches, Christoph's 1-5 misc HMM
> > > clean ups,
> > > and Christoph's 1-4 device private page changes applied.
> >
> > I'd like to get this to mergable, it looks pretty good now, but I have
> > no idea about selftests - and I'm struggling to even compile the tools
> > dir
> >
> > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > > index 69def4a9df00..4d22ce7879a7 100644
> > > +++ b/lib/Kconfig.debug
> > > @@ -2162,6 +2162,18 @@ config TEST_MEMINIT
> > > If unsure, say N.
> > > +config TEST_HMM
> > > + tristate "Test HMM (Heterogeneous Memory Management)"
> > > + depends on DEVICE_PRIVATE
> > > + select HMM_MIRROR
> > > +select MMU_NOTIFIER
> >
> > extra spaces
>
> Will fix in v8.
>
> > In general I wonder if it even makes sense that DEVICE_PRIVATE is user
> > selectable?
>
> Should tests enable the feature or the feature enable the test?
> IMHO, if the feature is being compiled into the kernel, that should
> enable the menu item for the test. If the feature isn't selected,
> no need to test it :-)
I ment if DEVICE_PRIVATE should be a user selectable option at all, or
should it be turned on when a driver like nouveau is selected.
Is there some downside to enabling DEVICE_PRIVATE?
> > The notifier holds a mmgrab, no need for another one
>
> OK. I'll replace dmirror->mm with dmirror->notifier.mm.
Right that is good too
> > > + filp->private_data = dmirror;
> >
> > Not sure what this comment means
>
> I'll change the comment to:
> /*
> * The first open of the device character file registers the address
> * space of the process doing the open() system call with the device.
> * Subsequent file opens by other processes will have access to the
> * first process' address space.
> */
How does this happen? The function looks like it always does the same thing
> > > +static bool dmirror_interval_invalidate(struct mmu_interval_notifier
> > > *mni,
> > > + const struct mmu_notifier_range *range,
> > > + unsigned long cur_seq)
> > > +{
> > > + struct dmirror *dmirror = container_of(mni, struct dmirror, notifier);
> > > + struct mm_struct *mm = dmirror->mm;
> > > +
> > > + /*
> > > + * If the process doesn't exist, we don't need to invalidate the
> > > + * device page table since the address space will be torn down.
> > > + */
> > > + if (!mmget_not_zero(mm))
> > > + return true;
> >
> > Why? Don't the notifiers provide for this already.
> >
> > mmget_not_zero() is required before calling hmm_range_fault() though
Oh... This is the invalidate_all path during invalidation
IMHO you should test the invalidation reason in the range to exclude
this.
But xa_erase looks totally safe so there should be no reason to do
that.
> This is a workaround for a problem I don't quite understand.
> If you change tools/testing/selftests/vm/hmm-tests.c line 868 to
> ASSERT_EQ(ret, -1);
> Then the test will abort, core dump, and cause two problems,
> 1) the migrated page will be faulted back to system memory in order to write
>it to the core dump. This triggers lockdep_assert_held(>mmap_sem)
>in walk_page_range().
Has the migration stuff become entangled with the xarray?
> [ 137.980718] Code: 80 2f 1a 83 c6 05 e9 8d 7b 01 01 e8 3e b1 b1 fe e9 05 ff
> ff ff 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 41 56 41 55 41 54 55 <48> 89
> fd 53 4c 8d 6d 10 e8 3c fc ff ff 49 89 c4 4c 89 e0 83 e0 03
> [ 137.999461] RSP: 0018:c900015e77c8 EFLAGS: 0246 ORIG_RAX:
> ff13
> [ 138.007028] RAX: 8886e508c408 RBX: RCX:
> 82626c89
> [ 138.014159] RDX: dc00 RSI: RDI:
> c900015e78a0
> [ 138.021293] RBP: c900015e78a0 R08: 811461c4 R09:
> f520002bcf17
> [ 138.028426] R10: f520002bcf16 R11: 0003 R12:
> 02606d10
> [ 138.035557] R13: 8886e508c448 R14: 0031 R15:
>
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On 3/19/20 5:14 PM, Jason Gunthorpe wrote:
On Tue, Mar 17, 2020 at 04:14:31PM -0700, Ralph Campbell wrote:
+static int dmirror_fault(struct dmirror *dmirror, unsigned long start,
+unsigned long end, bool write)
+{
+ struct mm_struct *mm = dmirror->mm;
+ unsigned long addr;
+ uint64_t pfns[64];
+ struct hmm_range range = {
+ .notifier = >notifier,
+ .pfns = pfns,
+ .flags = dmirror_hmm_flags,
+ .values = dmirror_hmm_values,
+ .pfn_shift = DPT_SHIFT,
+ .pfn_flags_mask = ~(dmirror_hmm_flags[HMM_PFN_VALID] |
+ dmirror_hmm_flags[HMM_PFN_WRITE]),
Since pfns is not initialized pfn_flags_mask should be 0.
Good point.
+ .default_flags = dmirror_hmm_flags[HMM_PFN_VALID] |
+ (write ? dmirror_hmm_flags[HMM_PFN_WRITE] : 0),
+ .dev_private_owner = dmirror->mdevice,
+ };
+ int ret = 0;
+static int dmirror_snapshot(struct dmirror *dmirror,
+ struct hmm_dmirror_cmd *cmd)
+{
+ struct mm_struct *mm = dmirror->mm;
+ unsigned long start, end;
+ unsigned long size = cmd->npages << PAGE_SHIFT;
+ unsigned long addr;
+ unsigned long next;
+ uint64_t pfns[64];
+ unsigned char perm[64];
+ char __user *uptr;
+ struct hmm_range range = {
+ .pfns = pfns,
+ .flags = dmirror_hmm_flags,
+ .values = dmirror_hmm_values,
+ .pfn_shift = DPT_SHIFT,
+ .pfn_flags_mask = ~0ULL,
Same here, especially since this is snapshot
Jason
Actually, snapshot ignores pfn_flags_mask and default_flags.
In hmm_pte_need_fault(), HMM_FAULT_SNAPSHOT is checked and returns early before
checking pfn_flags_mask and default_flags since no faults are being requested.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On Tue, Mar 17, 2020 at 04:14:31PM -0700, Ralph Campbell wrote:
>
> On 3/17/20 5:59 AM, Christoph Hellwig wrote:
> > On Tue, Mar 17, 2020 at 09:47:55AM -0300, Jason Gunthorpe wrote:
> > > I've been using v7 of Ralph's tester and it is working well - it has
> > > DEVICE_PRIVATE support so I think it can test this flow too. Ralph are
> > > you able?
> > >
> > > This hunk seems trivial enough to me, can we include it now?
> >
> > I can send a separate patch for it once the tester covers it. I don't
> > want to add it to the original patch as it is a significant behavior
> > change compared to the existing code.
> >
>
> Attached is an updated version of my HMM tests based on linux-5.6.0-rc6.
> I ran this OK with Jason's 8+1 HMM patches, Christoph's 1-5 misc HMM clean
> ups,
> and Christoph's 1-4 device private page changes applied.
I'd like to get this to mergable, it looks pretty good now, but I have
no idea about selftests - and I'm struggling to even compile the tools
dir
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 69def4a9df00..4d22ce7879a7 100644
> +++ b/lib/Kconfig.debug
> @@ -2162,6 +2162,18 @@ config TEST_MEMINIT
>
> If unsure, say N.
>
> +config TEST_HMM
> + tristate "Test HMM (Heterogeneous Memory Management)"
> + depends on DEVICE_PRIVATE
> + select HMM_MIRROR
> +select MMU_NOTIFIER
extra spaces
In general I wonder if it even makes sense that DEVICE_PRIVATE is user
selectable?
> +static int dmirror_fops_open(struct inode *inode, struct file *filp)
> +{
> + struct cdev *cdev = inode->i_cdev;
> + struct dmirror *dmirror;
> + int ret;
> +
> + /* Mirror this process address space */
> + dmirror = kzalloc(sizeof(*dmirror), GFP_KERNEL);
> + if (dmirror == NULL)
> + return -ENOMEM;
> +
> + dmirror->mdevice = container_of(cdev, struct dmirror_device, cdevice);
> + mutex_init(>mutex);
> + xa_init(>pt);
> +
> + ret = mmu_interval_notifier_insert(>notifier, current->mm,
> + 0, ULONG_MAX & PAGE_MASK, _min_ops);
> + if (ret) {
> + kfree(dmirror);
> + return ret;
> + }
> +
> + /* Pairs with the mmdrop() in dmirror_fops_release(). */
> + mmgrab(current->mm);
> + dmirror->mm = current->mm;
The notifier holds a mmgrab, no need for another one
> + /* Only the first open registers the address space. */
> + filp->private_data = dmirror;
Not sure what this comment means
> +static inline struct dmirror_device *dmirror_page_to_device(struct page
> *page)
> +
> +{
> + struct dmirror_chunk *devmem;
> +
> + devmem = container_of(page->pgmap, struct dmirror_chunk, pagemap);
> + return devmem->mdevice;
> +}
extra devmem var is not really needed
> +
> +static bool dmirror_device_is_mine(struct dmirror_device *mdevice,
> +struct page *page)
> +{
> + if (!is_zone_device_page(page))
> + return false;
> + return page->pgmap->ops == _devmem_ops &&
> + dmirror_page_to_device(page) == mdevice;
> +}
Use new owner stuff, right? Actually this is redunant now, the check
should be just WARN_ON pageowner != self owner
> +static int dmirror_do_fault(struct dmirror *dmirror, struct hmm_range *range)
> +{
> + uint64_t *pfns = range->pfns;
> + unsigned long pfn;
> +
> + for (pfn = (range->start >> PAGE_SHIFT);
> + pfn < (range->end >> PAGE_SHIFT);
> + pfn++, pfns++) {
> + struct page *page;
> + void *entry;
> +
> + /*
> + * HMM_PFN_ERROR is returned if it is accessing invalid memory
> + * either because of memory error (hardware detected memory
> + * corruption) or more likely because of truncate on mmap
> + * file.
> + */
> + if (*pfns == range->values[HMM_PFN_ERROR])
> + return -EFAULT;
Unless that snapshot is use hmm_range_fault() never returns success
and sets PFN_ERROR, so this should be a WARN_ON
> + if (!(*pfns & range->flags[HMM_PFN_VALID]))
> + return -EFAULT;
Same with valid.
> + page = hmm_device_entry_to_page(range, *pfns);
> + /* We asked for pages to be populated but check anyway. */
> + if (!page)
> + return -EFAULT;
WARN_ON
> + if (is_zone_device_page(page)) {
> + /*
> + * TODO: need a way to ask HMM to fault foreign zone
> + * device private pages.
> + */
> + if (!dmirror_device_is_mine(dmirror->mdevice, page))
> + continue;
Actually re
> +static bool dmirror_interval_invalidate(struct mmu_interval_notifier *mni,
> + const struct mmu_notifier_range *range,
> + unsigned long cur_seq)
> +{
> + struct
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On Tue, Mar 17, 2020 at 03:43:47PM -0700, Ralph Campbell wrote: >> Obviously no driver cared for that so far. Once we have test cases >> for that and thus testable code we can add code to fault it in from >> hmm_vma_handle_pte. >> > > I'm OK with the series. I think I would have been less confused if I looked at > patch 4 then 3. I guess I could just merge 3 and 4 if it is too confusing otherwise. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On 3/17/20 5:59 AM, Christoph Hellwig wrote:
On Tue, Mar 17, 2020 at 09:47:55AM -0300, Jason Gunthorpe wrote:
I've been using v7 of Ralph's tester and it is working well - it has
DEVICE_PRIVATE support so I think it can test this flow too. Ralph are
you able?
This hunk seems trivial enough to me, can we include it now?
I can send a separate patch for it once the tester covers it. I don't
want to add it to the original patch as it is a significant behavior
change compared to the existing code.
Attached is an updated version of my HMM tests based on linux-5.6.0-rc6.
I ran this OK with Jason's 8+1 HMM patches, Christoph's 1-5 misc HMM clean ups,
and Christoph's 1-4 device private page changes applied.
I'm working on getting my nouveau tests running again on a different test
machine and will report on that when ready.
>From d499fb343bfa9764695ecdcd759fb16bc1ca3c93 Mon Sep 17 00:00:00 2001
From: Ralph Campbell
Date: Tue, 17 Mar 2020 11:10:38 -0700
Subject: [PATCH] mm/hmm/test: add self tests for HMM
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Add some basic stand alone self tests for HMM.
Signed-off-by: Ralph Campbell
Signed-off-by: Jérôme Glisse
---
MAINTAINERS|3 +
include/uapi/linux/test_hmm.h | 59 ++
lib/Kconfig.debug | 12 +
lib/Makefile |1 +
lib/test_hmm.c | 1210 +
tools/testing/selftests/vm/.gitignore |1 +
tools/testing/selftests/vm/Makefile|3 +
tools/testing/selftests/vm/config |2 +
tools/testing/selftests/vm/hmm-tests.c | 1353
tools/testing/selftests/vm/run_vmtests | 16 +
tools/testing/selftests/vm/test_hmm.sh | 97 ++
11 files changed, 2757 insertions(+)
create mode 100644 include/uapi/linux/test_hmm.h
create mode 100644 lib/test_hmm.c
create mode 100644 tools/testing/selftests/vm/hmm-tests.c
create mode 100755 tools/testing/selftests/vm/test_hmm.sh
diff --git a/MAINTAINERS b/MAINTAINERS
index cc1d18cb5d18..98c80a589a52 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7608,7 +7608,10 @@ L: linux...@kvack.org
S: Maintained
F: mm/hmm*
F: include/linux/hmm*
+F: include/uapi/linux/test_hmm*
F: Documentation/vm/hmm.rst
+F: lib/test_hmm*
+F: tools/testing/selftests/vm/*hmm*
HOST AP DRIVER
M: Jouni Malinen
diff --git a/include/uapi/linux/test_hmm.h b/include/uapi/linux/test_hmm.h
new file mode 100644
index ..8c5f70c160bf
--- /dev/null
+++ b/include/uapi/linux/test_hmm.h
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * This is a module to test the HMM (Heterogeneous Memory Management) API
+ * of the kernel. It allows a userspace program to expose its entire address
+ * space through the HMM test module device file.
+ */
+#ifndef _UAPI_LINUX_HMM_DMIRROR_H
+#define _UAPI_LINUX_HMM_DMIRROR_H
+
+#include
+#include
+
+/*
+ * Structure to pass to the HMM test driver to mimic a device accessing
+ * system memory and ZONE_DEVICE private memory through device page tables.
+ *
+ * @addr: (in) user address the device will read/write
+ * @ptr: (in) user address where device data is copied to/from
+ * @npages: (in) number of pages to read/write
+ * @cpages: (out) number of pages copied
+ * @faults: (out) number of device page faults seen
+ */
+struct hmm_dmirror_cmd {
+ __u64 addr;
+ __u64 ptr;
+ __u64 npages;
+ __u64 cpages;
+ __u64 faults;
+};
+
+/* Expose the address space of the calling process through hmm device file */
+#define HMM_DMIRROR_READ _IOWR('H', 0x00, 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)
+
+/*
+ * Values returned in hmm_dmirror_cmd.ptr for HMM_DMIRROR_SNAPSHOT.
+ * HMM_DMIRROR_PROT_ERROR: no valid mirror PTE for this page
+ * HMM_DMIRROR_PROT_NONE: unpopulated PTE or PTE with no access
+ * HMM_DMIRROR_PROT_READ: read-only PTE
+ * HMM_DMIRROR_PROT_WRITE: read/write PTE
+ * HMM_DMIRROR_PROT_ZERO: special read-only zero page
+ * HMM_DMIRROR_PROT_DEV_PRIVATE_LOCAL: Migrated device private page on the
+ * device the ioctl() is made
+ * HMM_DMIRROR_PROT_DEV_PRIVATE_REMOTE: Migrated device private page on some
+ * other device
+ */
+enum {
+ HMM_DMIRROR_PROT_ERROR = 0xFF,
+ HMM_DMIRROR_PROT_NONE = 0x00,
+ HMM_DMIRROR_PROT_READ = 0x01,
+ HMM_DMIRROR_PROT_WRITE = 0x02,
+ HMM_DMIRROR_PROT_ZERO = 0x10,
+ HMM_DMIRROR_PROT_DEV_PRIVATE_LOCAL = 0x20,
+ HMM_DMIRROR_PROT_DEV_PRIVATE_REMOTE = 0x30,
+};
+
+#endif /* _UAPI_LINUX_HMM_DMIRROR_H */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 69def4a9df00..4d22ce7879a7 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2162,6 +2162,18 @@ config TEST_MEMINIT
If unsure, say N.
+config TEST_HMM
+ tristate "Test
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On 3/17/20 12:34 AM, Christoph Hellwig wrote: On Mon, Mar 16, 2020 at 03:49:51PM -0700, Ralph Campbell wrote: On 3/16/20 12:32 PM, Christoph Hellwig wrote: Remove the code to fault device private pages back into system memory that has never been used by any driver. Also replace the usage of the HMM_PFN_DEVICE_PRIVATE flag in the pfns array with a simple is_device_private_page check in nouveau. Signed-off-by: Christoph Hellwig Getting rid of HMM_PFN_DEVICE_PRIVATE seems reasonable to me since a driver can look at the struct page but what if a driver needs to fault in a page from another device's private memory? Should it call handle_mm_fault()? Obviously no driver cared for that so far. Once we have test cases for that and thus testable code we can add code to fault it in from hmm_vma_handle_pte. I'm OK with the series. I think I would have been less confused if I looked at patch 4 then 3. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On Tue, Mar 17, 2020 at 01:59:55PM +0100, Christoph Hellwig wrote: > On Tue, Mar 17, 2020 at 09:47:55AM -0300, Jason Gunthorpe wrote: > > I've been using v7 of Ralph's tester and it is working well - it has > > DEVICE_PRIVATE support so I think it can test this flow too. Ralph are > > you able? > > > > This hunk seems trivial enough to me, can we include it now? > > I can send a separate patch for it once the tester covers it. I don't > want to add it to the original patch as it is a significant behavior > change compared to the existing code. Okay. I'm happy enough for now that amdgpu will get ERROR on device_private pages. That is a bug fix in of itself. Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On Tue, Mar 17, 2020 at 09:47:55AM -0300, Jason Gunthorpe wrote: > I've been using v7 of Ralph's tester and it is working well - it has > DEVICE_PRIVATE support so I think it can test this flow too. Ralph are > you able? > > This hunk seems trivial enough to me, can we include it now? I can send a separate patch for it once the tester covers it. I don't want to add it to the original patch as it is a significant behavior change compared to the existing code. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On Tue, Mar 17, 2020 at 09:15:36AM -0300, Jason Gunthorpe wrote: > > Getting rid of HMM_PFN_DEVICE_PRIVATE seems reasonable to me since a driver > > can > > look at the struct page but what if a driver needs to fault in a page from > > another device's private memory? Should it call handle_mm_fault()? > > Isn't that what this series basically does? > > The dev_private_owner is set to the type of pgmap the device knows how > to handle, and everything else is automatically faulted for the > device. > > If the device does not know how to handle device_private then it sets > dev_private_owner to NULL and it never gets device_private pfns. > > Since the device_private pfn cannot be dma mapped, drivers must have > explicit support for them. No, with this series (and all actual callers before this series) we never fault in device private pages. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On Mon, Mar 16, 2020 at 04:59:23PM -0300, Jason Gunthorpe wrote: > However, between patch 3 and 4 doesn't this break amd gpu as it will > return device_private pages now if not requested? Squash the two? No change in behavior in this patch as long as HMM_PFN_DEVICE_PRIVATE isn't set in ->pfns or ->default_flags, which is the case for both nouveau and amdgpu. The existing behavior is broken for private pages not known to the driver, but that is fixed in the next patch. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On Mon, Mar 16, 2020 at 03:49:51PM -0700, Ralph Campbell wrote: > > On 3/16/20 12:32 PM, Christoph Hellwig wrote: > > Remove the code to fault device private pages back into system memory > > that has never been used by any driver. Also replace the usage of the > > HMM_PFN_DEVICE_PRIVATE flag in the pfns array with a simple > > is_device_private_page check in nouveau. > > > > Signed-off-by: Christoph Hellwig > > Getting rid of HMM_PFN_DEVICE_PRIVATE seems reasonable to me since a driver > can > look at the struct page but what if a driver needs to fault in a page from > another device's private memory? Should it call handle_mm_fault()? Isn't that what this series basically does? The dev_private_owner is set to the type of pgmap the device knows how to handle, and everything else is automatically faulted for the device. If the device does not know how to handle device_private then it sets dev_private_owner to NULL and it never gets device_private pfns. Since the device_private pfn cannot be dma mapped, drivers must have explicit support for them. Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On Tue, Mar 17, 2020 at 01:24:45PM +0100, Christoph Hellwig wrote:
> On Tue, Mar 17, 2020 at 09:15:36AM -0300, Jason Gunthorpe wrote:
> > > Getting rid of HMM_PFN_DEVICE_PRIVATE seems reasonable to me since a
> > > driver can
> > > look at the struct page but what if a driver needs to fault in a page from
> > > another device's private memory? Should it call handle_mm_fault()?
> >
> > Isn't that what this series basically does?
> >
> > The dev_private_owner is set to the type of pgmap the device knows how
> > to handle, and everything else is automatically faulted for the
> > device.
> >
> > If the device does not know how to handle device_private then it sets
> > dev_private_owner to NULL and it never gets device_private pfns.
> >
> > Since the device_private pfn cannot be dma mapped, drivers must have
> > explicit support for them.
>
> No, with this series (and all actual callers before this series)
> we never fault in device private pages.
IFF we want to fault it in we'd need something like this. But I'd
really prefer to see test cases for that first.
diff --git a/mm/hmm.c b/mm/hmm.c
index b75b3750e03d..2884a3d11a1f 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -276,7 +276,7 @@ static int hmm_vma_handle_pte(struct mm_walk *walk,
unsigned long addr,
if (!fault && !write_fault)
return 0;
- if (!non_swap_entry(entry))
+ if (!non_swap_entry(entry) || is_device_private_entry(entry))
goto fault;
if (is_migration_entry(entry)) {
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On Mon, Mar 16, 2020 at 03:49:51PM -0700, Ralph Campbell wrote: > On 3/16/20 12:32 PM, Christoph Hellwig wrote: >> Remove the code to fault device private pages back into system memory >> that has never been used by any driver. Also replace the usage of the >> HMM_PFN_DEVICE_PRIVATE flag in the pfns array with a simple >> is_device_private_page check in nouveau. >> >> Signed-off-by: Christoph Hellwig > > Getting rid of HMM_PFN_DEVICE_PRIVATE seems reasonable to me since a driver > can > look at the struct page but what if a driver needs to fault in a page from > another device's private memory? Should it call handle_mm_fault()? Obviously no driver cared for that so far. Once we have test cases for that and thus testable code we can add code to fault it in from hmm_vma_handle_pte. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On Tue, Mar 17, 2020 at 01:28:13PM +0100, Christoph Hellwig wrote: > On Tue, Mar 17, 2020 at 01:24:45PM +0100, Christoph Hellwig wrote: > > On Tue, Mar 17, 2020 at 09:15:36AM -0300, Jason Gunthorpe wrote: > > > > Getting rid of HMM_PFN_DEVICE_PRIVATE seems reasonable to me since a > > > > driver can > > > > look at the struct page but what if a driver needs to fault in a page > > > > from > > > > another device's private memory? Should it call handle_mm_fault()? > > > > > > Isn't that what this series basically does? > > > > > > The dev_private_owner is set to the type of pgmap the device knows how > > > to handle, and everything else is automatically faulted for the > > > device. > > > > > > If the device does not know how to handle device_private then it sets > > > dev_private_owner to NULL and it never gets device_private pfns. > > > > > > Since the device_private pfn cannot be dma mapped, drivers must have > > > explicit support for them. > > > > No, with this series (and all actual callers before this series) > > we never fault in device private pages. > > IFF we want to fault it in we'd need something like this. But I'd > really prefer to see test cases for that first. In general I think hmm_range_fault should have a mode that is the same as get_user_pages in terms of when it returns a hard failure, and generates faults. AFAIK, GUP will fault in this case? I need this for making ODP use this API. ODP is the one that is highly likely to see other driver's device_private pages and must have them always fault to CPU. > diff --git a/mm/hmm.c b/mm/hmm.c > index b75b3750e03d..2884a3d11a1f 100644 > +++ b/mm/hmm.c > @@ -276,7 +276,7 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, > unsigned long addr, > if (!fault && !write_fault) > return 0; > > - if (!non_swap_entry(entry)) > + if (!non_swap_entry(entry) || is_device_private_entry(entry)) > goto fault; Yes, OK, makes sense. I've been using v7 of Ralph's tester and it is working well - it has DEVICE_PRIVATE support so I think it can test this flow too. Ralph are you able? This hunk seems trivial enough to me, can we include it now? Thanks, Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On 3/16/20 12:32 PM, Christoph Hellwig wrote:
Remove the code to fault device private pages back into system memory
that has never been used by any driver. Also replace the usage of the
HMM_PFN_DEVICE_PRIVATE flag in the pfns array with a simple
is_device_private_page check in nouveau.
Signed-off-by: Christoph Hellwig
Getting rid of HMM_PFN_DEVICE_PRIVATE seems reasonable to me since a driver can
look at the struct page but what if a driver needs to fault in a page from
another device's private memory? Should it call handle_mm_fault()?
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 -
drivers/gpu/drm/nouveau/nouveau_dmem.c | 5 +++--
drivers/gpu/drm/nouveau/nouveau_svm.c | 1 -
include/linux/hmm.h | 2 --
mm/hmm.c| 25 +
5 files changed, 8 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index dee446278417..90821ce5e6ca 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -776,7 +776,6 @@ struct amdgpu_ttm_tt {
static const uint64_t hmm_range_flags[HMM_PFN_FLAG_MAX] = {
(1 << 0), /* HMM_PFN_VALID */
(1 << 1), /* HMM_PFN_WRITE */
- 0 /* HMM_PFN_DEVICE_PRIVATE */
};
static const uint64_t hmm_range_values[HMM_PFN_VALUE_MAX] = {
diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c
b/drivers/gpu/drm/nouveau/nouveau_dmem.c
index 0e36345d395c..edfd0805fba4 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
@@ -28,6 +28,7 @@
#include
#include
+#include
#include
#include
@@ -692,9 +693,8 @@ nouveau_dmem_convert_pfn(struct nouveau_drm *drm,
if (page == NULL)
continue;
- if (!(range->pfns[i] & range->flags[HMM_PFN_DEVICE_PRIVATE])) {
+ if (!is_device_private_page(page))
continue;
- }
if (!nouveau_dmem_page(drm, page)) {
WARN(1, "Some unknown device memory !\n");
@@ -705,5 +705,6 @@ nouveau_dmem_convert_pfn(struct nouveau_drm *drm,
addr = nouveau_dmem_page_addr(page);
range->pfns[i] &= ((1UL << range->pfn_shift) - 1);
range->pfns[i] |= (addr >> PAGE_SHIFT) << range->pfn_shift;
+ range->pfns[i] |= NVIF_VMM_PFNMAP_V0_VRAM;
}
}
diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c
b/drivers/gpu/drm/nouveau/nouveau_svm.c
index df9bf1fd1bc0..39c731a99937 100644
--- a/drivers/gpu/drm/nouveau/nouveau_svm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
@@ -367,7 +367,6 @@ static const u64
nouveau_svm_pfn_flags[HMM_PFN_FLAG_MAX] = {
[HMM_PFN_VALID ] = NVIF_VMM_PFNMAP_V0_V,
[HMM_PFN_WRITE ] = NVIF_VMM_PFNMAP_V0_W,
- [HMM_PFN_DEVICE_PRIVATE] = NVIF_VMM_PFNMAP_V0_VRAM,
};
static const u64
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 4bf8d6997b12..5e6034f105c3 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -74,7 +74,6 @@
* Flags:
* HMM_PFN_VALID: pfn is valid. It has, at least, read permission.
* HMM_PFN_WRITE: CPU page table has write permission set
- * HMM_PFN_DEVICE_PRIVATE: private device memory (ZONE_DEVICE)
*
* The driver provides a flags array for mapping page protections to device
* PTE bits. If the driver valid bit for an entry is bit 3,
@@ -86,7 +85,6 @@
enum hmm_pfn_flag_e {
HMM_PFN_VALID = 0,
HMM_PFN_WRITE,
- HMM_PFN_DEVICE_PRIVATE,
HMM_PFN_FLAG_MAX
};
diff --git a/mm/hmm.c b/mm/hmm.c
index 180e398170b0..cfad65f6a67b 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -118,15 +118,6 @@ static inline void hmm_pte_need_fault(const struct
hmm_vma_walk *hmm_vma_walk,
/* We aren't ask to do anything ... */
if (!(pfns & range->flags[HMM_PFN_VALID]))
return;
- /* If this is device memory then only fault if explicitly requested */
- if ((cpu_flags & range->flags[HMM_PFN_DEVICE_PRIVATE])) {
- /* Do we fault on device memory ? */
- if (pfns & range->flags[HMM_PFN_DEVICE_PRIVATE]) {
- *write_fault = pfns & range->flags[HMM_PFN_WRITE];
- *fault = true;
- }
- return;
- }
/* If CPU page table is not valid then we need to fault */
*fault = !(cpu_flags & range->flags[HMM_PFN_VALID]);
@@ -260,21 +251,15 @@ static int hmm_vma_handle_pte(struct mm_walk *walk,
unsigned long addr,
swp_entry_t entry = pte_to_swp_entry(pte);
/*
-* This is a special swap entry, ignore migration, use
-* device and report anything else as error.
+* Never fault in device private pages pages, but just report
+* the PFN even if not present.
*/
Re: [PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
On Mon, Mar 16, 2020 at 08:32:15PM +0100, Christoph Hellwig wrote:
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 180e398170b0..cfad65f6a67b 100644
> +++ b/mm/hmm.c
> @@ -118,15 +118,6 @@ static inline void hmm_pte_need_fault(const struct
> hmm_vma_walk *hmm_vma_walk,
> /* We aren't ask to do anything ... */
> if (!(pfns & range->flags[HMM_PFN_VALID]))
> return;
> - /* If this is device memory then only fault if explicitly requested */
> - if ((cpu_flags & range->flags[HMM_PFN_DEVICE_PRIVATE])) {
> - /* Do we fault on device memory ? */
> - if (pfns & range->flags[HMM_PFN_DEVICE_PRIVATE]) {
> - *write_fault = pfns & range->flags[HMM_PFN_WRITE];
> - *fault = true;
> - }
> - return;
> - }
Yes, this is an elegant solution to the input flags.
However, between patch 3 and 4 doesn't this break amd gpu as it will
return device_private pages now if not requested? Squash the two?
Jason
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 3/4] mm: simplify device private page handling in hmm_range_fault
Remove the code to fault device private pages back into system memory
that has never been used by any driver. Also replace the usage of the
HMM_PFN_DEVICE_PRIVATE flag in the pfns array with a simple
is_device_private_page check in nouveau.
Signed-off-by: Christoph Hellwig
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 -
drivers/gpu/drm/nouveau/nouveau_dmem.c | 5 +++--
drivers/gpu/drm/nouveau/nouveau_svm.c | 1 -
include/linux/hmm.h | 2 --
mm/hmm.c| 25 +
5 files changed, 8 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index dee446278417..90821ce5e6ca 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -776,7 +776,6 @@ struct amdgpu_ttm_tt {
static const uint64_t hmm_range_flags[HMM_PFN_FLAG_MAX] = {
(1 << 0), /* HMM_PFN_VALID */
(1 << 1), /* HMM_PFN_WRITE */
- 0 /* HMM_PFN_DEVICE_PRIVATE */
};
static const uint64_t hmm_range_values[HMM_PFN_VALUE_MAX] = {
diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c
b/drivers/gpu/drm/nouveau/nouveau_dmem.c
index 0e36345d395c..edfd0805fba4 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
@@ -28,6 +28,7 @@
#include
#include
+#include
#include
#include
@@ -692,9 +693,8 @@ nouveau_dmem_convert_pfn(struct nouveau_drm *drm,
if (page == NULL)
continue;
- if (!(range->pfns[i] & range->flags[HMM_PFN_DEVICE_PRIVATE])) {
+ if (!is_device_private_page(page))
continue;
- }
if (!nouveau_dmem_page(drm, page)) {
WARN(1, "Some unknown device memory !\n");
@@ -705,5 +705,6 @@ nouveau_dmem_convert_pfn(struct nouveau_drm *drm,
addr = nouveau_dmem_page_addr(page);
range->pfns[i] &= ((1UL << range->pfn_shift) - 1);
range->pfns[i] |= (addr >> PAGE_SHIFT) << range->pfn_shift;
+ range->pfns[i] |= NVIF_VMM_PFNMAP_V0_VRAM;
}
}
diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c
b/drivers/gpu/drm/nouveau/nouveau_svm.c
index df9bf1fd1bc0..39c731a99937 100644
--- a/drivers/gpu/drm/nouveau/nouveau_svm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
@@ -367,7 +367,6 @@ static const u64
nouveau_svm_pfn_flags[HMM_PFN_FLAG_MAX] = {
[HMM_PFN_VALID ] = NVIF_VMM_PFNMAP_V0_V,
[HMM_PFN_WRITE ] = NVIF_VMM_PFNMAP_V0_W,
- [HMM_PFN_DEVICE_PRIVATE] = NVIF_VMM_PFNMAP_V0_VRAM,
};
static const u64
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 4bf8d6997b12..5e6034f105c3 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -74,7 +74,6 @@
* Flags:
* HMM_PFN_VALID: pfn is valid. It has, at least, read permission.
* HMM_PFN_WRITE: CPU page table has write permission set
- * HMM_PFN_DEVICE_PRIVATE: private device memory (ZONE_DEVICE)
*
* The driver provides a flags array for mapping page protections to device
* PTE bits. If the driver valid bit for an entry is bit 3,
@@ -86,7 +85,6 @@
enum hmm_pfn_flag_e {
HMM_PFN_VALID = 0,
HMM_PFN_WRITE,
- HMM_PFN_DEVICE_PRIVATE,
HMM_PFN_FLAG_MAX
};
diff --git a/mm/hmm.c b/mm/hmm.c
index 180e398170b0..cfad65f6a67b 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -118,15 +118,6 @@ static inline void hmm_pte_need_fault(const struct
hmm_vma_walk *hmm_vma_walk,
/* We aren't ask to do anything ... */
if (!(pfns & range->flags[HMM_PFN_VALID]))
return;
- /* If this is device memory then only fault if explicitly requested */
- if ((cpu_flags & range->flags[HMM_PFN_DEVICE_PRIVATE])) {
- /* Do we fault on device memory ? */
- if (pfns & range->flags[HMM_PFN_DEVICE_PRIVATE]) {
- *write_fault = pfns & range->flags[HMM_PFN_WRITE];
- *fault = true;
- }
- return;
- }
/* If CPU page table is not valid then we need to fault */
*fault = !(cpu_flags & range->flags[HMM_PFN_VALID]);
@@ -260,21 +251,15 @@ static int hmm_vma_handle_pte(struct mm_walk *walk,
unsigned long addr,
swp_entry_t entry = pte_to_swp_entry(pte);
/*
-* This is a special swap entry, ignore migration, use
-* device and report anything else as error.
+* Never fault in device private pages pages, but just report
+* the PFN even if not present.
*/
if (is_device_private_entry(entry)) {
- cpu_flags = range->flags[HMM_PFN_VALID] |
- range->flags[HMM_PFN_DEVICE_PRIVATE];
- cpu_flags |= is_write_device_private_entry(entry) ?
-
