mm, oom: distinguish blockable mode for mmu notifiers

From: Michal Hocko <[email protected]>

There are several blockable mmu notifiers which might sleep in mmu_notifier_invalidate_range_start and that is a problem for the oom_reaper because it needs to guarantee a forward progress so it cannot depend on any sleepable locks. Currently we simply back off and mark an oom victim with blockable mmu notifiers as done after a short sleep. That can result in selecting a new oom victim prematurely because the previous one still hasn't torn its memory down yet.

We can do much better though. Even if mmu notifiers use sleepable locks there is no reason to automatically assume those locks are held. Moreover most notifiers only care about a portion of the address space. This patch handles the first part of the problem. __mmu_notifier_invalidate_range_start gets a blockable flag and callbacks are not allowed to sleep if the flag is set to false. This is achieved by using trylock instead of the sleepable lock for most callbacks. I think we can improve that even further because there is a common pattern to do a range lookup first and then do something about that. The first part can be done without a sleeping lock I presume.

Anyway, what does the oom_reaper do with all that? We do not have to fail right away. We simply retry if there is at least one notifier which couldn't make any progress. A retry loop is already implemented to wait for the mmap_sem and this is basically the same thing.

Cc "David (ChunMing) Zhou" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc Alex Deucher <[email protected]>

Cc "Christian König" <[email protected]>

Cc David Airlie <[email protected]>

Cc Jani Nikula <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Cc Doug Ledford <[email protected]>

Cc Jason Gunthorpe <[email protected]>

Cc Mike Marciniszyn <[email protected]>

Cc Dennis Dalessandro <[email protected]>

Cc Sudeep Dutt <[email protected]>

Cc Ashutosh Dixit <[email protected]>

Cc Dimitri Sivanich <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Cc Juergen Gross <[email protected]>

Cc "Jérôme Glisse" <[email protected]>

Cc Andrea Arcangeli <[email protected]>

Cc [email protected] (open list:KERNEL VIRTUAL MACHINE FOR X86 (KVM/x86))

Cc [email protected] (open list:X86 ARCHITECTURE (32-BIT AND 64-BIT))

Cc [email protected] (open list:RADEON and AMDGPU DRM DRIVERS)

Cc [email protected] (open list:DRM DRIVERS)

Cc [email protected] (open list:INTEL DRM DRIVERS (excluding Poulsbo, Moorestow...)

Cc [email protected] (open list:INFINIBAND SUBSYSTEM)

Cc [email protected] (moderated list:XEN HYPERVISOR INTERFACE)

Cc [email protected] (open list:HMM - Heterogeneous Memory Management)

Reported-by David Rientjes <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---

Hi,
this is an RFC and not tested at all. I am not very familiar with the mmu notifiers semantics very much so this is a crude attempt to achieve what I need basically. It might be completely wrong but I would like to discuss what would be a better way if that is the case.

get_maintainers gave me quite large list of people to CC so I had to trim it down. If you think I have forgot somebody, please let me know

Any feedback is highly appreciated.

  arch/x86/kvm/x86.c                      |  7 ++++--
  drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c  | 33 +++++++++++++++++++------
  drivers/gpu/drm/i915/i915_gem_userptr.c | 10 +++++---
  drivers/gpu/drm/radeon/radeon_mn.c      | 15 ++++++++---
  drivers/infiniband/core/umem_odp.c      | 15 ++++++++---
  drivers/infiniband/hw/hfi1/mmu_rb.c     |  7 ++++--
  drivers/misc/mic/scif/scif_dma.c        |  7 ++++--
  drivers/misc/sgi-gru/grutlbpurge.c      |  7 ++++--
  drivers/xen/gntdev.c                    | 14 ++++++++---
  include/linux/kvm_host.h                |  2 +-
  include/linux/mmu_notifier.h            | 15 +++++++++--
  mm/hmm.c                                |  7 ++++--
  mm/mmu_notifier.c                       | 15 ++++++++---
  mm/oom_kill.c                           | 29 +++++++++++-----------
  virt/kvm/kvm_main.c                     | 12 ++++++---
  15 files changed, 137 insertions(+), 58 deletions(-)

arch/x86/kvm/x86.c +5 -2

index 6bcecc325e7e..ac08f5d711be 100644
@@ -7203,8 +7203,9 @@ static void vcpu_load_eoi_exitmap(struct kvm_vcpu *vcpu)
  	kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
  }
 
-void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
-		unsigned long start, unsigned long end)
+int kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
+		unsigned long start, unsigned long end,
+		bool blockable)
  {
  	unsigned long apic_address;
 

@@ -7215,6 +7216,8 @@ void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
  	apic_address = gfn_to_hva(kvm, APIC_DEFAULT_PHYS_BASE >> PAGE_SHIFT);
  	if (start <= apic_address && apic_address < end)
  		kvm_make_all_cpus_request(kvm, KVM_REQ_APIC_PAGE_RELOAD);
+
+	return 0;
  }
 
  void kvm_vcpu_reload_apic_access_page(struct kvm_vcpu *vcpu)

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +25 -8

index 83e344fbb50a..d138a526feff 100644
@@ -136,12 +136,18 @@ void amdgpu_mn_unlock(struct amdgpu_mn *mn)
   *
   * Take the rmn read side lock.
   */
-static void amdgpu_mn_read_lock(struct amdgpu_mn *rmn)
+static int amdgpu_mn_read_lock(struct amdgpu_mn *rmn, bool blockable)
  {
-	mutex_lock(&rmn->read_lock);
+	if (blockable)
+		mutex_lock(&rmn->read_lock);
+	else if (!mutex_trylock(&rmn->read_lock))
+		return -EAGAIN;
+
  	if (atomic_inc_return(&rmn->recursion) == 1)
  		down_read_non_owner(&rmn->lock);
  	mutex_unlock(&rmn->read_lock);
+
+	return 0;
  }
 
  /**

@@ -197,10 +203,11 @@ static void amdgpu_mn_invalidate_node(struct amdgpu_mn_node *node,
   * We block for all BOs between start and end to be idle and
   * unmap them by move them into system domain again.
   */
-static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
+static int amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
  						 struct mm_struct *mm,
  						 unsigned long start,
-						 unsigned long end)
+						 unsigned long end,
+						 bool blockable)
  {
  	struct amdgpu_mn *rmn = container_of(mn, struct amdgpu_mn, mn);
  	struct interval_tree_node *it;

@@ -208,7 +215,11 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
  	/* notification is exclusive, but interval is inclusive */
  	end -= 1;
 
-	amdgpu_mn_read_lock(rmn);
+	/* TODO we should be able to split locking for interval tree and
+	 * amdgpu_mn_invalidate_node
+	 */
+	if (amdgpu_mn_read_lock(rmn, blockable))
+		return -EAGAIN;
 
  	it = interval_tree_iter_first(&rmn->objects, start, end);
  	while (it) {

@@ -219,6 +230,8 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
 
  		amdgpu_mn_invalidate_node(node, start, end);
  	}
+
+	return 0;
  }
 
  /**

@@ -233,10 +246,11 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
   * necessitates evicting all user-mode queues of the process. The BOs
   * are restorted in amdgpu_mn_invalidate_range_end_hsa.
   */
-static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
+static int amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  						 struct mm_struct *mm,
  						 unsigned long start,
-						 unsigned long end)
+						 unsigned long end,
+						 bool blockable)
  {
  	struct amdgpu_mn *rmn = container_of(mn, struct amdgpu_mn, mn);
  	struct interval_tree_node *it;

@@ -244,7 +258,8 @@ static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  	/* notification is exclusive, but interval is inclusive */
  	end -= 1;
 
-	amdgpu_mn_read_lock(rmn);
+	if (amdgpu_mn_read_lock(rmn, blockable))
+		return -EAGAIN;
 
  	it = interval_tree_iter_first(&rmn->objects, start, end);
  	while (it) {

@@ -262,6 +277,8 @@ static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  				amdgpu_amdkfd_evict_userptr(mem, mm);
  		}
  	}
+
+	return 0;
  }
 
  /**

drivers/gpu/drm/i915/i915_gem_userptr.c +6 -4

index 854bd51b9478..5285df9331fa 100644
@@ -112,10 +112,11 @@ static void del_object(struct i915_mmu_object *mo)
  	mo->attached = false;
  }
 
-static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
+static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
  						       struct mm_struct *mm,
  						       unsigned long start,
-						       unsigned long end)
+						       unsigned long end,
+						       bool blockable)
  {
  	struct i915_mmu_notifier *mn =
  		container_of(_mn, struct i915_mmu_notifier, mn);

@@ -124,7 +125,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
  	LIST_HEAD(cancelled);
 
  	if (RB_EMPTY_ROOT(&mn->objects.rb_root))
-		return;
+		return 0;
 
  	/* interval ranges are inclusive, but invalidate range is exclusive */
  	end--;

@@ -152,7 +153,8 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
  		del_object(mo);
  	spin_unlock(&mn->lock);
 
-	if (!list_empty(&cancelled))
+	/* TODO: can we skip waiting here? */
+	if (!list_empty(&cancelled) && blockable)
  		flush_workqueue(mn->wq);
  }
 

drivers/gpu/drm/radeon/radeon_mn.c +12 -3

