[PATCH 00/14] drm/msm: de-struct_mutex-ification

[PATCH 00/14] drm/msm: de-struct_mutex-ification

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

This doesn't remove *all* the struct_mutex, but it covers the worst
of it, ie. shrinker/madvise/free/retire.  The submit path still uses
struct_mutex, but it still needs *something* serialize a portion of
the submit path, and lock_stat mostly just shows the lock contention
there being with other submits.  And there are a few other bits of
struct_mutex usage in less critical paths (debugfs, etc).  But this
seems like a reasonable step in the right direction.

Rob Clark (14):
  drm/msm: Use correct drm_gem_object_put() in fail case
  drm/msm: Drop chatty trace
  drm/msm: Move update_fences()
  drm/msm: Add priv->mm_lock to protect active/inactive lists
  drm/msm: Document and rename preempt_lock
  drm/msm: Protect ring->submits with it's own lock
  drm/msm: Refcount submits
  drm/msm: Remove obj->gpu
  drm/msm: Drop struct_mutex from the retire path
  drm/msm: Drop struct_mutex in free_object() path
  drm/msm: remove msm_gem_free_work
  drm/msm: drop struct_mutex in madvise path
  drm/msm: Drop struct_mutex in shrinker path
  drm/msm: Don't implicit-sync if only a single ring

 drivers/gpu/drm/msm/adreno/a5xx_gpu.c     |  4 +-
 drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 12 +--
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c     |  4 +-
 drivers/gpu/drm/msm/msm_debugfs.c         |  7 ++
 drivers/gpu/drm/msm/msm_drv.c             | 15 +---
 drivers/gpu/drm/msm/msm_drv.h             | 19 +++--
 drivers/gpu/drm/msm/msm_gem.c             | 76 ++++++------------
 drivers/gpu/drm/msm/msm_gem.h             | 53 +++++++++----
 drivers/gpu/drm/msm/msm_gem_shrinker.c    | 58 ++------------
 drivers/gpu/drm/msm/msm_gem_submit.c      | 17 ++--
 drivers/gpu/drm/msm/msm_gpu.c             | 96 ++++++++++++++---------
 drivers/gpu/drm/msm/msm_gpu.h             |  5 +-
 drivers/gpu/drm/msm/msm_ringbuffer.c      |  3 +-
 drivers/gpu/drm/msm/msm_ringbuffer.h      | 13 ++-
 14 files changed, 188 insertions(+), 194 deletions(-)

-- 
2.26.2

[PATCH 12/14] drm/msm: drop struct_mutex in madvise path

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

The obj->lock is sufficient for what we need.

This *does* have the implication that userspace can try to shoot
themselves in the foot by racing madvise(DONTNEED) with submit.  But
the result will be about the same if they did madvise(DONTNEED) before
the submit ioctl, ie. they might not get want they want if they race
with shrinker.  But iova fault handling is robust enough, and userspace
is only shooting it's own foot.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/msm_drv.c          | 11 ++------
 drivers/gpu/drm/msm/msm_gem.c          |  6 ++--
 drivers/gpu/drm/msm/msm_gem.h          | 38 ++++++++++++++++++--------
 drivers/gpu/drm/msm/msm_gem_shrinker.c |  4 +--
 4 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index e766c1f45045..d2488816ce48 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -906,14 +906,9 @@ static int msm_ioctl_gem_madvise(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	ret = mutex_lock_interruptible(&dev->struct_mutex);
-	if (ret)
-		return ret;
-
 	obj = drm_gem_object_lookup(file, args->handle);
 	if (!obj) {
-		ret = -ENOENT;
-		goto unlock;
+		return -ENOENT;
 	}
 
 	ret = msm_gem_madvise(obj, args->madv);
@@ -922,10 +917,8 @@ static int msm_ioctl_gem_madvise(struct drm_device *dev, void *data,
 		ret = 0;
 	}
 
-	drm_gem_object_put_locked(obj);
+	drm_gem_object_put(obj);
 
-unlock:
-	mutex_unlock(&dev->struct_mutex);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 5e75d644ce41..9cdac4f7228c 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -639,8 +639,6 @@ int msm_gem_madvise(struct drm_gem_object *obj, unsigned madv)
 
 	mutex_lock(&msm_obj->lock);
 
-	WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
-
 	if (msm_obj->madv != __MSM_MADV_PURGED)
 		msm_obj->madv = madv;
 
@@ -657,7 +655,7 @@ void msm_gem_purge(struct drm_gem_object *obj, enum msm_gem_lock subclass)
 	struct msm_gem_object *msm_obj = to_msm_bo(obj);
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
-	WARN_ON(!is_purgeable(msm_obj));
+	WARN_ON(!is_purgeable(msm_obj, subclass));
 	WARN_ON(obj->import_attach);
 
 	mutex_lock_nested(&msm_obj->lock, subclass);
@@ -749,7 +747,7 @@ void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu)
 	struct msm_drm_private *priv = obj->dev->dev_private;
 
 	might_sleep();
