Skip to contentSkip to conversation

mm/hmm/test: add self tests for HMM

PATCH Superseded Ralph Campbell 4 participants 17 messages Jan 13, 2020 – Feb 20, 2020

Subsystems (9)
DRM drivers Memory management DRM misc Kselftest +5 more subsystems

v6 of 10 Superseded Latest is v10 →

lore
Share & tools
lore

Ralph Campbell

author
#0 lore raw

v6 cover mm/hmm/test: add self tests for HMM

This series adds new functions to the mmu interval notifier API to allow device drivers with MMUs to dynamically mirror a process' page tables based on device faults and invalidation callbacks. The Nouveau driver is updated to use the extended API and a set of stand alone self tests is added to help validate and maintain correctness.

The patches are based on linux-5.5.0-rc6 and are for Jason's rdma/hmm tree since I believe he is planning some interval notifier changes.

Changes v5 -> v6:
Rebase to linux-5.5.0-rc6
Refactored mmu interval notifier patches Converted nouveau to use the new mmu interval notifier API

Changes v4 -> v5:
Added mmu interval notifier insert/remove/update callable from the
invalidate() callback
Updated HMM tests to use the new core interval notifier API

Changes v1 -> v4:
https://lore.kernel.org/linux-mm/20191104222141.5173-1-rcampbell-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org

Ralph Campbell (6):
mm/mmu_notifier: add mmu_interval_notifier_insert_safe() mm/mmu_notifier: add mmu_interval_notifier_put() mm/notifier: add mmu_interval_notifier_update() mm/mmu_notifier: add mmu_interval_notifier_find()
nouveau: use new mmu interval notifiers mm/hmm/test: add self tests for HMM

 MAINTAINERS                            |    3 +
 drivers/gpu/drm/nouveau/nouveau_svm.c  |  313 ++++--
 include/linux/mmu_notifier.h           |   29 +
 lib/Kconfig.debug                      |   11 +
 lib/Makefile                           |    1 +
 lib/test_hmm.c                         | 1368 ++++++++++++++++++++++++
 mm/mmu_notifier.c                      |  223 +++-
 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 | 1354 +++++++++++++++++++++++
 tools/testing/selftests/vm/run_vmtests |   16 +
 tools/testing/selftests/vm/test_hmm.sh |   97 ++
 13 files changed, 3289 insertions(+), 132 deletions(-)
 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
Signature · 1 line
2.20.1
author
#1 lore raw

PATCH v6 1/6 mm/mmu_notifier: add mmu_interval_notifier_insert_safe()

mmu_interval_notifier_insert() can't be called safely from inside the invalidate() callback because it can acquire the mmap_sem lock which might already be held. Insertion might be needed when the invalidate() callback creates a "hole" in the interval being tracked (i.e., the event type MMU_NOTIFY_UNMAP) and the interval needs to be split in order to continue receiving callbacks for the remaining left and right intervals.

Add a new function mmu_interval_notifier_insert_safe() which can be called from the invalidate() callback.

Signed-off-by Ralph Campbell <[email protected]>
---
 include/linux/mmu_notifier.h |  4 ++++
 mm/mmu_notifier.c            | 45 ++++++++++++++++++++++++++++++++----
 2 files changed, 45 insertions(+), 4 deletions(-)
include/linux/mmu_notifier.h +4 -0
index 9e6caa8ecd19..027c9c8f3a69 100644
@@ -299,6 +299,10 @@ int mmu_interval_notifier_insert_locked(
 	struct mmu_interval_notifier *mni, struct mm_struct *mm,
 	unsigned long start, unsigned long length,
 	const struct mmu_interval_notifier_ops *ops);