index abd24975c9b1..b47e828b725d 100644
@@ -118,10 +118,11 @@ static void radeon_mn_release(struct mmu_notifier *mn,
   * We block for all BOs between start and end to be idle and
   * unmap them by move them into system domain again.
   */
-static void radeon_mn_invalidate_range_start(struct mmu_notifier *mn,
+static int radeon_mn_invalidate_range_start(struct mmu_notifier *mn,
  					     struct mm_struct *mm,
  					     unsigned long start,
-					     unsigned long end)
+					     unsigned long end,
+					     bool blockable)
  {
  	struct radeon_mn *rmn = container_of(mn, struct radeon_mn, mn);
  	struct ttm_operation_ctx ctx = { false, false };

@@ -130,7 +131,13 @@ static void radeon_mn_invalidate_range_start(struct mmu_notifier *mn,
  	/* notification is exclusive, but interval is inclusive */
  	end -= 1;
 
-	mutex_lock(&rmn->lock);
+	/* TODO we should be able to split locking for interval tree and
+	 * the tear down.
+	 */
+	if (blockable)
+		mutex_lock(&rmn->lock);
+	else if (!mutex_trylock(&rmn->lock))
+		return -EAGAIN;
 
  	it = interval_tree_iter_first(&rmn->objects, start, end);
  	while (it) {

@@ -167,6 +174,8 @@ static void radeon_mn_invalidate_range_start(struct mmu_notifier *mn,
  	}
 
  	mutex_unlock(&rmn->lock);
+
+	return 0;
  }
 
  static const struct mmu_notifier_ops radeon_mn_ops = {

drivers/infiniband/core/umem_odp.c +11 -4

index 182436b92ba9..f65f6a29daae 100644
@@ -207,22 +207,29 @@ static int invalidate_range_start_trampoline(struct ib_umem *item, u64 start,
  	return 0;
  }
 
-static void ib_umem_notifier_invalidate_range_start(struct mmu_notifier *mn,
+static int ib_umem_notifier_invalidate_range_start(struct mmu_notifier *mn,
  						    struct mm_struct *mm,
  						    unsigned long start,
-						    unsigned long end)
+						    unsigned long end,
+						    bool blockable)
  {
  	struct ib_ucontext *context = container_of(mn, struct ib_ucontext, mn);
 
  	if (!context->invalidate_range)
-		return;
+		return 0;
+
+	if (blockable)
+		down_read(&context->umem_rwsem);
+	else if (!down_read_trylock(&context->umem_rwsem))
+		return -EAGAIN;
 
  	ib_ucontext_notifier_start_account(context);
-	down_read(&context->umem_rwsem);
  	rbt_ib_umem_for_each_in_range(&context->umem_tree, start,
  				      end,
  				      invalidate_range_start_trampoline, NULL);
  	up_read(&context->umem_rwsem);
+
+	return 0;
  }
 
  static int invalidate_range_end_trampoline(struct ib_umem *item, u64 start,

drivers/infiniband/hw/hfi1/mmu_rb.c +5 -2

index 70aceefe14d5..8780560d1623 100644
@@ -284,10 +284,11 @@ void hfi1_mmu_rb_remove(struct mmu_rb_handler *handler,
  	handler->ops->remove(handler->ops_arg, node);
  }
 
-static void mmu_notifier_range_start(struct mmu_notifier *mn,
+static int mmu_notifier_range_start(struct mmu_notifier *mn,
  				     struct mm_struct *mm,
  				     unsigned long start,
-				     unsigned long end)
+				     unsigned long end,
+				     bool blockable)
  {
  	struct mmu_rb_handler *handler =
  		container_of(mn, struct mmu_rb_handler, mn);

@@ -313,6 +314,8 @@ static void mmu_notifier_range_start(struct mmu_notifier *mn,
 
  	if (added)
  		queue_work(handler->wq, &handler->del_work);
+
+	return 0;
  }
 
  /*

drivers/misc/mic/scif/scif_dma.c +5 -2

index 63d6246d6dff..d940568bed87 100644
@@ -200,15 +200,18 @@ static void scif_mmu_notifier_release(struct mmu_notifier *mn,
  	schedule_work(&scif_info.misc_work);
  }
 
-static void scif_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
+static int scif_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
  						     struct mm_struct *mm,
  						     unsigned long start,
-						     unsigned long end)
+						     unsigned long end,
+						     bool blockable)
  {
  	struct scif_mmu_notif	*mmn;
 
  	mmn = container_of(mn, struct scif_mmu_notif, ep_mmu_notifier);
  	scif_rma_destroy_tcw(mmn, start, end - start);
+
+	return 0
  }
 
  static void scif_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,

drivers/misc/sgi-gru/grutlbpurge.c +5 -2

index a3454eb56fbf..be28f05bfafa 100644
@@ -219,9 +219,10 @@ void gru_flush_all_tlb(struct gru_state *gru)
  /*
   * MMUOPS notifier callout functions
   */
-static void gru_invalidate_range_start(struct mmu_notifier *mn,
+static int gru_invalidate_range_start(struct mmu_notifier *mn,
  				       struct mm_struct *mm,
-				       unsigned long start, unsigned long end)
+				       unsigned long start, unsigned long end,
+				       bool blockable)
  {
  	struct gru_mm_struct *gms = container_of(mn, struct gru_mm_struct,
  						 ms_notifier);

@@ -231,6 +232,8 @@ static void gru_invalidate_range_start(struct mmu_notifier *mn,
  	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);
+
+	return 0;
  }
 
  static void gru_invalidate_range_end(struct mmu_notifier *mn,

drivers/xen/gntdev.c +11 -3

index bd56653b9bbc..50724d09fe5c 100644
@@ -465,14 +465,20 @@ static void unmap_if_in_range(struct grant_map *map,
  	WARN_ON(err);
  }
 
-static void mn_invl_range_start(struct mmu_notifier *mn,
+static int mn_invl_range_start(struct mmu_notifier *mn,
  				struct mm_struct *mm,
-				unsigned long start, unsigned long end)
+				unsigned long start, unsigned long end,
+				bool blockable)
  {
  	struct gntdev_priv *priv = container_of(mn, struct gntdev_priv, mn);
  	struct grant_map *map;
 
-	mutex_lock(&priv->lock);
+	/* TODO do we really need a mutex here? */
+	if (blockable)
+		mutex_lock(&priv->lock);
+	else if (!mutex_trylock(&priv->lock))
+		return -EAGAIN;
+
  	list_for_each_entry(map, &priv->maps, next) {
  		unmap_if_in_range(map, start, end);
  	}

@@ -480,6 +486,8 @@ static void mn_invl_range_start(struct mmu_notifier *mn,
  		unmap_if_in_range(map, start, end);
  	}
  	mutex_unlock(&priv->lock);
+
+	return true;
  }
 
  static void mn_release(struct mmu_notifier *mn,

include/linux/kvm_host.h +1 -1

index 4ee7bc548a83..e4181063e755 100644
@@ -1275,7 +1275,7 @@ static inline long kvm_arch_vcpu_async_ioctl(struct file *filp,
  }
  #endif /* CONFIG_HAVE_KVM_VCPU_ASYNC_IOCTL */
 
-void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
+int kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
  		unsigned long start, unsigned long end);
 
  #ifdef CONFIG_HAVE_KVM_VCPU_RUN_PID_CHANGE

include/linux/mmu_notifier.h +13 -2

index 392e6af82701..369867501bed 100644
@@ -230,7 +230,8 @@ extern int __mmu_notifier_test_young(struct mm_struct *mm,
  extern void __mmu_notifier_change_pte(struct mm_struct *mm,
  				      unsigned long address, pte_t pte);
  extern void __mmu_notifier_invalidate_range_start(struct mm_struct *mm,
-				  unsigned long start, unsigned long end);
+				  unsigned long start, unsigned long end,
+				  bool blockable);
  extern void __mmu_notifier_invalidate_range_end(struct mm_struct *mm,
  				  unsigned long start, unsigned long end,
  				  bool only_end);

@@ -281,7 +282,17 @@ static inline void mmu_notifier_invalidate_range_start(struct mm_struct *mm,
  				  unsigned long start, unsigned long end)
  {
  	if (mm_has_notifiers(mm))
-		__mmu_notifier_invalidate_range_start(mm, start, end);
+		__mmu_notifier_invalidate_range_start(mm, start, end, true);
+}
+
+static inline int mmu_notifier_invalidate_range_start_nonblock(struct mm_struct *mm,
+				  unsigned long start, unsigned long end)
+{
+	int ret = 0;
+	if (mm_has_notifiers(mm))
+		ret = __mmu_notifier_invalidate_range_start(mm, start, end, false);
+
+	return ret;
  }
 
  static inline void mmu_notifier_invalidate_range_end(struct mm_struct *mm,

mm/hmm.c +5 -2

index de7b6bf77201..81fd57bd2634 100644
@@ -177,16 +177,19 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
  	up_write(&hmm->mirrors_sem);
  }
 
-static void hmm_invalidate_range_start(struct mmu_notifier *mn,
+static int hmm_invalidate_range_start(struct mmu_notifier *mn,
  				       struct mm_struct *mm,
  				       unsigned long start,
-				       unsigned long end)
+				       unsigned long end,
+				       bool blockable)
  {
  	struct hmm *hmm = mm->hmm;
 
  	VM_BUG_ON(!hmm);
 
  	atomic_inc(&hmm->sequence);
+
+	return 0;
  }
 
  static void hmm_invalidate_range_end(struct mmu_notifier *mn,

mm/mmu_notifier.c +11 -4

index eff6b88a993f..30cc43121da9 100644
@@ -174,18 +174,25 @@ void __mmu_notifier_change_pte(struct mm_struct *mm, unsigned long address,
  	srcu_read_unlock(&srcu, id);
  }
 
-void __mmu_notifier_invalidate_range_start(struct mm_struct *mm,
-				  unsigned long start, unsigned long end)
+int __mmu_notifier_invalidate_range_start(struct mm_struct *mm,
+				  unsigned long start, unsigned long end,
+				  bool blockable)
  {
  	struct mmu_notifier *mn;
+	int ret = 0;
  	int id;
 
  	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);
+		if (mn->ops->invalidate_range_start) {
+			int _ret = mn->ops->invalidate_range_start(mn, mm, start, end, blockable);
+			if (_ret)
+				ret = _ret;
+		}
  	}
  	srcu_read_unlock(&srcu, id);
+
+	return ret;
  }
  EXPORT_SYMBOL_GPL(__mmu_notifier_invalidate_range_start);
 

mm/oom_kill.c +14 -15

index 84081e77bc51..7e0c6e78ae5c 100644
@@ -479,9 +479,10 @@ static DECLARE_WAIT_QUEUE_HEAD(oom_reaper_wait);
  static struct task_struct *oom_reaper_list;
  static DEFINE_SPINLOCK(oom_reaper_lock);
 
-void __oom_reap_task_mm(struct mm_struct *mm)
+bool __oom_reap_task_mm(struct mm_struct *mm)
  {
  	struct vm_area_struct *vma;
+	bool ret = true;
 
  	/*
  	 * Tell all users of get_user/copy_from_user etc... that the content

@@ -511,12 +512,17 @@ void __oom_reap_task_mm(struct mm_struct *mm)
  			struct mmu_gather tlb;
 
  			tlb_gather_mmu(&tlb, mm, start, end);
-			mmu_notifier_invalidate_range_start(mm, start, end);
+			if (mmu_notifier_invalidate_range_start_nonblock(mm, start, end)) {
+				ret = false;
+				continue;
+			}
  			unmap_page_range(&tlb, vma, start, end, NULL);
  			mmu_notifier_invalidate_range_end(mm, start, end);
  			tlb_finish_mmu(&tlb, start, end);
  		}
  	}
+
+	return ret;
  }
 
  static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)

@@ -545,18 +551,6 @@ static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
  		goto unlock_oom;
  	}
 
-	/*
-	 * If the mm has invalidate_{start,end}() notifiers that could block,
-	 * sleep to give the oom victim some more time.
-	 * TODO: we really want to get rid of this ugly hack and make sure that
-	 * notifiers cannot block for unbounded amount of time
-	 */
-	if (mm_has_blockable_invalidate_notifiers(mm)) {
-		up_read(&mm->mmap_sem);
-		schedule_timeout_idle(HZ);
-		goto unlock_oom;
-	}
-
  	/*
  	 * MMF_OOM_SKIP is set by exit_mmap when the OOM reaper can't
  	 * work on the mm anymore. The check for MMF_OOM_SKIP must run

@@ -571,7 +565,12 @@ static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 
  	trace_start_task_reaping(tsk->pid);
 
-	__oom_reap_task_mm(mm);
+	/* failed to reap part of the address space. Try again later */
+	if (!__oom_reap_task_mm(mm)) {
+		up_read(&mm->mmap_sem);
+		ret = false;
+		goto out_unlock;
+	}
 
  	pr_info("oom_reaper: reaped process %d (%s), now anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB\n",
  			task_pid_nr(tsk), tsk->comm,

virt/kvm/kvm_main.c +5 -3

index ada21f47f22b..6f7e709d2944 100644
@@ -135,7 +135,7 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm);
  static unsigned long long kvm_createvm_count;
  static unsigned long long kvm_active_vms;
 
-__weak void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
+__weak int kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
  		unsigned long start, unsigned long end)
  {
  }

@@ -354,13 +354,15 @@ static void kvm_mmu_notifier_change_pte(struct mmu_notifier *mn,
  	srcu_read_unlock(&kvm->srcu, idx);
  }
 
-static void kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
+static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
  						    struct mm_struct *mm,
  						    unsigned long start,
-						    unsigned long end)
+						    unsigned long end,
+						    bool blockable)
  {
  	struct kvm *kvm = mmu_notifier_to_kvm(mn);
  	int need_tlb_flush = 0, idx;
+	int ret;
 
  	idx = srcu_read_lock(&kvm->srcu);
  	spin_lock(&kvm->mmu_lock);

@@ -378,9 +380,11 @@ static void kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
 
  	spin_unlock(&kvm->mmu_lock);

Hi Michal,

[Adding Felix as well]

Well first of all you have a misconception why at least the AMD graphics driver need to be able to sleep in an MMU notifier: We need to sleep because we need to wait for hardware operations to finish and *NOT* because we need to wait for locks.

I'm not sure if your flag now means that you generally can't sleep in MMU notifiers any more, but if that's the case at least AMD hardware will break badly. In our case the approach of waiting for a short time for the process to be reaped and then select another victim actually sounds like the right thing to do.

Quoted 86 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +25 -8

index 83e344fbb50a..d138a526feff 100644
@@ -136,12 +136,18 @@ void amdgpu_mn_unlock(struct amdgpu_mn *mn)
   *
   * Take the rmn read side lock.
   */
-static void amdgpu_mn_read_lock(struct amdgpu_mn *rmn)
+static int amdgpu_mn_read_lock(struct amdgpu_mn *rmn, bool blockable)
  {
-	mutex_lock(&rmn->read_lock);
+	if (blockable)
+		mutex_lock(&rmn->read_lock);
+	else if (!mutex_trylock(&rmn->read_lock))
+		return -EAGAIN;
+
  	if (atomic_inc_return(&rmn->recursion) == 1)
  		down_read_non_owner(&rmn->lock);
  	mutex_unlock(&rmn->read_lock);
+
+	return 0;
  }
  /**

@@ -197,10 +203,11 @@ static void amdgpu_mn_invalidate_node(struct amdgpu_mn_node *node,
   * We block for all BOs between start and end to be idle and
   * unmap them by move them into system domain again.
   */
-static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
+static int amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
  						 struct mm_struct *mm,
  						 unsigned long start,
-						 unsigned long end)
+						 unsigned long end,
+						 bool blockable)
  {
  	struct amdgpu_mn *rmn = container_of(mn, struct amdgpu_mn, mn);
  	struct interval_tree_node *it;

@@ -208,7 +215,11 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
  	/* notification is exclusive, but interval is inclusive */
  	end -= 1;
-	amdgpu_mn_read_lock(rmn);
+	/* TODO we should be able to split locking for interval tree and
+	 * amdgpu_mn_invalidate_node
+	 */
+	if (amdgpu_mn_read_lock(rmn, blockable))
+		return -EAGAIN;
  	it = interval_tree_iter_first(&rmn->objects, start, end);
  	while (it) {

@@ -219,6 +230,8 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
  		amdgpu_mn_invalidate_node(node, start, end);
  	}
+
+	return 0;
  }
  /**

@@ -233,10 +246,11 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
   * necessitates evicting all user-mode queues of the process. The BOs
   * are restorted in amdgpu_mn_invalidate_range_end_hsa.
   */
-static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
+static int amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  						 struct mm_struct *mm,
  						 unsigned long start,
-						 unsigned long end)
+						 unsigned long end,
+						 bool blockable)
  {
  	struct amdgpu_mn *rmn = container_of(mn, struct amdgpu_mn, mn);
  	struct interval_tree_node *it;

@@ -244,7 +258,8 @@ static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  	/* notification is exclusive, but interval is inclusive */
  	end -= 1;
-	amdgpu_mn_read_lock(rmn);
+	if (amdgpu_mn_read_lock(rmn, blockable))
+		return -EAGAIN;
  	it = interval_tree_iter_first(&rmn->objects, start, end);
  	while (it) {

@@ -262,6 +277,8 @@ static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  				amdgpu_amdkfd_evict_userptr(mem, mm);
  		}
  	}
+
+	return 0;
  }
  /**

Quoted 9 lines · Christian König

Hi Michal,

[Adding Felix as well]

Well first of all you have a misconception why at least the AMD graphics driver need to be able to sleep in an MMU notifier: We need to sleep because we need to wait for hardware operations to finish and *NOT* because we need to wait for locks.

Quoted 5 lines

I'm not sure if your flag now means that you generally can't sleep in MMU notifiers any more, but if that's the case at least AMD hardware will break badly. In our case the approach of waiting for a short time for the process to be reaped and then select another victim actually sounds like the right thing to do.

Well, I do not need to make the notifier code non blocking all the time. All I need is to ensure that it won't sleep if the flag says so and return -EAGAIN instead.

So here is what I do for amdgpu:

Quoted 79 lines

Quoted 79 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +23 -8

index 83e344fbb50a..d138a526feff 100644
@@ -136,12 +136,18 @@ void amdgpu_mn_unlock(struct amdgpu_mn *mn)
   *
   * Take the rmn read side lock.
   */
