Skip to contentSkip to conversation

IB/core: Implement support for MMU notifiers regarding on demand paging regions

PATCH Jerome Glisse 3 participants 10 messages Sep 4, 2014 – Oct 14, 2014 10 messages not in the archive

Subsystems (15)
DRM drivers Memory management KVM Intel i915 +11 more subsystems
lore
Share & tools
lore

Jerome Glisse

author
#0 lore raw
Quoted 17 lines

* Add an interval tree implementation for ODP umems. Create an interval tree for each ucontext (including a count of the number of ODP MRs in this context, mutex, etc.), and register ODP umems in the interval tree.
* Add MMU notifiers handling functions, using the interval tree to notify only the relevant umems and underlying MRs.
* Register to receive MMU notifier events from the MM subsystem upon ODP MR registration (and unregister accordingly).
* Add a completion object to synchronize the destruction of ODP umems.
* Add mechanism to abort page faults when there's a concurrent invalidation.

The way we synchronize between concurrent invalidations and page faults is by keeping a counter of currently running invalidations, and a sequence number that is incremented whenever an invalidation is caught. The page fault code checks the counter and also verifies that the sequence number hasn't progressed before it updates the umem's page tables. This is similar to what the kvm module does.

Quoted 7 lines

There's currently a rare race in the code when registering a umem in the middle of an ongoing notifier. The proper fix is to either serialize the insertion to our umem tree with mm_lock_all or use a ucontext wide running notifiers count for retries decision. Either is ugly and can lead to some sort of starvation. The current workaround is ugly as well - now the user can end up with mapped addresses that are not in the user's address space (although it is highly unlikely).

I have been trying to wrap my head around this comment. I am totaly unfamiliar with RDMA code, but from quick look at it when registering umem you take the mmap_sem in read mode so any munmap from userspace would be serialize. Really the worst that can happen is that a umem pointing to a mmaped file that is concurently truncated but even then the address is still valid, but it should result in a SIGBUS which here is obviously harder to report (again dunno how RDMA works).

So am i missing something ?

Quoted 782 lines
Signed-off-by Sagi Grimberg <sagig@xxxxxxxxxxxx>
Signed-off-by Shachar Raindel <raindel@xxxxxxxxxxxx>
Signed-off-by Haggai Eran <haggaie@xxxxxxxxxxxx>
Signed-off-by Yuval Dagan <yuvalda@xxxxxxxxxxxx>
---
 drivers/infiniband/Kconfig            |   1 +
 drivers/infiniband/core/Makefile      |   2 +-
 drivers/infiniband/core/umem.c        |   2 +-
 drivers/infiniband/core/umem_odp.c    | 337 +++++++++++++++++++++++++++++++++-
 drivers/infiniband/core/umem_rbtree.c |  94 ++++++++++
 drivers/infiniband/core/uverbs_cmd.c  |  16 ++
 include/rdma/ib_umem_odp.h            |  56 ++++++
 include/rdma/ib_verbs.h               |  16 ++
 8 files changed, 512 insertions(+), 12 deletions(-)
 create mode 100644 drivers/infiniband/core/umem_rbtree.c
drivers/infiniband/Kconfig +1 -0
index 089a2c2..b899531 100644
@@ -41,6 +41,7 @@ config INFINIBAND_USER_MEM
 config INFINIBAND_ON_DEMAND_PAGING
 	bool "InfiniBand on-demand paging support"
 	depends on INFINIBAND_USER_MEM
+	select MMU_NOTIFIER
 	default y
 	---help---
 	  On demand paging support for the InfiniBand subsystem.
drivers/infiniband/core/Makefile +1 -1
index c58f791..acf7367 100644
@@ -11,7 +11,7 @@ obj-$(CONFIG_INFINIBAND_USER_ACCESS) +=	ib_uverbs.o ib_ucm.o \
 ib_core-y :=			packer.o ud_header.o verbs.o sysfs.o \
 				device.o fmr_pool.o cache.o netlink.o
 ib_core-$(CONFIG_INFINIBAND_USER_MEM) += umem.o
-ib_core-$(CONFIG_INFINIBAND_ON_DEMAND_PAGING) += umem_odp.o
+ib_core-$(CONFIG_INFINIBAND_ON_DEMAND_PAGING) += umem_odp.o umem_rbtree.o
 
 ib_mad-y :=			mad.o smi.o agent.o mad_rmpp.o
drivers/infiniband/core/umem.c +1 -1
index e9798e0..014977f 100644
@@ -72,7 +72,7 @@ static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int d
  * ib_umem_get - Pin and DMA map userspace memory.
  *
  * If access flags indicate ODP memory, avoid pinning. Instead, stores
- * the mm for future page fault handling.
+ * the mm for future page fault handling in conjuction with MMU notifiers.
  *
  * @context: userspace context to pin memory for
  * @addr: userspace virtual address to start at
drivers/infiniband/core/umem_odp.c +327 -10
index 0c90ce50..c048269 100644
@@ -41,26 +41,204 @@
 #include <rdma/ib_umem.h>
 #include <rdma/ib_umem_odp.h>
 