+int mmu_interval_notifier_insert_safe(
+	struct mmu_interval_notifier *mni, struct mm_struct *mm,
+	unsigned long start, unsigned long length,
+	const struct mmu_interval_notifier_ops *ops);
 void mmu_interval_notifier_remove(struct mmu_interval_notifier *mni);
 
 /**
mm/mmu_notifier.c +41 -4

63 lines not on this page · raw message

Signature · 1 line
2.20.1
#2 lore raw

Re: [PATCH v6 1/6] mm/mmu_notifier: add mmu_interval_notifier_insert_safe()

The name is weird, normally we call functions like this _locked.

Additionally it will need a lockdep_assert_held to ensure the caller actually holds the lock.

author
#3 lore raw

PATCH v6 2/6 mm/mmu_notifier: add mmu_interval_notifier_put()

In reply to Ralph Campbell

mmu_interval_notifier_remove() can't be called safely from inside the invalidate() callback because it sleeps waiting for invalidate callbacks to finish. Removals might be needed when the invalidate() callback is for munmap() (i.e., the event type MMU_NOTIFY_UNMAP), and the interval being tracked is no longer needed.

Add a new function mmu_interval_notifier_put() which is safe to call from the invalidate() callback. The ops->release() function will be called when all callbacks are finished and no CPUs are accessing the mmu_interval_notifier.

Signed-off-by Ralph Campbell <[email protected]>
---
 include/linux/mmu_notifier.h |  6 +++
 mm/mmu_notifier.c            | 86 ++++++++++++++++++++++++++++--------
 2 files changed, 74 insertions(+), 18 deletions(-)
include/linux/mmu_notifier.h +6 -0
index 027c9c8f3a69..6dcaa632eef7 100644
@@ -233,11 +233,16 @@ struct mmu_notifier {
  * @invalidate: Upon return the caller must stop using any SPTEs within this
  *              range. This function can sleep. Return false only if sleeping
  *              was required but mmu_notifier_range_blockable(range) is false.
+ * @release:	This function should be defined when using
+ *		mmu_interval_notifier_put(). It will be called when the
+ *		mmu_interval_notifier is removed from the interval tree.
+ *		No other callbacks will be generated after this returns.
  */
 struct mmu_interval_notifier_ops {
 	bool (*invalidate)(struct mmu_interval_notifier *mni,
 			   const struct mmu_notifier_range *range,
 			   unsigned long cur_seq);
+	void (*release)(struct mmu_interval_notifier *mni);
 };
 
 struct mmu_interval_notifier {

@@ -304,6 +309,7 @@ int mmu_interval_notifier_insert_safe(
 	unsigned long start, unsigned long length,
 	const struct mmu_interval_notifier_ops *ops);
 void mmu_interval_notifier_remove(struct mmu_interval_notifier *mni);
+void mmu_interval_notifier_put(struct mmu_interval_notifier *mni);
 
 /**
  * mmu_interval_set_seq - Save the invalidation sequence
mm/mmu_notifier.c +68 -18

135 lines not on this page · raw message

Signature · 1 line
2.20.1
author
#4 lore raw

PATCH v6 3/6 mm/notifier: add mmu_interval_notifier_update()

In reply to Ralph Campbell

When using mmu interval notifiers, the underlying VMA range that the interval is tracking might be expanded or shrunk. Add a helper function to allow the interval to be updated safely from the ops->invalidate() callback in response to VMA range changes. This is effectively removing and inserting the mmu interval notifier but more efficient.

Signed-off-by Ralph Campbell <[email protected]>
---
 include/linux/mmu_notifier.h |  4 +++
 mm/mmu_notifier.c            | 69 ++++++++++++++++++++++++++++++++++--
 2 files changed, 70 insertions(+), 3 deletions(-)
include/linux/mmu_notifier.h +4 -0
index 6dcaa632eef7..0ce59b4f22c2 100644
@@ -251,6 +251,8 @@ struct mmu_interval_notifier {
 	struct mm_struct *mm;
 	struct hlist_node deferred_item;
 	unsigned long invalidate_seq;
+	unsigned long updated_start;
+	unsigned long updated_last;
 };
 
 #ifdef CONFIG_MMU_NOTIFIER

@@ -310,6 +312,8 @@ int mmu_interval_notifier_insert_safe(
 	const struct mmu_interval_notifier_ops *ops);
 void mmu_interval_notifier_remove(struct mmu_interval_notifier *mni);
 void mmu_interval_notifier_put(struct mmu_interval_notifier *mni);
+void mmu_interval_notifier_update(struct mmu_interval_notifier *mni,
+				  unsigned long start, unsigned long last);
 
 /**
  * mmu_interval_set_seq - Save the invalidation sequence
mm/mmu_notifier.c +66 -3

93 lines not on this page · raw message

Signature · 1 line
2.20.1
author
#5 lore raw

PATCH v6 4/6 mm/mmu_notifier: add mmu_interval_notifier_find()

In reply to Ralph Campbell

Device drivers may or may not have a convenient range based data structure to look up and find intervals that are registered with the mmu interval notifiers. Rather than forcing drivers to duplicate the interval tree, provide an API to look up intervals that are registered and accessor functions to return the start and last address of the interval.

Signed-off-by Ralph Campbell <[email protected]>
---
 include/linux/mmu_notifier.h | 15 +++++++++++++++
 mm/mmu_notifier.c            | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)
include/linux/mmu_notifier.h +15 -0
index 0ce59b4f22c2..cdbbad13b278 100644
@@ -314,6 +314,21 @@ void mmu_interval_notifier_remove(struct mmu_interval_notifier *mni);
 void mmu_interval_notifier_put(struct mmu_interval_notifier *mni);
 void mmu_interval_notifier_update(struct mmu_interval_notifier *mni,
 				  unsigned long start, unsigned long last);
+struct mmu_interval_notifier *mmu_interval_notifier_find(struct mm_struct *mm,
+				const struct mmu_interval_notifier_ops *ops,
+				unsigned long start, unsigned long last);
+
+static inline unsigned long mmu_interval_notifier_start(
+				struct mmu_interval_notifier *mni)
+{
+	return mni->interval_tree.start;
+}
+
+static inline unsigned long mmu_interval_notifier_last(
+				struct mmu_interval_notifier *mni)
+{
+	return mni->interval_tree.last;
+}
 
 /**
  * mmu_interval_set_seq - Save the invalidation sequence
mm/mmu_notifier.c +33 -0
index 47ad9cc89aab..4efecc0f13cb 100644
@@ -1171,6 +1171,39 @@ void mmu_interval_notifier_update(struct mmu_interval_notifier *mni,
 }
 EXPORT_SYMBOL_GPL(mmu_interval_notifier_update);
 
+struct mmu_interval_notifier *mmu_interval_notifier_find(struct mm_struct *mm,
+				const struct mmu_interval_notifier_ops *ops,
+				unsigned long start, unsigned long last)
+{
+	struct mmu_notifier_mm *mmn_mm = mm->mmu_notifier_mm;
+	struct interval_tree_node *node;
+	struct mmu_interval_notifier *mni;
+	struct mmu_interval_notifier *res = NULL;
+
+	spin_lock(&mmn_mm->lock);
+	node = interval_tree_iter_first(&mmn_mm->itree, start, last);
+	if (node) {
+		mni = container_of(node, struct mmu_interval_notifier,
+				   interval_tree);
+		while (true) {
+			if (mni->ops == ops) {
+				res = mni;
+				break;
+			}
+			node = interval_tree_iter_next(&mni->interval_tree,
+						       start, last);
+			if (!node)
+				break;
+			mni = container_of(node, struct mmu_interval_notifier,
+					   interval_tree);
+		}
+	}
+	spin_unlock(&mmn_mm->lock);
+
+	return res;
+}
+EXPORT_SYMBOL_GPL(mmu_interval_notifier_find);
+
 /**
  * mmu_notifier_synchronize - Ensure all mmu_notifiers are freed
  *
Signature · 1 line
2.20.1
#6 lore raw

Re: [PATCH v6 4/6] mm/mmu_notifier: add mmu_interval_notifier_find()

Quoted 25 lines · Ralph Campbell
mm/mmu_notifier.c +19 -0
index 47ad9cc89aab..4efecc0f13cb 100644
+++ b/mm/mmu_notifier.c

@@ -1171,6 +1171,39 @@ void mmu_interval_notifier_update(struct mmu_interval_notifier *mni,
 }
 EXPORT_SYMBOL_GPL(mmu_interval_notifier_update);
 
+struct mmu_interval_notifier *mmu_interval_notifier_find(struct mm_struct *mm,
+				const struct mmu_interval_notifier_ops *ops,
+				unsigned long start, unsigned long last)
+{
+	struct mmu_notifier_mm *mmn_mm = mm->mmu_notifier_mm;
+	struct interval_tree_node *node;
+	struct mmu_interval_notifier *mni;
+	struct mmu_interval_notifier *res = NULL;
+
+	spin_lock(&mmn_mm->lock);
+	node = interval_tree_iter_first(&mmn_mm->itree, start, last);
+	if (node) {
+		mni = container_of(node, struct mmu_interval_notifier,
+				   interval_tree);
+		while (true) {
+			if (mni->ops == ops) {
+				res = mni;
+				break;

mm/mmu_notifier.c void mmu_interval_notifier_update(struct mmu_interval_notifier *mni, · line 1201

Quoted 10 lines
mm/mmu_notifier.c +10 -0
+			}
+			node = interval_tree_iter_next(&mni->interval_tree,
+						       start, last);
+			if (!node)
+				break;
+			mni = container_of(node, struct mmu_interval_notifier,
+					   interval_tree);
+		}
+	}
+	spin_unlock(&mmn_mm->lock);

This doesn't seem safe at all, here we are returning a pointer to memory from the interval tree with out any kind of lifetime protection.

If the interval tree is read it must be left in the read lock state until the caller is done with the pointer.

.. and this poses all sorts of questions about consistency with items on the deferred list. Should find return an item undergoing deletion?

Should find return items using the old interval tree values or their new updated values?

Jason

author
#7 lore raw

Re: [PATCH v6 4/6] mm/mmu_notifier: add mmu_interval_notifier_find()

Quoted 32 lines · Jason Gunthorpe
Quoted 31 lines · Ralph Campbell
mm/mmu_notifier.c +25 -0
index 47ad9cc89aab..4efecc0f13cb 100644
+++ b/mm/mmu_notifier.c

@@ -1171,6 +1171,39 @@ void mmu_interval_notifier_update(struct mmu_interval_notifier *mni,
  }
  EXPORT_SYMBOL_GPL(mmu_interval_notifier_update);
 
+struct mmu_interval_notifier *mmu_interval_notifier_find(struct mm_struct *mm,
+				const struct mmu_interval_notifier_ops *ops,
+				unsigned long start, unsigned long last)
+{
+	struct mmu_notifier_mm *mmn_mm = mm->mmu_notifier_mm;
+	struct interval_tree_node *node;
+	struct mmu_interval_notifier *mni;
+	struct mmu_interval_notifier *res = NULL;
+
+	spin_lock(&mmn_mm->lock);
+	node = interval_tree_iter_first(&mmn_mm->itree, start, last);
+	if (node) {
+		mni = container_of(node, struct mmu_interval_notifier,
+				   interval_tree);
+		while (true) {
+			if (mni->ops == ops) {
+				res = mni;
+				break;
+			}
+			node = interval_tree_iter_next(&mni->interval_tree,
+						       start, last);
+			if (!node)
+				break;
+			mni = container_of(node, struct mmu_interval_notifier,
Quoted 8 lines

mm/mmu_notifier.c void mmu_interval_notifier_update(struct mmu_interval_notifier *mni, · line 1201

Quoted 4 lines
mm/mmu_notifier.c +4 -0
+					   interval_tree);
+		}
+	}
+	spin_unlock(&mmn_mm->lock);

This doesn't seem safe at all, here we are returning a pointer to memory from the interval tree with out any kind of lifetime protection.

It is memory that the driver has allocated and has full control over the lifetime since the driver does all the insertions and removals. The driver does have to hold the HW page table lock so lookups are synchronized with interval insertions and removals and page table entry insertions and removals.

Quoted 5 lines

If the interval tree is read it must be left in the read lock state until the caller is done with the pointer.

.. and this poses all sorts of questions about consistency with items on the deferred list. Should find return an item undergoing deletion?

I don't think so. The deferred operations are all complete when mmu_interval_read_begin() returns, and the sequence number check with mmu_interval_read_retry() guarantees there have been no changes while not holding the driver page table lock and calling hmm_range_fault().

Quoted 4 lines

Should find return items using the old interval tree values or their new updated values?

Jason

I think it should only look at the old interval tree and not the deferred insert/remove/updates as explained above.

#8 lore raw

Re: [PATCH v6 4/6] mm/mmu_notifier: add mmu_interval_notifier_find()

Quoted 42 lines · Ralph Campbell
Quoted 31 lines · Jason Gunthorpe
Quoted 30 lines · Ralph Campbell
mm/mmu_notifier.c +25 -0
index 47ad9cc89aab..4efecc0f13cb 100644
+++ b/mm/mmu_notifier.c

@@ -1171,6 +1171,39 @@ void mmu_interval_notifier_update(struct mmu_interval_notifier *mni,
  }
  EXPORT_SYMBOL_GPL(mmu_interval_notifier_update);
+struct mmu_interval_notifier *mmu_interval_notifier_find(struct mm_struct *mm,
+				const struct mmu_interval_notifier_ops *ops,
+				unsigned long start, unsigned long last)
+{
+	struct mmu_notifier_mm *mmn_mm = mm->mmu_notifier_mm;
+	struct interval_tree_node *node;
+	struct mmu_interval_notifier *mni;
+	struct mmu_interval_notifier *res = NULL;
+
+	spin_lock(&mmn_mm->lock);
+	node = interval_tree_iter_first(&mmn_mm->itree, start, last);
+	if (node) {
+		mni = container_of(node, struct mmu_interval_notifier,
+				   interval_tree);
+		while (true) {
+			if (mni->ops == ops) {
+				res = mni;
+				break;
+			}
+			node = interval_tree_iter_next(&mni->interval_tree,
+						       start, last);
+			if (!node)
+				break;
+			mni = container_of(node, struct mmu_interval_notifier,
Quoted 8 lines

mm/mmu_notifier.c void mmu_interval_notifier_update(struct mmu_interval_notifier *mni, · line 1201

Quoted 4 lines
mm/mmu_notifier.c +4 -0
+					   interval_tree);
+		}
+	}
+	spin_unlock(&mmn_mm->lock);

This doesn't seem safe at all, here we are returning a pointer to memory from the interval tree with out any kind of lifetime protection.

Quoted 5 lines

It is memory that the driver has allocated and has full control over the lifetime since the driver does all the insertions and removals. The driver does have to hold the HW page table lock so lookups are synchronized with interval insertions and removals and page table entry insertions and removals.

No.. the ->release is async, so having the driver hold a lock around all the mmu_interval_ APIS still doesn't make it safe. The element could be on the defered list and it could become freed at any moment.

Quoted 10 lines
Quoted 5 lines

If the interval tree is read it must be left in the read lock state until the caller is done with the pointer.

.. and this poses all sorts of questions about consistency with items on the deferred list. Should find return an item undergoing deletion?

I don't think so. The deferred operations are all complete when mmu_interval_read_begin() returns, and the sequence number check with mmu_interval_read_retry() guarantees there have been no changes while not holding the driver page table lock and calling hmm_range_fault().

It seems very dangerous to say, on one hand, that the driver is serialized because it holds a lock around all mmu_interval_* calls, while on the other saying that on rare edge cases find does not return a result that matches the serial-program-order sequence.

This seems like a way to create bugs.

For instance, if find is consistent with the defered list then it will not return any element that has a pending deletion and the above issue with lifetime wouldn't happen.

However, I'm still not sure that providing an API tha requires the driver to provide tricky locking is the best idea. This basically says that if a driver uses find then every single other call to mmu_interval_* must be serialized with a single lock.

Jason

author
#9 lore raw

PATCH v6 5/6 nouveau: use new mmu interval notifiers

In reply to Ralph Campbell

Update nouveau to only use the mmu interval notifiers.

Signed-off-by Ralph Campbell <[email protected]>
---
 drivers/gpu/drm/nouveau/nouveau_svm.c | 313 +++++++++++++++++---------
 1 file changed, 201 insertions(+), 112 deletions(-)
drivers/gpu/drm/nouveau/nouveau_svm.c +201 -112
index df9bf1fd1bc0..0343e48d41d7 100644
@@ -88,7 +88,7 @@ nouveau_ivmm_find(struct nouveau_svm *svm, u64 inst)
 }
 
 struct nouveau_svmm {
-	struct mmu_notifier notifier;
+	struct mm_struct *mm;
 	struct nouveau_vmm *vmm;
 	struct {
 		unsigned long start;

@@ -98,6 +98,13 @@ struct nouveau_svmm {
 	struct mutex mutex;
 };
 
+struct svmm_interval {
+	struct mmu_interval_notifier notifier;
+	struct nouveau_svmm *svmm;
+};
+
+static const struct mmu_interval_notifier_ops nouveau_svm_mni_ops;
+
 #define SVMM_DBG(s,f,a...)                                                     \
 	NV_DEBUG((s)->vmm->cli->drm, "svm-%p: "f"\n", (s), ##a)
 #define SVMM_ERR(s,f,a...)                                                     \

@@ -236,6 +243,8 @@ nouveau_svmm_join(struct nouveau_svmm *svmm, u64 inst)
 static void
 nouveau_svmm_invalidate(struct nouveau_svmm *svmm, u64 start, u64 limit)
 {
+	SVMM_DBG(svmm, "invalidate %016llx-%016llx", start, limit);
+
 	if (limit > start) {
 		bool super = svmm->vmm->vmm.object.client->super;
 		svmm->vmm->vmm.object.client->super = true;

@@ -248,58 +257,25 @@ nouveau_svmm_invalidate(struct nouveau_svmm *svmm, u64 start, u64 limit)
 	}
 }
 
-static int
-nouveau_svmm_invalidate_range_start(struct mmu_notifier *mn,
-				    const struct mmu_notifier_range *update)
-{
-	struct nouveau_svmm *svmm =
-		container_of(mn, struct nouveau_svmm, notifier);
-	unsigned long start = update->start;
-	unsigned long limit = update->end;
-
-	if (!mmu_notifier_range_blockable(update))
-		return -EAGAIN;
-
-	SVMM_DBG(svmm, "invalidate %016lx-%016lx", start, limit);
-
-	mutex_lock(&svmm->mutex);
-	if (unlikely(!svmm->vmm))
-		goto out;
-
-	if (limit > svmm->unmanaged.start && start < svmm->unmanaged.limit) {
-		if (start < svmm->unmanaged.start) {
-			nouveau_svmm_invalidate(svmm, start,
-						svmm->unmanaged.limit);
-		}
-		start = svmm->unmanaged.limit;
-	}
-
-	nouveau_svmm_invalidate(svmm, start, limit);
-
-out:
-	mutex_unlock(&svmm->mutex);
-	return 0;
-}
-
-static void nouveau_svmm_free_notifier(struct mmu_notifier *mn)
-{
-	kfree(container_of(mn, struct nouveau_svmm, notifier));
-}
-
-static const struct mmu_notifier_ops nouveau_mn_ops = {
-	.invalidate_range_start = nouveau_svmm_invalidate_range_start,
-	.free_notifier = nouveau_svmm_free_notifier,
-};
-
 void
 nouveau_svmm_fini(struct nouveau_svmm **psvmm)
 {
 	struct nouveau_svmm *svmm = *psvmm;
+	struct mmu_interval_notifier *mni;
+
 	if (svmm) {
 		mutex_lock(&svmm->mutex);
+		while (true) {
+			mni = mmu_interval_notifier_find(svmm->mm,
+					&nouveau_svm_mni_ops, 0UL, ~0UL);
+			if (!mni)
+				break;
+			mmu_interval_notifier_put(mni);
+		}
 		svmm->vmm = NULL;
 		mutex_unlock(&svmm->mutex);
-		mmu_notifier_put(&svmm->notifier);
+		mmdrop(svmm->mm);
+		kfree(svmm);
 		*psvmm = NULL;
 	}
 }

@@ -343,11 +319,12 @@ nouveau_svmm_init(struct drm_device *dev, void *data,
 		goto out_free;
 
 	down_write(&current->mm->mmap_sem);
-	svmm->notifier.ops = &nouveau_mn_ops;
-	ret = __mmu_notifier_register(&svmm->notifier, current->mm);
+	ret = __mmu_notifier_register(NULL, current->mm);
 	if (ret)
 		goto out_mm_unlock;
-	/* Note, ownership of svmm transfers to mmu_notifier */
+
+	mmgrab(current->mm);
+	svmm->mm = current->mm;
 
 	cli->svm.svmm = svmm;
 	cli->svm.cli = cli;

@@ -482,65 +459,212 @@ nouveau_svm_fault_cache(struct nouveau_svm *svm,
 		fault->inst, fault->addr, fault->access);
 }
 