-static void amdgpu_mn_read_lock(struct amdgpu_mn *rmn)
+static int amdgpu_mn_read_lock(struct amdgpu_mn *rmn, bool blockable)
  {
-	mutex_lock(&rmn->read_lock);
+	if (blockable)
+		mutex_lock(&rmn->read_lock);
+	else if (!mutex_trylock(&rmn->read_lock))
+		return -EAGAIN;
+
  	if (atomic_inc_return(&rmn->recursion) == 1)
  		down_read_non_owner(&rmn->lock);
  	mutex_unlock(&rmn->read_lock);
+
+	return 0;
  }
  /**

@@ -197,10 +203,11 @@ static void amdgpu_mn_invalidate_node(struct amdgpu_mn_node *node,
   * We block for all BOs between start and end to be idle and
   * unmap them by move them into system domain again.
   */
-static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
+static int amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
  						 struct mm_struct *mm,
  						 unsigned long start,
-						 unsigned long end)
+						 unsigned long end,
+						 bool blockable)
  {
  	struct amdgpu_mn *rmn = container_of(mn, struct amdgpu_mn, mn);
  	struct interval_tree_node *it;

@@ -208,7 +215,11 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
  	/* notification is exclusive, but interval is inclusive */
  	end -= 1;
-	amdgpu_mn_read_lock(rmn);
+	/* TODO we should be able to split locking for interval tree and
+	 * amdgpu_mn_invalidate_node
+	 */
+	if (amdgpu_mn_read_lock(rmn, blockable))
+		return -EAGAIN;
  	it = interval_tree_iter_first(&rmn->objects, start, end);
  	while (it) {

@@ -219,6 +230,8 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
  		amdgpu_mn_invalidate_node(node, start, end);
  	}
+
+	return 0;
  }
  /**

@@ -233,10 +246,11 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
   * necessitates evicting all user-mode queues of the process. The BOs
   * are restorted in amdgpu_mn_invalidate_range_end_hsa.
   */
-static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
+static int amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  						 struct mm_struct *mm,
  						 unsigned long start,
-						 unsigned long end)
+						 unsigned long end,
+						 bool blockable)
  {
  	struct amdgpu_mn *rmn = container_of(mn, struct amdgpu_mn, mn);
  	struct interval_tree_node *it;

@@ -244,7 +258,8 @@ static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  	/* notification is exclusive, but interval is inclusive */
  	end -= 1;
-	amdgpu_mn_read_lock(rmn);
+	if (amdgpu_mn_read_lock(rmn, blockable))
+		return -EAGAIN;
  	it = interval_tree_iter_first(&rmn->objects, start, end);
  	while (it) {

Quoted 8 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn, · line 283

Quoted 8 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +2 -0

@@ -262,6 +277,8 @@ static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  				amdgpu_amdkfd_evict_userptr(mem, mm);
  		}
  	}
+
+	return 0;
  }
  /**

Quoted 15 lines · Michal Hocko

Quoted 9 lines · Christian König

Hi Michal,

[Adding Felix as well]

Well first of all you have a misconception why at least the AMD graphics driver need to be able to sleep in an MMU notifier: We need to sleep because we need to wait for hardware operations to finish and *NOT* because we need to wait for locks.

Quoted 5 lines

I'm not sure if your flag now means that you generally can't sleep in MMU notifiers any more, but if that's the case at least AMD hardware will break badly. In our case the approach of waiting for a short time for the process to be reaped and then select another victim actually sounds like the right thing to do.

Quoted 5 lines

Well, I do not need to make the notifier code non blocking all the time. All I need is to ensure that it won't sleep if the flag says so and return -EAGAIN instead.

So here is what I do for amdgpu:

In the case of KFD we also need to take the DQM lock:

amdgpu_mn_invalidate_range_start_hsa -> amdgpu_amdkfd_evict_userptr -> kgd2kfd_quiesce_mm -> kfd_process_evict_queues -> evict_process_queues_cpsch

So we'd need to pass the blockable parameter all the way through that call chain.

Quoted 22 lines · Felix Kuehling

Quoted 15 lines · Michal Hocko

Quoted 9 lines · Christian König

Hi Michal,

[Adding Felix as well]

Well first of all you have a misconception why at least the AMD graphics driver need to be able to sleep in an MMU notifier: We need to sleep because we need to wait for hardware operations to finish and *NOT* because we need to wait for locks.

Quoted 5 lines

I'm not sure if your flag now means that you generally can't sleep in MMU notifiers any more, but if that's the case at least AMD hardware will break badly. In our case the approach of waiting for a short time for the process to be reaped and then select another victim actually sounds like the right thing to do.

Quoted 5 lines

Well, I do not need to make the notifier code non blocking all the time. All I need is to ensure that it won't sleep if the flag says so and return -EAGAIN instead.

So here is what I do for amdgpu:

Quoted 7 lines

In the case of KFD we also need to take the DQM lock:

amdgpu_mn_invalidate_range_start_hsa -> amdgpu_amdkfd_evict_userptr -> kgd2kfd_quiesce_mm -> kfd_process_evict_queues -> evict_process_queues_cpsch

So we'd need to pass the blockable parameter all the way through that call chain.

Thanks, I have missed that part. So I guess I will start with something similar to intel-gfx and back off when the current range needs some treatment. So this on top. Does it look correct?

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +0 -0

index d138a526feff..e2d422b3eb0b 100644

Quoted 22 lines · Felix Kuehling

Quoted 15 lines · Michal Hocko

Quoted 9 lines · Christian König

Hi Michal,

[Adding Felix as well]

Well first of all you have a misconception why at least the AMD graphics driver need to be able to sleep in an MMU notifier: We need to sleep because we need to wait for hardware operations to finish and *NOT* because we need to wait for locks.

Quoted 5 lines

I'm not sure if your flag now means that you generally can't sleep in MMU notifiers any more, but if that's the case at least AMD hardware will break badly. In our case the approach of waiting for a short time for the process to be reaped and then select another victim actually sounds like the right thing to do.

Quoted 5 lines

Well, I do not need to make the notifier code non blocking all the time. All I need is to ensure that it won't sleep if the flag says so and return -EAGAIN instead.

So here is what I do for amdgpu:

Quoted 7 lines

In the case of KFD we also need to take the DQM lock:

amdgpu_mn_invalidate_range_start_hsa -> amdgpu_amdkfd_evict_userptr -> kgd2kfd_quiesce_mm -> kfd_process_evict_queues -> evict_process_queues_cpsch

So we'd need to pass the blockable parameter all the way through that call chain.

Thanks, I have missed that part. So I guess I will start with something similar to intel-gfx and back off when the current range needs some treatment. So this on top. Does it look correct?

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +0 -0

index d138a526feff..e2d422b3eb0b 100644

Quoted 15 lines · Michal Hocko

Quoted 9 lines · Christian König

Hi Michal,

[Adding Felix as well]

Well first of all you have a misconception why at least the AMD graphics driver need to be able to sleep in an MMU notifier: We need to sleep because we need to wait for hardware operations to finish and *NOT* because we need to wait for locks.

Quoted 5 lines

I'm not sure if your flag now means that you generally can't sleep in MMU notifiers any more, but if that's the case at least AMD hardware will break badly. In our case the approach of waiting for a short time for the process to be reaped and then select another victim actually sounds like the right thing to do.

Quoted 5 lines

Well, I do not need to make the notifier code non blocking all the time. All I need is to ensure that it won't sleep if the flag says so and return -EAGAIN instead.

So here is what I do for amdgpu:

In the case of KFD we also need to take the DQM lock:

amdgpu_mn_invalidate_range_start_hsa -> amdgpu_amdkfd_evict_userptr -> kgd2kfd_quiesce_mm -> kfd_process_evict_queues -> evict_process_queues_cpsch

So we'd need to pass the blockable parameter all the way through that call chain.

Quoted 9 lines · Christian König

Hi Michal,

[Adding Felix as well]

Well first of all you have a misconception why at least the AMD graphics driver need to be able to sleep in an MMU notifier: We need to sleep because we need to wait for hardware operations to finish and *NOT* because we need to wait for locks.

Quoted 5 lines

I'm not sure if your flag now means that you generally can't sleep in MMU notifiers any more, but if that's the case at least AMD hardware will break badly. In our case the approach of waiting for a short time for the process to be reaped and then select another victim actually sounds like the right thing to do.

Well, I do not need to make the notifier code non blocking all the time. All I need is to ensure that it won't sleep if the flag says so and return -EAGAIN instead.

So here is what I do for amdgpu:

Quoted 79 lines

Quoted 79 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +23 -8

index 83e344fbb50a..d138a526feff 100644
@@ -136,12 +136,18 @@ void amdgpu_mn_unlock(struct amdgpu_mn *mn)
   *
   * Take the rmn read side lock.
   */
-static void amdgpu_mn_read_lock(struct amdgpu_mn *rmn)
+static int amdgpu_mn_read_lock(struct amdgpu_mn *rmn, bool blockable)
  {
-	mutex_lock(&rmn->read_lock);
+	if (blockable)
+		mutex_lock(&rmn->read_lock);
+	else if (!mutex_trylock(&rmn->read_lock))
+		return -EAGAIN;
+
  	if (atomic_inc_return(&rmn->recursion) == 1)
  		down_read_non_owner(&rmn->lock);
  	mutex_unlock(&rmn->read_lock);
+
+	return 0;
  }
  /**

@@ -197,10 +203,11 @@ static void amdgpu_mn_invalidate_node(struct amdgpu_mn_node *node,
   * We block for all BOs between start and end to be idle and
   * unmap them by move them into system domain again.
   */