-	WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED);
+	WARN_ON(msm_gem_madv(msm_obj, OBJ_LOCK_NORMAL) != MSM_MADV_WILLNEED);
 
 	if (!atomic_fetch_inc(&msm_obj->active_count)) {
 		mutex_lock(&priv->mm_lock);
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index e98a8004813b..bb8aa6b1b254 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -97,18 +97,6 @@ static inline bool is_active(struct msm_gem_object *msm_obj)
 	return atomic_read(&msm_obj->active_count);
 }
 
-static inline bool is_purgeable(struct msm_gem_object *msm_obj)
-{
-	WARN_ON(!mutex_is_locked(&msm_obj->base.dev->struct_mutex));
-	return (msm_obj->madv == MSM_MADV_DONTNEED) && msm_obj->sgt &&
-			!msm_obj->base.dma_buf && !msm_obj->base.import_attach;
-}
-
-static inline bool is_vunmapable(struct msm_gem_object *msm_obj)
-{
-	return (msm_obj->vmap_count == 0) && msm_obj->vaddr;
-}
-
 /* The shrinker can be triggered while we hold objA->lock, and need
  * to grab objB->lock to purge it.  Lockdep just sees these as a single
  * class of lock, so we use subclasses to teach it the difference.
@@ -125,6 +113,32 @@ enum msm_gem_lock {
 	OBJ_LOCK_SHRINKER,
 };
 
+/* Use this helper to read msm_obj->madv when msm_obj->lock not held: */
+static inline unsigned
+msm_gem_madv(struct msm_gem_object *msm_obj, enum msm_gem_lock subclass)
+{
+	unsigned madv;
+
+	mutex_lock_nested(&msm_obj->lock, subclass);
+	madv = msm_obj->madv;
+	mutex_unlock(&msm_obj->lock);
+
+	return madv;
+}
+
+static inline bool
+is_purgeable(struct msm_gem_object *msm_obj, enum msm_gem_lock subclass)
+{
+	return (msm_gem_madv(msm_obj, subclass) == MSM_MADV_DONTNEED) &&
+			msm_obj->sgt && !msm_obj->base.dma_buf &&
+			!msm_obj->base.import_attach;
+}
+
+static inline bool is_vunmapable(struct msm_gem_object *msm_obj)
+{
+	return (msm_obj->vmap_count == 0) && msm_obj->vaddr;
+}
+
 void msm_gem_purge(struct drm_gem_object *obj, enum msm_gem_lock subclass);
 void msm_gem_vunmap(struct drm_gem_object *obj, enum msm_gem_lock subclass);
 
diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c b/drivers/gpu/drm/msm/msm_gem_shrinker.c
index c41b84a3a484..39a1b5327267 100644
--- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
+++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
@@ -54,7 +54,7 @@ msm_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
 	mutex_lock(&priv->mm_lock);
 
 	list_for_each_entry(msm_obj, &priv->inactive_list, mm_list) {
-		if (is_purgeable(msm_obj))
+		if (is_purgeable(msm_obj, OBJ_LOCK_SHRINKER))
 			count += msm_obj->base.size >> PAGE_SHIFT;
 	}
 