-struct svm_notifier {
-	struct mmu_interval_notifier notifier;
-	struct nouveau_svmm *svmm;
-};
+static struct svmm_interval *nouveau_svmm_new_interval(
+					struct nouveau_svmm *svmm,
+					unsigned long start,
+					unsigned long last)
+{
+	struct svmm_interval *smi;
+	int ret;
+
+	smi = kmalloc(sizeof(*smi), GFP_ATOMIC);
+	if (!smi)
+		return NULL;
+
+	smi->svmm = svmm;
+
+	ret = mmu_interval_notifier_insert_safe(&smi->notifier, svmm->mm,
+				start, last - start + 1, &nouveau_svm_mni_ops);
+	if (ret) {
+		kfree(smi);
+		return NULL;
+	}
+
+	return smi;
+}
+
+static void nouveau_svmm_do_unmap(struct mmu_interval_notifier *mni,
+				 const struct mmu_notifier_range *range)
+{
+	struct svmm_interval *smi =
+		container_of(mni, struct svmm_interval, notifier);
+	struct nouveau_svmm *svmm = smi->svmm;
+	unsigned long start = mmu_interval_notifier_start(mni);
+	unsigned long last = mmu_interval_notifier_last(mni);
+
+	if (start >= range->start) {
+		/* Remove the whole interval or keep the right-hand part. */
+		if (last <= range->end)
+			mmu_interval_notifier_put(mni);
+		else
+			mmu_interval_notifier_update(mni, range->end, last);
+		return;
+	}
+
+	/* Keep the left-hand part of the interval. */
+	mmu_interval_notifier_update(mni, start, range->start - 1);
+
+	/* If a hole is created, create an interval for the right-hand part. */
+	if (last >= range->end) {
+		smi = nouveau_svmm_new_interval(svmm, range->end, last);
+		/*
+		 * If we can't allocate an interval, we won't get invalidation
+		 * callbacks so clear the mapping and rely on faults to reload
+		 * the mappings if needed.
+		 */
+		if (!smi)
+			nouveau_svmm_invalidate(svmm, range->end, last + 1);
+	}
+}
 