-static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
+static int amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
  						 struct mm_struct *mm,
  						 unsigned long start,
-						 unsigned long end)
+						 unsigned long end,
+						 bool blockable)
  {
  	struct amdgpu_mn *rmn = container_of(mn, struct amdgpu_mn, mn);
  	struct interval_tree_node *it;

@@ -208,7 +215,11 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
  	/* notification is exclusive, but interval is inclusive */
  	end -= 1;
-	amdgpu_mn_read_lock(rmn);
+	/* TODO we should be able to split locking for interval tree and
+	 * amdgpu_mn_invalidate_node
+	 */
+	if (amdgpu_mn_read_lock(rmn, blockable))
+		return -EAGAIN;
  	it = interval_tree_iter_first(&rmn->objects, start, end);
  	while (it) {

@@ -219,6 +230,8 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
  		amdgpu_mn_invalidate_node(node, start, end);
  	}
+
+	return 0;
  }
  /**

@@ -233,10 +246,11 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
   * necessitates evicting all user-mode queues of the process. The BOs
   * are restorted in amdgpu_mn_invalidate_range_end_hsa.
   */
-static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
+static int amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  						 struct mm_struct *mm,
  						 unsigned long start,
-						 unsigned long end)
+						 unsigned long end,
+						 bool blockable)
  {
  	struct amdgpu_mn *rmn = container_of(mn, struct amdgpu_mn, mn);
  	struct interval_tree_node *it;

@@ -244,7 +258,8 @@ static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  	/* notification is exclusive, but interval is inclusive */
  	end -= 1;
-	amdgpu_mn_read_lock(rmn);
+	if (amdgpu_mn_read_lock(rmn, blockable))
+		return -EAGAIN;
  	it = interval_tree_iter_first(&rmn->objects, start, end);
  	while (it) {

Quoted 8 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn, · line 283

Quoted 8 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +2 -0

@@ -262,6 +277,8 @@ static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  				amdgpu_amdkfd_evict_userptr(mem, mm);
  		}
  	}
+
+	return 0;
  }
  /**

Hi Michal,

[Adding Felix as well]

Well first of all you have a misconception why at least the AMD graphics driver need to be able to sleep in an MMU notifier: We need to sleep because we need to wait for hardware operations to finish and *NOT* because we need to wait for locks.

I'm not sure if your flag now means that you generally can't sleep in MMU notifiers any more, but if that's the case at least AMD hardware will break badly. In our case the approach of waiting for a short time for the process to be reaped and then select another victim actually sounds like the right thing to do.

Quoted 86 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +25 -8

index 83e344fbb50a..d138a526feff 100644
@@ -136,12 +136,18 @@ void amdgpu_mn_unlock(struct amdgpu_mn *mn)
   *
   * Take the rmn read side lock.
   */
-static void amdgpu_mn_read_lock(struct amdgpu_mn *rmn)
+static int amdgpu_mn_read_lock(struct amdgpu_mn *rmn, bool blockable)
  {
-	mutex_lock(&rmn->read_lock);
+	if (blockable)
+		mutex_lock(&rmn->read_lock);
+	else if (!mutex_trylock(&rmn->read_lock))
+		return -EAGAIN;
+
  	if (atomic_inc_return(&rmn->recursion) == 1)
  		down_read_non_owner(&rmn->lock);
  	mutex_unlock(&rmn->read_lock);
+
+	return 0;
  }
  /**

@@ -197,10 +203,11 @@ static void amdgpu_mn_invalidate_node(struct amdgpu_mn_node *node,
   * We block for all BOs between start and end to be idle and
   * unmap them by move them into system domain again.
   */
-static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
+static int amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
  						 struct mm_struct *mm,
  						 unsigned long start,
-						 unsigned long end)
+						 unsigned long end,
+						 bool blockable)
  {
  	struct amdgpu_mn *rmn = container_of(mn, struct amdgpu_mn, mn);
  	struct interval_tree_node *it;

@@ -208,7 +215,11 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
  	/* notification is exclusive, but interval is inclusive */
  	end -= 1;
-	amdgpu_mn_read_lock(rmn);
+	/* TODO we should be able to split locking for interval tree and
+	 * amdgpu_mn_invalidate_node
+	 */
+	if (amdgpu_mn_read_lock(rmn, blockable))
+		return -EAGAIN;
  	it = interval_tree_iter_first(&rmn->objects, start, end);
  	while (it) {

@@ -219,6 +230,8 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
  		amdgpu_mn_invalidate_node(node, start, end);
  	}
+
+	return 0;
  }
  /**

@@ -233,10 +246,11 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
   * necessitates evicting all user-mode queues of the process. The BOs
   * are restorted in amdgpu_mn_invalidate_range_end_hsa.
   */
-static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
+static int amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  						 struct mm_struct *mm,
  						 unsigned long start,
-						 unsigned long end)
+						 unsigned long end,
+						 bool blockable)
  {
  	struct amdgpu_mn *rmn = container_of(mn, struct amdgpu_mn, mn);
  	struct interval_tree_node *it;

@@ -244,7 +258,8 @@ static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  	/* notification is exclusive, but interval is inclusive */
  	end -= 1;
-	amdgpu_mn_read_lock(rmn);
+	if (amdgpu_mn_read_lock(rmn, blockable))
+		return -EAGAIN;
  	it = interval_tree_iter_first(&rmn->objects, start, end);
  	while (it) {

@@ -262,6 +277,8 @@ static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  				amdgpu_amdkfd_evict_userptr(mem, mm);
  		}
  	}
+
+	return 0;
  }
  /**

From: Michal Hocko <[email protected]>

There are several blockable mmu notifiers which might sleep in mmu_notifier_invalidate_range_start and that is a problem for the oom_reaper because it needs to guarantee a forward progress so it cannot depend on any sleepable locks. Currently we simply back off and mark an oom victim with blockable mmu notifiers as done after a short sleep. That can result in selecting a new oom victim prematurely because the previous one still hasn't torn its memory down yet.

We can do much better though. Even if mmu notifiers use sleepable locks there is no reason to automatically assume those locks are held. Moreover most notifiers only care about a portion of the address space. This patch handles the first part of the problem. __mmu_notifier_invalidate_range_start gets a blockable flag and callbacks are not allowed to sleep if the flag is set to false. This is achieved by using trylock instead of the sleepable lock for most callbacks. I think we can improve that even further because there is a common pattern to do a range lookup first and then do something about that. The first part can be done without a sleeping lock I presume.

Anyway, what does the oom_reaper do with all that? We do not have to fail right away. We simply retry if there is at least one notifier which couldn't make any progress. A retry loop is already implemented to wait for the mmap_sem and this is basically the same thing.

Cc "David (ChunMing) Zhou" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc Alex Deucher <[email protected]>

Cc "Christian König" <[email protected]>

Cc David Airlie <[email protected]>

Cc Jani Nikula <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Cc Doug Ledford <[email protected]>

Cc Jason Gunthorpe <[email protected]>

Cc Mike Marciniszyn <[email protected]>

Cc Dennis Dalessandro <[email protected]>

Cc Sudeep Dutt <[email protected]>

Cc Ashutosh Dixit <[email protected]>

Cc Dimitri Sivanich <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Cc Juergen Gross <[email protected]>

Cc "Jérôme Glisse" <[email protected]>

Cc Andrea Arcangeli <[email protected]>

Cc [email protected] (open list:KERNEL VIRTUAL MACHINE FOR X86 (KVM/x86))

Cc [email protected] (open list:X86 ARCHITECTURE (32-BIT AND 64-BIT))

Cc [email protected] (open list:RADEON and AMDGPU DRM DRIVERS)

Cc [email protected] (open list:DRM DRIVERS)

Cc [email protected] (open list:INTEL DRM DRIVERS (excluding Poulsbo, Moorestow...)

Cc [email protected] (open list:INFINIBAND SUBSYSTEM)

Cc [email protected] (moderated list:XEN HYPERVISOR INTERFACE)

Cc [email protected] (open list:HMM - Heterogeneous Memory Management)

Reported-by David Rientjes <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---

Hi,
this is an RFC and not tested at all. I am not very familiar with the mmu notifiers semantics very much so this is a crude attempt to achieve what I need basically. It might be completely wrong but I would like to discuss what would be a better way if that is the case.

get_maintainers gave me quite large list of people to CC so I had to trim it down. If you think I have forgot somebody, please let me know

Any feedback is highly appreciated.

  arch/x86/kvm/x86.c                      |  7 ++++--
  drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c  | 33 +++++++++++++++++++------
  drivers/gpu/drm/i915/i915_gem_userptr.c | 10 +++++---
  drivers/gpu/drm/radeon/radeon_mn.c      | 15 ++++++++---
  drivers/infiniband/core/umem_odp.c      | 15 ++++++++---
  drivers/infiniband/hw/hfi1/mmu_rb.c     |  7 ++++--
  drivers/misc/mic/scif/scif_dma.c        |  7 ++++--
  drivers/misc/sgi-gru/grutlbpurge.c      |  7 ++++--
  drivers/xen/gntdev.c                    | 14 ++++++++---
  include/linux/kvm_host.h                |  2 +-
  include/linux/mmu_notifier.h            | 15 +++++++++--
  mm/hmm.c                                |  7 ++++--
  mm/mmu_notifier.c                       | 15 ++++++++---
  mm/oom_kill.c                           | 29 +++++++++++-----------
  virt/kvm/kvm_main.c                     | 12 ++++++---
  15 files changed, 137 insertions(+), 58 deletions(-)

arch/x86/kvm/x86.c +5 -2

index 6bcecc325e7e..ac08f5d711be 100644
@@ -7203,8 +7203,9 @@ static void vcpu_load_eoi_exitmap(struct kvm_vcpu *vcpu)
  	kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
  }
 
-void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
-		unsigned long start, unsigned long end)
+int kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
+		unsigned long start, unsigned long end,
+		bool blockable)
  {
  	unsigned long apic_address;
 

@@ -7215,6 +7216,8 @@ void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
  	apic_address = gfn_to_hva(kvm, APIC_DEFAULT_PHYS_BASE >> PAGE_SHIFT);
  	if (start <= apic_address && apic_address < end)
  		kvm_make_all_cpus_request(kvm, KVM_REQ_APIC_PAGE_RELOAD);
+
+	return 0;
  }
 
  void kvm_vcpu_reload_apic_access_page(struct kvm_vcpu *vcpu)

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +25 -8

index 83e344fbb50a..d138a526feff 100644
@@ -136,12 +136,18 @@ void amdgpu_mn_unlock(struct amdgpu_mn *mn)
   *
   * Take the rmn read side lock.
   */
-static void amdgpu_mn_read_lock(struct amdgpu_mn *rmn)
+static int amdgpu_mn_read_lock(struct amdgpu_mn *rmn, bool blockable)
  {
-	mutex_lock(&rmn->read_lock);
+	if (blockable)
+		mutex_lock(&rmn->read_lock);
+	else if (!mutex_trylock(&rmn->read_lock))
+		return -EAGAIN;
+
  	if (atomic_inc_return(&rmn->recursion) == 1)
  		down_read_non_owner(&rmn->lock);
  	mutex_unlock(&rmn->read_lock);
+
+	return 0;
  }
 
  /**

@@ -197,10 +203,11 @@ static void amdgpu_mn_invalidate_node(struct amdgpu_mn_node *node,
   * We block for all BOs between start and end to be idle and
   * unmap them by move them into system domain again.
   */
-static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
+static int amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
  						 struct mm_struct *mm,
  						 unsigned long start,
-						 unsigned long end)
+						 unsigned long end,
+						 bool blockable)
  {
  	struct amdgpu_mn *rmn = container_of(mn, struct amdgpu_mn, mn);
  	struct interval_tree_node *it;

@@ -208,7 +215,11 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
  	/* notification is exclusive, but interval is inclusive */
  	end -= 1;
 
-	amdgpu_mn_read_lock(rmn);
+	/* TODO we should be able to split locking for interval tree and
+	 * amdgpu_mn_invalidate_node
+	 */
+	if (amdgpu_mn_read_lock(rmn, blockable))
+		return -EAGAIN;
 
  	it = interval_tree_iter_first(&rmn->objects, start, end);
  	while (it) {

@@ -219,6 +230,8 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
 
  		amdgpu_mn_invalidate_node(node, start, end);
  	}
+
+	return 0;
  }
 
  /**

@@ -233,10 +246,11 @@ static void amdgpu_mn_invalidate_range_start_gfx(struct mmu_notifier *mn,
   * necessitates evicting all user-mode queues of the process. The BOs
   * are restorted in amdgpu_mn_invalidate_range_end_hsa.
   */