+void ib_umem_notifier_start_account(struct ib_umem *item)
+{
+	int notifiers_count;
+	mutex_lock(&item->odp_data->umem_mutex);
+	/*
+	 * Avoid performing another locked operation, as we are
+	 * already protected by the wrapping mutex.
+	 */
+	notifiers_count = atomic_read(&item->odp_data->notifiers_count) + 1;
+	if (notifiers_count == 1)
+		reinit_completion(&item->odp_data->notifier_completion);
+	atomic_set(&item->odp_data->notifiers_count,
+		   notifiers_count);
+	mutex_unlock(&item->odp_data->umem_mutex);
+}
+EXPORT_SYMBOL(ib_umem_notifier_start_account);
+
+void ib_umem_notifier_end_account(struct ib_umem *item)
+{
+	int notifiers_count, notifiers_seq;
+	mutex_lock(&item->odp_data->umem_mutex);
+	/*
+	 * This sequence increase will notify the QP page fault that
+	 * the page that is going to be mapped in the spte could have
+	 * been freed.
+	 */
+	notifiers_seq = atomic_read(&item->odp_data->notifiers_seq) + 1;
+	atomic_set(&item->odp_data->notifiers_seq,
+		   notifiers_seq);
+	/*
+	 * The above sequence increase must be visible before the
+	 * below count decrease, which is ensured by the smp_wmb below
+	 * in conjunction with the smp_rmb in mmu_notifier_retry().
+	 */
+	smp_wmb();
+
+	notifiers_count = atomic_read(&item->odp_data->notifiers_count);
+	/*
+	 * This is a workaround for the unlikely case where we register a umem
+	 * in the middle of an ongoing notifier.
+	 */
+	if (notifiers_count > 0)
+		notifiers_count -= 1;
+	else
+		pr_warn("Got notifier end call without a previous start call");
+	atomic_set(&item->odp_data->notifiers_count,
+		   notifiers_count);
+	if (notifiers_count == 0)
+		complete_all(&item->odp_data->notifier_completion);
+	mutex_unlock(&item->odp_data->umem_mutex);
+}
+
+
+static int ib_umem_notifier_release_trampoline(struct ib_umem *item, u64 start,
+					       u64 end, void *cookie) {
+	/*
+	 * Increase the number of notifiers running, to
+	 * prevent any further fault handling on this MR.
+	 */
+	ib_umem_notifier_start_account(item);
+	item->odp_data->dying = 1;
+	/* Make sure that the fact the umem is dying is out before we release
+	 * all pending page faults. */
+	smp_wmb();
+	complete_all(&item->odp_data->notifier_completion);
+	item->context->invalidate_range(item, ib_umem_start(item),
+					ib_umem_end(item));
+	return 0;
+}
+
+static void ib_umem_notifier_release(struct mmu_notifier *mn,
+				     struct mm_struct *mm)
+{
+	struct ib_ucontext *context = container_of(mn, struct ib_ucontext, mn);
+
+	if (!context->invalidate_range)
+		return;
+
+	down_read(&context->umem_mutex);
+
+	rbt_ib_umem_for_each_in_range(&context->umem_tree, 0,
+				      ULLONG_MAX,
+				      ib_umem_notifier_release_trampoline,
+				      NULL);
+
+	up_read(&context->umem_mutex);
+}
+
+static int invalidate_page_trampoline(struct ib_umem *item, u64 start,
+				      u64 end, void *cookie)
+{
+	ib_umem_notifier_start_account(item);
+	item->context->invalidate_range(item, start, start + PAGE_SIZE);
+	ib_umem_notifier_end_account(item);
+	return 0;
+}
+
+static void ib_umem_notifier_invalidate_page(struct mmu_notifier *mn,
+					     struct mm_struct *mm,
+					     unsigned long address)
+{
+	struct ib_ucontext *context = container_of(mn, struct ib_ucontext, mn);
+
+	if (!context->invalidate_range)
+		return;
+
+	down_read(&context->umem_mutex);
+	rbt_ib_umem_for_each_in_range(&context->umem_tree, address,
+				      address + PAGE_SIZE,
+				      invalidate_page_trampoline, NULL);
+	up_read(&context->umem_mutex);
+}
+
+static int invalidate_range_start_trampoline(struct ib_umem *item, u64 start,
+					     u64 end, void *cookie)
+{
+	ib_umem_notifier_start_account(item);
+	item->context->invalidate_range(item, start, end);
+	return 0;
+}
+
+static void ib_umem_notifier_invalidate_range_start(struct mmu_notifier *mn,
+						    struct mm_struct *mm,
+						    unsigned long start,
+						    unsigned long end)
+{
+	struct ib_ucontext *context = container_of(mn, struct ib_ucontext, mn);
+
+	if (!context->invalidate_range)
+		return;
+
+	down_read(&context->umem_mutex);
+	rbt_ib_umem_for_each_in_range(&context->umem_tree, start,
+				      end,
+				      invalidate_range_start_trampoline, NULL);
+	up_read(&context->umem_mutex);
+}
+
+static int invalidate_range_end_trampoline(struct ib_umem *item, u64 start,
+					   u64 end, void *cookie)
+{
+	ib_umem_notifier_end_account(item);
+	return 0;
+}
+
+static void ib_umem_notifier_invalidate_range_end(struct mmu_notifier *mn,
+						  struct mm_struct *mm,
+						  unsigned long start,
+						  unsigned long end)
+{
+	struct ib_ucontext *context = container_of(mn, struct ib_ucontext, mn);
+
+	if (!context->invalidate_range)
+		return;
+
+	down_read(&context->umem_mutex);
+	rbt_ib_umem_for_each_in_range(&context->umem_tree, start,
+				      end,
+				      invalidate_range_end_trampoline, NULL);
+	up_read(&context->umem_mutex);
+}
+
+static struct mmu_notifier_ops ib_umem_notifiers = {
+	.release                    = ib_umem_notifier_release,
+	.invalidate_page            = ib_umem_notifier_invalidate_page,
+	.invalidate_range_start     = ib_umem_notifier_invalidate_range_start,
+	.invalidate_range_end       = ib_umem_notifier_invalidate_range_end,
+};
+
 int ib_umem_odp_get(struct ib_ucontext *context, struct ib_umem *umem)
 {
 	int ret_val;
 	struct pid *our_pid;
+	struct mm_struct *mm = get_task_mm(current);
+	BUG_ON(!mm);
 
 	/* Prevent creating ODP MRs in child processes */
 	rcu_read_lock();
 	our_pid = get_task_pid(current->group_leader, PIDTYPE_PID);
 	rcu_read_unlock();
 	put_pid(our_pid);
-	if (context->tgid != our_pid)
-		return -EINVAL;
+	if (context->tgid != our_pid) {
+		ret_val = -EINVAL;
+		goto out_mm;
+	}
 
 	umem->hugetlb = 0;
 	umem->odp_data = kzalloc(sizeof(*umem->odp_data), GFP_KERNEL);
-	if (!umem->odp_data)
-		return -ENOMEM;
+	if (!umem->odp_data) {
+		ret_val = -ENOMEM;
+		goto out_mm;
+	}
+	umem->odp_data->umem = umem;
 
 	mutex_init(&umem->odp_data->umem_mutex);
 
+	init_completion(&umem->odp_data->notifier_completion);
+
 	umem->odp_data->page_list = vzalloc(ib_umem_num_pages(umem) *
 					    sizeof(*umem->odp_data->page_list));
 	if (!umem->odp_data->page_list) {

@@ -75,17 +253,66 @@ int ib_umem_odp_get(struct ib_ucontext *context, struct ib_umem *umem)
 		goto out_page_list;
 	}
 
+	/*
+	 * When using MMU notifiers, we will get a
+	 * notification before the "current" task (and MM) is
+	 * destroyed. We use the umem_mutex lock to synchronize.
+	 */
+	down_write(&context->umem_mutex);
+	context->odp_mrs_count++;
+	if (likely(ib_umem_start(umem) != ib_umem_end(umem)))
+		rbt_ib_umem_insert(&umem->odp_data->interval_tree,
+				   &context->umem_tree);
+	downgrade_write(&context->umem_mutex);
+
+	if (context->odp_mrs_count == 1) {
+		/*
+		 * Note that at this point, no MMU notifier is running
+		 * for this context!
+		 */
+		INIT_HLIST_NODE(&context->mn.hlist);
+		context->mn.ops = &ib_umem_notifiers;
+		/*
+		 * Lock-dep detects a false positive for mmap_sem vs.
+		 * umem_mutex, due to not grasping downgrade_write correctly.
+		 */
+		lockdep_off();
+		ret_val = mmu_notifier_register(&context->mn, mm);
+		lockdep_on();
+		if (ret_val) {
+			pr_err("Failed to register mmu_notifier %d\n", ret_val);
+			ret_val = -EBUSY;
+			goto out_mutex;
+		}
+	}
+
+	up_read(&context->umem_mutex);
+
+	/*
+	 * Note that doing an mmput can cause a notifier for the relevant mm.
+	 * If the notifier is called while we hold the umem_mutex, this will
+	 * cause a deadlock. Therefore, we release the reference only after we
+	 * released the mutex.
+	 */
+	mmput(mm);
 	return 0;
 
+out_mutex:
+	up_read(&context->umem_mutex);
+	vfree(umem->odp_data->dma_list);
 out_page_list:
 	vfree(umem->odp_data->page_list);
 out_odp_data:
 	kfree(umem->odp_data);
+out_mm:
+	mmput(mm);
 	return ret_val;
 }
 
 void ib_umem_odp_release(struct ib_umem *umem)
 {
+	struct ib_ucontext *context = umem->context;
+
 	/*
 	 * Ensure that no more pages are mapped in the umem.
 	 *

@@ -95,6 +322,49 @@ void ib_umem_odp_release(struct ib_umem *umem)
 	ib_umem_odp_unmap_dma_pages(umem, ib_umem_start(umem),
 				    ib_umem_end(umem));
 
+	down_write(&context->umem_mutex);
+	if (likely(ib_umem_start(umem) != ib_umem_end(umem)))
+		rbt_ib_umem_remove(&umem->odp_data->interval_tree,
+				   &context->umem_tree);
+	context->odp_mrs_count--;
+
+	/*
+	 * Downgrade the lock to a read lock. This ensures that the notifiers
+	 * (who lock the mutex for reading) will be able to finish, and we
+	 * will be able to enventually obtain the mmu notifiers SRCU. Note
+	 * that since we are doing it atomically, no other user could register
+	 * and unregister while we do the check.
+	 */
+	downgrade_write(&context->umem_mutex);
+	if (!context->odp_mrs_count) {
+		struct task_struct *owning_process = NULL;
+		struct mm_struct *owning_mm        = NULL;
+		owning_process = get_pid_task(context->tgid,
+					      PIDTYPE_PID);
+		if (owning_process == NULL)
+			/*
+			 * The process is already dead, notifier were removed
+			 * already.
+			 */
+			goto out;
+
+		owning_mm = get_task_mm(owning_process);
+		if (owning_mm == NULL)
+			/*
+			 * The process' mm is already dead, notifier were
+			 * removed already.
+			 */
+			goto out_put_task;
+		mmu_notifier_unregister(&context->mn, owning_mm);
+
+		mmput(owning_mm);
+
+out_put_task:
+		put_task_struct(owning_process);
+	}
+out:
+	up_read(&context->umem_mutex);
+
 	vfree(umem->odp_data->dma_list);
 	vfree(umem->odp_data->page_list);
 	kfree(umem);

@@ -111,7 +381,8 @@ void ib_umem_odp_release(struct ib_umem *umem)
  *               the sequence number is taken from
  *               umem->odp_data->notifiers_seq.
  *
- * The function returns -EFAULT if the DMA mapping operation fails.
+ * The function returns -EFAULT if the DMA mapping operation fails. It returns
+ * -EAGAIN if a concurrent invalidation prevents us from updating the page. It
  *
  * The page is released via put_page even if the operation failed. For
  * on-demand pinning, the page is released whenever it isn't stored in the

@@ -120,6 +391,7 @@ void ib_umem_odp_release(struct ib_umem *umem)
 static int ib_umem_odp_map_dma_single_page(
 		struct ib_umem *umem,
 		int page_index,
+		u64 base_virt_addr,
 		struct page *page,
 		u64 access_mask,
 		unsigned long current_seq)

@@ -127,8 +399,18 @@ static int ib_umem_odp_map_dma_single_page(
 	struct ib_device *dev = umem->context->device;
 	dma_addr_t dma_addr;
 	int stored_page = 0;
+	int remove_existing_mapping = 0;
 	int ret = 0;
 	mutex_lock(&umem->odp_data->umem_mutex);
+	/*
+	 * Note: we avoid writing if seq is different from the initial seq, to
+	 * handle case of a racing notifier. This check also allows us to bail
+	 * early if we have a notifier running in parallel with us.
+	 */
+	if (ib_umem_mmu_notifier_retry(umem, current_seq)) {
+		ret = -EAGAIN;
+		goto out;
+	}
 	if (!(umem->odp_data->dma_list[page_index])) {
 		dma_addr = ib_dma_map_page(dev,
 					   page,

@@ -146,14 +428,27 @@ static int ib_umem_odp_map_dma_single_page(
 	} else {
 		pr_err("error: got different pages in IB device and from get_user_pages. IB device page: %p, gup page: %p\n",
 		       umem->odp_data->page_list[page_index], page);
+		/* Better remove the mapping now, to prevent any further
+		 * damage. */
+		remove_existing_mapping = 1;
 	}
 
 out:
 	mutex_unlock(&umem->odp_data->umem_mutex);
 
-	if (!stored_page)
+	/* On Demand Paging - avoid pinning the page */
+	if (umem->context->invalidate_range || !stored_page)
 		put_page(page);
 
+	if (remove_existing_mapping && umem->context->invalidate_range) {
+		invalidate_page_trampoline(
+			umem,
+			base_virt_addr + (page_index * PAGE_SIZE),
+			base_virt_addr + ((page_index+1)*PAGE_SIZE),
+			NULL);
+		ret = -EAGAIN;
+	}
+
 	return ret;
 }
 

@@ -166,6 +461,8 @@ out:
  *
  * Returns the number of pages mapped in success, negative error code
  * for failure.
+ * An -EAGAIN error code is returned when a concurrent mmu notifier prevents
+ * the function from completing its task.
  *
  * @umem: the umem to map and pin
  * @user_virt: the address from which we need to map.

@@ -187,6 +484,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem *umem, u64 user_virt, u64 bcnt,
 	struct page       **local_page_list = NULL;
 	u64 off;
 	int j, k, ret = 0, start_idx, npages = 0;
+	u64 base_virt_addr;
 
 	if (access_mask == 0)
 		return -EINVAL;

@@ -201,6 +499,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem *umem, u64 user_virt, u64 bcnt,
 
 	off = user_virt & (~PAGE_MASK);
 	user_virt = user_virt & PAGE_MASK;
+	base_virt_addr = user_virt;
 	bcnt += off; /* Charge for the first page offset as well. */
 
 	start_idx = (user_virt - ib_umem_start(umem)) >> PAGE_SHIFT;

@@ -242,8 +541,8 @@ int ib_umem_odp_map_dma_pages(struct ib_umem *umem, u64 user_virt, u64 bcnt,
 		user_virt += npages << PAGE_SHIFT;
 		for (j = 0; j < npages; ++j) {
 			ret = ib_umem_odp_map_dma_single_page(
-				umem, k, local_page_list[j], access_mask,
-				current_seq);
+				umem, k, base_virt_addr, local_page_list[j],
+				access_mask, current_seq);
 			if (ret < 0)
 				break;
 			k++;

@@ -280,6 +579,11 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem *umem, u64 virt,
 	struct ib_device *dev = umem->context->device;
 	virt  = max_t(u64, virt,  ib_umem_start(umem));
 	bound = min_t(u64, bound, ib_umem_end(umem));
+	/* Note that during the run of this function, the
+	 * notifiers_count of the MR is > 0, preventing any racing
+	 * faults from completion. We might be racing with other
+	 * invalidations, so we must make sure we free each page only
+	 * once. */
 	for (addr = virt; addr < bound; addr += (u64)umem->page_size) {
 		idx = (addr - ib_umem_start(umem)) / PAGE_SIZE;
 		mutex_lock(&umem->odp_data->umem_mutex);

@@ -294,8 +598,21 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem *umem, u64 virt,
 			ib_dma_unmap_page(dev, dma_addr, PAGE_SIZE,
 					  DMA_BIDIRECTIONAL);
 			if (umem->writable)
-				set_page_dirty_lock(head_page);
-			put_page(page);
+				/*
+				 * set_page_dirty prefers being called with
+				 * the page lock. However, MMU notifiers are
+				 * called sometimes with and sometimes without
+				 * the lock. We rely on the umem_mutex instead
+				 * to prevent other mmu notifiers from
+				 * continuing and allowing the page mapping to
+				 * be removed.
+				 */
+				set_page_dirty(head_page);
+			/* on demand pinning support */
+			if (!umem->context->invalidate_range)
+				put_page(page);
+			umem->odp_data->page_list[idx] = NULL;
+			umem->odp_data->dma_list[idx] = 0;
 		}
 		mutex_unlock(&umem->odp_data->umem_mutex);
 	}
drivers/infiniband/core/umem_rbtree.c new file +94 -0
new file mode 100644
index 0000000..727d788
@@ -0,0 +1,94 @@
+/*
+ * Copyright (c) 2014 Mellanox Technologies. All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      - Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *
+ *      - Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/interval_tree_generic.h>
+#include <linux/sched.h>
+#include <linux/gfp.h>
+#include <rdma/ib_umem_odp.h>
+
+/*
+ * The ib_umem list keeps track of memory regions for which the HW
+ * device request to receive notification when the related memory
+ * mapping is changed.
+ *
+ * ib_umem_lock protects the list.
+ */
+
+static inline u64 node_start(struct umem_odp_node *n)
+{
+	struct ib_umem_odp *umem_odp =
+			container_of(n, struct ib_umem_odp, interval_tree);
+
+	return ib_umem_start(umem_odp->umem);
+}
+
+/* Note that the representation of the intervals in the interval tree
+ * considers the ending point as contained in the interval, while the
+ * function ib_umem_end returns the first address which is not contained
+ * in the umem.
+ */
+static inline u64 node_last(struct umem_odp_node *n)
+{
+	struct ib_umem_odp *umem_odp =
+			container_of(n, struct ib_umem_odp, interval_tree);
+
+	return ib_umem_end(umem_odp->umem) - 1;
+}
+
+INTERVAL_TREE_DEFINE(struct umem_odp_node, rb, u64, __subtree_last,
+		     node_start, node_last, , rbt_ib_umem)
+
+/* @last is not a part of the interval. See comment for function
+ * node_last.
+ */
+int rbt_ib_umem_for_each_in_range(struct rb_root *root,
+				  u64 start, u64 last,
+				  umem_call_back cb,
+				  void *cookie)
+{
+	int ret_val = 0;
+	struct umem_odp_node *node;
+	struct ib_umem_odp *umem;
+
+	if (unlikely(start == last))
+		return ret_val;
+
+	for (node = rbt_ib_umem_iter_first(root, start, last - 1); node;
+			node = rbt_ib_umem_iter_next(node, start, last - 1)) {
+		umem = container_of(node, struct ib_umem_odp, interval_tree);
+		ret_val = cb(umem->umem, start, last, cookie) || ret_val;
+	}
+
+	return ret_val;
+}
drivers/infiniband/core/uverbs_cmd.c +16 -0
index fe709ca..a81d0c7 100644
@@ -289,6 +289,9 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file,
 	struct ib_uverbs_get_context_resp resp;
 	struct ib_udata                   udata;
 	struct ib_device                 *ibdev = file->device->ib_dev;
+#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING
+	struct ib_device_attr		  dev_attr;
+#endif
 	struct ib_ucontext		 *ucontext;
 	struct file			 *filp;
 	int ret;

@@ -331,6 +334,19 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file,
 	rcu_read_unlock();
 	ucontext->closing = 0;
 
+#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING
+	ucontext->umem_tree = RB_ROOT;
+	init_rwsem(&ucontext->umem_mutex);
+	ucontext->odp_mrs_count = 0;
+
+	ret = ib_query_device(ibdev, &dev_attr);
+	if (ret)
+		goto err_free;
+	if (!(dev_attr.device_cap_flags & IB_DEVICE_ON_DEMAND_PAGING))
+		ucontext->invalidate_range = NULL;
+
+#endif
+
 	resp.num_comp_vectors = file->device->num_comp_vectors;
 
 	ret = get_unused_fd_flags(O_CLOEXEC);
include/rdma/ib_umem_odp.h +56 -0
index 375ce28..9b93206 100644
@@ -34,6 +34,12 @@
 #define IB_UMEM_ODP_H
 
 #include <rdma/ib_umem.h>
+#include <linux/interval_tree.h>
+
+struct umem_odp_node {
+	u64 __subtree_last;
+	struct rb_node rb;
+};
 
 struct ib_umem_odp {
 	/*

@@ -58,6 +64,14 @@ struct ib_umem_odp {
 
 	atomic_t		notifiers_seq;
 	atomic_t		notifiers_count;
+
+	struct ib_umem		*umem;
+
+	/* Tree tracking */
+	struct umem_odp_node	interval_tree;
+
+	struct completion	notifier_completion;
+	int			dying;
 };
 
 #ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING

@@ -85,6 +99,48 @@ int ib_umem_odp_map_dma_pages(struct ib_umem *umem, u64 start_offset, u64 bcnt,
 void ib_umem_odp_unmap_dma_pages(struct ib_umem *umem, u64 start_offset,
 				 u64 bound);
 
+void rbt_ib_umem_insert(struct umem_odp_node *node, struct rb_root *root);
+void rbt_ib_umem_remove(struct umem_odp_node *node, struct rb_root *root);
+typedef int (*umem_call_back)(struct ib_umem *item, u64 start, u64 end,
+			      void *cookie);
+/*
+ * Call the callback on each ib_umem in the range. Returns the logical or of
+ * the return values of the functions called.
+ */
+int rbt_ib_umem_for_each_in_range(struct rb_root *root, u64 start, u64 end,
+				  umem_call_back cb, void *cookie);
+
+struct umem_odp_node *rbt_ib_umem_iter_first(struct rb_root *root,
+					     u64 start, u64 last);
+struct umem_odp_node *rbt_ib_umem_iter_next(struct umem_odp_node *node,
+					    u64 start, u64 last);
+
+static inline int ib_umem_mmu_notifier_retry(struct ib_umem *item,
+					     unsigned long mmu_seq)
+{
+	/*
+	 * This code is strongly based on the KVM code from
+	 * mmu_notifier_retry. Should be called with
+	 * item->odp_data->umem_mutex locked.
+	 */
+	if (unlikely(atomic_read(&item->odp_data->notifiers_count)))
+		return 1;
+	/*
+	 * Ensure the read of mmu_notifier_count happens before the read
+	 * of mmu_notifier_seq.  This interacts with the smp_wmb() in
+	 * mmu_notifier_invalidate_range_end to make sure that the caller
+	 * either sees the old (non-zero) value of mmu_notifier_count or
+	 * the new (incremented) value of mmu_notifier_seq.
+	 */
+	smp_rmb();
+	if (atomic_read(&item->odp_data->notifiers_seq) != mmu_seq)
+		return 1;
+	return 0;
+}
+
+void ib_umem_notifier_start_account(struct ib_umem *item);
+void ib_umem_notifier_end_account(struct ib_umem *item);
+
 #else /* CONFIG_INFINIBAND_ON_DEMAND_PAGING */
 
 static inline int ib_umem_odp_get(struct ib_ucontext *context,
include/rdma/ib_verbs.h +16 -0
index 6d7f75e..54df9a8 100644
@@ -51,6 +51,7 @@
 #include <uapi/linux/if_ether.h>
 
 #include <linux/atomic.h>
+#include <linux/mmu_notifier.h>
 #include <asm/uaccess.h>
 
 extern struct workqueue_struct *ib_wq;

@@ -1142,6 +1143,8 @@ struct ib_fmr_attr {
 	u8	page_shift;
 };
 
+struct ib_umem;
+
 struct ib_ucontext {
 	struct ib_device       *device;
 	struct list_head	pd_list;

@@ -1157,6 +1160,19 @@ struct ib_ucontext {
 
 	/* For ODP support: */
 	struct pid             *tgid;
+#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING
+	struct rb_root      umem_tree;
+	/*
+	 * Protects .umem_rbroot and tree, as well as odp_mrs_count and
+	 * mmu notifiers registration.
+	 */
+	struct rw_semaphore	umem_mutex;
+	void (*invalidate_range)(struct ib_umem *umem,
+				 unsigned long start, unsigned long end);
+
+	struct mmu_notifier	mn;
+	int                     odp_mrs_count;
+#endif
 };
 
 struct ib_uobject {
Signature · 1 line
1.7.11.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw

Hi,

Quoted 62 lines

-----Original Message-----
From: Jerome Glisse [mailto:[email protected]]
Sent: Thursday, September 04, 2014 11:25 PM
To: Haggai Eran; [email protected]
Cc: Shachar Raindel; Sagi Grimberg
Subject: Re: [PATCH v1 for-next 06/16] IB/core: Implement support for MMU notifiers regarding on demand paging regions

Quoted 1 line

* Add an interval tree implementation for ODP umems. Create an

interval tree

Quoted 1 line

for each ucontext (including a count of the number of ODP MRs in

this

Quoted 2 lines

context, mutex, etc.), and register ODP umems in the interval tree.
* Add MMU notifiers handling functions, using the interval tree to

notify only

Quoted 2 lines

the relevant umems and underlying MRs.
* Register to receive MMU notifier events from the MM subsystem upon

ODP MR

Quoted 3 lines

registration (and unregister accordingly).
* Add a completion object to synchronize the destruction of ODP umems.
* Add mechanism to abort page faults when there's a concurrent

invalidation.

Quoted 2 lines

The way we synchronize between concurrent invalidations and page

faults is by

Quoted 1 line

keeping a counter of currently running invalidations, and a sequence

number

Quoted 1 line

that is incremented whenever an invalidation is caught. The page fault

code

Quoted 2 lines

checks the counter and also verifies that the sequence number hasn't progressed before it updates the umem's page tables. This is similar

to what

Quoted 3 lines

the kvm module does.

There's currently a rare race in the code when registering a umem in

the

Quoted 1 line

middle of an ongoing notifier. The proper fix is to either serialize

the

Quoted 1 line

insertion to our umem tree with mm_lock_all or use a ucontext wide

running

Quoted 1 line

notifiers count for retries decision. Either is ugly and can lead to

some sort

Quoted 1 line

of starvation. The current workaround is ugly as well - now the user

can end

Quoted 1 line

up with mapped addresses that are not in the user's address space

(although it

Quoted 1 line

is highly unlikely).

I have been trying to wrap my head around this comment. I am totaly unfamiliar
with RDMA code, but from quick look at it when registering umem you take the
mmap_sem in read mode so any munmap from userspace would be serialize. Really
the worst that can happen is that a umem pointing to a mmaped file that is
concurently truncated but even then the address is still valid, but it should
result in a SIGBUS which here is obviously harder to report (again dunno how
RDMA works).

Quoted 2 lines

So am i missing something ?

Sadly, taking mmap_sem in read-only mode does not prevent all possible invalidations from happening.
For example, a call to madvise requesting MADVISE_DONTNEED will lock the mmap_sem for reading only, allowing a notifier to run in parallel to the MR registration As a result, the following sequence of events could happen:

Thread 1: | Thread 2
--------------------------------+-------------------------

madvise                         |
down_read(mmap_sem)             |
notifier_start                  |
                                |   down_read(mmap_sem)
                                |   register_mr
notifier_end                    |
reduce_mr_notifiers_count       |

The end result of this sequence is an mr with running notifiers count of -1, which is bad.
The current workaround is to avoid decreasing the notifiers count if it is zero, which can cause other issues.
The proper fix would be to prevent notifiers from running in parallel to registration. For this, taking mmap_sem in write mode might be sufficient, but we are not sure about this. We will be happy to hear additional input on this subject, to make sure we got it covered properly.

Thanks,
--Shachar
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 6B2A6E60C06CCC42AE31809BF572352B010E2442C7-LSMZvP3E4uyuSA5JZHE7gA@public.gmane.org

author
#4 lore raw
Quoted 80 lines · Shachar Raindel

Hi,

Quoted 62 lines

-----Original Message-----
From: Jerome Glisse [mailto:[email protected]]
Sent: Thursday, September 04, 2014 11:25 PM
To: Haggai Eran; [email protected]
Cc: Shachar Raindel; Sagi Grimberg
Subject: Re: [PATCH v1 for-next 06/16] IB/core: Implement support for MMU notifiers regarding on demand paging regions

Quoted 1 line

* Add an interval tree implementation for ODP umems. Create an

interval tree

Quoted 1 line

for each ucontext (including a count of the number of ODP MRs in

this

Quoted 2 lines

context, mutex, etc.), and register ODP umems in the interval tree.
* Add MMU notifiers handling functions, using the interval tree to

notify only

Quoted 2 lines

the relevant umems and underlying MRs.
* Register to receive MMU notifier events from the MM subsystem upon

ODP MR

Quoted 3 lines

registration (and unregister accordingly).
* Add a completion object to synchronize the destruction of ODP umems.
* Add mechanism to abort page faults when there's a concurrent

invalidation.

Quoted 2 lines

The way we synchronize between concurrent invalidations and page

faults is by

Quoted 1 line

keeping a counter of currently running invalidations, and a sequence

number

Quoted 1 line

that is incremented whenever an invalidation is caught. The page fault

code

Quoted 2 lines

checks the counter and also verifies that the sequence number hasn't progressed before it updates the umem's page tables. This is similar

to what

Quoted 3 lines

the kvm module does.

There's currently a rare race in the code when registering a umem in

the

Quoted 1 line

middle of an ongoing notifier. The proper fix is to either serialize

the

Quoted 1 line

insertion to our umem tree with mm_lock_all or use a ucontext wide

running

Quoted 1 line

notifiers count for retries decision. Either is ugly and can lead to

some sort

Quoted 1 line

of starvation. The current workaround is ugly as well - now the user

can end

Quoted 1 line

up with mapped addresses that are not in the user's address space

(although it

Quoted 1 line

is highly unlikely).

I have been trying to wrap my head around this comment. I am totaly unfamiliar
with RDMA code, but from quick look at it when registering umem you take the
mmap_sem in read mode so any munmap from userspace would be serialize. Really
the worst that can happen is that a umem pointing to a mmaped file that is
concurently truncated but even then the address is still valid, but it should
result in a SIGBUS which here is obviously harder to report (again dunno how
RDMA works).

Quoted 2 lines

So am i missing something ?

Sadly, taking mmap_sem in read-only mode does not prevent all possible invalidations from happening.
For example, a call to madvise requesting MADVISE_DONTNEED will lock the mmap_sem for reading only, allowing a notifier to run in parallel to the MR registration As a result, the following sequence of events could happen:

Thread 1: | Thread 2
--------------------------------+-------------------------

madvise                         |
down_read(mmap_sem)             |
notifier_start                  |
                                |   down_read(mmap_sem)
                                |   register_mr
notifier_end                    |
reduce_mr_notifiers_count       |
Quoted 4 lines

The end result of this sequence is an mr with running notifiers count of -1, which is bad.
The current workaround is to avoid decreasing the notifiers count if it is zero, which can cause other issues.
The proper fix would be to prevent notifiers from running in parallel to registration. For this, taking mmap_sem in write mode might be sufficient, but we are not sure about this. We will be happy to hear additional input on this subject, to make sure we got it covered properly.

So in HMM i solve this by having a struct allocated in the start range callback and the end range callback just ignore things when it can not find the matching struct.

That being said when registering the mmu_notifier you need 2 things, first you need a pin on the mm (either mm is current ie current->mm or you took a reference on it). Second you need to that the mmap smemaphore in write mode so that no concurrent mmap/munmap/madvise can happen. By doing that you protect yourself from concurrent range_start/range_end that can happen and that does matter. The only concurrent range_start/end that can happen is through file invalidation which is fine because subsequent page fault will go through the file layer and bring back page or return error (if file was truncated for instance).

So as long as you hold the mmap_sem in write mode you should not worry about concurrent range_start/range_end (well they might happen but only for file backed vma).

Given that you face the same issue as i have with the range_start/range_end i will stich up a patch to make it easier to track those.

Cheers,
Jérôme

Quoted 7 lines

Thanks,
--Shachar
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

#6 lore raw
Quoted 120 lines

-----Original Message-----
From: Jerome Glisse [mailto:[email protected]]
Sent: Tuesday, September 09, 2014 6:37 PM
To: Shachar Raindel
Cc: 1404377069-20585-1-git-send-email-haggaie-VPRAkNaXOzVWk0Htik3J/[email protected]; Haggai Eran; [email protected]; Jerome Glisse; Sagi Grimberg
Subject: Re: [PATCH v1 for-next 06/16] IB/core: Implement support for
MMU notifiers regarding on demand paging regions

Quoted 8 lines · Shachar Raindel

Hi,

Quoted 6 lines

-----Original Message-----
From: Jerome Glisse [mailto:[email protected]]
Sent: Thursday, September 04, 2014 11:25 PM
To: Haggai Eran; [email protected]
Cc: Shachar Raindel; Sagi Grimberg
Subject: Re: [PATCH v1 for-next 06/16] IB/core: Implement support

for

Quoted 7 lines
Quoted 7 lines

MMU notifiers regarding on demand paging regions

Quoted 1 line

* Add an interval tree implementation for ODP umems. Create an

interval tree

Quoted 1 line

for each ucontext (including a count of the number of ODP MRs in

this

Quoted 1 line

context, mutex, etc.), and register ODP umems in the interval

tree.

Quoted 4 lines
Quoted 4 lines
Quoted 1 line

* Add MMU notifiers handling functions, using the interval tree to

notify only

Quoted 2 lines

the relevant umems and underlying MRs.
* Register to receive MMU notifier events from the MM subsystem

upon

Quoted 3 lines
Quoted 3 lines

ODP MR

Quoted 2 lines

registration (and unregister accordingly).
* Add a completion object to synchronize the destruction of ODP

umems.

Quoted 6 lines
Quoted 6 lines
Quoted 1 line

* Add mechanism to abort page faults when there's a concurrent

invalidation.

Quoted 2 lines

The way we synchronize between concurrent invalidations and page

faults is by

Quoted 1 line

keeping a counter of currently running invalidations, and a

sequence

Quoted 2 lines
Quoted 2 lines

number

Quoted 1 line

that is incremented whenever an invalidation is caught. The page

fault

Quoted 2 lines
Quoted 2 lines

code

Quoted 1 line

checks the counter and also verifies that the sequence number

hasn't

Quoted 1 line
Quoted 1 line
Quoted 1 line

progressed before it updates the umem's page tables. This is

similar

Quoted 4 lines
Quoted 4 lines

to what

Quoted 3 lines

the kvm module does.

There's currently a rare race in the code when registering a umem

in

Quoted 2 lines
Quoted 2 lines

the

Quoted 1 line

middle of an ongoing notifier. The proper fix is to either

serialize

Quoted 4 lines
Quoted 4 lines

the

Quoted 1 line

insertion to our umem tree with mm_lock_all or use a ucontext wide

running

Quoted 1 line

notifiers count for retries decision. Either is ugly and can lead

to

Quoted 2 lines
Quoted 2 lines

some sort

Quoted 1 line

of starvation. The current workaround is ugly as well - now the

user

Quoted 8 lines
Quoted 8 lines

can end

Quoted 1 line

up with mapped addresses that are not in the user's address space

(although it

Quoted 1 line

is highly unlikely).

I have been trying to wrap my head around this comment. I am totaly unfamiliar
with RDMA code, but from quick look at it when registering umem you

take

Quoted 2 lines
Quoted 2 lines

the
mmap_sem in read mode so any munmap from userspace would be

serialize.

Quoted 2 lines
Quoted 2 lines

Really
the worst that can happen is that a umem pointing to a mmaped file

that

Quoted 2 lines
Quoted 2 lines

is
concurently truncated but even then the address is still valid, but

it

Quoted 2 lines
Quoted 2 lines

should
result in a SIGBUS which here is obviously harder to report (again

dunno

Quoted 7 lines
Quoted 5 lines

how
RDMA works).

So am i missing something ?

Sadly, taking mmap_sem in read-only mode does not prevent all possible

invalidations from happening.

Quoted 1 line

For example, a call to madvise requesting MADVISE_DONTNEED will lock

the mmap_sem for reading only, allowing a notifier to run in parallel to the MR registration As a result, the following sequence of events could happen:

Quoted 11 lines

Thread 1: | Thread 2
--------------------------------+-------------------------

madvise                         |
down_read(mmap_sem)             |
notifier_start                  |
                                |   down_read(mmap_sem)
                                |   register_mr
notifier_end                    |
reduce_mr_notifiers_count       |
Quoted 1 line

The end result of this sequence is an mr with running notifiers count

of -1, which is bad.

Quoted 1 line

The current workaround is to avoid decreasing the notifiers count if

it is zero, which can cause other issues.

Quoted 1 line

The proper fix would be to prevent notifiers from running in parallel

to registration. For this, taking mmap_sem in write mode might be sufficient, but we are not sure about this.

Quoted 8 lines
Quoted 1 line

We will be happy to hear additional input on this subject, to make

sure we got it covered properly.

So in HMM i solve this by having a struct allocated in the start range callback
and the end range callback just ignore things when it can not find the matching
struct.

This kind of mechanism sounds like it has a bigger risk for deadlocking the system, causing an OOM kill without a real need or significantly slowing down the system.
If you are doing non-atomic memory allocations, you can deadlock the system by requesting memory in the swapper flow.
Even if you are doing atomic memory allocations, you need to handle the case of failing allocation, the solution to which is unclear to me. If you are using a pre-allocated pool, what are you doing when you run out of available entries in the pool? If you are blocking until some entries free up, what guarantees you that this will not cause a deadlock?

Quoted 16 lines

That being said when registering the mmu_notifier you need 2 things, first you
need a pin on the mm (either mm is current ie current->mm or you took a reference
on it). Second you need to that the mmap smemaphore in write mode so that
no concurrent mmap/munmap/madvise can happen. By doing that you protect yourself
from concurrent range_start/range_end that can happen and that does matter.
The only concurrent range_start/end that can happen is through file invalidation
which is fine because subsequent page fault will go through the file layer and
bring back page or return error (if file was truncated for instance).

Sadly, this is not sufficient for our use case. We are registering a single MMU notifier handler, and broadcast the notifications to all relevant listeners, which are stored in an interval tree.

Each listener represents a section of the address space that has been exposed to the network. Such implementation allows us to limit the impact of invalidations, and only block racing page faults to the affected areas.

Each of the listeners maintain a counter of the number of invalidate_range notifications that are currently affecting it. The counter is increased for each invalidate_range_start callback received, and decrease for each invalidate_range_end callback received. If we add a listener to the interval tree after the invalidate_range_start callback happened, but before the invalidate_range_end callback happened, it will decrease the counter, reaching negative numbers and breaking the logic.

The mmu_notifiers registration code avoid such issues by taking all relevant locks on the MM. This effectively blocks all possible notifiers from happening when registering a new notifier. Sadly, this function is not exported for modules to use it.

Our options at the moment are:
- Use a tracking mechanism similar to what HMM uses, alongside the challenges involved in allocating memory from notifiers

- Use a per-process counter for invalidations, causing a possible performance degradation. This can possibly be used as a fallback to the first option (i.e. have a pool of X notifier identifiers, once it is full, increase/decrease a per-MM counter)

- Export the mm_take_all_locks function for modules. This will allow us to lock the MM when adding a new listener.

Quoted 7 lines

So as long as you hold the mmap_sem in write mode you should not worry about
concurrent range_start/range_end (well they might happen but only for file
backed vma).

Sadly, the mmap_sem is not enough to protect us :(.

Quoted 4 lines

Given that you face the same issue as i have with the range_start/range_end i
will stich up a patch to make it easier to track those.

That would be nice, especially if we could easily integrate it into our code and reduce the code size.

Quoted 4 lines

Cheers,
Jérôme

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 6B2A6E60C06CCC42AE31809BF572352B010E244E86-LSMZvP3E4uyuSA5JZHE7gA@public.gmane.org

author
#8 lore raw
Quoted 129 lines · Shachar Raindel
Quoted 120 lines

-----Original Message-----
From: Jerome Glisse [mailto:[email protected]]
Sent: Tuesday, September 09, 2014 6:37 PM
To: Shachar Raindel
Cc: 1404377069-20585-1-git-send-email-haggaie-VPRAkNaXOzVWk0Htik3J/[email protected]; Haggai Eran; [email protected]; Jerome Glisse; Sagi Grimberg
Subject: Re: [PATCH v1 for-next 06/16] IB/core: Implement support for
MMU notifiers regarding on demand paging regions

Quoted 8 lines · Shachar Raindel

Hi,

Quoted 6 lines

-----Original Message-----
From: Jerome Glisse [mailto:[email protected]]
Sent: Thursday, September 04, 2014 11:25 PM
To: Haggai Eran; [email protected]
Cc: Shachar Raindel; Sagi Grimberg
Subject: Re: [PATCH v1 for-next 06/16] IB/core: Implement support

for

Quoted 7 lines
Quoted 7 lines

MMU notifiers regarding on demand paging regions

Quoted 1 line

* Add an interval tree implementation for ODP umems. Create an

interval tree

Quoted 1 line

for each ucontext (including a count of the number of ODP MRs in

this

Quoted 1 line

context, mutex, etc.), and register ODP umems in the interval

tree.

Quoted 4 lines
Quoted 4 lines
Quoted 1 line

* Add MMU notifiers handling functions, using the interval tree to

notify only

Quoted 2 lines

the relevant umems and underlying MRs.
* Register to receive MMU notifier events from the MM subsystem

upon

Quoted 3 lines
Quoted 3 lines

ODP MR

Quoted 2 lines

registration (and unregister accordingly).
* Add a completion object to synchronize the destruction of ODP

umems.

Quoted 6 lines
Quoted 6 lines
Quoted 1 line

* Add mechanism to abort page faults when there's a concurrent

invalidation.

Quoted 2 lines

The way we synchronize between concurrent invalidations and page

faults is by

Quoted 1 line

keeping a counter of currently running invalidations, and a

sequence

Quoted 2 lines
Quoted 2 lines

number

Quoted 1 line

that is incremented whenever an invalidation is caught. The page

fault

Quoted 2 lines
Quoted 2 lines

code

Quoted 1 line

checks the counter and also verifies that the sequence number

hasn't

Quoted 1 line
Quoted 1 line
Quoted 1 line

progressed before it updates the umem's page tables. This is

similar

Quoted 4 lines
Quoted 4 lines

to what

Quoted 3 lines

the kvm module does.

There's currently a rare race in the code when registering a umem

in

Quoted 2 lines
Quoted 2 lines

the

Quoted 1 line

middle of an ongoing notifier. The proper fix is to either

serialize

Quoted 4 lines
Quoted 4 lines

the

Quoted 1 line

insertion to our umem tree with mm_lock_all or use a ucontext wide

running

Quoted 1 line

notifiers count for retries decision. Either is ugly and can lead

to

Quoted 2 lines
Quoted 2 lines

some sort

Quoted 1 line

of starvation. The current workaround is ugly as well - now the

user

Quoted 8 lines
Quoted 8 lines

can end

Quoted 1 line

up with mapped addresses that are not in the user's address space

(although it

Quoted 1 line

is highly unlikely).

I have been trying to wrap my head around this comment. I am totaly unfamiliar
with RDMA code, but from quick look at it when registering umem you

take

Quoted 2 lines
Quoted 2 lines

the
mmap_sem in read mode so any munmap from userspace would be

serialize.

Quoted 2 lines
Quoted 2 lines

Really
the worst that can happen is that a umem pointing to a mmaped file

that

Quoted 2 lines
Quoted 2 lines

is
concurently truncated but even then the address is still valid, but

it

Quoted 2 lines
Quoted 2 lines

should
result in a SIGBUS which here is obviously harder to report (again

dunno

Quoted 7 lines
Quoted 5 lines

how
RDMA works).

So am i missing something ?

Sadly, taking mmap_sem in read-only mode does not prevent all possible

invalidations from happening.

Quoted 1 line

For example, a call to madvise requesting MADVISE_DONTNEED will lock

the mmap_sem for reading only, allowing a notifier to run in parallel to the MR registration As a result, the following sequence of events could happen:

Quoted 11 lines

Thread 1: | Thread 2
--------------------------------+-------------------------

madvise                         |
down_read(mmap_sem)             |
notifier_start                  |
                                |   down_read(mmap_sem)
                                |   register_mr
notifier_end                    |
reduce_mr_notifiers_count       |
Quoted 1 line

The end result of this sequence is an mr with running notifiers count

of -1, which is bad.

Quoted 1 line

The current workaround is to avoid decreasing the notifiers count if

it is zero, which can cause other issues.

Quoted 1 line

The proper fix would be to prevent notifiers from running in parallel

to registration. For this, taking mmap_sem in write mode might be sufficient, but we are not sure about this.

Quoted 8 lines
Quoted 1 line

We will be happy to hear additional input on this subject, to make

sure we got it covered properly.

So in HMM i solve this by having a struct allocated in the start range callback
and the end range callback just ignore things when it can not find the matching
struct.

Quoted 10 lines

This kind of mechanism sounds like it has a bigger risk for deadlocking the system, causing an OOM kill without a real need or significantly slowing down the system.
If you are doing non-atomic memory allocations, you can deadlock the system by requesting memory in the swapper flow.
Even if you are doing atomic memory allocations, you need to handle the case of failing allocation, the solution to which is unclear to me. If you are using a pre-allocated pool, what are you doing when you run out of available entries in the pool? If you are blocking until some entries free up, what guarantees you that this will not cause a deadlock?

So i am using a fixed pool and when it runs out it block in start callback until one is freed. But as i said i have a patch to use the stack that will solve this and avoid a pool.

Quoted 43 lines
Quoted 17 lines

That being said when registering the mmu_notifier you need 2 things, first you
need a pin on the mm (either mm is current ie current->mm or you took a reference
on it). Second you need to that the mmap smemaphore in write mode so that
no concurrent mmap/munmap/madvise can happen. By doing that you protect yourself
from concurrent range_start/range_end that can happen and that does matter.
The only concurrent range_start/end that can happen is through file invalidation
which is fine because subsequent page fault will go through the file layer and
bring back page or return error (if file was truncated for instance).

Sadly, this is not sufficient for our use case. We are registering a single MMU notifier handler, and broadcast the notifications to all relevant listeners, which are stored in an interval tree.

Each listener represents a section of the address space that has been exposed to the network. Such implementation allows us to limit the impact of invalidations, and only block racing page faults to the affected areas.

Each of the listeners maintain a counter of the number of invalidate_range notifications that are currently affecting it. The counter is increased for each invalidate_range_start callback received, and decrease for each invalidate_range_end callback received. If we add a listener to the interval tree after the invalidate_range_start callback happened, but before the invalidate_range_end callback happened, it will decrease the counter, reaching negative numbers and breaking the logic.

The mmu_notifiers registration code avoid such issues by taking all relevant locks on the MM. This effectively blocks all possible notifiers from happening when registering a new notifier. Sadly, this function is not exported for modules to use it.

Our options at the moment are:
- Use a tracking mechanism similar to what HMM uses, alongside the challenges involved in allocating memory from notifiers

Quoted 7 lines

- Use a per-process counter for invalidations, causing a possible performance degradation. This can possibly be used as a fallback to the first option (i.e. have a pool of X notifier identifiers, once it is full, increase/decrease a per-MM counter)

- Export the mm_take_all_locks function for modules. This will allow us to lock the MM when adding a new listener.

I was not clear enough, you need to take the mmap_sem in write mode accross mmu_notifier_register(). This is only to partialy solve your issue that if a mmu_notifier is already register for the mm you are trying to registering against then there is a chance for you to be inside an active range_start/ range_end section which would lead to invalid counter inside your tracking structure. But, sadly, taking mmap_sem in write mode is not enough as file invalidation might still happen concurrently so you will need to make sure you invalidation counters does not go negative but from page fault point of view you will be fine because the page fault will synchronize through the pagecache. So scenario (A and B are to anonymous overlapping address range) :

  APP_TOTO_RDMA_THREAD           |  APP_TOTO_SOME_OTHER_THREAD
                                 |  mmu_notifier_invalidate_range_start(A)
  odp_register()                 |
    down_read(mmap_sem)          |
    mmu_notifier_register()      |
    up_read(mmap_sem)            |
  odp_add_new_region(B)          |
  odp_page_fault(B)              |
    down_read(mmap_sem)          |
    ...                          |
    up_read(mmap_sem)            |
                                 |  mmu_notifier_invalidate_range_end(A)

The odp_page_fault(B) might see invalid cpu page table but you have no idea about it because you registered after the range_start(). But if you take the mmap_sem in write mode then the only case where you might still have this scenario is if A and B are range of a file backed vma and that the file is undergoing some change (most likely truncation). But the file case is fine because the odp_page_fault() will go through the pagecache which is properly synchronize against the current range invalidation.

Now for the the general case outside of mmu_notifier_register() HMM also track active invalidation range to avoid page faulting into those range as we can not trust the cpu page table for as long as the range invalidation is on going.

Quoted 9 lines
Quoted 7 lines

So as long as you hold the mmap_sem in write mode you should not worry about
concurrent range_start/range_end (well they might happen but only for file
backed vma).

Sadly, the mmap_sem is not enough to protect us :(.

This is enough like i explain above, but i am only talking about the mmu notifier registration. For the general case once you register you only need to take the mmap_sem in read mode during page fault.

Quoted 7 lines
Quoted 4 lines

Given that you face the same issue as i have with the range_start/range_end i
will stich up a patch to make it easier to track those.

That would be nice, especially if we could easily integrate it into our code and reduce the code size.

Yes it's a "small modification" to the mmu_notifier api, i have been side tracked on other thing. But i will have it soon.

Quoted 5 lines
Quoted 5 lines

Cheers,
Jérôme

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

#10 lore raw
Quoted 37 lines

-----Original Message-----
From: Jerome Glisse [mailto:[email protected]]
Sent: Wednesday, September 10, 2014 11:15 PM
To: Shachar Raindel
Cc: Haggai Eran; [email protected]; Sagi Grimberg
Subject: Re: [PATCH v1 for-next 06/16] IB/core: Implement support for MMU notifiers regarding on demand paging regions

Quoted 7 lines · Shachar Raindel
Quoted 7 lines

-----Original Message-----
From: Jerome Glisse [mailto:[email protected]]
Sent: Tuesday, September 09, 2014 6:37 PM
To: Shachar Raindel
Cc: 1404377069-20585-1-git-send-email-haggaie-VPRAkNaXOzVWk0Htik3J/[email protected]; Haggai

Eran;

Quoted 2 lines
Quoted 2 lines

[email protected]; Jerome Glisse; Sagi Grimberg
Subject: Re: [PATCH v1 for-next 06/16] IB/core: Implement support

for

Quoted 11 lines
Quoted 11 lines

MMU notifiers regarding on demand paging regions

Quoted 8 lines · Shachar Raindel

Hi,

Quoted 6 lines

-----Original Message-----
From: Jerome Glisse [mailto:[email protected]]
Sent: Thursday, September 04, 2014 11:25 PM
To: Haggai Eran; [email protected]
Cc: Shachar Raindel; Sagi Grimberg
Subject: Re: [PATCH v1 for-next 06/16] IB/core: Implement

support

Quoted 3 lines
Quoted 3 lines

for

Quoted 2 lines
Quoted 2 lines

MMU notifiers regarding on demand paging regions

<SNIP>

Quoted 55 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

Sadly, taking mmap_sem in read-only mode does not prevent all

possible

Quoted 2 lines
Quoted 2 lines

invalidations from happening.

Quoted 1 line

For example, a call to madvise requesting MADVISE_DONTNEED will

lock

Quoted 1 line
Quoted 1 line

the mmap_sem for reading only, allowing a notifier to run in

parallel to

Quoted 1 line
Quoted 1 line

the MR registration As a result, the following sequence of events

could

Quoted 12 lines
Quoted 12 lines

happen:

Quoted 11 lines

Thread 1: | Thread 2
--------------------------------+-------------------------

madvise                         |
down_read(mmap_sem)             |
notifier_start                  |
                                |   down_read(mmap_sem)
                                |   register_mr
notifier_end                    |
reduce_mr_notifiers_count       |
Quoted 1 line
Quoted 1 line
Quoted 1 line

The end result of this sequence is an mr with running notifiers

count

Quoted 2 lines
Quoted 2 lines

of -1, which is bad.

Quoted 1 line

The current workaround is to avoid decreasing the notifiers count

if

Quoted 2 lines
Quoted 2 lines

it is zero, which can cause other issues.

Quoted 1 line

The proper fix would be to prevent notifiers from running in

parallel

Quoted 6 lines
Quoted 6 lines

to registration. For this, taking mmap_sem in write mode might be sufficient, but we are not sure about this.

Quoted 1 line

We will be happy to hear additional input on this subject, to make

sure we got it covered properly.

So in HMM i solve this by having a struct allocated in the start

range

Quoted 2 lines
Quoted 2 lines

callback
and the end range callback just ignore things when it can not find

the

Quoted 4 lines
Quoted 2 lines

matching
struct.

This kind of mechanism sounds like it has a bigger risk for

deadlocking

Quoted 5 lines

the system, causing an OOM kill without a real need or significantly slowing down the system.
If you are doing non-atomic memory allocations, you can deadlock the system by requesting memory in the swapper flow.
Even if you are doing atomic memory allocations, you need to handle

the

Quoted 4 lines

case of failing allocation, the solution to which is unclear to me. If you are using a pre-allocated pool, what are you doing when you run out of available entries in the pool? If you are blocking until some entries free up, what guarantees you that this will not cause a

Quoted 5 lines

deadlock?

So i am using a fixed pool and when it runs out it block in start callback
until one is freed.

This sounds scary. You now create a possible locking dependency between two code flows which could have run in parallel. This can cause circular locking bugs, from code which functioned properly until now. For example, assume code with a single lock, and the following code paths:

Code 1:
notify_start()
lock()
unlock()
notify_end()

Code 2:
lock()
notify_start()
... (no locking)
notify_end()
unlock()

This code can now create the following deadlock:

Thread 1: | Thread 2:
-----------------+-----------------------------------

notify_start()   |
                 | lock()
lock() - blocking|
                 | notify_start() - blocking for slot
Quoted 3 lines

But as i said i have a patch to use the stack that will
solve this and avoid a pool.

How are you allocating from the stack an entry which you need to keep alive until another function is called? You can't allocate the entry on the notify_start stack, so you must do this in all of the call points to the mmu_notifiers. Given the notifiers listener subscription pattern, this seems like something which is not practical.

Quoted 99 lines
Quoted 6 lines
Quoted 6 lines

That being said when registering the mmu_notifier you need 2 things, first you
need a pin on the mm (either mm is current ie current->mm or you

took a

Quoted 4 lines
Quoted 4 lines

reference
on it). Second you need to that the mmap smemaphore in write mode so that
no concurrent mmap/munmap/madvise can happen. By doing that you

protect

Quoted 8 lines
Quoted 8 lines

yourself
from concurrent range_start/range_end that can happen and that does matter.
The only concurrent range_start/end that can happen is through file invalidation
which is fine because subsequent page fault will go through the file layer and
bring back page or return error (if file was truncated for

instance).

Quoted 7 lines

Sadly, this is not sufficient for our use case. We are registering a single MMU notifier handler, and broadcast the notifications to all relevant listeners, which are stored in an interval tree.

Each listener represents a section of the address space that has been exposed to the network. Such implementation allows us to limit the

impact

Quoted 1 line

of invalidations, and only block racing page faults to the affected

areas.

Quoted 2 lines

Each of the listeners maintain a counter of the number of

invalidate_range

Quoted 1 line

notifications that are currently affecting it. The counter is

increased

Quoted 1 line

for each invalidate_range_start callback received, and decrease for

each

Quoted 3 lines

invalidate_range_end callback received. If we add a listener to the interval tree after the invalidate_range_start callback happened, but before the invalidate_range_end callback happened, it will decrease

the

Quoted 4 lines

counter, reaching negative numbers and breaking the logic.

The mmu_notifiers registration code avoid such issues by taking all relevant locks on the MM. This effectively blocks all possible

notifiers

Quoted 1 line

from happening when registering a new notifier. Sadly, this function

is

Quoted 8 lines

not exported for modules to use it.

Our options at the moment are:
- Use a tracking mechanism similar to what HMM uses, alongside the challenges involved in allocating memory from notifiers

- Use a per-process counter for invalidations, causing a possible performance degradation. This can possibly be used as a fallback to

the

Quoted 4 lines

first option (i.e. have a pool of X notifier identifiers, once it is full, increase/decrease a per-MM counter)

- Export the mm_take_all_locks function for modules. This will allow

us

Quoted 1 line

to lock the MM when adding a new listener.

I was not clear enough, you need to take the mmap_sem in write mode accross
mmu_notifier_register(). This is only to partialy solve your issue that if
a mmu_notifier is already register for the mm you are trying to registering
against then there is a chance for you to be inside an active range_start/
range_end section which would lead to invalid counter inside your tracking
structure. But, sadly, taking mmap_sem in write mode is not enough as file
invalidation might still happen concurrently so you will need to make sure
you invalidation counters does not go negative but from page fault point of
view you will be fine because the page fault will synchronize through the
pagecache. So scenario (A and B are to anonymous overlapping address range) :

  APP_TOTO_RDMA_THREAD           |  APP_TOTO_SOME_OTHER_THREAD
                                 |  mmu_notifier_invalidate_range_start(A)
  odp_register()                 |
    down_read(mmap_sem)          |
    mmu_notifier_register()      |
    up_read(mmap_sem)            |
  odp_add_new_region(B)          |
  odp_page_fault(B)              |
    down_read(mmap_sem)          |
    ...                          |
    up_read(mmap_sem)            |
                                 |  mmu_notifier_invalidate_range_end(A)
Quoted 13 lines

The odp_page_fault(B) might see invalid cpu page table but you have no idea
about it because you registered after the range_start(). But if you take the
mmap_sem in write mode then the only case where you might still have this
scenario is if A and B are range of a file backed vma and that the file is
undergoing some change (most likely truncation). But the file case is fine
because the odp_page_fault() will go through the pagecache which is properly
synchronize against the current range invalidation.

Specifically, if you call mmu_notifier_register you are OK and the above scenario will not happen. You are supposed to hold mmap_sem for writing, and mmu_notifier_register is calling mm_take_all_locks, which guarantees no racing notifier during the registration step.

However, we want to dynamically add sub-notifiers in our code. Each will get notified only about invalidations touching a specific sub-sections of the address space. To avoid providing unneeded notifications, we use an interval tree that filters only the needed notifications.
When adding entries to the interval tree, we cannot lock the mm to prevent any racing invalidations. As such, we might end up in a case where a newly registered memory region will get a "notify_end" call without the relevant "notify_start". Even if we prevent the value from dropping below zero, it means we can cause data corruption. For example, if we have another notifier running after the MR registers, which is due to munmap, but we get first the notify_end of the previous notifier for which we didn't see the notify_start.

The solution we are coming up with now is using a global counter of running invalidations for new regions allocated. When the global counter is at zero, we can safely switch to the region local invalidations counter.

Quoted 19 lines

Now for the the general case outside of mmu_notifier_register() HMM also track
active invalidation range to avoid page faulting into those range as we can not
trust the cpu page table for as long as the range invalidation is on going.

Quoted 2 lines
Quoted 2 lines

So as long as you hold the mmap_sem in write mode you should not

worry

Quoted 2 lines
Quoted 2 lines

about
concurrent range_start/range_end (well they might happen but only

for

Quoted 4 lines
Quoted 3 lines

file
backed vma).

Quoted 6 lines
Quoted 1 line

Sadly, the mmap_sem is not enough to protect us :(.

This is enough like i explain above, but i am only talking about the mmu notifier registration. For the general case once you register you only need to take the mmap_sem in read mode during page fault.

I think we are not broadcasting on the same wavelength here. The issue I'm worried about is of adding a sub-area to our tracking system. It is built quite differently from how HMM is built, we are defining areas to track a-priori, and later on account how many notifiers are blocking page-faults for each area. You are keeping track of the active notifiers, and check each page fault against your notifier list. This difference makes for different locking needs.

Quoted 5 lines
Quoted 5 lines
Quoted 4 lines

Given that you face the same issue as i have with the range_start/range_end i
will stich up a patch to make it easier to track those.

Quoted 8 lines
Quoted 1 line

That would be nice, especially if we could easily integrate it into

our

Quoted 1 line

code and reduce the code size.

Yes it's a "small modification" to the mmu_notifier api, i have been side
tracked on other thing. But i will have it soon.

Being side tracked is a well-known professional risk in our line of work ;)

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

Cheers,
Jérôme

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 6B2A6E60C06CCC42AE31809BF572352B010E2456AE-LSMZvP3E4uyuSA5JZHE7gA@public.gmane.org

author
#12 lore raw
Quoted 112 lines · Shachar Raindel
Quoted 37 lines

-----Original Message-----
From: Jerome Glisse [mailto:[email protected]]
Sent: Wednesday, September 10, 2014 11:15 PM
To: Shachar Raindel
Cc: Haggai Eran; [email protected]; Sagi Grimberg
Subject: Re: [PATCH v1 for-next 06/16] IB/core: Implement support for MMU notifiers regarding on demand paging regions

Quoted 7 lines · Shachar Raindel
Quoted 7 lines

-----Original Message-----
From: Jerome Glisse [mailto:[email protected]]
Sent: Tuesday, September 09, 2014 6:37 PM
To: Shachar Raindel
Cc: 1404377069-20585-1-git-send-email-haggaie-VPRAkNaXOzVWk0Htik3J/[email protected]; Haggai

Eran;

Quoted 2 lines
Quoted 2 lines

[email protected]; Jerome Glisse; Sagi Grimberg
Subject: Re: [PATCH v1 for-next 06/16] IB/core: Implement support

for

Quoted 11 lines
Quoted 11 lines

MMU notifiers regarding on demand paging regions

Quoted 8 lines · Shachar Raindel

Hi,

Quoted 6 lines

-----Original Message-----
From: Jerome Glisse [mailto:[email protected]]
Sent: Thursday, September 04, 2014 11:25 PM
To: Haggai Eran; [email protected]
Cc: Shachar Raindel; Sagi Grimberg
Subject: Re: [PATCH v1 for-next 06/16] IB/core: Implement

support

Quoted 3 lines
Quoted 3 lines

for

Quoted 2 lines
Quoted 2 lines

MMU notifiers regarding on demand paging regions

<SNIP>

Quoted 55 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

Sadly, taking mmap_sem in read-only mode does not prevent all

possible

Quoted 2 lines
Quoted 2 lines

invalidations from happening.

Quoted 1 line

For example, a call to madvise requesting MADVISE_DONTNEED will

lock

Quoted 1 line
Quoted 1 line

the mmap_sem for reading only, allowing a notifier to run in

parallel to

Quoted 1 line
Quoted 1 line

the MR registration As a result, the following sequence of events

could

Quoted 12 lines
Quoted 12 lines

happen:

Quoted 11 lines

Thread 1: | Thread 2
--------------------------------+-------------------------

madvise                         |
down_read(mmap_sem)             |
notifier_start                  |
                                |   down_read(mmap_sem)
                                |   register_mr
notifier_end                    |
reduce_mr_notifiers_count       |
Quoted 1 line
Quoted 1 line
Quoted 1 line

The end result of this sequence is an mr with running notifiers

count

Quoted 2 lines
Quoted 2 lines

of -1, which is bad.

Quoted 1 line

The current workaround is to avoid decreasing the notifiers count

if

Quoted 2 lines
Quoted 2 lines

it is zero, which can cause other issues.

Quoted 1 line

The proper fix would be to prevent notifiers from running in

parallel

Quoted 6 lines
Quoted 6 lines

to registration. For this, taking mmap_sem in write mode might be sufficient, but we are not sure about this.

Quoted 1 line

We will be happy to hear additional input on this subject, to make

sure we got it covered properly.

So in HMM i solve this by having a struct allocated in the start

range

Quoted 2 lines
Quoted 2 lines

callback
and the end range callback just ignore things when it can not find

the

Quoted 4 lines
Quoted 2 lines

matching
struct.

This kind of mechanism sounds like it has a bigger risk for

deadlocking

Quoted 5 lines

the system, causing an OOM kill without a real need or significantly slowing down the system.
If you are doing non-atomic memory allocations, you can deadlock the system by requesting memory in the swapper flow.
Even if you are doing atomic memory allocations, you need to handle

the

Quoted 4 lines

case of failing allocation, the solution to which is unclear to me. If you are using a pre-allocated pool, what are you doing when you run out of available entries in the pool? If you are blocking until some entries free up, what guarantees you that this will not cause a

Quoted 5 lines

deadlock?

So i am using a fixed pool and when it runs out it block in start callback
until one is freed.

This sounds scary. You now create a possible locking dependency between two code flows which could have run in parallel. This can cause circular locking bugs, from code which functioned properly until now. For example, assume code with a single lock, and the following code paths:

Code 1:
notify_start()
lock()
unlock()
notify_end()

Quoted 7 lines

Code 2:
lock()
notify_start()
... (no locking)
notify_end()
unlock()

This can not happen because all lock taken before notify_start() are never taken after it and all lock taken inside a start/end section are never hold accross a notify_start() callback.

Quoted 18 lines

This code can now create the following deadlock:

Thread 1: | Thread 2:
-----------------+-----------------------------------

notify_start()   |
                 | lock()
lock() - blocking|
                 | notify_start() - blocking for slot
Quoted 3 lines

But as i said i have a patch to use the stack that will
solve this and avoid a pool.

Quoted 5 lines

How are you allocating from the stack an entry which you need to keep alive until another function is called? You can't allocate the entry on the notify_start stack, so you must do this in all of the call points to the mmu_notifiers. Given the notifiers listener subscription pattern, this seems like something which is not practical.

Yes the patch add a struct in each callsite of mmu_notifier_invalidate_range as in all case both start and end are call from same function. The only draw back is that it increase stack consumption in some of those callsite (not all). I attach the patch i am thinking of (it is untested) but idea is that through two new helper function user of mmu_notifier can query active invalid range and synchronize with those (also require some code in the range_start() callback).

Quoted 133 lines
Quoted 101 lines
Quoted 8 lines
Quoted 8 lines

That being said when registering the mmu_notifier you need 2 things, first you
need a pin on the mm (either mm is current ie current->mm or you

took a

Quoted 4 lines
Quoted 4 lines

reference
on it). Second you need to that the mmap smemaphore in write mode so that
no concurrent mmap/munmap/madvise can happen. By doing that you

protect

Quoted 8 lines
Quoted 8 lines

yourself
from concurrent range_start/range_end that can happen and that does matter.
The only concurrent range_start/end that can happen is through file invalidation
which is fine because subsequent page fault will go through the file layer and
bring back page or return error (if file was truncated for

instance).

Quoted 7 lines

Sadly, this is not sufficient for our use case. We are registering a single MMU notifier handler, and broadcast the notifications to all relevant listeners, which are stored in an interval tree.

Each listener represents a section of the address space that has been exposed to the network. Such implementation allows us to limit the

impact

Quoted 1 line

of invalidations, and only block racing page faults to the affected

areas.

Quoted 2 lines

Each of the listeners maintain a counter of the number of

invalidate_range

Quoted 1 line

notifications that are currently affecting it. The counter is

increased

Quoted 1 line

for each invalidate_range_start callback received, and decrease for

each

Quoted 3 lines

invalidate_range_end callback received. If we add a listener to the interval tree after the invalidate_range_start callback happened, but before the invalidate_range_end callback happened, it will decrease

the

Quoted 4 lines

counter, reaching negative numbers and breaking the logic.

The mmu_notifiers registration code avoid such issues by taking all relevant locks on the MM. This effectively blocks all possible

notifiers

Quoted 1 line

from happening when registering a new notifier. Sadly, this function

is

Quoted 8 lines

not exported for modules to use it.

Our options at the moment are:
- Use a tracking mechanism similar to what HMM uses, alongside the challenges involved in allocating memory from notifiers

- Use a per-process counter for invalidations, causing a possible performance degradation. This can possibly be used as a fallback to

the

Quoted 4 lines

first option (i.e. have a pool of X notifier identifiers, once it is full, increase/decrease a per-MM counter)

- Export the mm_take_all_locks function for modules. This will allow

us

Quoted 1 line

to lock the MM when adding a new listener.

I was not clear enough, you need to take the mmap_sem in write mode accross
mmu_notifier_register(). This is only to partialy solve your issue that if
a mmu_notifier is already register for the mm you are trying to registering
against then there is a chance for you to be inside an active range_start/
range_end section which would lead to invalid counter inside your tracking
structure. But, sadly, taking mmap_sem in write mode is not enough as file
invalidation might still happen concurrently so you will need to make sure
you invalidation counters does not go negative but from page fault point of
view you will be fine because the page fault will synchronize through the
pagecache. So scenario (A and B are to anonymous overlapping address range) :

  APP_TOTO_RDMA_THREAD           |  APP_TOTO_SOME_OTHER_THREAD
                                 |  mmu_notifier_invalidate_range_start(A)
  odp_register()                 |
    down_read(mmap_sem)          |
    mmu_notifier_register()      |
    up_read(mmap_sem)            |
  odp_add_new_region(B)          |
  odp_page_fault(B)              |
    down_read(mmap_sem)          |
    ...                          |
    up_read(mmap_sem)            |
                                 |  mmu_notifier_invalidate_range_end(A)
Quoted 13 lines

The odp_page_fault(B) might see invalid cpu page table but you have no idea
about it because you registered after the range_start(). But if you take the
mmap_sem in write mode then the only case where you might still have this
scenario is if A and B are range of a file backed vma and that the file is
undergoing some change (most likely truncation). But the file case is fine
because the odp_page_fault() will go through the pagecache which is properly
synchronize against the current range invalidation.

Specifically, if you call mmu_notifier_register you are OK and the above scenario will not happen. You are supposed to hold mmap_sem for writing, and mmu_notifier_register is calling mm_take_all_locks, which guarantees no racing notifier during the registration step.

However, we want to dynamically add sub-notifiers in our code. Each will get notified only about invalidations touching a specific sub-sections of the address space. To avoid providing unneeded notifications, we use an interval tree that filters only the needed notifications.
When adding entries to the interval tree, we cannot lock the mm to prevent any racing invalidations. As such, we might end up in a case where a newly registered memory region will get a "notify_end" call without the relevant "notify_start". Even if we prevent the value from dropping below zero, it means we can cause data corruption. For example, if we have another notifier running after the MR registers, which is due to munmap, but we get first the notify_end of the previous notifier for which we didn't see the notify_start.

Quoted 3 lines

The solution we are coming up with now is using a global counter of running invalidations for new regions allocated. When the global counter is at zero, we can safely switch to the region local invalidations counter.

Yes i fully understood that design but as i said this kind of broken and this is what the attached patch try to address as HMM have the same issue of having to track all active invalidation range.

Quoted 54 lines
Quoted 21 lines

Now for the the general case outside of mmu_notifier_register() HMM also track
active invalidation range to avoid page faulting into those range as we can not
trust the cpu page table for as long as the range invalidation is on going.

Quoted 2 lines
Quoted 2 lines

So as long as you hold the mmap_sem in write mode you should not

worry

Quoted 2 lines
Quoted 2 lines

about
concurrent range_start/range_end (well they might happen but only

for

Quoted 4 lines
Quoted 3 lines

file
backed vma).

Quoted 6 lines
Quoted 1 line

Sadly, the mmap_sem is not enough to protect us :(.

This is enough like i explain above, but i am only talking about the mmu notifier registration. For the general case once you register you only need to take the mmap_sem in read mode during page fault.

I think we are not broadcasting on the same wavelength here. The issue I'm worried about is of adding a sub-area to our tracking system. It is built quite differently from how HMM is built, we are defining areas to track a-priori, and later on account how many notifiers are blocking page-faults for each area. You are keeping track of the active notifiers, and check each page fault against your notifier list. This difference makes for different locking needs.

Quoted 5 lines
Quoted 5 lines
Quoted 4 lines

Given that you face the same issue as i have with the range_start/range_end i
will stich up a patch to make it easier to track those.

Quoted 8 lines
Quoted 1 line

That would be nice, especially if we could easily integrate it into

our

Quoted 1 line

code and reduce the code size.

Yes it's a "small modification" to the mmu_notifier api, i have been side
tracked on other thing. But i will have it soon.

Being side tracked is a well-known professional risk in our line of work ;)

Quoted 1 line
Quoted 1 line
Quoted 5 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

Cheers,
Jérôme

From 037195e49fbed468d16b78f0364fe302bc732d12 Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Glisse?= <[email protected]>
Date: Thu, 11 Sep 2014 11:22:12 -0400
Subject: [PATCH] mmu_notifier: keep track of active invalidation ranges
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The mmu_notifier_invalidate_range_start() and mmu_notifier_invalidate_range_end() can be considered as forming an "atomic" section for the cpu page table update point of view. Between this two function the cpu page table content is unreliable for the affected range of address.

Current user such as kvm need to know when they can trust a the content of the cpu page table. This becomes even more important to new users of the mmu_notifier api (such as HMM or ODP).

This patch use a structure define at all call site to invalidate_range_start() that is added to a list for the duration of the invalidation. It adds two new helpers to allow querying if a range is being invalidated or to wait for a range to become valid.

This two new function does not provide strong synchronization but are intended to be use as helper. User of the mmu_notifier must also synchronize with themself inside their range_start() and range_end() callback.

Signed-off-by Jérôme Glisse <[email protected]>
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 13 +++---
 drivers/iommu/amd_iommu_v2.c            |  8 +---
 drivers/misc/sgi-gru/grutlbpurge.c      | 15 +++----
 drivers/xen/gntdev.c                    |  8 ++--
 fs/proc/task_mmu.c                      | 12 +++--
 include/linux/mmu_notifier.h            | 55 ++++++++++++-----------
 mm/fremap.c                             |  8 +++-
 mm/huge_memory.c                        | 78 ++++++++++++++-------------------
 mm/hugetlb.c                            | 49 +++++++++++----------
 mm/memory.c                             | 73 ++++++++++++++++--------------
 mm/migrate.c                            | 16 +++----
 mm/mmu_notifier.c                       | 73 +++++++++++++++++++++++++-----
 mm/mprotect.c                           | 17 ++++---
 mm/mremap.c                             | 14 +++---
 mm/rmap.c                               | 15 +++----
 virt/kvm/kvm_main.c                     | 10 ++---
 16 files changed, 256 insertions(+), 208 deletions(-)
drivers/gpu/drm/i915/i915_gem_userptr.c +6 -7
index a13307d..373ffbb 100644
@@ -123,26 +123,25 @@ restart:
 
 static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 						       struct mm_struct *mm,
-						       unsigned long start,
-						       unsigned long end,
-						       enum mmu_event event)
+						       const struct mmu_notifier_range *range)
 {
 	struct i915_mmu_notifier *mn = container_of(_mn, struct i915_mmu_notifier, mn);
 	struct interval_tree_node *it = NULL;
-	unsigned long next = start;
+	unsigned long next = range->start;
 	unsigned long serial = 0;
+	/* interval ranges are inclusive, but invalidate range is exclusive */
+	unsigned long end = range.end - 1;
 
-	end--; /* interval ranges are inclusive, but invalidate range is exclusive */
 	while (next < end) {
 		struct drm_i915_gem_object *obj = NULL;
 
 		spin_lock(&mn->lock);
 		if (mn->has_linear)
-			it = invalidate_range__linear(mn, mm, start, end);
+			it = invalidate_range__linear(mn, mm, range->start, end);
 		else if (serial == mn->serial)
 			it = interval_tree_iter_next(it, next, end);
 		else
-			it = interval_tree_iter_first(&mn->objects, start, end);
+			it = interval_tree_iter_first(&mn->objects, range->start, end);
 		if (it != NULL) {
 			obj = container_of(it, struct i915_mmu_object, it)->obj;
 			drm_gem_object_reference(&obj->base);
drivers/iommu/amd_iommu_v2.c +2 -6
index 9a6b837..5945300 100644
@@ -419,9 +419,7 @@ static void mn_invalidate_page(struct mmu_notifier *mn,
 
 static void mn_invalidate_range_start(struct mmu_notifier *mn,
 				      struct mm_struct *mm,
-				      unsigned long start,
-				      unsigned long end,
-				      enum mmu_event event)
+				      const struct mmu_notifier_range *range)
 {
 	struct pasid_state *pasid_state;
 	struct device_state *dev_state;

@@ -442,9 +440,7 @@ static void mn_invalidate_range_start(struct mmu_notifier *mn,
 
 static void mn_invalidate_range_end(struct mmu_notifier *mn,
 				    struct mm_struct *mm,
-				    unsigned long start,
-				    unsigned long end,
-				    enum mmu_event event)
+				    const struct mmu_notifier_range *range)
 {
 	struct pasid_state *pasid_state;
 	struct device_state *dev_state;
drivers/misc/sgi-gru/grutlbpurge.c +7 -8
index e67fed1..44b41b7 100644
@@ -221,8 +221,7 @@ void gru_flush_all_tlb(struct gru_state *gru)
  */
 static void gru_invalidate_range_start(struct mmu_notifier *mn,
 				       struct mm_struct *mm,
-				       unsigned long start, unsigned long end,
-				       enum mmu_event event)
+				       const struct mmu_notifier_range *range)
 {
 	struct gru_mm_struct *gms = container_of(mn, struct gru_mm_struct,
 						 ms_notifier);

@@ -230,14 +229,13 @@ static void gru_invalidate_range_start(struct mmu_notifier *mn,
 	STAT(mmu_invalidate_range);
 	atomic_inc(&gms->ms_range_active);
 	gru_dbg(grudev, "gms %p, start 0x%lx, end 0x%lx, act %d\n", gms,
-		start, end, atomic_read(&gms->ms_range_active));
-	gru_flush_tlb_range(gms, start, end - start);
+		range->start, range->end, atomic_read(&gms->ms_range_active));
+	gru_flush_tlb_range(gms, range->start, range->end - range->start);
 }
 
 static void gru_invalidate_range_end(struct mmu_notifier *mn,
-				     struct mm_struct *mm, unsigned long start,
-				     unsigned long end,
-				     enum mmu_event event)
+				     struct mm_struct *mm,
+				     const struct mmu_notifier_range *range)
 {
 	struct gru_mm_struct *gms = container_of(mn, struct gru_mm_struct,
 						 ms_notifier);

@@ -246,7 +244,8 @@ static void gru_invalidate_range_end(struct mmu_notifier *mn,
 	(void)atomic_dec_and_test(&gms->ms_range_active);
 
 	wake_up_all(&gms->ms_wait_queue);
-	gru_dbg(grudev, "gms %p, start 0x%lx, end 0x%lx\n", gms, start, end);
+	gru_dbg(grudev, "gms %p, start 0x%lx, end 0x%lx\n", gms,
+		range->start, range->end);
 }
 
 static void gru_invalidate_page(struct mmu_notifier *mn, struct mm_struct *mm,
drivers/xen/gntdev.c +3 -5
index fe9da94..51f9188 100644
@@ -428,19 +428,17 @@ static void unmap_if_in_range(struct grant_map *map,
 
 static void mn_invl_range_start(struct mmu_notifier *mn,
 				struct mm_struct *mm,
-				unsigned long start,
-				unsigned long end,
-				enum mmu_event event)
+				const struct mmu_notifier_range *range)
 {
 	struct gntdev_priv *priv = container_of(mn, struct gntdev_priv, mn);
 	struct grant_map *map;
 
 	spin_lock(&priv->lock);
 	list_for_each_entry(map, &priv->maps, next) {
-		unmap_if_in_range(map, start, end);
+		unmap_if_in_range(map, range->start, range->end);
 	}
 	list_for_each_entry(map, &priv->freeable_maps, next) {
-		unmap_if_in_range(map, start, end);
+		unmap_if_in_range(map, range->start, range->end);
 	}
 	spin_unlock(&priv->lock);
 }
fs/proc/task_mmu.c +8 -4
index 0ddb975..532a230 100644
@@ -828,10 +828,15 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
 			.mm = mm,
 			.private = &cp,
 		};
+		struct mmu_notifier_range range = {
+			.start = 0,
+			.end = -1UL,
+			.event = MMU_ISDIRTY,
+		};
+
 		down_read(&mm->mmap_sem);
 		if (type == CLEAR_REFS_SOFT_DIRTY)
-			mmu_notifier_invalidate_range_start(mm, 0,
-							    -1, MMU_ISDIRTY);
+			mmu_notifier_invalidate_range_start(mm, &range);
 		for (vma = mm->mmap; vma; vma = vma->vm_next) {
 			cp.vma = vma;
 			if (is_vm_hugetlb_page(vma))

@@ -859,8 +864,7 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
 					&clear_refs_walk);
 		}
 		if (type == CLEAR_REFS_SOFT_DIRTY)
-			mmu_notifier_invalidate_range_end(mm, 0,
-							  -1, MMU_ISDIRTY);
+			mmu_notifier_invalidate_range_end(mm, &range);
 		flush_tlb_mm(mm);
 		up_read(&mm->mmap_sem);
 		mmput(mm);
include/linux/mmu_notifier.h +29 -26

105 lines not on this page · raw message

mm/fremap.c +6 -2
index 37b2904..03a5ddc 100644
@@ -148,6 +148,7 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size,
 	int err = -EINVAL;
 	int has_write_lock = 0;
 	vm_flags_t vm_flags = 0;
+	struct mmu_notifier_range range;
 
 	pr_warn_once("%s (%d) uses deprecated remap_file_pages() syscall. "
 			"See Documentation/vm/remap_file_pages.txt.\n",

@@ -258,9 +259,12 @@ get_write_lock:
 		vma->vm_flags = vm_flags;
 	}
 
-	mmu_notifier_invalidate_range_start(mm, start, start + size, MMU_MUNMAP);
+	range.start = start;
+	range.end = start + size;
+	range.event = MMU_MUNMAP;
+	mmu_notifier_invalidate_range_start(mm, &range);
 	err = vma->vm_ops->remap_pages(vma, start, size, pgoff);
-	mmu_notifier_invalidate_range_end(mm, start, start + size, MMU_MUNMAP);
+	mmu_notifier_invalidate_range_end(mm, &range);
 
 	/*
 	 * We can't clear VM_NONLINEAR because we'd have to do
mm/huge_memory.c +33 -45

175 lines not on this page · raw message

mm/hugetlb.c +25 -24

118 lines not on this page · raw message

mm/memory.c +40 -33

137 lines not on this page · raw message

mm/migrate.c +8 -8
index 30417d5..d866771 100644
@@ -1781,10 +1781,13 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 	int isolated = 0;
 	struct page *new_page = NULL;
 	int page_lru = page_is_file_cache(page);
-	unsigned long mmun_start = address & HPAGE_PMD_MASK;
-	unsigned long mmun_end = mmun_start + HPAGE_PMD_SIZE;
+	struct mmu_notifier_range range;
 	pmd_t orig_entry;
 
+	range.start = address & HPAGE_PMD_MASK;
+	range.end = range.start + HPAGE_PMD_SIZE;
+	range.event = MMU_MIGRATE;
+
 	/*
 	 * Rate-limit the amount of data that is being migrated to a node.
 	 * Optimal placement is no good if the memory bus is saturated and

@@ -1819,14 +1822,12 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 	WARN_ON(PageLRU(new_page));
 
 	/* Recheck the target PMD */
-	mmu_notifier_invalidate_range_start(mm, mmun_start,
-					    mmun_end, MMU_MIGRATE);
+	mmu_notifier_invalidate_range_start(mm, &range);
 	ptl = pmd_lock(mm, pmd);
 	if (unlikely(!pmd_same(*pmd, entry) || page_count(page) != 2)) {
 fail_putback:
 		spin_unlock(ptl);
-		mmu_notifier_invalidate_range_end(mm, mmun_start,
-						  mmun_end, MMU_MIGRATE);
+		mmu_notifier_invalidate_range_end(mm, &range);
 
 		/* Reverse changes made by migrate_page_copy() */
 		if (TestClearPageActive(new_page))

@@ -1879,8 +1880,7 @@ fail_putback:
 	page_remove_rmap(page);
 
 	spin_unlock(ptl);
-	mmu_notifier_invalidate_range_end(mm, mmun_start,
-					  mmun_end, MMU_MIGRATE);
+	mmu_notifier_invalidate_range_end(mm, &range);
 
 	/* Take an "isolate" reference and put new page on the LRU. */
 	get_page(new_page);
mm/mmu_notifier.c +63 -10

109 lines not on this page · raw message

mm/mprotect.c +10 -7
index 886405b..a178b22 100644
@@ -144,7 +144,9 @@ static inline unsigned long change_pmd_range(struct vm_area_struct *vma,
 	unsigned long next;
 	unsigned long pages = 0;
 	unsigned long nr_huge_updates = 0;
-	unsigned long mni_start = 0;
+	struct mmu_notifier_range range = {
+		.start = 0,
+	};
 
 	pmd = pmd_offset(pud, addr);
 	do {

@@ -155,10 +157,11 @@ static inline unsigned long change_pmd_range(struct vm_area_struct *vma,
 			continue;
 
 		/* invoke the mmu notifier if the pmd is populated */
-		if (!mni_start) {
-			mni_start = addr;
-			mmu_notifier_invalidate_range_start(mm, mni_start,
-							    end, MMU_MPROT);
+		if (!range.start) {
+			range.start = addr;
+			range.end = end;
+			range.event = MMU_MPROT;
+			mmu_notifier_invalidate_range_start(mm, &range);
 		}
 
 		if (pmd_trans_huge(*pmd)) {

@@ -185,8 +188,8 @@ static inline unsigned long change_pmd_range(struct vm_area_struct *vma,
 		pages += this_pages;
 	} while (pmd++, addr = next, addr != end);
 
-	if (mni_start)
-		mmu_notifier_invalidate_range_end(mm, mni_start, end, MMU_MPROT);
+	if (range.start)
+		mmu_notifier_invalidate_range_end(mm, &range);
 
 	if (nr_huge_updates)
 		count_vm_numa_events(NUMA_HUGE_PTE_UPDATES, nr_huge_updates);
mm/mremap.c +6 -8
index 6827d2f..83c5eed 100644
@@ -167,18 +167,17 @@ unsigned long move_page_tables(struct vm_area_struct *vma,
 		bool need_rmap_locks)
 {
 	unsigned long extent, next, old_end;
+	struct mmu_notifier_range range;
 	pmd_t *old_pmd, *new_pmd;
 	bool need_flush = false;
-	unsigned long mmun_start;	/* For mmu_notifiers */
-	unsigned long mmun_end;		/* For mmu_notifiers */
 
 	old_end = old_addr + len;
 	flush_cache_range(vma, old_addr, old_end);
 
-	mmun_start = old_addr;
-	mmun_end   = old_end;
-	mmu_notifier_invalidate_range_start(vma->vm_mm, mmun_start,
-					    mmun_end, MMU_MIGRATE);
+	range.start = old_addr;
+	range.end = old_end;
+	range.event = MMU_MIGRATE;
+	mmu_notifier_invalidate_range_start(vma->vm_mm, &range);
 
 	for (; old_addr < old_end; old_addr += extent, new_addr += extent) {
 		cond_resched();

@@ -229,8 +228,7 @@ unsigned long move_page_tables(struct vm_area_struct *vma,
 	if (likely(need_flush))
 		flush_tlb_range(vma, old_end-len, old_addr);
 
-	mmu_notifier_invalidate_range_end(vma->vm_mm, mmun_start,
-					  mmun_end, MMU_MIGRATE);
+	mmu_notifier_invalidate_range_end(vma->vm_mm, &range);
 
 	return len + old_addr - old_end;	/* how much done */
 }
mm/rmap.c +7 -8
index 0b67e7d..b8b8a60 100644
@@ -1302,15 +1302,14 @@ static int try_to_unmap_cluster(unsigned long cursor, unsigned int *mapcount,
 	spinlock_t *ptl;
 	struct page *page;
 	unsigned long address;
-	unsigned long mmun_start;	/* For mmu_notifiers */
-	unsigned long mmun_end;		/* For mmu_notifiers */
+	struct mmu_notifier_range range;
 	unsigned long end;
 	int ret = SWAP_AGAIN;
 	int locked_vma = 0;
-	enum mmu_event event = MMU_MIGRATE;
 
+	range.event = MMU_MIGRATE;
 	if (flags & TTU_MUNLOCK)
-		event = MMU_MUNLOCK;
+		range.event = MMU_MUNLOCK;
 
 	address = (vma->vm_start + cursor) & CLUSTER_MASK;
 	end = address + CLUSTER_SIZE;

@@ -1323,9 +1322,9 @@ static int try_to_unmap_cluster(unsigned long cursor, unsigned int *mapcount,
 	if (!pmd)
 		return ret;
 
-	mmun_start = address;
-	mmun_end   = end;
-	mmu_notifier_invalidate_range_start(mm, mmun_start, mmun_end, event);
+	range.start = address;
+	range.end = end;
+	mmu_notifier_invalidate_range_start(mm, &range);
 
 	/*
 	 * If we can acquire the mmap_sem for read, and vma is VM_LOCKED,

@@ -1390,7 +1389,7 @@ static int try_to_unmap_cluster(unsigned long cursor, unsigned int *mapcount,
 		(*mapcount)--;
 	}
 	pte_unmap_unlock(pte - 1, ptl);
-	mmu_notifier_invalidate_range_end(mm, mmun_start, mmun_end, event);
+	mmu_notifier_invalidate_range_end(mm, &range);
 	if (locked_vma)
 		up_read(&vma->vm_mm->mmap_sem);
 	return ret;
virt/kvm/kvm_main.c +3 -7
index 0ed3e88..8d8c2ce 100644
@@ -318,9 +318,7 @@ static void kvm_mmu_notifier_change_pte(struct mmu_notifier *mn,
 
 static void kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
 						    struct mm_struct *mm,
-						    unsigned long start,
-						    unsigned long end,
-						    enum mmu_event event)
+						    const struct mmu_notifier_range *range)
 {
 	struct kvm *kvm = mmu_notifier_to_kvm(mn);
 	int need_tlb_flush = 0, idx;

@@ -333,7 +331,7 @@ static void kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
 	 * count is also read inside the mmu_lock critical section.
 	 */
 	kvm->mmu_notifier_count++;
-	need_tlb_flush = kvm_unmap_hva_range(kvm, start, end);
+	need_tlb_flush = kvm_unmap_hva_range(kvm, range->start, range->end);
 	need_tlb_flush |= kvm->tlbs_dirty;
 	/* we've to flush the tlb before the pages can be freed */
 	if (need_tlb_flush)

@@ -345,9 +343,7 @@ static void kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
 
 static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,
 						  struct mm_struct *mm,
-						  unsigned long start,
-						  unsigned long end,
-						  enum mmu_event event)
+						  const struct mmu_notifier_range *range)
 {
 	struct kvm *kvm = mmu_notifier_to_kvm(mn);
Signature · 1 line
1.9.3

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#14 lore raw
Quoted 289 lines · Jerome Glisse
Quoted 112 lines · Shachar Raindel
Quoted 37 lines

-----Original Message-----
From: Jerome Glisse [mailto:[email protected]]
Sent: Wednesday, September 10, 2014 11:15 PM
To: Shachar Raindel
Cc: Haggai Eran; [email protected]; Sagi Grimberg
Subject: Re: [PATCH v1 for-next 06/16] IB/core: Implement support for MMU notifiers regarding on demand paging regions

Quoted 7 lines · Shachar Raindel
Quoted 7 lines

-----Original Message-----
From: Jerome Glisse [mailto:[email protected]]
Sent: Tuesday, September 09, 2014 6:37 PM
To: Shachar Raindel
Cc: 1404377069-20585-1-git-send-email-haggaie-VPRAkNaXOzVWk0Htik3J/[email protected]; Haggai

Eran;

Quoted 2 lines
Quoted 2 lines

[email protected]; Jerome Glisse; Sagi Grimberg
Subject: Re: [PATCH v1 for-next 06/16] IB/core: Implement support

for

Quoted 11 lines
Quoted 11 lines

MMU notifiers regarding on demand paging regions

Quoted 8 lines · Shachar Raindel

Hi,

Quoted 6 lines

-----Original Message-----
From: Jerome Glisse [mailto:[email protected]]
Sent: Thursday, September 04, 2014 11:25 PM
To: Haggai Eran; [email protected]
Cc: Shachar Raindel; Sagi Grimberg
Subject: Re: [PATCH v1 for-next 06/16] IB/core: Implement

support

Quoted 3 lines
Quoted 3 lines

for

Quoted 2 lines
Quoted 2 lines

MMU notifiers regarding on demand paging regions

<SNIP>

Quoted 55 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

Sadly, taking mmap_sem in read-only mode does not prevent all

possible

Quoted 2 lines
Quoted 2 lines

invalidations from happening.

Quoted 1 line

For example, a call to madvise requesting MADVISE_DONTNEED will

lock

Quoted 1 line
Quoted 1 line

the mmap_sem for reading only, allowing a notifier to run in

parallel to

Quoted 1 line
Quoted 1 line

the MR registration As a result, the following sequence of events

could

Quoted 12 lines
Quoted 12 lines

happen:

Quoted 11 lines

Thread 1: | Thread 2
--------------------------------+-------------------------

madvise                         |
down_read(mmap_sem)             |
notifier_start                  |
                                |   down_read(mmap_sem)
                                |   register_mr
notifier_end                    |
reduce_mr_notifiers_count       |
Quoted 1 line
Quoted 1 line
Quoted 1 line

The end result of this sequence is an mr with running notifiers

count

Quoted 2 lines
Quoted 2 lines

of -1, which is bad.

Quoted 1 line

The current workaround is to avoid decreasing the notifiers count

if

Quoted 2 lines
Quoted 2 lines

it is zero, which can cause other issues.

Quoted 1 line

The proper fix would be to prevent notifiers from running in

parallel

Quoted 6 lines
Quoted 6 lines

to registration. For this, taking mmap_sem in write mode might be sufficient, but we are not sure about this.

Quoted 1 line

We will be happy to hear additional input on this subject, to make

sure we got it covered properly.

So in HMM i solve this by having a struct allocated in the start

range

Quoted 2 lines
Quoted 2 lines

callback
and the end range callback just ignore things when it can not find

the

Quoted 4 lines
Quoted 2 lines

matching
struct.

This kind of mechanism sounds like it has a bigger risk for

deadlocking

Quoted 5 lines

the system, causing an OOM kill without a real need or significantly slowing down the system.
If you are doing non-atomic memory allocations, you can deadlock the system by requesting memory in the swapper flow.
Even if you are doing atomic memory allocations, you need to handle

the

Quoted 4 lines

case of failing allocation, the solution to which is unclear to me. If you are using a pre-allocated pool, what are you doing when you run out of available entries in the pool? If you are blocking until some entries free up, what guarantees you that this will not cause a

Quoted 5 lines

deadlock?

So i am using a fixed pool and when it runs out it block in start callback
until one is freed.

This sounds scary. You now create a possible locking dependency between two code flows which could have run in parallel. This can cause circular locking bugs, from code which functioned properly until now. For example, assume code with a single lock, and the following code paths:

Code 1:
notify_start()
lock()
unlock()
notify_end()

Quoted 7 lines

Code 2:
lock()
notify_start()
... (no locking)
notify_end()
unlock()

This can not happen because all lock taken before notify_start() are never taken after it and all lock taken inside a start/end section are never hold accross a notify_start() callback.

Quoted 18 lines

This code can now create the following deadlock:

Thread 1: | Thread 2:
-----------------+-----------------------------------

notify_start()   |
                 | lock()
lock() - blocking|
                 | notify_start() - blocking for slot
Quoted 3 lines

But as i said i have a patch to use the stack that will
solve this and avoid a pool.

Quoted 5 lines

How are you allocating from the stack an entry which you need to keep alive until another function is called? You can't allocate the entry on the notify_start stack, so you must do this in all of the call points to the mmu_notifiers. Given the notifiers listener subscription pattern, this seems like something which is not practical.

Yes the patch add a struct in each callsite of mmu_notifier_invalidate_range as in all case both start and end are call from same function. The only draw back is that it increase stack consumption in some of those callsite (not all). I attach the patch i am thinking of (it is untested) but idea is that through two new helper function user of mmu_notifier can query active invalid range and synchronize with those (also require some code in the range_start() callback).

Quoted 133 lines
Quoted 101 lines
Quoted 8 lines
Quoted 8 lines

That being said when registering the mmu_notifier you need 2 things, first you
need a pin on the mm (either mm is current ie current->mm or you

took a

Quoted 4 lines
Quoted 4 lines

reference
on it). Second you need to that the mmap smemaphore in write mode so that
no concurrent mmap/munmap/madvise can happen. By doing that you

protect

Quoted 8 lines
Quoted 8 lines

yourself
from concurrent range_start/range_end that can happen and that does matter.
The only concurrent range_start/end that can happen is through file invalidation
which is fine because subsequent page fault will go through the file layer and
bring back page or return error (if file was truncated for

instance).

Quoted 7 lines

Sadly, this is not sufficient for our use case. We are registering a single MMU notifier handler, and broadcast the notifications to all relevant listeners, which are stored in an interval tree.

Each listener represents a section of the address space that has been exposed to the network. Such implementation allows us to limit the

impact

Quoted 1 line

of invalidations, and only block racing page faults to the affected

areas.

Quoted 2 lines

Each of the listeners maintain a counter of the number of

invalidate_range

Quoted 1 line

notifications that are currently affecting it. The counter is

increased

Quoted 1 line

for each invalidate_range_start callback received, and decrease for

each

Quoted 3 lines

invalidate_range_end callback received. If we add a listener to the interval tree after the invalidate_range_start callback happened, but before the invalidate_range_end callback happened, it will decrease

the

Quoted 4 lines

counter, reaching negative numbers and breaking the logic.

The mmu_notifiers registration code avoid such issues by taking all relevant locks on the MM. This effectively blocks all possible

notifiers

Quoted 1 line

from happening when registering a new notifier. Sadly, this function

is

Quoted 8 lines

not exported for modules to use it.

Our options at the moment are:
- Use a tracking mechanism similar to what HMM uses, alongside the challenges involved in allocating memory from notifiers

- Use a per-process counter for invalidations, causing a possible performance degradation. This can possibly be used as a fallback to

the

Quoted 4 lines

first option (i.e. have a pool of X notifier identifiers, once it is full, increase/decrease a per-MM counter)

- Export the mm_take_all_locks function for modules. This will allow

us

Quoted 1 line

to lock the MM when adding a new listener.

I was not clear enough, you need to take the mmap_sem in write mode accross
mmu_notifier_register(). This is only to partialy solve your issue that if
a mmu_notifier is already register for the mm you are trying to registering
against then there is a chance for you to be inside an active range_start/
range_end section which would lead to invalid counter inside your tracking
structure. But, sadly, taking mmap_sem in write mode is not enough as file
invalidation might still happen concurrently so you will need to make sure
you invalidation counters does not go negative but from page fault point of
view you will be fine because the page fault will synchronize through the
pagecache. So scenario (A and B are to anonymous overlapping address range) :

  APP_TOTO_RDMA_THREAD           |  APP_TOTO_SOME_OTHER_THREAD
                                 |  mmu_notifier_invalidate_range_start(A)
  odp_register()                 |
    down_read(mmap_sem)          |
    mmu_notifier_register()      |
    up_read(mmap_sem)            |
  odp_add_new_region(B)          |
  odp_page_fault(B)              |
    down_read(mmap_sem)          |
    ...                          |
    up_read(mmap_sem)            |
                                 |  mmu_notifier_invalidate_range_end(A)
Quoted 13 lines

The odp_page_fault(B) might see invalid cpu page table but you have no idea
about it because you registered after the range_start(). But if you take the
mmap_sem in write mode then the only case where you might still have this
scenario is if A and B are range of a file backed vma and that the file is
undergoing some change (most likely truncation). But the file case is fine
because the odp_page_fault() will go through the pagecache which is properly
synchronize against the current range invalidation.

Specifically, if you call mmu_notifier_register you are OK and the above scenario will not happen. You are supposed to hold mmap_sem for writing, and mmu_notifier_register is calling mm_take_all_locks, which guarantees no racing notifier during the registration step.

However, we want to dynamically add sub-notifiers in our code. Each will get notified only about invalidations touching a specific sub-sections of the address space. To avoid providing unneeded notifications, we use an interval tree that filters only the needed notifications.
When adding entries to the interval tree, we cannot lock the mm to prevent any racing invalidations. As such, we might end up in a case where a newly registered memory region will get a "notify_end" call without the relevant "notify_start". Even if we prevent the value from dropping below zero, it means we can cause data corruption. For example, if we have another notifier running after the MR registers, which is due to munmap, but we get first the notify_end of the previous notifier for which we didn't see the notify_start.

Quoted 7 lines
Quoted 3 lines

The solution we are coming up with now is using a global counter of running invalidations for new regions allocated. When the global counter is at zero, we can safely switch to the region local invalidations counter.

Yes i fully understood that design but as i said this kind of broken and this is what the attached patch try to address as HMM have the same issue of having to track all active invalidation range.

I should also stress that my point was that you need mmap_sem in write mode while registering specificaly because otherwise there is a risk that your global mmu notifier counter is missing a running invalidate range and thus there is a window for a one of your new struct that mirror a range to be registered and to use invalid pages (pages that are about to be freed). So this is very important to hold the mmap_sem in write mode while you are registering and before you allow any of your region to be register.

As i said i was not talking about the general case after registering the mmu notifier.

Quoted 1,212 lines
Quoted 44 lines
Quoted 22 lines

Now for the the general case outside of mmu_notifier_register() HMM also track
active invalidation range to avoid page faulting into those range as we can not
trust the cpu page table for as long as the range invalidation is on going.

Quoted 2 lines
Quoted 2 lines

So as long as you hold the mmap_sem in write mode you should not

worry

Quoted 2 lines
Quoted 2 lines

about
concurrent range_start/range_end (well they might happen but only

for

Quoted 4 lines
Quoted 3 lines

file
backed vma).

Quoted 6 lines
Quoted 1 line

Sadly, the mmap_sem is not enough to protect us :(.

This is enough like i explain above, but i am only talking about the mmu notifier registration. For the general case once you register you only need to take the mmap_sem in read mode during page fault.

I think we are not broadcasting on the same wavelength here. The issue I'm worried about is of adding a sub-area to our tracking system. It is built quite differently from how HMM is built, we are defining areas to track a-priori, and later on account how many notifiers are blocking page-faults for each area. You are keeping track of the active notifiers, and check each page fault against your notifier list. This difference makes for different locking needs.

Quoted 7 lines
Quoted 6 lines
Quoted 4 lines

Given that you face the same issue as i have with the range_start/range_end i
will stich up a patch to make it easier to track those.

That would be nice, especially if we could easily integrate it into

our

Quoted 15 lines
Quoted 6 lines
Quoted 1 line

code and reduce the code size.

Yes it's a "small modification" to the mmu_notifier api, i have been side
tracked on other thing. But i will have it soon.

Being side tracked is a well-known professional risk in our line of work ;)

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

Cheers,
Jérôme

From 037195e49fbed468d16b78f0364fe302bc732d12 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Glisse?= <[email protected]>
Date: Thu, 11 Sep 2014 11:22:12 -0400
Subject: [PATCH] mmu_notifier: keep track of active invalidation ranges
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The mmu_notifier_invalidate_range_start() and mmu_notifier_invalidate_range_end() can be considered as forming an "atomic" section for the cpu page table update point of view. Between this two function the cpu page table content is unreliable for the affected range of address.

Current user such as kvm need to know when they can trust a the content of the cpu page table. This becomes even more important to new users of the mmu_notifier api (such as HMM or ODP).

This patch use a structure define at all call site to invalidate_range_start() that is added to a list for the duration of the invalidation. It adds two new helpers to allow querying if a range is being invalidated or to wait for a range to become valid.

This two new function does not provide strong synchronization but are intended to be use as helper. User of the mmu_notifier must also synchronize with themself inside their range_start() and range_end() callback.

Signed-off-by Jérôme Glisse <[email protected]>
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 13 +++---
 drivers/iommu/amd_iommu_v2.c            |  8 +---
 drivers/misc/sgi-gru/grutlbpurge.c      | 15 +++----
 drivers/xen/gntdev.c                    |  8 ++--
 fs/proc/task_mmu.c                      | 12 +++--
 include/linux/mmu_notifier.h            | 55 ++++++++++++-----------
 mm/fremap.c                             |  8 +++-
 mm/huge_memory.c                        | 78 ++++++++++++++-------------------
 mm/hugetlb.c                            | 49 +++++++++++----------
 mm/memory.c                             | 73 ++++++++++++++++--------------
 mm/migrate.c                            | 16 +++----
 mm/mmu_notifier.c                       | 73 +++++++++++++++++++++++++-----
 mm/mprotect.c                           | 17 ++++---
 mm/mremap.c                             | 14 +++---
 mm/rmap.c                               | 15 +++----
 virt/kvm/kvm_main.c                     | 10 ++---
 16 files changed, 256 insertions(+), 208 deletions(-)
drivers/gpu/drm/i915/i915_gem_userptr.c +6 -7
index a13307d..373ffbb 100644
@@ -123,26 +123,25 @@ restart:
 
 static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 						       struct mm_struct *mm,
-						       unsigned long start,
-						       unsigned long end,
-						       enum mmu_event event)
+						       const struct mmu_notifier_range *range)
 {
 	struct i915_mmu_notifier *mn = container_of(_mn, struct i915_mmu_notifier, mn);
 	struct interval_tree_node *it = NULL;
-	unsigned long next = start;
+	unsigned long next = range->start;
 	unsigned long serial = 0;
+	/* interval ranges are inclusive, but invalidate range is exclusive */
+	unsigned long end = range.end - 1;
 
-	end--; /* interval ranges are inclusive, but invalidate range is exclusive */
 	while (next < end) {
 		struct drm_i915_gem_object *obj = NULL;
 
 		spin_lock(&mn->lock);
 		if (mn->has_linear)
-			it = invalidate_range__linear(mn, mm, start, end);
+			it = invalidate_range__linear(mn, mm, range->start, end);
 		else if (serial == mn->serial)
 			it = interval_tree_iter_next(it, next, end);
 		else
-			it = interval_tree_iter_first(&mn->objects, start, end);
+			it = interval_tree_iter_first(&mn->objects, range->start, end);
 		if (it != NULL) {
 			obj = container_of(it, struct i915_mmu_object, it)->obj;
 			drm_gem_object_reference(&obj->base);
drivers/iommu/amd_iommu_v2.c +2 -6
index 9a6b837..5945300 100644
@@ -419,9 +419,7 @@ static void mn_invalidate_page(struct mmu_notifier *mn,
 
 static void mn_invalidate_range_start(struct mmu_notifier *mn,
 				      struct mm_struct *mm,
-				      unsigned long start,
-				      unsigned long end,
-				      enum mmu_event event)
+				      const struct mmu_notifier_range *range)
 {
 	struct pasid_state *pasid_state;
 	struct device_state *dev_state;

@@ -442,9 +440,7 @@ static void mn_invalidate_range_start(struct mmu_notifier *mn,
 
 static void mn_invalidate_range_end(struct mmu_notifier *mn,
 				    struct mm_struct *mm,
-				    unsigned long start,
-				    unsigned long end,
-				    enum mmu_event event)
+				    const struct mmu_notifier_range *range)
 {
 	struct pasid_state *pasid_state;
 	struct device_state *dev_state;
drivers/misc/sgi-gru/grutlbpurge.c +7 -8
index e67fed1..44b41b7 100644
@@ -221,8 +221,7 @@ void gru_flush_all_tlb(struct gru_state *gru)
  */
 static void gru_invalidate_range_start(struct mmu_notifier *mn,
 				       struct mm_struct *mm,
-				       unsigned long start, unsigned long end,
-				       enum mmu_event event)
+				       const struct mmu_notifier_range *range)
 {
 	struct gru_mm_struct *gms = container_of(mn, struct gru_mm_struct,
 						 ms_notifier);

@@ -230,14 +229,13 @@ static void gru_invalidate_range_start(struct mmu_notifier *mn,
 	STAT(mmu_invalidate_range);
 	atomic_inc(&gms->ms_range_active);
 	gru_dbg(grudev, "gms %p, start 0x%lx, end 0x%lx, act %d\n", gms,
-		start, end, atomic_read(&gms->ms_range_active));
-	gru_flush_tlb_range(gms, start, end - start);
+		range->start, range->end, atomic_read(&gms->ms_range_active));
+	gru_flush_tlb_range(gms, range->start, range->end - range->start);
 }
 
 static void gru_invalidate_range_end(struct mmu_notifier *mn,
-				     struct mm_struct *mm, unsigned long start,
-				     unsigned long end,
-				     enum mmu_event event)
+				     struct mm_struct *mm,
+				     const struct mmu_notifier_range *range)
 {
 	struct gru_mm_struct *gms = container_of(mn, struct gru_mm_struct,
 						 ms_notifier);

@@ -246,7 +244,8 @@ static void gru_invalidate_range_end(struct mmu_notifier *mn,
 	(void)atomic_dec_and_test(&gms->ms_range_active);
 
 	wake_up_all(&gms->ms_wait_queue);
-	gru_dbg(grudev, "gms %p, start 0x%lx, end 0x%lx\n", gms, start, end);
+	gru_dbg(grudev, "gms %p, start 0x%lx, end 0x%lx\n", gms,
+		range->start, range->end);
 }
 
 static void gru_invalidate_page(struct mmu_notifier *mn, struct mm_struct *mm,
drivers/xen/gntdev.c +3 -5
index fe9da94..51f9188 100644
@@ -428,19 +428,17 @@ static void unmap_if_in_range(struct grant_map *map,
 
 static void mn_invl_range_start(struct mmu_notifier *mn,
 				struct mm_struct *mm,
-				unsigned long start,
-				unsigned long end,
-				enum mmu_event event)
+				const struct mmu_notifier_range *range)
 {
 	struct gntdev_priv *priv = container_of(mn, struct gntdev_priv, mn);
 	struct grant_map *map;
 
 	spin_lock(&priv->lock);
 	list_for_each_entry(map, &priv->maps, next) {
-		unmap_if_in_range(map, start, end);
+		unmap_if_in_range(map, range->start, range->end);
 	}
 	list_for_each_entry(map, &priv->freeable_maps, next) {
-		unmap_if_in_range(map, start, end);
+		unmap_if_in_range(map, range->start, range->end);
 	}
 	spin_unlock(&priv->lock);
 }
fs/proc/task_mmu.c +8 -4
index 0ddb975..532a230 100644
@@ -828,10 +828,15 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
 			.mm = mm,
 			.private = &cp,
 		};
+		struct mmu_notifier_range range = {
+			.start = 0,
+			.end = -1UL,
+			.event = MMU_ISDIRTY,
+		};
+
 		down_read(&mm->mmap_sem);
 		if (type == CLEAR_REFS_SOFT_DIRTY)
-			mmu_notifier_invalidate_range_start(mm, 0,
-							    -1, MMU_ISDIRTY);
+			mmu_notifier_invalidate_range_start(mm, &range);
 		for (vma = mm->mmap; vma; vma = vma->vm_next) {
 			cp.vma = vma;
 			if (is_vm_hugetlb_page(vma))

@@ -859,8 +864,7 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
 					&clear_refs_walk);
 		}
 		if (type == CLEAR_REFS_SOFT_DIRTY)
-			mmu_notifier_invalidate_range_end(mm, 0,
-							  -1, MMU_ISDIRTY);
+			mmu_notifier_invalidate_range_end(mm, &range);
 		flush_tlb_mm(mm);
 		up_read(&mm->mmap_sem);
 		mmput(mm);
include/linux/mmu_notifier.h +29 -26
index 94f6890..f4a2a74 100644
@@ -69,6 +69,13 @@ enum mmu_event {
 	MMU_WRITE_PROTECT,
 };
 
+struct mmu_notifier_range {
+	struct list_head list;
+	unsigned long start;
+	unsigned long end;
+	enum mmu_event event;
+};
+
 #ifdef CONFIG_MMU_NOTIFIER
 
 /*

@@ -82,6 +89,12 @@ struct mmu_notifier_mm {
 	struct hlist_head list;
 	/* to serialize the list modifications and hlist_unhashed */
 	spinlock_t lock;
+	/* List of all active range invalidations. */
+	struct list_head ranges;
+	/* Number of active range invalidations. */
+	int nranges;
+	/* For threads waiting on range invalidations. */
+	wait_queue_head_t wait_queue;
 };
 
 struct mmu_notifier_ops {

@@ -199,14 +212,10 @@ struct mmu_notifier_ops {
 	 */
 	void (*invalidate_range_start)(struct mmu_notifier *mn,
 				       struct mm_struct *mm,
-				       unsigned long start,
-				       unsigned long end,
-				       enum mmu_event event);
+				       const struct mmu_notifier_range *range);
 	void (*invalidate_range_end)(struct mmu_notifier *mn,
 				     struct mm_struct *mm,
-				     unsigned long start,
-				     unsigned long end,
-				     enum mmu_event event);
+				     const struct mmu_notifier_range *range);
 };
 
 /*

@@ -252,13 +261,15 @@ extern void __mmu_notifier_invalidate_page(struct mm_struct *mm,
 					  unsigned long address,
 					  enum mmu_event event);
 extern void __mmu_notifier_invalidate_range_start(struct mm_struct *mm,
-						  unsigned long start,
-						  unsigned long end,
-						  enum mmu_event event);
+						  struct mmu_notifier_range *range);
 extern void __mmu_notifier_invalidate_range_end(struct mm_struct *mm,
-						unsigned long start,
-						unsigned long end,
-						enum mmu_event event);
+						struct mmu_notifier_range *range);
+extern bool mmu_notifier_range_is_valid(struct mm_struct *mm,
+					unsigned long start,
+					unsigned long end);
+extern void mmu_notifier_range_wait_valid(struct mm_struct *mm,
+					  unsigned long start,
+					  unsigned long end);
 
 static inline void mmu_notifier_release(struct mm_struct *mm)
 {

@@ -300,21 +311,17 @@ static inline void mmu_notifier_invalidate_page(struct mm_struct *mm,
 }
 
 static inline void mmu_notifier_invalidate_range_start(struct mm_struct *mm,
-						       unsigned long start,
-						       unsigned long end,
-						       enum mmu_event event)
+						       struct mmu_notifier_range *range)
 {
 	if (mm_has_notifiers(mm))
-		__mmu_notifier_invalidate_range_start(mm, start, end, event);
+		__mmu_notifier_invalidate_range_start(mm, range);
 }
 
 static inline void mmu_notifier_invalidate_range_end(struct mm_struct *mm,
-						     unsigned long start,
-						     unsigned long end,
-						     enum mmu_event event)
+						     struct mmu_notifier_range *range)
 {
 	if (mm_has_notifiers(mm))
-		__mmu_notifier_invalidate_range_end(mm, start, end, event);
+		__mmu_notifier_invalidate_range_end(mm, range);
 }
 
 static inline void mmu_notifier_mm_init(struct mm_struct *mm)

@@ -406,16 +413,12 @@ static inline void mmu_notifier_invalidate_page(struct mm_struct *mm,
 }
 
 static inline void mmu_notifier_invalidate_range_start(struct mm_struct *mm,
-						       unsigned long start,
-						       unsigned long end,
-						       enum mmu_event event)
+						       struct mmu_notifier_range *range)
 {
 }
 
 static inline void mmu_notifier_invalidate_range_end(struct mm_struct *mm,
-						     unsigned long start,
-						     unsigned long end,
-						     enum mmu_event event)
+						     struct mmu_notifier_range *range)
 {
 }
mm/fremap.c +6 -2
index 37b2904..03a5ddc 100644
@@ -148,6 +148,7 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size,
 	int err = -EINVAL;
 	int has_write_lock = 0;
 	vm_flags_t vm_flags = 0;
+	struct mmu_notifier_range range;
 
 	pr_warn_once("%s (%d) uses deprecated remap_file_pages() syscall. "
 			"See Documentation/vm/remap_file_pages.txt.\n",

@@ -258,9 +259,12 @@ get_write_lock:
 		vma->vm_flags = vm_flags;
 	}
 
-	mmu_notifier_invalidate_range_start(mm, start, start + size, MMU_MUNMAP);
+	range.start = start;
+	range.end = start + size;
+	range.event = MMU_MUNMAP;
+	mmu_notifier_invalidate_range_start(mm, &range);
 	err = vma->vm_ops->remap_pages(vma, start, size, pgoff);
-	mmu_notifier_invalidate_range_end(mm, start, start + size, MMU_MUNMAP);
+	mmu_notifier_invalidate_range_end(mm, &range);
 
 	/*
 	 * We can't clear VM_NONLINEAR because we'd have to do
mm/huge_memory.c +33 -45
index e3efba5..4b116dd 100644
@@ -988,8 +988,7 @@ static int do_huge_pmd_wp_page_fallback(struct mm_struct *mm,
 	pmd_t _pmd;
 	int ret = 0, i;
 	struct page **pages;
-	unsigned long mmun_start;	/* For mmu_notifiers */
-	unsigned long mmun_end;		/* For mmu_notifiers */
+	struct mmu_notifier_range range;
 
 	pages = kmalloc(sizeof(struct page *) * HPAGE_PMD_NR,
 			GFP_KERNEL);

@@ -1027,10 +1026,10 @@ static int do_huge_pmd_wp_page_fallback(struct mm_struct *mm,
 		cond_resched();
 	}
 
-	mmun_start = haddr;
-	mmun_end   = haddr + HPAGE_PMD_SIZE;
-	mmu_notifier_invalidate_range_start(mm, mmun_start, mmun_end,
-					    MMU_MIGRATE);
+	range.start = haddr;
+	range.end = haddr + HPAGE_PMD_SIZE;
+	range.event = MMU_MIGRATE;
+	mmu_notifier_invalidate_range_start(mm, &range);
 
 	ptl = pmd_lock(mm, pmd);
 	if (unlikely(!pmd_same(*pmd, orig_pmd)))

@@ -1064,8 +1063,7 @@ static int do_huge_pmd_wp_page_fallback(struct mm_struct *mm,
 	page_remove_rmap(page);
 	spin_unlock(ptl);
 
-	mmu_notifier_invalidate_range_end(mm, mmun_start,
-					  mmun_end, MMU_MIGRATE);
+	mmu_notifier_invalidate_range_end(mm, &range);
 
 	ret |= VM_FAULT_WRITE;
 	put_page(page);

@@ -1075,8 +1073,7 @@ out:
 
 out_free_pages:
 	spin_unlock(ptl);
-	mmu_notifier_invalidate_range_end(mm, mmun_start,
-					  mmun_end, MMU_MIGRATE);
+	mmu_notifier_invalidate_range_end(mm, &range);
 	for (i = 0; i < HPAGE_PMD_NR; i++) {
 		memcg = (void *)page_private(pages[i]);
 		set_page_private(pages[i], 0);

@@ -1095,8 +1092,7 @@ int do_huge_pmd_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
 	struct page *page = NULL, *new_page;
 	struct mem_cgroup *memcg;
 	unsigned long haddr;
-	unsigned long mmun_start;	/* For mmu_notifiers */
-	unsigned long mmun_end;		/* For mmu_notifiers */
+	struct mmu_notifier_range range;
 
 	ptl = pmd_lockptr(mm, pmd);
 	VM_BUG_ON(!vma->anon_vma);

@@ -1166,10 +1162,10 @@ alloc:
 		copy_user_huge_page(new_page, page, haddr, vma, HPAGE_PMD_NR);
 	__SetPageUptodate(new_page);
 
-	mmun_start = haddr;
-	mmun_end   = haddr + HPAGE_PMD_SIZE;
-	mmu_notifier_invalidate_range_start(mm, mmun_start, mmun_end,
-					    MMU_MIGRATE);
+	range.start = haddr;
+	range.end = haddr + HPAGE_PMD_SIZE;
+	range.event = MMU_MIGRATE;
+	mmu_notifier_invalidate_range_start(mm, &range);
 
 	spin_lock(ptl);
 	if (page)

@@ -1201,8 +1197,7 @@ alloc:
 	}
 	spin_unlock(ptl);
 out_mn:
-	mmu_notifier_invalidate_range_end(mm, mmun_start,
-					  mmun_end, MMU_MIGRATE);
+	mmu_notifier_invalidate_range_end(mm, &range);
 out:
 	return ret;
 out_unlock:

@@ -1633,12 +1628,12 @@ static int __split_huge_page_splitting(struct page *page,
 	spinlock_t *ptl;
 	pmd_t *pmd;
 	int ret = 0;
-	/* For mmu_notifiers */
-	const unsigned long mmun_start = address;
-	const unsigned long mmun_end   = address + HPAGE_PMD_SIZE;
+	struct mmu_notifier_range range;
 
-	mmu_notifier_invalidate_range_start(mm, mmun_start,
-					    mmun_end, MMU_HSPLIT);
+	range.start = address;
+	range.end = address + HPAGE_PMD_SIZE;
+	range.event = MMU_HSPLIT;
+	mmu_notifier_invalidate_range_start(mm, &range);
 	pmd = page_check_address_pmd(page, mm, address,
 			PAGE_CHECK_ADDRESS_PMD_NOTSPLITTING_FLAG, &ptl);
 	if (pmd) {

@@ -1653,8 +1648,7 @@ static int __split_huge_page_splitting(struct page *page,
 		ret = 1;
 		spin_unlock(ptl);
 	}
-	mmu_notifier_invalidate_range_end(mm, mmun_start,
-					  mmun_end, MMU_HSPLIT);
+	mmu_notifier_invalidate_range_end(mm, &range);
 
 	return ret;
 }

@@ -2434,8 +2428,7 @@ static void collapse_huge_page(struct mm_struct *mm,
 	int isolated;
 	unsigned long hstart, hend;
 	struct mem_cgroup *memcg;
-	unsigned long mmun_start;	/* For mmu_notifiers */
-	unsigned long mmun_end;		/* For mmu_notifiers */
+	struct mmu_notifier_range range;
 
 	VM_BUG_ON(address & ~HPAGE_PMD_MASK);
 

@@ -2475,10 +2468,10 @@ static void collapse_huge_page(struct mm_struct *mm,
 	pte = pte_offset_map(pmd, address);
 	pte_ptl = pte_lockptr(mm, pmd);
 
-	mmun_start = address;
-	mmun_end   = address + HPAGE_PMD_SIZE;
-	mmu_notifier_invalidate_range_start(mm, mmun_start,
-					    mmun_end, MMU_MIGRATE);
+	range.start = address;
+	range.end = address + HPAGE_PMD_SIZE;
+	range.event = MMU_MIGRATE;
+	mmu_notifier_invalidate_range_start(mm, &range);
 	pmd_ptl = pmd_lock(mm, pmd); /* probably unnecessary */
 	/*
 	 * After this gup_fast can't run anymore. This also removes

@@ -2488,8 +2481,7 @@ static void collapse_huge_page(struct mm_struct *mm,
 	 */
 	_pmd = pmdp_clear_flush(vma, address, pmd);
 	spin_unlock(pmd_ptl);
-	mmu_notifier_invalidate_range_end(mm, mmun_start,
-					  mmun_end, MMU_MIGRATE);
+	mmu_notifier_invalidate_range_end(mm, &range);
 
 	spin_lock(pte_ptl);
 	isolated = __collapse_huge_page_isolate(vma, address, pte);

@@ -2872,36 +2864,32 @@ void __split_huge_page_pmd(struct vm_area_struct *vma, unsigned long address,
 	struct page *page;
 	struct mm_struct *mm = vma->vm_mm;
 	unsigned long haddr = address & HPAGE_PMD_MASK;
-	unsigned long mmun_start;	/* For mmu_notifiers */
-	unsigned long mmun_end;		/* For mmu_notifiers */
+	struct mmu_notifier_range range;
 
 	BUG_ON(vma->vm_start > haddr || vma->vm_end < haddr + HPAGE_PMD_SIZE);
 
-	mmun_start = haddr;
-	mmun_end   = haddr + HPAGE_PMD_SIZE;
+	range.start = haddr;
+	range.end = haddr + HPAGE_PMD_SIZE;
+	range.event = MMU_MIGRATE;
 again:
-	mmu_notifier_invalidate_range_start(mm, mmun_start,
-					    mmun_end, MMU_MIGRATE);
+	mmu_notifier_invalidate_range_start(mm, &range);
 	ptl = pmd_lock(mm, pmd);
 	if (unlikely(!pmd_trans_huge(*pmd))) {
 		spin_unlock(ptl);
-		mmu_notifier_invalidate_range_end(mm, mmun_start,
-						  mmun_end, MMU_MIGRATE);
+		mmu_notifier_invalidate_range_end(mm, &range);
 		return;
 	}
 	if (is_huge_zero_pmd(*pmd)) {
 		__split_huge_zero_page_pmd(vma, haddr, pmd);
 		spin_unlock(ptl);
-		mmu_notifier_invalidate_range_end(mm, mmun_start,
-						  mmun_end, MMU_MIGRATE);
+		mmu_notifier_invalidate_range_end(mm, &range);
 		return;
 	}
 	page = pmd_page(*pmd);
 	VM_BUG_ON_PAGE(!page_count(page), page);
 	get_page(page);
 	spin_unlock(ptl);
-	mmu_notifier_invalidate_range_end(mm, mmun_start,
-					  mmun_end, MMU_MIGRATE);
+	mmu_notifier_invalidate_range_end(mm, &range);
 
 	split_huge_page(page);
mm/hugetlb.c +25 -24
index ae98b53..6484793 100644
@@ -2551,17 +2551,16 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
 	int cow;
 	struct hstate *h = hstate_vma(vma);
 	unsigned long sz = huge_page_size(h);
-	unsigned long mmun_start;	/* For mmu_notifiers */
-	unsigned long mmun_end;		/* For mmu_notifiers */
+	struct mmu_notifier_range range;
 	int ret = 0;
 
 	cow = (vma->vm_flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
 
-	mmun_start = vma->vm_start;
-	mmun_end = vma->vm_end;
+	range.start = vma->vm_start;
+	range.end = vma->vm_end;
+	range.event = MMU_MIGRATE;
 	if (cow)
-		mmu_notifier_invalidate_range_start(src, mmun_start,
-						    mmun_end, MMU_MIGRATE);
+		mmu_notifier_invalidate_range_start(src, &range);
 
 	for (addr = vma->vm_start; addr < vma->vm_end; addr += sz) {
 		spinlock_t *src_ptl, *dst_ptl;

@@ -2612,8 +2611,7 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
 	}
 
 	if (cow)
-		mmu_notifier_invalidate_range_end(src, mmun_start,
-						  mmun_end, MMU_MIGRATE);
+		mmu_notifier_invalidate_range_end(src, &range);
 
 	return ret;
 }

@@ -2631,16 +2629,17 @@ void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
 	struct page *page;
 	struct hstate *h = hstate_vma(vma);
 	unsigned long sz = huge_page_size(h);
-	const unsigned long mmun_start = start;	/* For mmu_notifiers */
-	const unsigned long mmun_end   = end;	/* For mmu_notifiers */
+	struct mmu_notifier_range range;
 
 	WARN_ON(!is_vm_hugetlb_page(vma));
 	BUG_ON(start & ~huge_page_mask(h));
 	BUG_ON(end & ~huge_page_mask(h));
 
+	range.start = start;
+	range.end = end;
+	range.event = MMU_MIGRATE;
 	tlb_start_vma(tlb, vma);
-	mmu_notifier_invalidate_range_start(mm, mmun_start,
-					    mmun_end, MMU_MIGRATE);
+	mmu_notifier_invalidate_range_start(mm, &range);
 again:
 	for (address = start; address < end; address += sz) {
 		ptep = huge_pte_offset(mm, address);

@@ -2711,8 +2710,7 @@ unlock:
 		if (address < end && !ref_page)
 			goto again;
 	}
-	mmu_notifier_invalidate_range_end(mm, mmun_start,
-					  mmun_end, MMU_MIGRATE);
+	mmu_notifier_invalidate_range_end(mm, &range);
 	tlb_end_vma(tlb, vma);
 }
 

@@ -2809,8 +2807,7 @@ static int hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma,
 	struct hstate *h = hstate_vma(vma);
 	struct page *old_page, *new_page;
 	int ret = 0, outside_reserve = 0;
-	unsigned long mmun_start;	/* For mmu_notifiers */
-	unsigned long mmun_end;		/* For mmu_notifiers */
+	struct mmu_notifier_range range;
 
 	old_page = pte_page(pte);
 

@@ -2888,10 +2885,11 @@ retry_avoidcopy:
 			    pages_per_huge_page(h));
 	__SetPageUptodate(new_page);
 
-	mmun_start = address & huge_page_mask(h);
-	mmun_end = mmun_start + huge_page_size(h);
-	mmu_notifier_invalidate_range_start(mm, mmun_start, mmun_end,
-					    MMU_MIGRATE);
+	range.start = address;
+	range.end = address + huge_page_size(h);
+	range.event = MMU_MIGRATE;
+	mmu_notifier_invalidate_range_start(mm, &range);
+
 	/*
 	 * Retake the page table lock to check for racing updates
 	 * before the page tables are altered

@@ -2911,8 +2909,7 @@ retry_avoidcopy:
 		new_page = old_page;
 	}
 	spin_unlock(ptl);
-	mmu_notifier_invalidate_range_end(mm, mmun_start, mmun_end,
-					  MMU_MIGRATE);
+	mmu_notifier_invalidate_range_end(mm, &range);
 out_release_all:
 	page_cache_release(new_page);
 out_release_old:

@@ -3346,11 +3343,15 @@ unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
 	pte_t pte;
 	struct hstate *h = hstate_vma(vma);
 	unsigned long pages = 0;
+	struct mmu_notifier_range range;
 
 	BUG_ON(address >= end);
 	flush_cache_range(vma, address, end);
 
-	mmu_notifier_invalidate_range_start(mm, start, end, MMU_MPROT);
+	range.start = start;
+	range.end = end;
+	range.event = MMU_MPROT;
+	mmu_notifier_invalidate_range_start(mm, &range);
 	mutex_lock(&vma->vm_file->f_mapping->i_mmap_mutex);
 	for (; address < end; address += huge_page_size(h)) {
 		spinlock_t *ptl;

@@ -3380,7 +3381,7 @@ unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
 	 */
 	flush_tlb_range(vma, start, end);
 	mutex_unlock(&vma->vm_file->f_mapping->i_mmap_mutex);
-	mmu_notifier_invalidate_range_end(mm, start, end, MMU_MPROT);
+	mmu_notifier_invalidate_range_end(mm, &range);
 
 	return pages << h->order;
 }
mm/memory.c +40 -33
index 1c212e6..c1c7ccc 100644
@@ -1008,8 +1008,7 @@ int copy_page_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 	unsigned long next;
 	unsigned long addr = vma->vm_start;
 	unsigned long end = vma->vm_end;
-	unsigned long mmun_start;	/* For mmu_notifiers */
-	unsigned long mmun_end;		/* For mmu_notifiers */
+	struct mmu_notifier_range range;
 	bool is_cow;
 	int ret;
 

@@ -1045,11 +1044,11 @@ int copy_page_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 	 * is_cow_mapping() returns true.
 	 */
 	is_cow = is_cow_mapping(vma->vm_flags);
-	mmun_start = addr;
-	mmun_end   = end;
+	range.start = addr;
+	range.end = end;
+	range.event = MMU_MIGRATE;
 	if (is_cow)
-		mmu_notifier_invalidate_range_start(src_mm, mmun_start,
-						    mmun_end, MMU_MIGRATE);
+		mmu_notifier_invalidate_range_start(src_mm, &range);
 
 	ret = 0;
 	dst_pgd = pgd_offset(dst_mm, addr);

@@ -1066,8 +1065,7 @@ int copy_page_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 	} while (dst_pgd++, src_pgd++, addr = next, addr != end);
 
 	if (is_cow)
-		mmu_notifier_invalidate_range_end(src_mm, mmun_start, mmun_end,
-						  MMU_MIGRATE);
+		mmu_notifier_invalidate_range_end(src_mm, &range);
 	return ret;
 }
 

@@ -1370,13 +1368,16 @@ void unmap_vmas(struct mmu_gather *tlb,
 		unsigned long end_addr)
 {
 	struct mm_struct *mm = vma->vm_mm;
+	struct mmu_notifier_range range = {
+		.start = start_addr,
+		.end = end_addr,
+		.event = MMU_MUNMAP,
+	};
 
-	mmu_notifier_invalidate_range_start(mm, start_addr,
-					    end_addr, MMU_MUNMAP);
+	mmu_notifier_invalidate_range_start(mm, &range);
 	for ( ; vma && vma->vm_start < end_addr; vma = vma->vm_next)
 		unmap_single_vma(tlb, vma, start_addr, end_addr, NULL);
-	mmu_notifier_invalidate_range_end(mm, start_addr,
-					  end_addr, MMU_MUNMAP);
+	mmu_notifier_invalidate_range_end(mm, &range);
 }
 
 /**

@@ -1393,16 +1394,20 @@ void zap_page_range(struct vm_area_struct *vma, unsigned long start,
 {
 	struct mm_struct *mm = vma->vm_mm;
 	struct mmu_gather tlb;
-	unsigned long end = start + size;
+	struct mmu_notifier_range range = {
+		.start = start,
+		.end = start + size,
+		.event = MMU_MUNMAP,
+	};
 
 	lru_add_drain();
-	tlb_gather_mmu(&tlb, mm, start, end);
+	tlb_gather_mmu(&tlb, mm, start, range.end);
 	update_hiwater_rss(mm);
-	mmu_notifier_invalidate_range_start(mm, start, end, MMU_MUNMAP);
-	for ( ; vma && vma->vm_start < end; vma = vma->vm_next)
-		unmap_single_vma(&tlb, vma, start, end, details);
-	mmu_notifier_invalidate_range_end(mm, start, end, MMU_MUNMAP);
-	tlb_finish_mmu(&tlb, start, end);
+	mmu_notifier_invalidate_range_start(mm, &range);
+	for ( ; vma && vma->vm_start < range.end; vma = vma->vm_next)
+		unmap_single_vma(&tlb, vma, start, range.end, details);
+	mmu_notifier_invalidate_range_end(mm, &range);
+	tlb_finish_mmu(&tlb, start, range.end);
 }
 
 /**

@@ -1419,15 +1424,19 @@ static void zap_page_range_single(struct vm_area_struct *vma, unsigned long addr
 {
 	struct mm_struct *mm = vma->vm_mm;
 	struct mmu_gather tlb;
-	unsigned long end = address + size;
+	struct mmu_notifier_range range = {
+		.start = address,
+		.end = address + size,
+		.event = MMU_MUNMAP,
+	};
 
 	lru_add_drain();
-	tlb_gather_mmu(&tlb, mm, address, end);
+	tlb_gather_mmu(&tlb, mm, address, range.end);
 	update_hiwater_rss(mm);
-	mmu_notifier_invalidate_range_start(mm, address, end, MMU_MUNMAP);
-	unmap_single_vma(&tlb, vma, address, end, details);
-	mmu_notifier_invalidate_range_end(mm, address, end, MMU_MUNMAP);
-	tlb_finish_mmu(&tlb, address, end);
+	mmu_notifier_invalidate_range_start(mm, &range);
+	unmap_single_vma(&tlb, vma, address, range.end, details);
+	mmu_notifier_invalidate_range_end(mm, &range);
+	tlb_finish_mmu(&tlb, address, range.end);
 }
 
 /**

@@ -2047,8 +2056,7 @@ static int do_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
 	int ret = 0;
 	int page_mkwrite = 0;
 	struct page *dirty_page = NULL;
-	unsigned long mmun_start = 0;	/* For mmu_notifiers */
-	unsigned long mmun_end = 0;	/* For mmu_notifiers */
+	struct mmu_notifier_range range;
 	struct mem_cgroup *memcg;
 
 	old_page = vm_normal_page(vma, address, orig_pte);

@@ -2208,10 +2216,10 @@ gotten:
 	if (mem_cgroup_try_charge(new_page, mm, GFP_KERNEL, &memcg))
 		goto oom_free_new;
 
-	mmun_start  = address & PAGE_MASK;
-	mmun_end    = mmun_start + PAGE_SIZE;
-	mmu_notifier_invalidate_range_start(mm, mmun_start,
-					    mmun_end, MMU_MIGRATE);
+	range.start = address & PAGE_MASK;
+	range.end = range.start + PAGE_SIZE;
+	range.event = MMU_MIGRATE;
+	mmu_notifier_invalidate_range_start(mm, &range);
 
 	/*
 	 * Re-check the pte - we dropped the lock

@@ -2282,8 +2290,7 @@ gotten:
 unlock:
 	pte_unmap_unlock(page_table, ptl);
 	if (mmun_end > mmun_start)
-		mmu_notifier_invalidate_range_end(mm, mmun_start,
-						  mmun_end, MMU_MIGRATE);
+		mmu_notifier_invalidate_range_end(mm, &range);
 	if (old_page) {
 		/*
 		 * Don't let another task, with possibly unlocked vma,
mm/migrate.c +8 -8
index 30417d5..d866771 100644
@@ -1781,10 +1781,13 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 	int isolated = 0;
 	struct page *new_page = NULL;
 	int page_lru = page_is_file_cache(page);
-	unsigned long mmun_start = address & HPAGE_PMD_MASK;
-	unsigned long mmun_end = mmun_start + HPAGE_PMD_SIZE;
+	struct mmu_notifier_range range;
 	pmd_t orig_entry;
 
+	range.start = address & HPAGE_PMD_MASK;
+	range.end = range.start + HPAGE_PMD_SIZE;
+	range.event = MMU_MIGRATE;
+
 	/*
 	 * Rate-limit the amount of data that is being migrated to a node.
 	 * Optimal placement is no good if the memory bus is saturated and

@@ -1819,14 +1822,12 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
 	WARN_ON(PageLRU(new_page));
 
 	/* Recheck the target PMD */
-	mmu_notifier_invalidate_range_start(mm, mmun_start,
-					    mmun_end, MMU_MIGRATE);
+	mmu_notifier_invalidate_range_start(mm, &range);
 	ptl = pmd_lock(mm, pmd);
 	if (unlikely(!pmd_same(*pmd, entry) || page_count(page) != 2)) {
 fail_putback:
 		spin_unlock(ptl);
-		mmu_notifier_invalidate_range_end(mm, mmun_start,
-						  mmun_end, MMU_MIGRATE);
+		mmu_notifier_invalidate_range_end(mm, &range);
 
 		/* Reverse changes made by migrate_page_copy() */
 		if (TestClearPageActive(new_page))

@@ -1879,8 +1880,7 @@ fail_putback:
 	page_remove_rmap(page);
 
 	spin_unlock(ptl);
-	mmu_notifier_invalidate_range_end(mm, mmun_start,
-					  mmun_end, MMU_MIGRATE);
+	mmu_notifier_invalidate_range_end(mm, &range);
 
 	/* Take an "isolate" reference and put new page on the LRU. */
 	get_page(new_page);
mm/mmu_notifier.c +63 -10
index de039e4..d0edb98 100644
@@ -173,9 +173,7 @@ void __mmu_notifier_invalidate_page(struct mm_struct *mm,
 }
 
 void __mmu_notifier_invalidate_range_start(struct mm_struct *mm,
-					   unsigned long start,
-					   unsigned long end,
-					   enum mmu_event event)
+					   struct mmu_notifier_range *range)
 
 {
 	struct mmu_notifier *mn;

@@ -184,31 +182,83 @@ void __mmu_notifier_invalidate_range_start(struct mm_struct *mm,
 	id = srcu_read_lock(&srcu);
 	hlist_for_each_entry_rcu(mn, &mm->mmu_notifier_mm->list, hlist) {
 		if (mn->ops->invalidate_range_start)
-			mn->ops->invalidate_range_start(mn, mm, start,
-							end, event);
+			mn->ops->invalidate_range_start(mn, mm, range);
 	}
 	srcu_read_unlock(&srcu, id);
+
+	/*
+	 * This must happen after the callback so that subsystem can block on
+	 * new invalidation range to synchronize itself.
+	 */
+	spin_lock(&mm->mmu_notifier_mm->lock);
+	list_add_tail(&range->list, &mm->mmu_notifier_mm->ranges);
+	mm->mmu_notifier_mm->nranges++;
+	spin_unlock(&mm->mmu_notifier_mm->lock);
 }
 EXPORT_SYMBOL_GPL(__mmu_notifier_invalidate_range_start);
 
 void __mmu_notifier_invalidate_range_end(struct mm_struct *mm,
-					 unsigned long start,
-					 unsigned long end,
-					 enum mmu_event event)
+					 struct mmu_notifier_range *range)
 {
 	struct mmu_notifier *mn;
 	int id;
 
+	/*
+	 * This must happen before the callback so that subsystem can unblock
+	 * when range invalidation end.
+	 */
+	spin_lock(&mm->mmu_notifier_mm->lock);
+	list_del_init(&range->list);
+	mm->mmu_notifier_mm->nranges--;
+	spin_unlock(&mm->mmu_notifier_mm->lock);
+
 	id = srcu_read_lock(&srcu);
 	hlist_for_each_entry_rcu(mn, &mm->mmu_notifier_mm->list, hlist) {
 		if (mn->ops->invalidate_range_end)
-			mn->ops->invalidate_range_end(mn, mm, start,
-						      end, event);
+			mn->ops->invalidate_range_end(mn, mm, range);
 	}
 	srcu_read_unlock(&srcu, id);
+
+	/*
+	 * Wakeup after callback so they can do their job before any of the
+	 * waiters resume.
+	 */
+	wake_up(&mm->mmu_notifier_mm->wait_queue);
 }
 EXPORT_SYMBOL_GPL(__mmu_notifier_invalidate_range_end);
 
+bool mmu_notifier_range_is_valid(struct mm_struct *mm,
+				 unsigned long start,
+				 unsigned long end)
+{
+	struct mmu_notifier_range range;
+
+	spin_lock(&mm->mmu_notifier_mm->lock);
+	list_for_each_entry(range, &mm->mmu_notifier_mm->ranges, list) {
+		if (!(range->end <= start || range->start >= end)) {
+			spin_unlock(&mm->mmu_notifier_mm->lock);
+			return false;
+		}
+	}
+	spin_unlock(&mm->mmu_notifier_mm->lock);
+	return true;
+}
+EXPORT_SYMBOL_GPL(mmu_notifier_range_is_valid);
+
+void mmu_notifier_range_wait_valid(struct mm_struct *mm,
+				   unsigned long start,
+				   unsigned long end)
+{
+	int nranges = mm->mmu_notifier_mm->nranges;
+
+	while (!mmu_notifier_range_is_valid(mm, start, end)) {
+		wait_event(mm->mmu_notifier_mm->wait_queue,
+			   nranges != mm->mmu_notifier_mm->nranges);
+		nranges = mm->mmu_notifier_mm->nranges;
+	}
+}
+EXPORT_SYMBOL_GPL(mmu_notifier_range_wait_valid);
+
 static int do_mmu_notifier_register(struct mmu_notifier *mn,
 				    struct mm_struct *mm,
 				    int take_mmap_sem)

@@ -238,6 +288,9 @@ static int do_mmu_notifier_register(struct mmu_notifier *mn,
 	if (!mm_has_notifiers(mm)) {
 		INIT_HLIST_HEAD(&mmu_notifier_mm->list);
 		spin_lock_init(&mmu_notifier_mm->lock);
+		INIT_LIST_HEAD(&mmu_notifier_mm->ranges);
+		mmu_notifier_mm->nranges = 0;
+		init_waitqueue_head(&mmu_notifier_mm->wait_queue);
 
 		mm->mmu_notifier_mm = mmu_notifier_mm;
 		mmu_notifier_mm = NULL;
mm/mprotect.c +10 -7
index 886405b..a178b22 100644
@@ -144,7 +144,9 @@ static inline unsigned long change_pmd_range(struct vm_area_struct *vma,
 	unsigned long next;
 	unsigned long pages = 0;
 	unsigned long nr_huge_updates = 0;
-	unsigned long mni_start = 0;
+	struct mmu_notifier_range range = {
+		.start = 0,
+	};
 
 	pmd = pmd_offset(pud, addr);
 	do {

@@ -155,10 +157,11 @@ static inline unsigned long change_pmd_range(struct vm_area_struct *vma,
 			continue;
 
 		/* invoke the mmu notifier if the pmd is populated */
-		if (!mni_start) {
-			mni_start = addr;
-			mmu_notifier_invalidate_range_start(mm, mni_start,
-							    end, MMU_MPROT);
+		if (!range.start) {
+			range.start = addr;
+			range.end = end;
+			range.event = MMU_MPROT;
+			mmu_notifier_invalidate_range_start(mm, &range);
 		}
 
 		if (pmd_trans_huge(*pmd)) {

@@ -185,8 +188,8 @@ static inline unsigned long change_pmd_range(struct vm_area_struct *vma,
 		pages += this_pages;
 	} while (pmd++, addr = next, addr != end);
 
-	if (mni_start)
-		mmu_notifier_invalidate_range_end(mm, mni_start, end, MMU_MPROT);
+	if (range.start)
+		mmu_notifier_invalidate_range_end(mm, &range);
 
 	if (nr_huge_updates)
 		count_vm_numa_events(NUMA_HUGE_PTE_UPDATES, nr_huge_updates);
mm/mremap.c +6 -8
index 6827d2f..83c5eed 100644
@@ -167,18 +167,17 @@ unsigned long move_page_tables(struct vm_area_struct *vma,
 		bool need_rmap_locks)
 {
 	unsigned long extent, next, old_end;
+	struct mmu_notifier_range range;
 	pmd_t *old_pmd, *new_pmd;
 	bool need_flush = false;
-	unsigned long mmun_start;	/* For mmu_notifiers */
-	unsigned long mmun_end;		/* For mmu_notifiers */
 
 	old_end = old_addr + len;
 	flush_cache_range(vma, old_addr, old_end);
 
-	mmun_start = old_addr;
-	mmun_end   = old_end;
-	mmu_notifier_invalidate_range_start(vma->vm_mm, mmun_start,
-					    mmun_end, MMU_MIGRATE);
+	range.start = old_addr;
+	range.end = old_end;
+	range.event = MMU_MIGRATE;
+	mmu_notifier_invalidate_range_start(vma->vm_mm, &range);
 
 	for (; old_addr < old_end; old_addr += extent, new_addr += extent) {
 		cond_resched();

@@ -229,8 +228,7 @@ unsigned long move_page_tables(struct vm_area_struct *vma,
 	if (likely(need_flush))
 		flush_tlb_range(vma, old_end-len, old_addr);
 
-	mmu_notifier_invalidate_range_end(vma->vm_mm, mmun_start,
-					  mmun_end, MMU_MIGRATE);
+	mmu_notifier_invalidate_range_end(vma->vm_mm, &range);
 
 	return len + old_addr - old_end;	/* how much done */
 }
mm/rmap.c +7 -8
index 0b67e7d..b8b8a60 100644
@@ -1302,15 +1302,14 @@ static int try_to_unmap_cluster(unsigned long cursor, unsigned int *mapcount,
 	spinlock_t *ptl;
 	struct page *page;
 	unsigned long address;
-	unsigned long mmun_start;	/* For mmu_notifiers */
-	unsigned long mmun_end;		/* For mmu_notifiers */
+	struct mmu_notifier_range range;
 	unsigned long end;
 	int ret = SWAP_AGAIN;
 	int locked_vma = 0;
-	enum mmu_event event = MMU_MIGRATE;
 
+	range.event = MMU_MIGRATE;
 	if (flags & TTU_MUNLOCK)
-		event = MMU_MUNLOCK;
+		range.event = MMU_MUNLOCK;
 
 	address = (vma->vm_start + cursor) & CLUSTER_MASK;
 	end = address + CLUSTER_SIZE;

@@ -1323,9 +1322,9 @@ static int try_to_unmap_cluster(unsigned long cursor, unsigned int *mapcount,
 	if (!pmd)
 		return ret;
 
-	mmun_start = address;
-	mmun_end   = end;
-	mmu_notifier_invalidate_range_start(mm, mmun_start, mmun_end, event);
+	range.start = address;
+	range.end = end;
+	mmu_notifier_invalidate_range_start(mm, &range);
 
 	/*
 	 * If we can acquire the mmap_sem for read, and vma is VM_LOCKED,

@@ -1390,7 +1389,7 @@ static int try_to_unmap_cluster(unsigned long cursor, unsigned int *mapcount,
 		(*mapcount)--;
 	}
 	pte_unmap_unlock(pte - 1, ptl);
-	mmu_notifier_invalidate_range_end(mm, mmun_start, mmun_end, event);
+	mmu_notifier_invalidate_range_end(mm, &range);
 	if (locked_vma)
 		up_read(&vma->vm_mm->mmap_sem);
 	return ret;
virt/kvm/kvm_main.c +3 -7
index 0ed3e88..8d8c2ce 100644
@@ -318,9 +318,7 @@ static void kvm_mmu_notifier_change_pte(struct mmu_notifier *mn,
 
 static void kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
 						    struct mm_struct *mm,
-						    unsigned long start,
-						    unsigned long end,
-						    enum mmu_event event)
+						    const struct mmu_notifier_range *range)
 {
 	struct kvm *kvm = mmu_notifier_to_kvm(mn);
 	int need_tlb_flush = 0, idx;

@@ -333,7 +331,7 @@ static void kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
 	 * count is also read inside the mmu_lock critical section.
 	 */
 	kvm->mmu_notifier_count++;
-	need_tlb_flush = kvm_unmap_hva_range(kvm, start, end);
+	need_tlb_flush = kvm_unmap_hva_range(kvm, range->start, range->end);
 	need_tlb_flush |= kvm->tlbs_dirty;
 	/* we've to flush the tlb before the pages can be freed */
 	if (need_tlb_flush)

@@ -345,9 +343,7 @@ static void kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
 
 static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,
 						  struct mm_struct *mm,
-						  unsigned long start,
-						  unsigned long end,
-						  enum mmu_event event)
+						  const struct mmu_notifier_range *range)
 {
 	struct kvm *kvm = mmu_notifier_to_kvm(mn);
Signature · 1 line
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): CAL1RGDWiabD0uAv-h8Cn13nt32h5p7sUG7CWBabvdb+TGDG5Ug@mail.gmail.com

A message that is not in the archive (sent off-list or not yet received): CAL1RGDWiabD0uAv-h8Cn13nt32h5p7sUG7CWBabvdb+TGDG5Ug-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#17 lore raw

Review

Quoted 3 lines
(quoted diff) +3 -0
+     notifiers_seq = atomic_read(&item->odp_data->notifiers_seq) + 1;
+     atomic_set(&item->odp_data->notifiers_seq,
+                notifiers_seq);

Is this code really as silly as it looks, or is there some deep reason for avoiding atomic_inc() that I'm missing? Do you need atomic_inc(), since as far as I can tell all modification of notifiers_seq and notifiers_count happens while holding the umem_mutex?

In general I find it very suspicious that you have these two atomic_t members, notifiers_seq and notifiers_count, but you never use anything except atomic_read() and atomic_set() to access them. Why are they atomic_t at all?

- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): CAL1RGDV4LmNYTypHO+_t1vtvGh-NpOtoNur=CS+fqvscwsja4Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#19 lore raw

SGkgUm9sYW5kLA0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IGxpbnV4 LXJkbWEtb3duZXJAdmdlci5rZXJuZWwub3JnIFttYWlsdG86bGludXgtcmRtYS0NCj4gb3duZXJA dmdlci5rZXJuZWwub3JnXSBPbiBCZWhhbGYgT2YgUm9sYW5kIERyZWllcg0KPiBTZW50OiBUdWVz ZGF5LCBPY3RvYmVyIDE0LCAyMDE0IDExOjMwIEFNDQo+IENjOiBIYWdnYWkgRXJhbjsgbGludXgt cmRtYUB2Z2VyLmtlcm5lbC5vcmc7IFNoYWNoYXIgUmFpbmRlbDsgU2FnaQ0KPiBHcmltYmVyZw0K PiBTdWJqZWN0OiBSZTogW1BBVENIIHYxIGZvci1uZXh0IDA2LzE2XSBJQi9jb3JlOiBJbXBsZW1l bnQgc3VwcG9ydCBmb3INCj4gTU1VIG5vdGlmaWVycyByZWdhcmRpbmcgb24gZGVtYW5kIHBhZ2lu ZyByZWdpb25zDQo+IA0KPiA+ICsgICAgIG5vdGlmaWVyc19zZXEgPSBhdG9taWNfcmVhZCgmaXRl bS0+b2RwX2RhdGEtPm5vdGlmaWVyc19zZXEpICsgMTsNCj4gPiArICAgICBhdG9taWNfc2V0KCZp dGVtLT5vZHBfZGF0YS0+bm90aWZpZXJzX3NlcSwNCj4gPiArICAgICAgICAgICAgICAgIG5vdGlm aWVyc19zZXEpOw0KPiANCj4gSXMgdGhpcyBjb2RlIHJlYWxseSBhcyBzaWxseSBhcyBpdCBsb29r cywgb3IgaXMgdGhlcmUgc29tZSBkZWVwIHJlYXNvbg0KPiBmb3IgYXZvaWRpbmcgYXRvbWljX2lu YygpIHRoYXQgSSdtIG1pc3Npbmc/ICBEbyB5b3UgbmVlZCBhdG9taWNfaW5jKCksDQo+IHNpbmNl IGFzIGZhciBhcyBJIGNhbiB0ZWxsIGFsbCBtb2RpZmljYXRpb24gb2Ygbm90aWZpZXJzX3NlcSBh bmQNCj4gbm90aWZpZXJzX2NvdW50IGhhcHBlbnMgd2hpbGUgaG9sZGluZyB0aGUgdW1lbV9tdXRl eD8NCj4gDQo+IEluIGdlbmVyYWwgSSBmaW5kIGl0IHZlcnkgc3VzcGljaW91cyB0aGF0IHlvdSBo YXZlIHRoZXNlIHR3byBhdG9taWNfdA0KPiBtZW1iZXJzLCBub3RpZmllcnNfc2VxIGFuZCBub3Rp ZmllcnNfY291bnQsIGJ1dCB5b3UgbmV2ZXIgdXNlIGFueXRoaW5nDQo+IGV4Y2VwdCBhdG9taWNf cmVhZCgpIGFuZCBhdG9taWNfc2V0KCkgdG8gYWNjZXNzIHRoZW0uICBXaHkgYXJlIHRoZXkNCj4g YXRvbWljX3QgYXQgYWxsPw0KPiANCg0KVGhlIHNlcXVlbmNlIGNvdW50ZXIgaXMgcmVhZCB3aGls ZSBub3QgaG9sZGluZyB0aGUgbG9jay4gVG8gYXZvaWQNCmNhc2VzIHdoZXJlIHRoZSB2YWx1ZSBp cyBub3QgY29tcGxldGVseSB1cGRhdGVkIGR1ZSB0byBhbiBpbmNyZWFzZSBvcg0KZGVjcmVhc2Ug b3BlcmF0aW9uLCByZXN1bHRpbmcgaW4gYSBjb21wbGV0ZWx5IGlycmVsZXZhbnQgdmFsdWUgKGNh bg0KaGFwcGVuIGluIHNvbWUgd2VpcmQgQ1BVIGFyY2hpdGVjdHVyZXMpLiBXZSBwcmVmZXJyZWQg dG8gdXNlIGFuDQpleHBsaWNpdCBhdG9taWNfdCB0eXBlLCB3aGljaCBndWFyYW50ZWUgYXRvbWlj IHVwZGF0ZXMgZm9yIHN1Y2ggY2FzZXMuDQoNCldlIGF2b2lkZWQgYXRvbWljX2luYyBhcyBpdCBy ZXN1bHRzIGluIGEgbG9ja2VkIG9wZXJhdGlvbiBpbiBpbnRlbA0KcGxhdGZvcm0sIHdoaWNoIGlz IG5vdCBuZWVkZWQgaWYgdGhlIG9wZXJhdGlvbiBpcyBkb25lIHVuZGVyIGENCmxvY2suIFdlIHRo ZXJlZm9yZSBkbyBhbiBleHBsaWNpdCByZWFkLW1vZGlmeS13cml0ZSwgYXZvaWRpbmcgYW55DQps b2NrZWQgb3BlcmF0aW9ucy4NCg0KV2Ugd2lsbCByZWV4YW1pbmUgdGhlIGNvZGUgdG8gZGV0ZXJt aW5lIGlmIHdlIGNhbiBzYWZlbHkgc3dpdGNoIHRvDQpub3JtYWwgdmFyaWFibGVzLg0KDQpUaGFu a3MsDQotLVNoYWNoYXINCg==
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html