-static bool nouveau_svm_range_invalidate(struct mmu_interval_notifier *mni,
-					 const struct mmu_notifier_range *range,
-					 unsigned long cur_seq)
+static bool nouveau_svmm_interval_invalidate(struct mmu_interval_notifier *mni,
+				const struct mmu_notifier_range *range,
+				unsigned long cur_seq)
 {
-	struct svm_notifier *sn =
-		container_of(mni, struct svm_notifier, notifier);
+	struct svmm_interval *smi =
+		container_of(mni, struct svmm_interval, notifier);
+	struct nouveau_svmm *svmm = smi->svmm;
 
 	/*
-	 * serializes the update to mni->invalidate_seq done by caller and
+	 * Serializes the update to mni->invalidate_seq done by the caller and
 	 * prevents invalidation of the PTE from progressing while HW is being
-	 * programmed. This is very hacky and only works because the normal
-	 * notifier that does invalidation is always called after the range
-	 * notifier.
+	 * programmed.
 	 */
 	if (mmu_notifier_range_blockable(range))
-		mutex_lock(&sn->svmm->mutex);
-	else if (!mutex_trylock(&sn->svmm->mutex))
+		mutex_lock(&svmm->mutex);
+	else if (!mutex_trylock(&svmm->mutex))
 		return false;
+
 	mmu_interval_set_seq(mni, cur_seq);
-	mutex_unlock(&sn->svmm->mutex);
+	nouveau_svmm_invalidate(svmm, range->start, range->end);
+
+	/* Stop tracking the range if it is an unmap. */
+	if (range->event == MMU_NOTIFY_UNMAP)
+		nouveau_svmm_do_unmap(mni, range);
+
+	mutex_unlock(&svmm->mutex);
 	return true;
 }
 
+static void nouveau_svmm_interval_release(struct mmu_interval_notifier *mni)
+{
+	struct svmm_interval *smi =
+		container_of(mni, struct svmm_interval, notifier);
+
+	kfree(smi);
+}
+
 static const struct mmu_interval_notifier_ops nouveau_svm_mni_ops = {
-	.invalidate = nouveau_svm_range_invalidate,
+	.invalidate = nouveau_svmm_interval_invalidate,
+	.release = nouveau_svmm_interval_release,
 };
 