-static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
+static int amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  						 struct mm_struct *mm,
  						 unsigned long start,
-						 unsigned long end)
+						 unsigned long end,
+						 bool blockable)
  {
  	struct amdgpu_mn *rmn = container_of(mn, struct amdgpu_mn, mn);
  	struct interval_tree_node *it;

@@ -244,7 +258,8 @@ static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  	/* notification is exclusive, but interval is inclusive */
  	end -= 1;
 
-	amdgpu_mn_read_lock(rmn);
+	if (amdgpu_mn_read_lock(rmn, blockable))
+		return -EAGAIN;
 
  	it = interval_tree_iter_first(&rmn->objects, start, end);
  	while (it) {

@@ -262,6 +277,8 @@ static void amdgpu_mn_invalidate_range_start_hsa(struct mmu_notifier *mn,
  				amdgpu_amdkfd_evict_userptr(mem, mm);
  		}
  	}
+
+	return 0;
  }
 
  /**

drivers/gpu/drm/i915/i915_gem_userptr.c +6 -4

index 854bd51b9478..5285df9331fa 100644
@@ -112,10 +112,11 @@ static void del_object(struct i915_mmu_object *mo)
  	mo->attached = false;
  }
 
-static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
+static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
  						       struct mm_struct *mm,
  						       unsigned long start,
-						       unsigned long end)
+						       unsigned long end,
+						       bool blockable)
  {
  	struct i915_mmu_notifier *mn =
  		container_of(_mn, struct i915_mmu_notifier, mn);

@@ -124,7 +125,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
  	LIST_HEAD(cancelled);
 
  	if (RB_EMPTY_ROOT(&mn->objects.rb_root))
-		return;
+		return 0;
 
  	/* interval ranges are inclusive, but invalidate range is exclusive */
  	end--;

@@ -152,7 +153,8 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
  		del_object(mo);
  	spin_unlock(&mn->lock);
 
-	if (!list_empty(&cancelled))
+	/* TODO: can we skip waiting here? */
+	if (!list_empty(&cancelled) && blockable)
  		flush_workqueue(mn->wq);
  }
 

drivers/gpu/drm/radeon/radeon_mn.c +12 -3

index abd24975c9b1..b47e828b725d 100644
@@ -118,10 +118,11 @@ static void radeon_mn_release(struct mmu_notifier *mn,
   * We block for all BOs between start and end to be idle and
   * unmap them by move them into system domain again.
   */
-static void radeon_mn_invalidate_range_start(struct mmu_notifier *mn,
+static int radeon_mn_invalidate_range_start(struct mmu_notifier *mn,
  					     struct mm_struct *mm,
  					     unsigned long start,
-					     unsigned long end)
+					     unsigned long end,
+					     bool blockable)
  {
  	struct radeon_mn *rmn = container_of(mn, struct radeon_mn, mn);
  	struct ttm_operation_ctx ctx = { false, false };

@@ -130,7 +131,13 @@ static void radeon_mn_invalidate_range_start(struct mmu_notifier *mn,
  	/* notification is exclusive, but interval is inclusive */
  	end -= 1;
 
-	mutex_lock(&rmn->lock);
+	/* TODO we should be able to split locking for interval tree and
+	 * the tear down.
+	 */
+	if (blockable)
+		mutex_lock(&rmn->lock);
+	else if (!mutex_trylock(&rmn->lock))
+		return -EAGAIN;
 
  	it = interval_tree_iter_first(&rmn->objects, start, end);
  	while (it) {

@@ -167,6 +174,8 @@ static void radeon_mn_invalidate_range_start(struct mmu_notifier *mn,
  	}
 
  	mutex_unlock(&rmn->lock);
+
+	return 0;
  }
 
  static const struct mmu_notifier_ops radeon_mn_ops = {

drivers/infiniband/core/umem_odp.c +11 -4

index 182436b92ba9..f65f6a29daae 100644
@@ -207,22 +207,29 @@ static int invalidate_range_start_trampoline(struct ib_umem *item, u64 start,
  	return 0;
  }
 
-static void ib_umem_notifier_invalidate_range_start(struct mmu_notifier *mn,
+static int ib_umem_notifier_invalidate_range_start(struct mmu_notifier *mn,
  						    struct mm_struct *mm,
  						    unsigned long start,
-						    unsigned long end)
+						    unsigned long end,
+						    bool blockable)
  {
  	struct ib_ucontext *context = container_of(mn, struct ib_ucontext, mn);
 
  	if (!context->invalidate_range)
-		return;
+		return 0;
+
+	if (blockable)
+		down_read(&context->umem_rwsem);
+	else if (!down_read_trylock(&context->umem_rwsem))
+		return -EAGAIN;
 
  	ib_ucontext_notifier_start_account(context);
-	down_read(&context->umem_rwsem);
  	rbt_ib_umem_for_each_in_range(&context->umem_tree, start,
  				      end,
  				      invalidate_range_start_trampoline, NULL);
  	up_read(&context->umem_rwsem);
+
+	return 0;
  }
 
  static int invalidate_range_end_trampoline(struct ib_umem *item, u64 start,

drivers/infiniband/hw/hfi1/mmu_rb.c +5 -2

index 70aceefe14d5..8780560d1623 100644
@@ -284,10 +284,11 @@ void hfi1_mmu_rb_remove(struct mmu_rb_handler *handler,
  	handler->ops->remove(handler->ops_arg, node);
  }
 
-static void mmu_notifier_range_start(struct mmu_notifier *mn,
+static int mmu_notifier_range_start(struct mmu_notifier *mn,
  				     struct mm_struct *mm,
  				     unsigned long start,
-				     unsigned long end)
+				     unsigned long end,
+				     bool blockable)
  {
  	struct mmu_rb_handler *handler =
  		container_of(mn, struct mmu_rb_handler, mn);

@@ -313,6 +314,8 @@ static void mmu_notifier_range_start(struct mmu_notifier *mn,
 
  	if (added)
  		queue_work(handler->wq, &handler->del_work);
+
+	return 0;
  }
 
  /*

drivers/misc/mic/scif/scif_dma.c +5 -2

index 63d6246d6dff..d940568bed87 100644
@@ -200,15 +200,18 @@ static void scif_mmu_notifier_release(struct mmu_notifier *mn,
  	schedule_work(&scif_info.misc_work);
  }
 
-static void scif_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
+static int scif_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
  						     struct mm_struct *mm,
  						     unsigned long start,
-						     unsigned long end)
+						     unsigned long end,
+						     bool blockable)
  {
  	struct scif_mmu_notif	*mmn;
 
  	mmn = container_of(mn, struct scif_mmu_notif, ep_mmu_notifier);
  	scif_rma_destroy_tcw(mmn, start, end - start);
+
+	return 0
  }
 
  static void scif_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,

drivers/misc/sgi-gru/grutlbpurge.c +5 -2

index a3454eb56fbf..be28f05bfafa 100644
@@ -219,9 +219,10 @@ void gru_flush_all_tlb(struct gru_state *gru)
  /*
   * MMUOPS notifier callout functions
   */
-static void gru_invalidate_range_start(struct mmu_notifier *mn,
+static int gru_invalidate_range_start(struct mmu_notifier *mn,
  				       struct mm_struct *mm,
-				       unsigned long start, unsigned long end)
+				       unsigned long start, unsigned long end,
+				       bool blockable)
  {
  	struct gru_mm_struct *gms = container_of(mn, struct gru_mm_struct,
  						 ms_notifier);

@@ -231,6 +232,8 @@ static void gru_invalidate_range_start(struct mmu_notifier *mn,
  	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);
+
+	return 0;
  }
 
  static void gru_invalidate_range_end(struct mmu_notifier *mn,

drivers/xen/gntdev.c +11 -3

index bd56653b9bbc..50724d09fe5c 100644
@@ -465,14 +465,20 @@ static void unmap_if_in_range(struct grant_map *map,
  	WARN_ON(err);
  }
 
-static void mn_invl_range_start(struct mmu_notifier *mn,
+static int mn_invl_range_start(struct mmu_notifier *mn,
  				struct mm_struct *mm,
-				unsigned long start, unsigned long end)
+				unsigned long start, unsigned long end,
+				bool blockable)
  {
  	struct gntdev_priv *priv = container_of(mn, struct gntdev_priv, mn);
  	struct grant_map *map;
 
-	mutex_lock(&priv->lock);
+	/* TODO do we really need a mutex here? */
+	if (blockable)
+		mutex_lock(&priv->lock);
+	else if (!mutex_trylock(&priv->lock))
+		return -EAGAIN;
+
  	list_for_each_entry(map, &priv->maps, next) {
  		unmap_if_in_range(map, start, end);
  	}

@@ -480,6 +486,8 @@ static void mn_invl_range_start(struct mmu_notifier *mn,
  		unmap_if_in_range(map, start, end);
  	}
  	mutex_unlock(&priv->lock);
+
+	return true;
  }
 
  static void mn_release(struct mmu_notifier *mn,

include/linux/kvm_host.h +1 -1

index 4ee7bc548a83..e4181063e755 100644
@@ -1275,7 +1275,7 @@ static inline long kvm_arch_vcpu_async_ioctl(struct file *filp,
  }
  #endif /* CONFIG_HAVE_KVM_VCPU_ASYNC_IOCTL */
 
-void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
+int kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
  		unsigned long start, unsigned long end);
 
  #ifdef CONFIG_HAVE_KVM_VCPU_RUN_PID_CHANGE

include/linux/mmu_notifier.h +13 -2

index 392e6af82701..369867501bed 100644
@@ -230,7 +230,8 @@ extern int __mmu_notifier_test_young(struct mm_struct *mm,
  extern void __mmu_notifier_change_pte(struct mm_struct *mm,
  				      unsigned long address, pte_t pte);
  extern void __mmu_notifier_invalidate_range_start(struct mm_struct *mm,
-				  unsigned long start, unsigned long end);
+				  unsigned long start, unsigned long end,
+				  bool blockable);
  extern void __mmu_notifier_invalidate_range_end(struct mm_struct *mm,
  				  unsigned long start, unsigned long end,
  				  bool only_end);

@@ -281,7 +282,17 @@ static inline void mmu_notifier_invalidate_range_start(struct mm_struct *mm,
  				  unsigned long start, unsigned long end)
  {
  	if (mm_has_notifiers(mm))
-		__mmu_notifier_invalidate_range_start(mm, start, end);
+		__mmu_notifier_invalidate_range_start(mm, start, end, true);
+}
+
+static inline int mmu_notifier_invalidate_range_start_nonblock(struct mm_struct *mm,
+				  unsigned long start, unsigned long end)
+{
+	int ret = 0;
+	if (mm_has_notifiers(mm))
+		ret = __mmu_notifier_invalidate_range_start(mm, start, end, false);
+
+	return ret;
  }
 
  static inline void mmu_notifier_invalidate_range_end(struct mm_struct *mm,

mm/hmm.c +5 -2

index de7b6bf77201..81fd57bd2634 100644
@@ -177,16 +177,19 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
  	up_write(&hmm->mirrors_sem);
  }
 
-static void hmm_invalidate_range_start(struct mmu_notifier *mn,
+static int hmm_invalidate_range_start(struct mmu_notifier *mn,
  				       struct mm_struct *mm,
  				       unsigned long start,
-				       unsigned long end)
+				       unsigned long end,
+				       bool blockable)
  {
  	struct hmm *hmm = mm->hmm;
 
  	VM_BUG_ON(!hmm);
 
  	atomic_inc(&hmm->sequence);
+
+	return 0;
  }
 
  static void hmm_invalidate_range_end(struct mmu_notifier *mn,

mm/mmu_notifier.c +11 -4

index eff6b88a993f..30cc43121da9 100644
@@ -174,18 +174,25 @@ void __mmu_notifier_change_pte(struct mm_struct *mm, unsigned long address,
  	srcu_read_unlock(&srcu, id);
  }
 
-void __mmu_notifier_invalidate_range_start(struct mm_struct *mm,
-				  unsigned long start, unsigned long end)
+int __mmu_notifier_invalidate_range_start(struct mm_struct *mm,
+				  unsigned long start, unsigned long end,
+				  bool blockable)
  {
  	struct mmu_notifier *mn;
+	int ret = 0;
  	int id;
 
  	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);
+		if (mn->ops->invalidate_range_start) {
+			int _ret = mn->ops->invalidate_range_start(mn, mm, start, end, blockable);
+			if (_ret)
+				ret = _ret;
+		}
  	}
  	srcu_read_unlock(&srcu, id);
+
+	return ret;
  }
  EXPORT_SYMBOL_GPL(__mmu_notifier_invalidate_range_start);
 

mm/oom_kill.c +14 -15

index 84081e77bc51..7e0c6e78ae5c 100644
@@ -479,9 +479,10 @@ static DECLARE_WAIT_QUEUE_HEAD(oom_reaper_wait);
  static struct task_struct *oom_reaper_list;
  static DEFINE_SPINLOCK(oom_reaper_lock);
 
-void __oom_reap_task_mm(struct mm_struct *mm)
+bool __oom_reap_task_mm(struct mm_struct *mm)
  {
  	struct vm_area_struct *vma;
+	bool ret = true;
 
  	/*
  	 * Tell all users of get_user/copy_from_user etc... that the content

@@ -511,12 +512,17 @@ void __oom_reap_task_mm(struct mm_struct *mm)
  			struct mmu_gather tlb;
 
  			tlb_gather_mmu(&tlb, mm, start, end);
-			mmu_notifier_invalidate_range_start(mm, start, end);
+			if (mmu_notifier_invalidate_range_start_nonblock(mm, start, end)) {
+				ret = false;
+				continue;
+			}
  			unmap_page_range(&tlb, vma, start, end, NULL);
  			mmu_notifier_invalidate_range_end(mm, start, end);
  			tlb_finish_mmu(&tlb, start, end);
  		}
  	}
+
+	return ret;
  }
 
  static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)

@@ -545,18 +551,6 @@ static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
  		goto unlock_oom;
  	}
 
-	/*
-	 * If the mm has invalidate_{start,end}() notifiers that could block,
-	 * sleep to give the oom victim some more time.
-	 * TODO: we really want to get rid of this ugly hack and make sure that
-	 * notifiers cannot block for unbounded amount of time
-	 */
-	if (mm_has_blockable_invalidate_notifiers(mm)) {
-		up_read(&mm->mmap_sem);
-		schedule_timeout_idle(HZ);
-		goto unlock_oom;
-	}
-
  	/*
  	 * MMF_OOM_SKIP is set by exit_mmap when the OOM reaper can't
  	 * work on the mm anymore. The check for MMF_OOM_SKIP must run

@@ -571,7 +565,12 @@ static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 
  	trace_start_task_reaping(tsk->pid);
 
-	__oom_reap_task_mm(mm);
+	/* failed to reap part of the address space. Try again later */
+	if (!__oom_reap_task_mm(mm)) {
+		up_read(&mm->mmap_sem);
+		ret = false;
+		goto out_unlock;
+	}
 
  	pr_info("oom_reaper: reaped process %d (%s), now anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB\n",
  			task_pid_nr(tsk), tsk->comm,

virt/kvm/kvm_main.c +5 -3

index ada21f47f22b..6f7e709d2944 100644
@@ -135,7 +135,7 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm);
  static unsigned long long kvm_createvm_count;
  static unsigned long long kvm_active_vms;
 
-__weak void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
+__weak int kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
  		unsigned long start, unsigned long end)
  {
  }

@@ -354,13 +354,15 @@ static void kvm_mmu_notifier_change_pte(struct mmu_notifier *mn,
  	srcu_read_unlock(&kvm->srcu, idx);
  }
 
-static void kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
+static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
  						    struct mm_struct *mm,
  						    unsigned long start,
-						    unsigned long end)
+						    unsigned long end,
+						    bool blockable)
  {
  	struct kvm *kvm = mmu_notifier_to_kvm(mn);
  	int need_tlb_flush = 0, idx;
+	int ret;
 
  	idx = srcu_read_lock(&kvm->srcu);
  	spin_lock(&kvm->mmu_lock);

@@ -378,9 +380,11 @@ static void kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
 
  	spin_unlock(&kvm->mmu_lock);

Hi,
this is an RFC and not tested at all. I am not very familiar with the mmu notifiers semantics very much so this is a crude attempt to achieve what I need basically. It might be completely wrong but I would like to discuss what would be a better way if that is the case.

get_maintainers gave me quite large list of people to CC so I had to trim it down. If you think I have forgot somebody, please let me know

drivers/gpu/drm/i915/i915_gem_userptr.c +3 -2

index 854bd51b9478..5285df9331fa 100644
@@ -112,10 +112,11 @@ static void del_object(struct i915_mmu_object *mo)
        mo->attached = false;
 }
 
-static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
+static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
                                                       struct mm_struct *mm,
                                                       unsigned long start,
-                                                      unsigned long end)
+                                                      unsigned long end,
+                                                      bool blockable)
 {
        struct i915_mmu_notifier *mn =
                container_of(_mn, struct i915_mmu_notifier, mn);

Quoting Michal Hocko (2018-06-22 16:02:42)

Quoted 30 lines

Hi,
this is an RFC and not tested at all. I am not very familiar with the mmu notifiers semantics very much so this is a crude attempt to achieve what I need basically. It might be completely wrong but I would like to discuss what would be a better way if that is the case.

get_maintainers gave me quite large list of people to CC so I had to trim it down. If you think I have forgot somebody, please let me know

drivers/gpu/drm/i915/i915_gem_userptr.c +3 -2

index 854bd51b9478..5285df9331fa 100644
@@ -112,10 +112,11 @@ static void del_object(struct i915_mmu_object *mo)
        mo->attached = false;
 }
 
-static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
+static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
                                                       struct mm_struct *mm,
                                                       unsigned long start,
-                                                      unsigned long end)
+                                                      unsigned long end,
+                                                      bool blockable)
 {
        struct i915_mmu_notifier *mn =
                container_of(_mn, struct i915_mmu_notifier, mn);

@@ -124,7 +125,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
        LIST_HEAD(cancelled);
 

drivers/gpu/drm/i915/i915_gem_userptr.c static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, · line 128

Quoted 3 lines

drivers/gpu/drm/i915/i915_gem_userptr.c +1 -1

        if (RB_EMPTY_ROOT(&mn->objects.rb_root))
-               return;
+               return 0;

The principle wait here is for the HW (even after fixing all the locks to be not so coarse, we still have to wait for the HW to finish its access).

The first pass would be then to not do anything here if !blockable.

Quoted 31 lines · Chris Wilson

Quoting Michal Hocko (2018-06-22 16:02:42)

Quoted 30 lines

Hi,
this is an RFC and not tested at all. I am not very familiar with the mmu notifiers semantics very much so this is a crude attempt to achieve what I need basically. It might be completely wrong but I would like to discuss what would be a better way if that is the case.

get_maintainers gave me quite large list of people to CC so I had to trim it down. If you think I have forgot somebody, please let me know

drivers/gpu/drm/i915/i915_gem_userptr.c +3 -2

index 854bd51b9478..5285df9331fa 100644
@@ -112,10 +112,11 @@ static void del_object(struct i915_mmu_object *mo)
        mo->attached = false;
 }
 