@@ -84,7 +84,7 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
 	list_for_each_entry(msm_obj, &priv->inactive_list, mm_list) {
 		if (freed >= sc->nr_to_scan)
 			break;
-		if (is_purgeable(msm_obj)) {
+		if (is_purgeable(msm_obj, OBJ_LOCK_SHRINKER)) {
 			msm_gem_purge(&msm_obj->base, OBJ_LOCK_SHRINKER);
 			freed += msm_obj->base.size >> PAGE_SHIFT;
 		}
-- 
2.26.2

Re: [Freedreno] [PATCH 00/14] drm/msm: de-struct_mutex-ification

[Kristian Høgsberg]

On Sun, Oct 4, 2020 at 9:21 PM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> This doesn't remove *all* the struct_mutex, but it covers the worst
> of it, ie. shrinker/madvise/free/retire.  The submit path still uses
> struct_mutex, but it still needs *something* serialize a portion of
> the submit path, and lock_stat mostly just shows the lock contention
> there being with other submits.  And there are a few other bits of
> struct_mutex usage in less critical paths (debugfs, etc).  But this
> seems like a reasonable step in the right direction.

What a great patch set. Daniel has some good points and nothing that
requires big changes, but on the other hand, I'm not sure it's
something that needs to block this set either.

Either way, for the series

Reviewed-by: Kristian H. Kristensen <hoegsberg@google.com>

> Rob Clark (14):
>   drm/msm: Use correct drm_gem_object_put() in fail case
>   drm/msm: Drop chatty trace
>   drm/msm: Move update_fences()
>   drm/msm: Add priv->mm_lock to protect active/inactive lists
>   drm/msm: Document and rename preempt_lock
>   drm/msm: Protect ring->submits with it's own lock
>   drm/msm: Refcount submits
>   drm/msm: Remove obj->gpu
>   drm/msm: Drop struct_mutex from the retire path
>   drm/msm: Drop struct_mutex in free_object() path
>   drm/msm: remove msm_gem_free_work
>   drm/msm: drop struct_mutex in madvise path
>   drm/msm: Drop struct_mutex in shrinker path
>   drm/msm: Don't implicit-sync if only a single ring
>
>  drivers/gpu/drm/msm/adreno/a5xx_gpu.c     |  4 +-
>  drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 12 +--
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c     |  4 +-
>  drivers/gpu/drm/msm/msm_debugfs.c         |  7 ++
>  drivers/gpu/drm/msm/msm_drv.c             | 15 +---
>  drivers/gpu/drm/msm/msm_drv.h             | 19 +++--
>  drivers/gpu/drm/msm/msm_gem.c             | 76 ++++++------------
>  drivers/gpu/drm/msm/msm_gem.h             | 53 +++++++++----
>  drivers/gpu/drm/msm/msm_gem_shrinker.c    | 58 ++------------
>  drivers/gpu/drm/msm/msm_gem_submit.c      | 17 ++--
>  drivers/gpu/drm/msm/msm_gpu.c             | 96 ++++++++++++++---------
>  drivers/gpu/drm/msm/msm_gpu.h             |  5 +-
>  drivers/gpu/drm/msm/msm_ringbuffer.c      |  3 +-
>  drivers/gpu/drm/msm/msm_ringbuffer.h      | 13 ++-
>  14 files changed, 188 insertions(+), 194 deletions(-)
>
> --
> 2.26.2
>
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 00/14] drm/msm: de-struct_mutex-ification

[Daniel Vetter]

On Mon, Oct 5, 2020 at 6:24 PM Kristian Høgsberg <hoegsberg@gmail.com> wrote:
>
> On Sun, Oct 4, 2020 at 9:21 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > From: Rob Clark <robdclark@chromium.org>
> >
> > This doesn't remove *all* the struct_mutex, but it covers the worst
> > of it, ie. shrinker/madvise/free/retire.  The submit path still uses
> > struct_mutex, but it still needs *something* serialize a portion of
> > the submit path, and lock_stat mostly just shows the lock contention
> > there being with other submits.  And there are a few other bits of
> > struct_mutex usage in less critical paths (debugfs, etc).  But this
> > seems like a reasonable step in the right direction.
>
> What a great patch set. Daniel has some good points and nothing that
> requires big changes, but on the other hand, I'm not sure it's
> something that needs to block this set either.

Personally I'd throw the lockdep priming on top to make sure this
stays correct (it's 3 lines), but yes imo this is all good to go. Just
figured I'll sprinkle the latest in terms of gem locking over the
series while it's here :-)
-Daniel

> Either way, for the series
>
> Reviewed-by: Kristian H. Kristensen <hoegsberg@google.com>
>
> > Rob Clark (14):
> >   drm/msm: Use correct drm_gem_object_put() in fail case
> >   drm/msm: Drop chatty trace
> >   drm/msm: Move update_fences()
> >   drm/msm: Add priv->mm_lock to protect active/inactive lists
> >   drm/msm: Document and rename preempt_lock
> >   drm/msm: Protect ring->submits with it's own lock
> >   drm/msm: Refcount submits
> >   drm/msm: Remove obj->gpu
> >   drm/msm: Drop struct_mutex from the retire path
> >   drm/msm: Drop struct_mutex in free_object() path
> >   drm/msm: remove msm_gem_free_work
> >   drm/msm: drop struct_mutex in madvise path
> >   drm/msm: Drop struct_mutex in shrinker path
> >   drm/msm: Don't implicit-sync if only a single ring
> >
> >  drivers/gpu/drm/msm/adreno/a5xx_gpu.c     |  4 +-
> >  drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 12 +--
> >  drivers/gpu/drm/msm/adreno/a6xx_gpu.c     |  4 +-
> >  drivers/gpu/drm/msm/msm_debugfs.c         |  7 ++
> >  drivers/gpu/drm/msm/msm_drv.c             | 15 +---
> >  drivers/gpu/drm/msm/msm_drv.h             | 19 +++--
> >  drivers/gpu/drm/msm/msm_gem.c             | 76 ++++++------------
> >  drivers/gpu/drm/msm/msm_gem.h             | 53 +++++++++----
> >  drivers/gpu/drm/msm/msm_gem_shrinker.c    | 58 ++------------
> >  drivers/gpu/drm/msm/msm_gem_submit.c      | 17 ++--
> >  drivers/gpu/drm/msm/msm_gpu.c             | 96 ++++++++++++++---------
> >  drivers/gpu/drm/msm/msm_gpu.h             |  5 +-
> >  drivers/gpu/drm/msm/msm_ringbuffer.c      |  3 +-
> >  drivers/gpu/drm/msm/msm_ringbuffer.h      | 13 ++-
> >  14 files changed, 188 insertions(+), 194 deletions(-)
> >
> > --
> > 2.26.2
> >
> > _______________________________________________
> > Freedreno mailing list
> > Freedreno@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/freedreno
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Freedreno] [PATCH 00/14] drm/msm: de-struct_mutex-ification

[Rob Clark]

On Mon, Oct 5, 2020 at 11:20 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Oct 5, 2020 at 6:24 PM Kristian Høgsberg <hoegsberg@gmail.com> wrote:
> >
> > On Sun, Oct 4, 2020 at 9:21 PM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > This doesn't remove *all* the struct_mutex, but it covers the worst
> > > of it, ie. shrinker/madvise/free/retire.  The submit path still uses
> > > struct_mutex, but it still needs *something* serialize a portion of
> > > the submit path, and lock_stat mostly just shows the lock contention
> > > there being with other submits.  And there are a few other bits of
> > > struct_mutex usage in less critical paths (debugfs, etc).  But this
> > > seems like a reasonable step in the right direction.
> >
> > What a great patch set. Daniel has some good points and nothing that
> > requires big changes, but on the other hand, I'm not sure it's
> > something that needs to block this set either.
>
> Personally I'd throw the lockdep priming on top to make sure this
> stays correct (it's 3 lines), but yes imo this is all good to go. Just
> figured I'll sprinkle the latest in terms of gem locking over the
> series while it's here :-)

Yeah, I'll defn throw the lockdep priming into v2.. and I've got using
obj->resv for locking on the todo list but looks like enough churn
that it will probably be it's own series (but seems like there is room
to introduce some lock/unlock helpers that don't really change
anything but make an obj->lock transition easier)

BR,
-R

> -Daniel
>
> > Either way, for the series
> >
> > Reviewed-by: Kristian H. Kristensen <hoegsberg@google.com>
> >
> > > Rob Clark (14):
> > >   drm/msm: Use correct drm_gem_object_put() in fail case
> > >   drm/msm: Drop chatty trace
> > >   drm/msm: Move update_fences()
> > >   drm/msm: Add priv->mm_lock to protect active/inactive lists
> > >   drm/msm: Document and rename preempt_lock
> > >   drm/msm: Protect ring->submits with it's own lock
> > >   drm/msm: Refcount submits
> > >   drm/msm: Remove obj->gpu
> > >   drm/msm: Drop struct_mutex from the retire path
> > >   drm/msm: Drop struct_mutex in free_object() path
> > >   drm/msm: remove msm_gem_free_work
> > >   drm/msm: drop struct_mutex in madvise path
> > >   drm/msm: Drop struct_mutex in shrinker path
> > >   drm/msm: Don't implicit-sync if only a single ring
> > >
> > >  drivers/gpu/drm/msm/adreno/a5xx_gpu.c     |  4 +-
> > >  drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 12 +--
> > >  drivers/gpu/drm/msm/adreno/a6xx_gpu.c     |  4 +-
> > >  drivers/gpu/drm/msm/msm_debugfs.c         |  7 ++
> > >  drivers/gpu/drm/msm/msm_drv.c             | 15 +---
> > >  drivers/gpu/drm/msm/msm_drv.h             | 19 +++--
> > >  drivers/gpu/drm/msm/msm_gem.c             | 76 ++++++------------
> > >  drivers/gpu/drm/msm/msm_gem.h             | 53 +++++++++----
> > >  drivers/gpu/drm/msm/msm_gem_shrinker.c    | 58 ++------------
> > >  drivers/gpu/drm/msm/msm_gem_submit.c      | 17 ++--
> > >  drivers/gpu/drm/msm/msm_gpu.c             | 96 ++++++++++++++---------
> > >  drivers/gpu/drm/msm/msm_gpu.h             |  5 +-
> > >  drivers/gpu/drm/msm/msm_ringbuffer.c      |  3 +-
> > >  drivers/gpu/drm/msm/msm_ringbuffer.h      | 13 ++-
> > >  14 files changed, 188 insertions(+), 194 deletions(-)
> > >
> > > --
> > > 2.26.2
> > >
> > > _______________________________________________
> > > Freedreno mailing list
> > > Freedreno@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/freedreno
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch