drm/i915/gem: Avoid kmalloc under i915->mm_lock

Rearrange the allocation of the mm_struct registration to avoid
allocating underneath the i915->mm_lock, so that we avoid tainting the
lock (and in turn many other locks that may be held as i915->mm_lock is
taken, and those locks we may want on the free [shrinker] paths). In
doing so, we convert the lookup to be RCU protected by courtesy of
converting the free-worker to be an rcu_work.

v2: Remember to use hash_rcu variants to protect the list iteration from
concurrent add/del.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20200619194038.5088-1-chris@chris-wilson.co.uk

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index 9c53eb8..e946032 100644 (file)
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -21,7 +21,7 @@ struct i915_mm_struct {
        struct i915_mmu_notifier *mn;
        struct hlist_node node;
        struct kref kref;
-       struct work_struct work;
+       struct rcu_work work;
 };
 
 #if defined(CONFIG_MMU_NOTIFIER)
@@ -189,40 +189,31 @@ i915_gem_userptr_release__mmu_notifier(struct drm_i915_gem_object *obj)
 static struct i915_mmu_notifier *
 i915_mmu_notifier_find(struct i915_mm_struct *mm)
 {
-       struct i915_mmu_notifier *mn;
-       int err = 0;
+       struct i915_mmu_notifier *mn, *old;
+       int err;
 
-       mn = mm->mn;
-       if (mn)
+       mn = READ_ONCE(mm->mn);
+       if (likely(mn))
                return mn;
 
        mn = i915_mmu_notifier_create(mm);
        if (IS_ERR(mn))
-               err = PTR_ERR(mn);
-
-       mmap_write_lock(mm->mm);
-       mutex_lock(&mm->i915->mm_lock);
-       if (mm->mn == NULL && !err) {
-               /* Protected by mmap_lock (write-lock) */
-               err = __mmu_notifier_register(&mn->mn, mm->mm);
-               if (!err) {
-                       /* Protected by mm_lock */
-                       mm->mn = fetch_and_zero(&mn);
-               }
-       } else if (mm->mn) {
-               /*
-                * Someone else raced and successfully installed the mmu
-                * notifier, we can cancel our own errors.
-                */
-               err = 0;
+               return mn;
+
+       err = mmu_notifier_register(&mn->mn, mm->mm);
+       if (err) {
+               kfree(mn);
+               return ERR_PTR(err);
        }
-       mutex_unlock(&mm->i915->mm_lock);
-       mmap_write_unlock(mm->mm);
 
-       if (mn && !IS_ERR(mn))
+       old = cmpxchg(&mm->mn, NULL, mn);
+       if (old) {
+               mmu_notifier_unregister(&mn->mn, mm->mm);
                kfree(mn);
+               mn = old;
+       }
 
-       return err ? ERR_PTR(err) : mm->mn;
+       return mn;
 }
 
 static int
@@ -301,23 +292,28 @@ i915_mmu_notifier_free(struct i915_mmu_notifier *mn,
 #endif
 
 static struct i915_mm_struct *
-__i915_mm_struct_find(struct drm_i915_private *dev_priv, struct mm_struct *real)
+__i915_mm_struct_find(struct drm_i915_private *i915, struct mm_struct *real)
 {
-       struct i915_mm_struct *mm;
-
-       /* Protected by dev_priv->mm_lock */
-       hash_for_each_possible(dev_priv->mm_structs, mm, node, (unsigned long)real)
-               if (mm->mm == real)
-                       return mm;
+       struct i915_mm_struct *it, *mm = NULL;
+
+       rcu_read_lock();
+       hash_for_each_possible_rcu(i915->mm_structs,
+                                  it, node,
+                                  (unsigned long)real)
+               if (it->mm == real && kref_get_unless_zero(&it->kref)) {
+                       mm = it;
+                       break;
+               }
+       rcu_read_unlock();
 
-       return NULL;
+       return mm;
 }
 
 static int
 i915_gem_userptr_init__mm_struct(struct drm_i915_gem_object *obj)
 {
-       struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
-       struct i915_mm_struct *mm;
+       struct drm_i915_private *i915 = to_i915(obj->base.dev);
+       struct i915_mm_struct *mm, *new;
        int ret = 0;
 
        /* During release of the GEM object we hold the struct_mutex. This
@@ -330,39 +326,42 @@ i915_gem_userptr_init__mm_struct(struct drm_i915_gem_object *obj)
         * struct_mutex, i.e. we need to schedule a worker to do the clean
         * up.
         */
-       mutex_lock(&dev_priv->mm_lock);
-       mm = __i915_mm_struct_find(dev_priv, current->mm);
-       if (mm == NULL) {
-               mm = kmalloc(sizeof(*mm), GFP_KERNEL);
-               if (mm == NULL) {
-                       ret = -ENOMEM;
-                       goto out;
-               }
+       mm = __i915_mm_struct_find(i915, current->mm);
+       if (mm)
+               goto out;
 
-               kref_init(&mm->kref);
-               mm->i915 = to_i915(obj->base.dev);
+       new = kmalloc(sizeof(*mm), GFP_KERNEL);
+       if (!new)
+               return -ENOMEM;
 
-               mm->mm = current->mm;
+       kref_init(&new->kref);
+       new->i915 = to_i915(obj->base.dev);
+       new->mm = current->mm;
+       new->mn = NULL;
+
+       spin_lock(&i915->mm_lock);
+       mm = __i915_mm_struct_find(i915, current->mm);
+       if (!mm) {
+               hash_add_rcu(i915->mm_structs,
+                            &new->node,
+                            (unsigned long)new->mm);
                mmgrab(current->mm);
+               mm = new;
+       }
+       spin_unlock(&i915->mm_lock);
+       if (mm != new)
+               kfree(new);
 
-               mm->mn = NULL;
-
-               /* Protected by dev_priv->mm_lock */
-               hash_add(dev_priv->mm_structs,
-                        &mm->node, (unsigned long)mm->mm);
-       } else
-               kref_get(&mm->kref);
-
-       obj->userptr.mm = mm;
 out:
-       mutex_unlock(&dev_priv->mm_lock);
+       obj->userptr.mm = mm;
        return ret;
 }
 
 static void
 __i915_mm_struct_free__worker(struct work_struct *work)
 {
-       struct i915_mm_struct *mm = container_of(work, typeof(*mm), work);
+       struct i915_mm_struct *mm = container_of(work, typeof(*mm), work.work);
+
        i915_mmu_notifier_free(mm->mn, mm->mm);
        mmdrop(mm->mm);
        kfree(mm);
@@ -373,12 +372,12 @@ __i915_mm_struct_free(struct kref *kref)
 {
        struct i915_mm_struct *mm = container_of(kref, typeof(*mm), kref);
 
-       /* Protected by dev_priv->mm_lock */
-       hash_del(&mm->node);
-       mutex_unlock(&mm->i915->mm_lock);
+       spin_lock(&mm->i915->mm_lock);
+       hash_del_rcu(&mm->node);
+       spin_unlock(&mm->i915->mm_lock);
 
-       INIT_WORK(&mm->work, __i915_mm_struct_free__worker);
-       queue_work(mm->i915->mm.userptr_wq, &mm->work);
+       INIT_RCU_WORK(&mm->work, __i915_mm_struct_free__worker);
+       queue_rcu_work(system_wq, &mm->work);
 }
 
 static void
@@ -387,9 +386,7 @@ i915_gem_userptr_release__mm_struct(struct drm_i915_gem_object *obj)
        if (obj->userptr.mm == NULL)
                return;
 
-       kref_put_mutex(&obj->userptr.mm->kref,
-                      __i915_mm_struct_free,
-                      &to_i915(obj->base.dev)->mm_lock);
+       kref_put(&obj->userptr.mm->kref, __i915_mm_struct_free);
        obj->userptr.mm = NULL;
 }
 
@@ -851,7 +848,7 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
 
 int i915_gem_init_userptr(struct drm_i915_private *dev_priv)
 {
-       mutex_init(&dev_priv->mm_lock);
+       spin_lock_init(&dev_priv->mm_lock);
        hash_init(dev_priv->mm_structs);
 
        dev_priv->mm.userptr_wq =

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a574bdf..6e9072a 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -993,7 +993,7 @@ struct drm_i915_private {
 
        struct i915_gem_mm mm;
        DECLARE_HASHTABLE(mm_structs, 7);
-       struct mutex mm_lock;
+       spinlock_t mm_lock;
 
        /* Kernel Modesetting */