-static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
+static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
                                                       struct mm_struct *mm,
                                                       unsigned long start,
-                                                      unsigned long end)
+                                                      unsigned long end,
+                                                      bool blockable)
 {
        struct i915_mmu_notifier *mn =
                container_of(_mn, struct i915_mmu_notifier, mn);

@@ -124,7 +125,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
        LIST_HEAD(cancelled);
 

Quoted 7 lines

drivers/gpu/drm/i915/i915_gem_userptr.c static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, · line 128

Quoted 3 lines

drivers/gpu/drm/i915/i915_gem_userptr.c +1 -1

        if (RB_EMPTY_ROOT(&mn->objects.rb_root))
-               return;
+               return 0;

The principle wait here is for the HW (even after fixing all the locks to be not so coarse, we still have to wait for the HW to finish its access).

Is this wait bound or it can take basically arbitrary amount of time?

Quoted 3 lines

The first pass would be then to not do anything here if !blockable.

something like this? (incremental diff)

Quoted 41 lines · Jerome Glisse

Quoted 40 lines · Michal Hocko

Quoted 38 lines · Chris Wilson

Quoting Michal Hocko (2018-06-22 16:02:42)

Quoted 28 lines

Hi,
this is an RFC and not tested at all. I am not very familiar with the mmu notifiers semantics very much so this is a crude attempt to achieve what I need basically. It might be completely wrong but I would like to discuss what would be a better way if that is the case.

get_maintainers gave me quite large list of people to CC so I had to trim it down. If you think I have forgot somebody, please let me know

drivers/gpu/drm/i915/i915_gem_userptr.c +3 -2

index 854bd51b9478..5285df9331fa 100644
@@ -112,10 +112,11 @@ static void del_object(struct i915_mmu_object *mo)
        mo->attached = false;
 }
 
-static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
+static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
                                                       struct mm_struct *mm,
                                                       unsigned long start,
-                                                      unsigned long end)
+                                                      unsigned long end,
+                                                      bool blockable)
 {
        struct i915_mmu_notifier *mn =
                container_of(_mn, struct i915_mmu_notifier, mn);

drivers/gpu/drm/i915/i915_gem_userptr.c static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, · line 128

Quoted 5 lines

drivers/gpu/drm/i915/i915_gem_userptr.c +1 -1

@@ -124,7 +125,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
        LIST_HEAD(cancelled);
 
        if (RB_EMPTY_ROOT(&mn->objects.rb_root))
-               return;
+               return 0;

The principle wait here is for the HW (even after fixing all the locks to be not so coarse, we still have to wait for the HW to finish its access).

Quoted 7 lines

Quoted 1 line

Is this wait bound or it can take basically arbitrary amount of time?

Arbitrary amount of time but in desktop use case you can assume that it should never go above 16ms for a 60frame per second rendering of your desktop (in GPU compute case this kind of assumption does not hold). Is the process exit_state already updated by the time this mmu notifier callbacks happen ?

What do you mean? The process is killed (by SIGKILL) at the time but we do not know much more than that. The task might be stuck anywhere in the kernel before handling that signal.

Quoted 11 lines

Quoted 4 lines

Quoted 2 lines

The first pass would be then to not do anything here if !blockable.

something like this? (incremental diff)

What i wanted to do with HMM and mmu notifier is split the invalidation in 2 pass. First pass tell the drivers to stop/cancel pending jobs that depends on the range and invalidate internal driver states (like clear buffer object pages array in case of GPU but not GPU page table). While the second callback would do the actual wait on the GPU to be done and update the GPU page table.

What can you do after the first phase? Can I unmap the range?

Quoted 10 lines

Now in this scheme in case the task is already in some exit state and that all CPU threads are frozen/kill then we can probably find a way to do the first path mostly lock less. AFAICR nor AMD nor Intel allow to share userptr bo hence a uptr bo should only ever be access through ioctl submited by the process.

The second call can then be delayed and ping from time to time to see if GPU jobs are done.

Quoted 7 lines

Note that what you propose might still be useful as in case there is no buffer object for a range then OOM can make progress in freeing a range of memory. It is very likely that significant virtual address range of a process and backing memory can be reclaim that way. This assume OOM reclaim vma by vma or in some form of granularity like reclaiming 1GB by 1GB. Or we could also update blocking callback to return range that are blocking that way OOM can reclaim around.

Exactly my point. What we have right now is all or nothing which is obviously too coarse to be useful.

[Resnding with the CC list fixed]

Quoted 53 lines · Michal Hocko

Quoted 41 lines · Jerome Glisse

Quoted 40 lines · Michal Hocko

Quoted 38 lines · Chris Wilson

Quoting Michal Hocko (2018-06-22 16:02:42)

Quoted 28 lines

Hi,
this is an RFC and not tested at all. I am not very familiar with the mmu notifiers semantics very much so this is a crude attempt to achieve what I need basically. It might be completely wrong but I would like to discuss what would be a better way if that is the case.

get_maintainers gave me quite large list of people to CC so I had to trim it down. If you think I have forgot somebody, please let me know

drivers/gpu/drm/i915/i915_gem_userptr.c +3 -2

index 854bd51b9478..5285df9331fa 100644
@@ -112,10 +112,11 @@ static void del_object(struct i915_mmu_object *mo)
        mo->attached = false;
 }
 