+/*
+ * Find or create a mmu_interval_notifier for the given range.
+ * Although mmu_interval_notifier_insert_safe() can handle overlapping
+ * intervals, we only create non-overlapping intervals, shrinking the hmm_range
+ * if it spans more than one svmm_interval.
+ */
+static int nouveau_svmm_interval_find(struct nouveau_svmm *svmm,
+				 struct hmm_range *range)
+{
+	struct mmu_interval_notifier *mni;
+	struct svmm_interval *smi;
+	struct vm_area_struct *vma;
+	unsigned long start = range->start;
+	unsigned long last = range->end - 1;
+	int ret;
+
+	mutex_lock(&svmm->mutex);
+	mni = mmu_interval_notifier_find(svmm->mm, &nouveau_svm_mni_ops, start,
+					 last);
+	if (mni) {
+		if (start >= mmu_interval_notifier_start(mni)) {
+			smi = container_of(mni, struct svmm_interval, notifier);
+			if (last > mmu_interval_notifier_last(mni))
+				range->end =
+					mmu_interval_notifier_last(mni) + 1;
+			goto found;
+		}
+		WARN_ON(last <= mmu_interval_notifier_start(mni));
+		range->end = mmu_interval_notifier_start(mni);
+		last = range->end - 1;
+	}
+	/*
+	 * Might as well create an interval covering the underlying VMA to
+	 * avoid having to create a bunch of small intervals.
+	 */
+	vma = find_vma(svmm->mm, range->start);
+	if (!vma || start < vma->vm_start) {
+		ret = -ENOENT;
+		goto err;
+	}
+	if (range->end > vma->vm_end) {
+		range->end = vma->vm_end;
+		last = range->end - 1;
+	} else if (!mni) {
+		/* Anything registered on the right part of the vma? */
+		mni = mmu_interval_notifier_find(svmm->mm, &nouveau_svm_mni_ops,
+						 range->end, vma->vm_end - 1);
+		if (mni)
+			last = mmu_interval_notifier_start(mni) - 1;
+		else
+			last = vma->vm_end - 1;
+	}
+	/* Anything registered on the left part of the vma? */
+	mni = mmu_interval_notifier_find(svmm->mm, &nouveau_svm_mni_ops,
+					 vma->vm_start, start - 1);
+	if (mni)
+		start = mmu_interval_notifier_last(mni) + 1;
+	else
+		start = vma->vm_start;
+	smi = nouveau_svmm_new_interval(svmm, start, last);
+	if (!smi) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+found:
+	range->notifier = &smi->notifier;
+	mutex_unlock(&svmm->mutex);
+	return 0;
+
+err:
+	mutex_unlock(&svmm->mutex);
+	return ret;
+}
+
 static int nouveau_range_fault(struct nouveau_svmm *svmm,
 			       struct nouveau_drm *drm, void *data, u32 size,
-			       u64 *pfns, struct svm_notifier *notifier)
+			       u64 *pfns, u64 start, u64 end)
 {
 	unsigned long timeout =
 		jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
 	/* Have HMM fault pages within the fault window to the GPU. */
 	struct hmm_range range = {
-		.notifier = &notifier->notifier,
-		.start = notifier->notifier.interval_tree.start,
-		.end = notifier->notifier.interval_tree.last + 1,
+		.start = start,
+		.end = end,
 		.pfns = pfns,
 		.flags = nouveau_svm_pfn_flags,
 		.values = nouveau_svm_pfn_values,
+		.default_flags = 0,
+		.pfn_flags_mask = ~0UL,
 		.pfn_shift = NVIF_VMM_PFNMAP_V0_ADDR_SHIFT,
 	};
-	struct mm_struct *mm = notifier->notifier.mm;
+	struct mm_struct *mm = svmm->mm;
 	long ret;
 
 	while (true) {
 		if (time_after(jiffies, timeout))
 			return -EBUSY;
 
-		range.notifier_seq = mmu_interval_read_begin(range.notifier);
-		range.default_flags = 0;
-		range.pfn_flags_mask = -1UL;
 		down_read(&mm->mmap_sem);
+		ret = nouveau_svmm_interval_find(svmm, &range);
+		if (ret) {
+			up_read(&mm->mmap_sem);
+			return ret;
+		}
+		range.notifier_seq = mmu_interval_read_begin(range.notifier);
 		ret = hmm_range_fault(&range, 0);
 		up_read(&mm->mmap_sem);
 		if (ret <= 0) {

@@ -585,7 +709,6 @@ nouveau_svm_fault(struct nvif_notify *notify)
 		} i;
 		u64 phys[16];
 	} args;
-	struct vm_area_struct *vma;
 	u64 inst, start, limit;
 	int fi, fn, pi, fill;
 	int replay = 0, ret;

@@ -640,7 +763,6 @@ nouveau_svm_fault(struct nvif_notify *notify)
 	args.i.p.version = 0;
 
 	for (fi = 0; fn = fi + 1, fi < buffer->fault_nr; fi = fn) {
-		struct svm_notifier notifier;
 		struct mm_struct *mm;
 
 		/* Cancel any faults from non-SVM channels. */

@@ -662,36 +784,12 @@ nouveau_svm_fault(struct nvif_notify *notify)
 			start = max_t(u64, start, svmm->unmanaged.limit);
 		SVMM_DBG(svmm, "wndw %016llx-%016llx", start, limit);
 
-		mm = svmm->notifier.mm;
+		mm = svmm->mm;
 		if (!mmget_not_zero(mm)) {
 			nouveau_svm_fault_cancel_fault(svm, buffer->fault[fi]);
 			continue;
 		}
 
-		/* Intersect fault window with the CPU VMA, cancelling
-		 * the fault if the address is invalid.
-		 */
-		down_read(&mm->mmap_sem);
-		vma = find_vma_intersection(mm, start, limit);
-		if (!vma) {
-			SVMM_ERR(svmm, "wndw %016llx-%016llx", start, limit);
-			up_read(&mm->mmap_sem);
-			mmput(mm);
-			nouveau_svm_fault_cancel_fault(svm, buffer->fault[fi]);
-			continue;
-		}
-		start = max_t(u64, start, vma->vm_start);
-		limit = min_t(u64, limit, vma->vm_end);
-		up_read(&mm->mmap_sem);
-		SVMM_DBG(svmm, "wndw %016llx-%016llx", start, limit);
-
-		if (buffer->fault[fi]->addr != start) {
-			SVMM_ERR(svmm, "addr %016llx", buffer->fault[fi]->addr);
-			mmput(mm);
-			nouveau_svm_fault_cancel_fault(svm, buffer->fault[fi]);
-			continue;
-		}
-
 		/* Prepare the GPU-side update of all pages within the
 		 * fault window, determining required pages and access
 		 * permissions based on pending faults.

@@ -743,18 +841,9 @@ nouveau_svm_fault(struct nvif_notify *notify)
 			 args.i.p.addr,
 			 args.i.p.addr + args.i.p.size, fn - fi);
 
-		notifier.svmm = svmm;
-		ret = mmu_interval_notifier_insert(&notifier.notifier,
-						   svmm->notifier.mm,
-						   args.i.p.addr, args.i.p.size,
-						   &nouveau_svm_mni_ops);
-		if (!ret) {
-			ret = nouveau_range_fault(
-				svmm, svm->drm, &args,
-				sizeof(args.i) + pi * sizeof(args.phys[0]),
-				args.phys, &notifier);
-			mmu_interval_notifier_remove(&notifier.notifier);
-		}
+		ret = nouveau_range_fault(svmm, svm->drm, &args,
+			sizeof(args.i) + pi * sizeof(args.phys[0]), args.phys,
+			start, start + args.i.p.size);
 		mmput(mm);
 
 		/* Cancel any faults in the window whose pages didn't manage
Signature · 1 line
2.20.1
#10 lore raw

Re: [PATCH v6 5/6] nouveau: use new mmu interval notifiers

Quoted 4 lines · Ralph Campbell

void
nouveau_svmm_fini(struct nouveau_svmm **psvmm)
{

drivers/gpu/drm/nouveau/nouveau_svm.c +0 -0
 	struct nouveau_svmm *svmm = *psvmm;

drivers/gpu/drm/nouveau/nouveau_svm.c nouveau_svmm_invalidate(struct nouveau_svmm *svmm, u64 start, u64 limit) · line 273

Quoted 10 lines
drivers/gpu/drm/nouveau/nouveau_svm.c +8 -0
+	struct mmu_interval_notifier *mni;
+
 	if (svmm) {
 		mutex_lock(&svmm->mutex);
+		while (true) {
+			mni = mmu_interval_notifier_find(svmm->mm,
+					&nouveau_svm_mni_ops, 0UL, ~0UL);
+			if (!mni)
+				break;
+			mmu_interval_notifier_put(mni);

Oh, now I really don't like the name 'put'. It looks like mni is refcounted here, and it isn't. put should be called 'remove_deferred'

And then you also need a way to barrier this scheme on driver unload.

drivers/gpu/drm/nouveau/nouveau_svm.c nouveau_svmm_invalidate(struct nouveau_svmm *svmm, u64 start, u64 limit) · line 302

Quoted 4 lines
drivers/gpu/drm/nouveau/nouveau_svm.c +1 -1
+		}
 		svmm->vmm = NULL;
 		mutex_unlock(&svmm->mutex);
-		mmu_notifier_put(&svmm->notifier);

While here it was actually a refcount.

drivers/gpu/drm/nouveau/nouveau_svm.c nouveau_svm_fault_cache(struct nouveau_svm *svm, · line 493

Quoted 8 lines
drivers/gpu/drm/nouveau/nouveau_svm.c +8 -0
+static void nouveau_svmm_do_unmap(struct mmu_interval_notifier *mni,
+				 const struct mmu_notifier_range *range)
+{
+	struct svmm_interval *smi =
+		container_of(mni, struct svmm_interval, notifier);
+	struct nouveau_svmm *svmm = smi->svmm;
+	unsigned long start = mmu_interval_notifier_start(mni);
+	unsigned long last = mmu_interval_notifier_last(mni);

This whole algorithm only works if it is protected by the read side of the interval tree lock. Deserves at least a comment if not an assertion too.

Quoted 23 lines

static int nouveau_range_fault(struct nouveau_svmm *svmm,

drivers/gpu/drm/nouveau/nouveau_svm.c +6 -5
 			       struct nouveau_drm *drm, void *data, u32 size,
-			       u64 *pfns, struct svm_notifier *notifier)
+			       u64 *pfns, u64 start, u64 end)
 {
 	unsigned long timeout =
 		jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
 	/* Have HMM fault pages within the fault window to the GPU. */
 	struct hmm_range range = {
-		.notifier = &notifier->notifier,
-		.start = notifier->notifier.interval_tree.start,
-		.end = notifier->notifier.interval_tree.last + 1,
+		.start = start,
+		.end = end,
 		.pfns = pfns,
 		.flags = nouveau_svm_pfn_flags,
 		.values = nouveau_svm_pfn_values,
+		.default_flags = 0,
+		.pfn_flags_mask = ~0UL,
 		.pfn_shift = NVIF_VMM_PFNMAP_V0_ADDR_SHIFT,
 	};
-	struct mm_struct *mm = notifier->notifier.mm;
+	struct mm_struct *mm = svmm->mm;

drivers/gpu/drm/nouveau/nouveau_svm.c nouveau_svm_fault_cache(struct nouveau_svm *svm, · line 661

Quoted 10 lines
drivers/gpu/drm/nouveau/nouveau_svm.c +0 -3
 	long ret;
 
 	while (true) {
 		if (time_after(jiffies, timeout))
 			return -EBUSY;
 
-		range.notifier_seq = mmu_interval_read_begin(range.notifier);
-		range.default_flags = 0;
-		range.pfn_flags_mask = -1UL;
 		down_read(&mm->mmap_sem);

mmap sem doesn't have to be held for the interval search, and again we have lifetime issues with the membership here.

drivers/gpu/drm/nouveau/nouveau_svm.c nouveau_svm_fault_cache(struct nouveau_svm *svm, · line 670

Quoted 9 lines
drivers/gpu/drm/nouveau/nouveau_svm.c +6 -0
+		ret = nouveau_svmm_interval_find(svmm, &range);
+		if (ret) {
+			up_read(&mm->mmap_sem);
+			return ret;
+		}
+		range.notifier_seq = mmu_interval_read_begin(range.notifier);
 		ret = hmm_range_fault(&range, 0);
 		up_read(&mm->mmap_sem);
 		if (ret <= 0) {

I'm still not sure this is a better approach than what ODP does. It looks very expensive on the fault path..

Jason

author
#11 lore raw

Re: [PATCH v6 5/6] nouveau: use new mmu interval notifiers

Quoted 10 lines · Jason Gunthorpe
Quoted 9 lines · Ralph Campbell
  void
  nouveau_svmm_fini(struct nouveau_svmm **psvmm)
  {
  	struct nouveau_svmm *svmm = *psvmm;
drivers/gpu/drm/nouveau/nouveau_svm.c +3 -0
+	struct mmu_interval_notifier *mni;
+
  	if (svmm) {
  		mutex_lock(&svmm->mutex);
+		while (true) {
Quoted 8 lines

drivers/gpu/drm/nouveau/nouveau_svm.c nouveau_svmm_invalidate(struct nouveau_svmm *svmm, u64 start, u64 limit) · line 273

Quoted 5 lines
drivers/gpu/drm/nouveau/nouveau_svm.c +5 -0
+			mni = mmu_interval_notifier_find(svmm->mm,
+					&nouveau_svm_mni_ops, 0UL, ~0UL);
+			if (!mni)
+				break;
+			mmu_interval_notifier_put(mni);

Oh, now I really don't like the name 'put'. It looks like mni is refcounted here, and it isn't. put should be called 'remove_deferred'

OK.

Quoted 1 line

And then you also need a way to barrier this scheme on driver unload.

Good point. I can add something like
void mmu_interval_notifier_synchronize(struct mm_struct *mm) that waits for deferred operations to complete similar to mmu_interval_read_begin().

Quoted 7 lines

drivers/gpu/drm/nouveau/nouveau_svm.c nouveau_svmm_invalidate(struct nouveau_svmm *svmm, u64 start, u64 limit) · line 302

Quoted 4 lines
drivers/gpu/drm/nouveau/nouveau_svm.c +1 -1
+		}
  		svmm->vmm = NULL;
  		mutex_unlock(&svmm->mutex);
-		mmu_notifier_put(&svmm->notifier);

While here it was actually a refcount.

Quoted 12 lines

drivers/gpu/drm/nouveau/nouveau_svm.c nouveau_svm_fault_cache(struct nouveau_svm *svm, · line 493

Quoted 8 lines
drivers/gpu/drm/nouveau/nouveau_svm.c +8 -0
+static void nouveau_svmm_do_unmap(struct mmu_interval_notifier *mni,
+				 const struct mmu_notifier_range *range)
+{
+	struct svmm_interval *smi =
+		container_of(mni, struct svmm_interval, notifier);
+	struct nouveau_svmm *svmm = smi->svmm;
+	unsigned long start = mmu_interval_notifier_start(mni);
+	unsigned long last = mmu_interval_notifier_last(mni);

This whole algorithm only works if it is protected by the read side of the interval tree lock. Deserves at least a comment if not an assertion too.

This is called from the invalidate() callback and while holding the driver page table lock so the struct mmu_interval_notifier and the interval tree can't change.
I will add comments for v7.

Quoted 29 lines
Quoted 29 lines

static int nouveau_range_fault(struct nouveau_svmm *svmm,
struct nouveau_drm *drm, void *data, u32 size,

drivers/gpu/drm/nouveau/nouveau_svm.c +6 -5
-			       u64 *pfns, struct svm_notifier *notifier)
+			       u64 *pfns, u64 start, u64 end)
  {
  	unsigned long timeout =
  		jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
  	/* Have HMM fault pages within the fault window to the GPU. */
  	struct hmm_range range = {
-		.notifier = &notifier->notifier,
-		.start = notifier->notifier.interval_tree.start,
-		.end = notifier->notifier.interval_tree.last + 1,
+		.start = start,
+		.end = end,
  		.pfns = pfns,
  		.flags = nouveau_svm_pfn_flags,
  		.values = nouveau_svm_pfn_values,
+		.default_flags = 0,
+		.pfn_flags_mask = ~0UL,
  		.pfn_shift = NVIF_VMM_PFNMAP_V0_ADDR_SHIFT,
  	};
-	struct mm_struct *mm = notifier->notifier.mm;
+	struct mm_struct *mm = svmm->mm;
  	long ret;
 
  	while (true) {
  		if (time_after(jiffies, timeout))
  			return -EBUSY;
Quoted 7 lines

drivers/gpu/drm/nouveau/nouveau_svm.c nouveau_svm_fault_cache(struct nouveau_svm *svm, · line 661

Quoted 4 lines
drivers/gpu/drm/nouveau/nouveau_svm.c +0 -3
-		range.notifier_seq = mmu_interval_read_begin(range.notifier);
-		range.default_flags = 0;
-		range.pfn_flags_mask = -1UL;
  		down_read(&mm->mmap_sem);

mmap sem doesn't have to be held for the interval search, and again we have lifetime issues with the membership here.

I agree mmap_sem isn't needed for the interval search, it is needed if the search doesn't find a registered interval and one needs to be created to cover the underlying VMA. If an arbitrary size interval was created instead, then mmap_sem wouldn't be needed.
I don't understand the lifetime/membership issue. The driver is the only thing that allocates, inserts, or removes struct mmu_interval_notifier and thus completely controls the lifetime.

Quoted 13 lines

drivers/gpu/drm/nouveau/nouveau_svm.c nouveau_svm_fault_cache(struct nouveau_svm *svm, · line 670

Quoted 9 lines
drivers/gpu/drm/nouveau/nouveau_svm.c +6 -0
+		ret = nouveau_svmm_interval_find(svmm, &range);
+		if (ret) {
+			up_read(&mm->mmap_sem);
+			return ret;
+		}
+		range.notifier_seq = mmu_interval_read_begin(range.notifier);
  		ret = hmm_range_fault(&range, 0);
  		up_read(&mm->mmap_sem);
  		if (ret <= 0) {

I'm still not sure this is a better approach than what ODP does. It looks very expensive on the fault path..

Quoted 2 lines

Jason

ODP doesn't have this problem because users have to call ib_reg_mr() before any I/O can happen to the process address space. That is when mmu_interval_notifier_insert() / mmu_interval_notifier_remove() can be called and the driver doesn't have to worry about the interval changing sizes or being removed while I/O is happening. For GPU like devices, I'm trying to allow hardware access to any user level address without pre-registering it. That means inserting mmu interval notifiers for the ranges the GPU page faults on and updating the intervals as munmap() calls remove parts of the address space. I don't want to register an interval per page so the logical range is the underlying VMA.

It isn't that expensive, there is an extra driver lock/unlock as part of the lookup and possibly a find_vma() and kmalloc(GFP_ATOMIC) for new intervals. Also, the deferred interval updates for munmap(). Compared to the cost of updating PTEs in the device and GPU fault handling, this is minimal overhead.

#12 lore raw

Re: [PATCH v6 5/6] nouveau: use new mmu interval notifiers

Quoted 3 lines · Ralph Campbell

I don't understand the lifetime/membership issue. The driver is the only thing that allocates, inserts, or removes struct mmu_interval_notifier and thus completely controls the lifetime.

If the returned value is on the defered list it could be freed at any moment. The existing locks do not prevent it.

Quoted 13 lines
Quoted 13 lines

drivers/gpu/drm/nouveau/nouveau_svm.c nouveau_svm_fault_cache(struct nouveau_svm *svm, · line 670

Quoted 9 lines
drivers/gpu/drm/nouveau/nouveau_svm.c +6 -0
+		ret = nouveau_svmm_interval_find(svmm, &range);
+		if (ret) {
+			up_read(&mm->mmap_sem);
+			return ret;
+		}
+		range.notifier_seq = mmu_interval_read_begin(range.notifier);
  		ret = hmm_range_fault(&range, 0);
  		up_read(&mm->mmap_sem);
  		if (ret <= 0) {

I'm still not sure this is a better approach than what ODP does. It looks very expensive on the fault path..

Quoted 5 lines
Quoted 2 lines

Jason

ODP doesn't have this problem because users have to call ib_reg_mr() before any I/O can happen to the process address space.

ODP supports a single 'full VA' call at process startup, just like these cases.

Quoted 4 lines

That is when mmu_interval_notifier_insert() / mmu_interval_notifier_remove() can be called and the driver doesn't have to worry about the interval changing sizes or being removed while I/O is happening.

No, for the 'ODP full process VA' (aka implicit ODP) mode it dynamically maintains a list of intervals. ODP chooses the align the dynamic intervals to it's HW page table levels, and not to SW VMAs. This is much simpler to manage and faster to fault, at the cost of capturing more VA for invalidations which have to be probed against the HW shadow PTEs.

Quoted 5 lines

It isn't that expensive, there is an extra driver lock/unlock as part of the lookup and possibly a find_vma() and kmalloc(GFP_ATOMIC) for new intervals. Also, the deferred interval updates for munmap(). Compared to the cost of updating PTEs in the device and GPU fault handling, this is minimal overhead.

Well, compared to ODP which does a single xa lookup with no lock to find its interval, this looks very expensive and not parallel.

I think if there is merit in having ranges cover the vmas and track changes then there is probably merit in having the core code provide much of that logic, not the driver.

But it would be interesting to see some kind of analysis on the two methods to decide if the complexity is worthwhile.

Jason

author
#13 lore raw

Re: [PATCH v6 5/6] nouveau: use new mmu interval notifiers

Quoted 52 lines · Jason Gunthorpe
Quoted 3 lines · Ralph Campbell

I don't understand the lifetime/membership issue. The driver is the only thing that allocates, inserts, or removes struct mmu_interval_notifier and thus completely controls the lifetime.

If the returned value is on the defered list it could be freed at any moment. The existing locks do not prevent it.

Quoted 13 lines
Quoted 13 lines

drivers/gpu/drm/nouveau/nouveau_svm.c nouveau_svm_fault_cache(struct nouveau_svm *svm, · line 670

Quoted 9 lines
drivers/gpu/drm/nouveau/nouveau_svm.c +6 -0
+		ret = nouveau_svmm_interval_find(svmm, &range);
+		if (ret) {
+			up_read(&mm->mmap_sem);
+			return ret;
+		}
+		range.notifier_seq = mmu_interval_read_begin(range.notifier);
   		ret = hmm_range_fault(&range, 0);
   		up_read(&mm->mmap_sem);
   		if (ret <= 0) {

I'm still not sure this is a better approach than what ODP does. It looks very expensive on the fault path..

Quoted 5 lines
Quoted 2 lines

Jason

ODP doesn't have this problem because users have to call ib_reg_mr() before any I/O can happen to the process address space.

ODP supports a single 'full VA' call at process startup, just like these cases.

Quoted 4 lines

That is when mmu_interval_notifier_insert() / mmu_interval_notifier_remove() can be called and the driver doesn't have to worry about the interval changing sizes or being removed while I/O is happening.

No, for the 'ODP full process VA' (aka implicit ODP) mode it dynamically maintains a list of intervals. ODP chooses the align the dynamic intervals to it's HW page table levels, and not to SW VMAs. This is much simpler to manage and faster to fault, at the cost of capturing more VA for invalidations which have to be probed against the HW shadow PTEs.

Quoted 5 lines

It isn't that expensive, there is an extra driver lock/unlock as part of the lookup and possibly a find_vma() and kmalloc(GFP_ATOMIC) for new intervals. Also, the deferred interval updates for munmap(). Compared to the cost of updating PTEs in the device and GPU fault handling, this is minimal overhead.

Well, compared to ODP which does a single xa lookup with no lock to find its interval, this looks very expensive and not parallel.

Quoted 9 lines

I think if there is merit in having ranges cover the vmas and track changes then there is probably merit in having the core code provide much of that logic, not the driver.

But it would be interesting to see some kind of analysis on the two methods to decide if the complexity is worthwhile.

Jason

Can you point me to the latest ODP code? Seems like my understanding is quite off.

#14 lore raw

Re: [PATCH v6 5/6] nouveau: use new mmu interval notifiers

Quoted 2 lines · Ralph Campbell

Can you point me to the latest ODP code? Seems like my understanding is quite off.

https://elixir.bootlin.com/linux/v5.5-rc6/source/drivers/infiniband/hw/mlx5/odp.c

Look for the word 'implicit'

mlx5_ib_invalidate_range() releases the interval_notifier when there are no populated shadow PTEs in its leaf

pagefault_implicit_mr() creates an interval_notifier that covers the level in the page table that needs population. Notice it just uses an unlocked xa_load to find the page table level.

The locking is pretty tricky as it relies on RCU, but the fault flow is fairly lightweight.

Jason

author
#15 lore raw

Re: [PATCH v6 5/6] nouveau: use new mmu interval notifiers

Quoted 11 lines · Jason Gunthorpe
Quoted 2 lines · Ralph Campbell

Can you point me to the latest ODP code? Seems like my understanding is quite off.

https://elixir.bootlin.com/linux/v5.5-rc6/source/drivers/infiniband/hw/mlx5/odp.c

Look for the word 'implicit'

mlx5_ib_invalidate_range() releases the interval_notifier when there are no populated shadow PTEs in its leaf

Quoted 9 lines

pagefault_implicit_mr() creates an interval_notifier that covers the level in the page table that needs population. Notice it just uses an unlocked xa_load to find the page table level.

The locking is pretty tricky as it relies on RCU, but the fault flow is fairly lightweight.

Jason

Thanks for the information, Jason.

I'm still interested in finding a way to support range based hints to device drivers. madvise() looks like it only sets a bit in vma->vm_flags or acts on the
advice immediately. mbind() and set_mempolicy() only work with CPUs and memory with NUMA a node number. What I'm looking for is a way for the device to know whether to migrate pages to device private memory on a fault, whether to duplicate read-only pages in device private memory, or remote map/access a page instead of migrating it. For example, there is a working draft extension to OpenCL,
https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/USM/cl_intel_unified_shared_memory.asciidoc
that could provide a way to specify this sort of advice.
C++ is also looking at extentions for specifying affinity attributes.
In any case, these are probably a long ways off before being finalized and implemented.

I also have some changes to support THP migration to device private memory but that will require updating nouveau to use 2MB TLB mappings.

In the mean time, I can update the HMM self tests to do something like ODP without changing mm/mmu_notifier.c but I don't think I can easily change nouveau to that model.

author
#16 lore raw

PATCH v6 6/6 mm/hmm/test: add self tests for HMM

In reply to Ralph Campbell

Add some basic stand alone self tests for HMM.

Signed-off-by Ralph Campbell <[email protected]>
Signed-off-by Jérôme Glisse <[email protected]>
---
 MAINTAINERS                            |    3 +
 lib/Kconfig.debug                      |   11 +
 lib/Makefile                           |    1 +
 lib/test_hmm.c                         | 1368 ++++++++++++++++++++++++
 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 | 1354 +++++++++++++++++++++++
 tools/testing/selftests/vm/run_vmtests |   16 +
 tools/testing/selftests/vm/test_hmm.sh |   97 ++
 10 files changed, 2856 insertions(+)
 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
MAINTAINERS +3 -0
index 4017e6b760be..08508a20e2cf 100644
@@ -7514,7 +7514,10 @@ L:	[email protected]
 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 <[email protected]>
lib/Kconfig.debug +11 -0
index 5ffe144c9794..d38d1e281dca 100644
@@ -2162,6 +2162,17 @@ config TEST_MEMINIT
 
 	  If unsure, say N.
 
+config TEST_HMM
+	tristate "Test HMM (Heterogeneous Memory Management)"
+	depends on HMM_MIRROR
+	depends on DEVICE_PRIVATE
+	help
+	  This is a pseudo device driver solely for testing HMM.
+	  Say M here if you want to build the HMM test module.
+	  Doing so will allow you to run tools/testing/selftest/vm/hmm-tests.
+
+	  If unsure, say N.
+
 endif # RUNTIME_TESTING_MENU
 
 config MEMTEST
lib/Makefile +1 -0
index 93217d44237f..348ce83cb21f 100644
@@ -88,6 +88,7 @@ obj-$(CONFIG_TEST_OBJAGG) += test_objagg.o
 obj-$(CONFIG_TEST_STACKINIT) += test_stackinit.o
 obj-$(CONFIG_TEST_BLACKHOLE_DEV) += test_blackhole_dev.o
 obj-$(CONFIG_TEST_MEMINIT) += test_meminit.o
+obj-$(CONFIG_TEST_HMM) += test_hmm.o
 
 obj-$(CONFIG_TEST_LIVEPATCH) += livepatch/
lib/test_hmm.c new file +1368 -0

1,368 lines not on this page · raw message

tools/testing/selftests/vm/.gitignore +1 -0
index 31b3c98b6d34..3054565b3f07 100644
@@ -14,3 +14,4 @@ virtual_address_range
 gup_benchmark
 va_128TBswitch
 map_fixed_noreplace
+hmm-tests
tools/testing/selftests/vm/Makefile +3 -0
index 7f9a8a8c31da..3fadab99d991 100644
@@ -7,6 +7,7 @@ CFLAGS = -Wall -I ../../../../usr/include $(EXTRA_CFLAGS)
 LDLIBS = -lrt
 TEST_GEN_FILES = compaction_test
 TEST_GEN_FILES += gup_benchmark
+TEST_GEN_FILES += hmm-tests
 TEST_GEN_FILES += hugepage-mmap
 TEST_GEN_FILES += hugepage-shm
 TEST_GEN_FILES += map_hugetlb

@@ -31,6 +32,8 @@ TEST_FILES := test_vmalloc.sh
 KSFT_KHDR_INSTALL := 1
 include ../lib.mk
 
+$(OUTPUT)/hmm-tests: LDLIBS += -lhugetlbfs -lpthread
+
 $(OUTPUT)/userfaultfd: LDLIBS += -lpthread
 
 $(OUTPUT)/mlock-random-test: LDLIBS += -lcap
tools/testing/selftests/vm/config +2 -0
index 93b90a9b1eeb..f6d0adad739f 100644
@@ -1,3 +1,5 @@
 CONFIG_SYSVIPC=y
 CONFIG_USERFAULTFD=y
 CONFIG_TEST_VMALLOC=m
+CONFIG_HMM_MIRROR=y
+CONFIG_DEVICE_PRIVATE=y
tools/testing/selftests/vm/hmm-tests.c new file +1354 -0

1,354 lines not on this page · raw message

tools/testing/selftests/vm/run_vmtests +16 -0
index a692ea828317..ea3728570585 100755
@@ -237,4 +237,20 @@ else
 	exitcode=1
 fi
 
+echo "------------------------------------"
+echo "running HMM smoke test"
+echo "------------------------------------"
+./test_hmm.sh smoke
+ret_val=$?
+
+if [ $ret_val -eq 0 ]; then
+	echo "[PASS]"
+elif [ $ret_val -eq $ksft_skip ]; then
+	echo "[SKIP]"
+	exitcode=$ksft_skip
+else
+	echo "[FAIL]"
+	exitcode=1
+fi
+
 exit $exitcode
tools/testing/selftests/vm/test_hmm.sh new file +97 -0

97 lines not on this page · raw message

Signature · 1 line
2.20.1