[Intel-gfx] [PATCH 1/3] drm/i915/gem: Avoid kmalloc under i915->mm_lock

[Chris Wilson]

Quoting Matthew Auld (2020-06-19 20:29:15)
> On Fri, 19 Jun 2020 at 15:31, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> >
> > 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.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 123 +++++++++-----------
> >  drivers/gpu/drm/i915/i915_drv.h             |   2 +-
> >  2 files changed, 59 insertions(+), 66 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > index 9c53eb883400..84766414a1f0 100644
> > --- 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,26 @@ 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;
> > +       struct i915_mm_struct *it, *mm = NULL;
> >
> > -       /* 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;
> > +       rcu_read_lock();
> > +       hash_for_each_possible(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 +324,40 @@ 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);
> 
> Is this really safe without the mm_lock, assuming concurrent
> hash_add/has_del with hash_for_each?

Hmm, not quite. There's hash_add_rcu, hash_del_rcu,
hash_for_each_possible_rcu, to ensure the write ordering is safe for the
concurrent readers. Good catch.
-Chris