-static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
+static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
                                                       struct mm_struct *mm,
                                                       unsigned long start,
-                                                      unsigned long end)
+                                                      unsigned long end,
+                                                      bool blockable)
 {
        struct i915_mmu_notifier *mn =
                container_of(_mn, struct i915_mmu_notifier, mn);

drivers/gpu/drm/i915/i915_gem_userptr.c static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, · line 128

Quoted 5 lines

drivers/gpu/drm/i915/i915_gem_userptr.c +1 -1

@@ -124,7 +125,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
        LIST_HEAD(cancelled);
 
        if (RB_EMPTY_ROOT(&mn->objects.rb_root))
-               return;
+               return 0;

The principle wait here is for the HW (even after fixing all the locks to be not so coarse, we still have to wait for the HW to finish its access).

Quoted 7 lines

Quoted 1 line

Is this wait bound or it can take basically arbitrary amount of time?

Arbitrary amount of time but in desktop use case you can assume that it should never go above 16ms for a 60frame per second rendering of your desktop (in GPU compute case this kind of assumption does not hold). Is the process exit_state already updated by the time this mmu notifier callbacks happen ?

What do you mean? The process is killed (by SIGKILL) at the time but we do not know much more than that. The task might be stuck anywhere in the kernel before handling that signal.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

The first pass would be then to not do anything here if !blockable.

something like this? (incremental diff)

Quoted 8 lines

Quoted 6 lines

What i wanted to do with HMM and mmu notifier is split the invalidation in 2 pass. First pass tell the drivers to stop/cancel pending jobs that depends on the range and invalidate internal driver states (like clear buffer object pages array in case of GPU but not GPU page table). While the second callback would do the actual wait on the GPU to be done and update the GPU page table.

What can you do after the first phase? Can I unmap the range?

Quoted 19 lines

Quoted 18 lines

Now in this scheme in case the task is already in some exit state and that all CPU threads are frozen/kill then we can probably find a way to do the first path mostly lock less. AFAICR nor AMD nor Intel allow to share userptr bo hence a uptr bo should only ever be access through ioctl submited by the process.

The second call can then be delayed and ping from time to time to see if GPU jobs are done.

Note that what you propose might still be useful as in case there is no buffer object for a range then OOM can make progress in freeing a range of memory. It is very likely that significant virtual address range of a process and backing memory can be reclaim that way. This assume OOM reclaim vma by vma or in some form of granularity like reclaiming 1GB by 1GB. Or we could also update blocking callback to return range that are blocking that way OOM can reclaim around.

Quoted 2 lines

Exactly my point. What we have right now is all or nothing which is obviously too coarse to be useful.

[Resnding with the CC list fixed]

Quoted 53 lines · Michal Hocko

Quoted 41 lines · Jerome Glisse

Quoted 40 lines · Michal Hocko

Quoted 38 lines · Chris Wilson

Quoting Michal Hocko (2018-06-22 16:02:42)

Quoted 28 lines

Hi,
this is an RFC and not tested at all. I am not very familiar with the mmu notifiers semantics very much so this is a crude attempt to achieve what I need basically. It might be completely wrong but I would like to discuss what would be a better way if that is the case.

get_maintainers gave me quite large list of people to CC so I had to trim it down. If you think I have forgot somebody, please let me know

drivers/gpu/drm/i915/i915_gem_userptr.c +3 -2

index 854bd51b9478..5285df9331fa 100644
@@ -112,10 +112,11 @@ static void del_object(struct i915_mmu_object *mo)
        mo->attached = false;
 }
 
-static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
+static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
                                                       struct mm_struct *mm,
                                                       unsigned long start,
-                                                      unsigned long end)
+                                                      unsigned long end,
+                                                      bool blockable)
 {
        struct i915_mmu_notifier *mn =
                container_of(_mn, struct i915_mmu_notifier, mn);

drivers/gpu/drm/i915/i915_gem_userptr.c static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, · line 128

Quoted 5 lines

drivers/gpu/drm/i915/i915_gem_userptr.c +1 -1

@@ -124,7 +125,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
        LIST_HEAD(cancelled);
 
        if (RB_EMPTY_ROOT(&mn->objects.rb_root))
-               return;
+               return 0;

The principle wait here is for the HW (even after fixing all the locks to be not so coarse, we still have to wait for the HW to finish its access).

Quoted 7 lines

Quoted 1 line

Is this wait bound or it can take basically arbitrary amount of time?

Arbitrary amount of time but in desktop use case you can assume that it should never go above 16ms for a 60frame per second rendering of your desktop (in GPU compute case this kind of assumption does not hold). Is the process exit_state already updated by the time this mmu notifier callbacks happen ?

What do you mean? The process is killed (by SIGKILL) at the time but we do not know much more than that. The task might be stuck anywhere in the kernel before handling that signal.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

The first pass would be then to not do anything here if !blockable.

something like this? (incremental diff)

Quoted 8 lines

Quoted 6 lines

What i wanted to do with HMM and mmu notifier is split the invalidation in 2 pass. First pass tell the drivers to stop/cancel pending jobs that depends on the range and invalidate internal driver states (like clear buffer object pages array in case of GPU but not GPU page table). While the second callback would do the actual wait on the GPU to be done and update the GPU page table.

What can you do after the first phase? Can I unmap the range?

Quoted 19 lines

Quoted 18 lines

Now in this scheme in case the task is already in some exit state and that all CPU threads are frozen/kill then we can probably find a way to do the first path mostly lock less. AFAICR nor AMD nor Intel allow to share userptr bo hence a uptr bo should only ever be access through ioctl submited by the process.

The second call can then be delayed and ping from time to time to see if GPU jobs are done.

Note that what you propose might still be useful as in case there is no buffer object for a range then OOM can make progress in freeing a range of memory. It is very likely that significant virtual address range of a process and backing memory can be reclaim that way. This assume OOM reclaim vma by vma or in some form of granularity like reclaiming 1GB by 1GB. Or we could also update blocking callback to return range that are blocking that way OOM can reclaim around.

Quoted 2 lines

Exactly my point. What we have right now is all or nothing which is obviously too coarse to be useful.

Quoted 41 lines · Jerome Glisse

Quoted 40 lines · Michal Hocko

Quoted 38 lines · Chris Wilson

Quoting Michal Hocko (2018-06-22 16:02:42)

Quoted 28 lines

Hi,
this is an RFC and not tested at all. I am not very familiar with the mmu notifiers semantics very much so this is a crude attempt to achieve what I need basically. It might be completely wrong but I would like to discuss what would be a better way if that is the case.

get_maintainers gave me quite large list of people to CC so I had to trim it down. If you think I have forgot somebody, please let me know

drivers/gpu/drm/i915/i915_gem_userptr.c +3 -2

index 854bd51b9478..5285df9331fa 100644
@@ -112,10 +112,11 @@ static void del_object(struct i915_mmu_object *mo)
        mo->attached = false;
 }
 
-static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
+static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
                                                       struct mm_struct *mm,
                                                       unsigned long start,
-                                                      unsigned long end)
+                                                      unsigned long end,
+                                                      bool blockable)
 {
        struct i915_mmu_notifier *mn =
                container_of(_mn, struct i915_mmu_notifier, mn);

drivers/gpu/drm/i915/i915_gem_userptr.c static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, · line 128

Quoted 5 lines

drivers/gpu/drm/i915/i915_gem_userptr.c +1 -1

@@ -124,7 +125,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
        LIST_HEAD(cancelled);
 
        if (RB_EMPTY_ROOT(&mn->objects.rb_root))
-               return;
+               return 0;

The principle wait here is for the HW (even after fixing all the locks to be not so coarse, we still have to wait for the HW to finish its access).

Quoted 7 lines

Quoted 1 line

Is this wait bound or it can take basically arbitrary amount of time?

Arbitrary amount of time but in desktop use case you can assume that it should never go above 16ms for a 60frame per second rendering of your desktop (in GPU compute case this kind of assumption does not hold). Is the process exit_state already updated by the time this mmu notifier callbacks happen ?

What do you mean? The process is killed (by SIGKILL) at the time but we do not know much more than that. The task might be stuck anywhere in the kernel before handling that signal.

Quoted 11 lines

Quoted 4 lines

Quoted 2 lines

The first pass would be then to not do anything here if !blockable.

something like this? (incremental diff)

What i wanted to do with HMM and mmu notifier is split the invalidation in 2 pass. First pass tell the drivers to stop/cancel pending jobs that depends on the range and invalidate internal driver states (like clear buffer object pages array in case of GPU but not GPU page table). While the second callback would do the actual wait on the GPU to be done and update the GPU page table.

What can you do after the first phase? Can I unmap the range?

Quoted 10 lines

Now in this scheme in case the task is already in some exit state and that all CPU threads are frozen/kill then we can probably find a way to do the first path mostly lock less. AFAICR nor AMD nor Intel allow to share userptr bo hence a uptr bo should only ever be access through ioctl submited by the process.

The second call can then be delayed and ping from time to time to see if GPU jobs are done.

Quoted 7 lines

Note that what you propose might still be useful as in case there is no buffer object for a range then OOM can make progress in freeing a range of memory. It is very likely that significant virtual address range of a process and backing memory can be reclaim that way. This assume OOM reclaim vma by vma or in some form of granularity like reclaiming 1GB by 1GB. Or we could also update blocking callback to return range that are blocking that way OOM can reclaim around.

Exactly my point. What we have right now is all or nothing which is obviously too coarse to be useful.

Quoted 31 lines · Chris Wilson

Quoting Michal Hocko (2018-06-22 16:02:42)

Quoted 30 lines

Hi,
this is an RFC and not tested at all. I am not very familiar with the mmu notifiers semantics very much so this is a crude attempt to achieve what I need basically. It might be completely wrong but I would like to discuss what would be a better way if that is the case.

get_maintainers gave me quite large list of people to CC so I had to trim it down. If you think I have forgot somebody, please let me know

drivers/gpu/drm/i915/i915_gem_userptr.c +3 -2

index 854bd51b9478..5285df9331fa 100644
@@ -112,10 +112,11 @@ static void del_object(struct i915_mmu_object *mo)
        mo->attached = false;
 }
 
-static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
+static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
                                                       struct mm_struct *mm,
                                                       unsigned long start,
-                                                      unsigned long end)
+                                                      unsigned long end,
+                                                      bool blockable)
 {
        struct i915_mmu_notifier *mn =
                container_of(_mn, struct i915_mmu_notifier, mn);

@@ -124,7 +125,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
        LIST_HEAD(cancelled);
 

Quoted 7 lines

drivers/gpu/drm/i915/i915_gem_userptr.c static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, · line 128

Quoted 3 lines

drivers/gpu/drm/i915/i915_gem_userptr.c +1 -1

        if (RB_EMPTY_ROOT(&mn->objects.rb_root))
-               return;
+               return 0;

The principle wait here is for the HW (even after fixing all the locks to be not so coarse, we still have to wait for the HW to finish its access).

Is this wait bound or it can take basically arbitrary amount of time?

Quoted 3 lines

The first pass would be then to not do anything here if !blockable.

something like this? (incremental diff)

Quoting Michal Hocko (2018-06-22 16:57:16)

Quoted 40 lines · Chris Wilson

Quoted 38 lines

Quoting Michal Hocko (2018-06-22 16:02:42)

Quoted 28 lines

Hi,
this is an RFC and not tested at all. I am not very familiar with the mmu notifiers semantics very much so this is a crude attempt to achieve what I need basically. It might be completely wrong but I would like to discuss what would be a better way if that is the case.

get_maintainers gave me quite large list of people to CC so I had to trim it down. If you think I have forgot somebody, please let me know

drivers/gpu/drm/i915/i915_gem_userptr.c +3 -2

index 854bd51b9478..5285df9331fa 100644
@@ -112,10 +112,11 @@ static void del_object(struct i915_mmu_object *mo)
        mo->attached = false;
 }
 
-static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
+static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
                                                       struct mm_struct *mm,
                                                       unsigned long start,
-                                                      unsigned long end)
+                                                      unsigned long end,
+                                                      bool blockable)
 {
        struct i915_mmu_notifier *mn =
                container_of(_mn, struct i915_mmu_notifier, mn);

drivers/gpu/drm/i915/i915_gem_userptr.c static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, · line 128

Quoted 5 lines

drivers/gpu/drm/i915/i915_gem_userptr.c +1 -1

@@ -124,7 +125,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
        LIST_HEAD(cancelled);
 
        if (RB_EMPTY_ROOT(&mn->objects.rb_root))
-               return;
+               return 0;

The principle wait here is for the HW (even after fixing all the locks to be not so coarse, we still have to wait for the HW to finish its access).

Quoted 1 line

Is this wait bound or it can take basically arbitrary amount of time?

Arbitrary. It waits for the last operation in the queue that needs that set of backing pages, and that queue is unbounded and not even confined to the local driver. (Though each operation should be bounded to be completed within an interval or be cancelled, that interval is on the order of 10s!)

Quoted 4 lines

Quoted 2 lines

The first pass would be then to not do anything here if !blockable.

something like this? (incremental diff)

Yup.

Quoting Michal Hocko (2018-06-22 16:57:16)

Quoted 40 lines · Chris Wilson

Quoted 38 lines

Quoting Michal Hocko (2018-06-22 16:02:42)

Quoted 28 lines

Hi,
this is an RFC and not tested at all. I am not very familiar with the mmu notifiers semantics very much so this is a crude attempt to achieve what I need basically. It might be completely wrong but I would like to discuss what would be a better way if that is the case.

get_maintainers gave me quite large list of people to CC so I had to trim it down. If you think I have forgot somebody, please let me know

drivers/gpu/drm/i915/i915_gem_userptr.c +3 -2

index 854bd51b9478..5285df9331fa 100644
@@ -112,10 +112,11 @@ static void del_object(struct i915_mmu_object *mo)
        mo->attached = false;
 }
 
-static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
+static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
                                                       struct mm_struct *mm,
                                                       unsigned long start,
-                                                      unsigned long end)
+                                                      unsigned long end,
+                                                      bool blockable)
 {
        struct i915_mmu_notifier *mn =
                container_of(_mn, struct i915_mmu_notifier, mn);

drivers/gpu/drm/i915/i915_gem_userptr.c static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, · line 128

Quoted 5 lines

drivers/gpu/drm/i915/i915_gem_userptr.c +1 -1

@@ -124,7 +125,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
        LIST_HEAD(cancelled);
 
        if (RB_EMPTY_ROOT(&mn->objects.rb_root))
-               return;
+               return 0;

The principle wait here is for the HW (even after fixing all the locks to be not so coarse, we still have to wait for the HW to finish its access).

Quoted 1 line

Is this wait bound or it can take basically arbitrary amount of time?

Arbitrary. It waits for the last operation in the queue that needs that set of backing pages, and that queue is unbounded and not even confined to the local driver. (Though each operation should be bounded to be completed within an interval or be cancelled, that interval is on the order of 10s!)

Quoted 4 lines

Quoted 2 lines

The first pass would be then to not do anything here if !blockable.

something like this? (incremental diff)

Yup.

Quoting Michal Hocko (2018-06-22 16:02:42)

Quoted 30 lines

Hi,
this is an RFC and not tested at all. I am not very familiar with the mmu notifiers semantics very much so this is a crude attempt to achieve what I need basically. It might be completely wrong but I would like to discuss what would be a better way if that is the case.

get_maintainers gave me quite large list of people to CC so I had to trim it down. If you think I have forgot somebody, please let me know

drivers/gpu/drm/i915/i915_gem_userptr.c +3 -2

index 854bd51b9478..5285df9331fa 100644
@@ -112,10 +112,11 @@ static void del_object(struct i915_mmu_object *mo)
        mo->attached = false;
 }
 
-static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
+static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
                                                       struct mm_struct *mm,
                                                       unsigned long start,
-                                                      unsigned long end)
+                                                      unsigned long end,
+                                                      bool blockable)
 {
        struct i915_mmu_notifier *mn =
                container_of(_mn, struct i915_mmu_notifier, mn);

@@ -124,7 +125,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
        LIST_HEAD(cancelled);
 

drivers/gpu/drm/i915/i915_gem_userptr.c static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, · line 128

Quoted 3 lines

drivers/gpu/drm/i915/i915_gem_userptr.c +1 -1

        if (RB_EMPTY_ROOT(&mn->objects.rb_root))
-               return;
+               return 0;

The principle wait here is for the HW (even after fixing all the locks to be not so coarse, we still have to wait for the HW to finish its access).

The first pass would be then to not do anything here if !blockable.

Quoting Michal Hocko (2018-06-22 16:02:42)

Quoted 28 lines

Hi,
this is an RFC and not tested at all. I am not very familiar with the mmu notifiers semantics very much so this is a crude attempt to achieve what I need basically. It might be completely wrong but I would like to discuss what would be a better way if that is the case.

get_maintainers gave me quite large list of people to CC so I had to trim it down. If you think I have forgot somebody, please let me know

drivers/gpu/drm/i915/i915_gem_userptr.c +3 -2

index 854bd51b9478..5285df9331fa 100644
@@ -112,10 +112,11 @@ static void del_object(struct i915_mmu_object *mo)
        mo->attached = false;
 }
 
-static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
+static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
                                                       struct mm_struct *mm,
                                                       unsigned long start,
-                                                      unsigned long end)
+                                                      unsigned long end,
+                                                      bool blockable)
 {
        struct i915_mmu_notifier *mn =
                container_of(_mn, struct i915_mmu_notifier, mn);

drivers/gpu/drm/i915/i915_gem_userptr.c static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, · line 128

Quoted 5 lines

drivers/gpu/drm/i915/i915_gem_userptr.c +1 -1

@@ -124,7 +125,7 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
        LIST_HEAD(cancelled);
 
        if (RB_EMPTY_ROOT(&mn->objects.rb_root))
-               return;
+               return 0;

The principle wait here is for the HW (even after fixing all the locks to be not so coarse, we still have to wait for the HW to finish its access). The first pass would be then to not do anything here if !blockable.

Jerome keeps on shaking his head and telling us we're doing it all wrong, so maybe it'll all fall out of HMM before we have to figure out how to differentiate between objects that can be invalidated immediately and those that need to acquire locks and/or wait.

Hi,
this is an RFC and not tested at all. I am not very familiar with the mmu notifiers semantics very much so this is a crude attempt to achieve what I need basically. It might be completely wrong but I would like to discuss what would be a better way if that is the case.

get_maintainers gave me quite large list of people to CC so I had to trim it down. If you think I have forgot somebody, please let me know

drivers/gpu/drm/i915/i915_gem_userptr.c +3 -2

index 854bd51b9478..5285df9331fa 100644
@@ -112,10 +112,11 @@ static void del_object(struct i915_mmu_object *mo)
        mo->attached = false;
 }
 
-static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
+static int i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
                                                       struct mm_struct *mm,
                                                       unsigned long start,
-                                                      unsigned long end)
+                                                      unsigned long end,
+                                                      bool blockable)
 {
        struct i915_mmu_notifier *mn =
                container_of(_mn, struct i915_mmu_notifier, mn);

arch/x86/kvm/x86.c void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm, · line 7220

Quoted 6 lines · Michal Hocko

arch/x86/kvm/x86.c +2 -0

@@ -7215,6 +7216,8 @@ void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
 	apic_address = gfn_to_hva(kvm, APIC_DEFAULT_PHYS_BASE >> PAGE_SHIFT);
 	if (start <= apic_address && apic_address < end)
 		kvm_make_all_cpus_request(kvm, KVM_REQ_APIC_PAGE_RELOAD);
+
+	return 0;

This is wrong, gfn_to_hva can sleep.

You could do the the kvm_make_all_cpus_request unconditionally, but only if !blockable is a really rare thing. OOM would be fine, since the request actually would never be processed, but I'm afraid of more uses of !blockable being introduced later.

Quoted 9 lines · Paolo Bonzini

arch/x86/kvm/x86.c void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm, · line 7220

Quoted 6 lines · Michal Hocko

arch/x86/kvm/x86.c +2 -0

@@ -7215,6 +7216,8 @@ void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
 	apic_address = gfn_to_hva(kvm, APIC_DEFAULT_PHYS_BASE >> PAGE_SHIFT);
 	if (start <= apic_address && apic_address < end)
 		kvm_make_all_cpus_request(kvm, KVM_REQ_APIC_PAGE_RELOAD);
+
+	return 0;

This is wrong, gfn_to_hva can sleep.

Hmm, I have tried to crawl the call chain and haven't found any sleepable locks taken. Maybe I am just missing something. __kvm_memslots has a complex locking assert. I do not see we would take slots_lock anywhere from the notifier call path. IIUC that means that users_count has to be zero at that time. I have no idea how that is guaranteed.

Quoted 11 lines · Michal Hocko

Quoted 9 lines · Paolo Bonzini

arch/x86/kvm/x86.c void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm, · line 7220

Quoted 6 lines · Michal Hocko

arch/x86/kvm/x86.c +2 -0

@@ -7215,6 +7216,8 @@ void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
 	apic_address = gfn_to_hva(kvm, APIC_DEFAULT_PHYS_BASE >> PAGE_SHIFT);
 	if (start <= apic_address && apic_address < end)
 		kvm_make_all_cpus_request(kvm, KVM_REQ_APIC_PAGE_RELOAD);
+
+	return 0;

This is wrong, gfn_to_hva can sleep.

Quoted 6 lines

Hmm, I have tried to crawl the call chain and haven't found any sleepable locks taken. Maybe I am just missing something. __kvm_memslots has a complex locking assert. I do not see we would take slots_lock anywhere from the notifier call path. IIUC that means that users_count has to be zero at that time. I have no idea how that is guaranteed.

Nevermind, ENOCOFFEE. This is gfn_to_hva, not gfn_to_pfn. It only needs SRCU.

Quoted 19 lines · Paolo Bonzini

Quoted 17 lines · Michal Hocko

Quoted 9 lines · Paolo Bonzini

arch/x86/kvm/x86.c void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm, · line 7220

Quoted 6 lines · Michal Hocko

arch/x86/kvm/x86.c +2 -0

@@ -7215,6 +7216,8 @@ void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
 	apic_address = gfn_to_hva(kvm, APIC_DEFAULT_PHYS_BASE >> PAGE_SHIFT);
 	if (start <= apic_address && apic_address < end)
 		kvm_make_all_cpus_request(kvm, KVM_REQ_APIC_PAGE_RELOAD);
+
+	return 0;

This is wrong, gfn_to_hva can sleep.

Hmm, I have tried to crawl the call chain and haven't found any sleepable locks taken. Maybe I am just missing something. __kvm_memslots has a complex locking assert. I do not see we would take slots_lock anywhere from the notifier call path. IIUC that means that users_count has to be zero at that time. I have no idea how that is guaranteed.

Quoted 2 lines

Nevermind, ENOCOFFEE. This is gfn_to_hva, not gfn_to_pfn. It only needs SRCU.

OK, so just the make sure I follow, the change above is correct?

Quoted 20 lines · Michal Hocko

Quoted 19 lines · Paolo Bonzini

Quoted 17 lines · Michal Hocko

Quoted 9 lines · Paolo Bonzini

arch/x86/kvm/x86.c void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm, · line 7220

Quoted 6 lines · Michal Hocko

arch/x86/kvm/x86.c +2 -0

@@ -7215,6 +7216,8 @@ void kvm_arch_mmu_notifier_invalidate_range(struct kvm *kvm,
 	apic_address = gfn_to_hva(kvm, APIC_DEFAULT_PHYS_BASE >> PAGE_SHIFT);
 	if (start <= apic_address && apic_address < end)
 		kvm_make_all_cpus_request(kvm, KVM_REQ_APIC_PAGE_RELOAD);
+
+	return 0;

This is wrong, gfn_to_hva can sleep.

Hmm, I have tried to crawl the call chain and haven't found any sleepable locks taken. Maybe I am just missing something. __kvm_memslots has a complex locking assert. I do not see we would take slots_lock anywhere from the notifier call path. IIUC that means that users_count has to be zero at that time. I have no idea how that is guaranteed.

Quoted 4 lines

Quoted 2 lines

Nevermind, ENOCOFFEE. This is gfn_to_hva, not gfn_to_pfn. It only needs SRCU.

OK, so just the make sure I follow, the change above is correct?

Yes. It's only gfn_to_pfn that calls get_user_pages and therefore can sleep.