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

Message ID 20201004192152.3298573-1-robdclark@gmail.com
Headers show
Series
  • drm/msm: de-struct_mutex-ification
Related show

Message

Rob Clark Oct. 4, 2020, 7:21 p.m. UTC
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(-)

Comments

Daniel Vetter Oct. 5, 2020, 2:02 p.m. UTC | #1
On Mon, Oct 05, 2020 at 05:24:19PM +0800, Hillf Danton wrote:
> 
> On Sun,  4 Oct 2020 12:21:45
> > From: Rob Clark <robdclark@chromium.org>
> > 
> > Now that the inactive_list is protected by mm_lock, and everything
> > else on per-obj basis is protected by obj->lock, we no longer depend
> > on struct_mutex.
> > 
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >  drivers/gpu/drm/msm/msm_gem.c          |  1 -
> >  drivers/gpu/drm/msm/msm_gem_shrinker.c | 54 --------------------------
> >  2 files changed, 55 deletions(-)
> > 
> [...]
> 
> > @@ -71,13 +33,8 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
> >  {
> >  	struct msm_drm_private *priv =
> >  		container_of(shrinker, struct msm_drm_private, shrinker);
> > -	struct drm_device *dev = priv->dev;
> >  	struct msm_gem_object *msm_obj;
> >  	unsigned long freed = 0;
> > -	bool unlock;
> > -
> > -	if (!msm_gem_shrinker_lock(dev, &unlock))
> > -		return SHRINK_STOP;
> >  
> >  	mutex_lock(&priv->mm_lock);
> 
> Better if the change in behavior is documented that SHRINK_STOP will
> no longer be needed.

btw I read through this and noticed you have your own obj lock, plus
mutex_lock_nested. I strongly recommend to just cut over to dma_resv_lock
for all object lock needs (soc drivers have been terrible with this
unfortuntaly), and in the shrinker just use dma_resv_trylock instead of
trying to play clever games outsmarting lockdep.

I recently wrote an entire blog length rant on why I think
mutex_lock_nested is too dangerous to be useful:

https://blog.ffwll.ch/2020/08/lockdep-false-positives.html

Not anything about this here, just general comment. The problem extends to
shmem helpers and all that also having their own locks for everything.
-Daniel
Kristian Høgsberg Oct. 5, 2020, 4:17 p.m. UTC | #2
On Mon, Oct 5, 2020 at 4:02 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Oct 05, 2020 at 05:24:19PM +0800, Hillf Danton wrote:
> >
> > On Sun,  4 Oct 2020 12:21:45
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > Now that the inactive_list is protected by mm_lock, and everything
> > > else on per-obj basis is protected by obj->lock, we no longer depend
> > > on struct_mutex.
> > >
> > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > ---
> > >  drivers/gpu/drm/msm/msm_gem.c          |  1 -
> > >  drivers/gpu/drm/msm/msm_gem_shrinker.c | 54 --------------------------
> > >  2 files changed, 55 deletions(-)
> > >
> > [...]
> >
> > > @@ -71,13 +33,8 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
> > >  {
> > >     struct msm_drm_private *priv =
> > >             container_of(shrinker, struct msm_drm_private, shrinker);
> > > -   struct drm_device *dev = priv->dev;
> > >     struct msm_gem_object *msm_obj;
> > >     unsigned long freed = 0;
> > > -   bool unlock;
> > > -
> > > -   if (!msm_gem_shrinker_lock(dev, &unlock))
> > > -           return SHRINK_STOP;
> > >
> > >     mutex_lock(&priv->mm_lock);
> >
> > Better if the change in behavior is documented that SHRINK_STOP will
> > no longer be needed.
>
> btw I read through this and noticed you have your own obj lock, plus
> mutex_lock_nested. I strongly recommend to just cut over to dma_resv_lock
> for all object lock needs (soc drivers have been terrible with this
> unfortuntaly), and in the shrinker just use dma_resv_trylock instead of
> trying to play clever games outsmarting lockdep.
>
> I recently wrote an entire blog length rant on why I think
> mutex_lock_nested is too dangerous to be useful:
>
> https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
>
> Not anything about this here, just general comment. The problem extends to
> shmem helpers and all that also having their own locks for everything.

This is definitely a tangible improvement though - very happy to see
msm_gem_shrinker_lock() go.

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

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Kristian Høgsberg Oct. 5, 2020, 4:24 p.m. UTC | #3
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
Rob Clark Oct. 5, 2020, 4:49 p.m. UTC | #4
On Mon, Oct 5, 2020 at 7:02 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Oct 05, 2020 at 05:24:19PM +0800, Hillf Danton wrote:
> >
> > On Sun,  4 Oct 2020 12:21:45
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > Now that the inactive_list is protected by mm_lock, and everything
> > > else on per-obj basis is protected by obj->lock, we no longer depend
> > > on struct_mutex.
> > >
> > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > ---
> > >  drivers/gpu/drm/msm/msm_gem.c          |  1 -
> > >  drivers/gpu/drm/msm/msm_gem_shrinker.c | 54 --------------------------
> > >  2 files changed, 55 deletions(-)
> > >
> > [...]
> >
> > > @@ -71,13 +33,8 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
> > >  {
> > >     struct msm_drm_private *priv =
> > >             container_of(shrinker, struct msm_drm_private, shrinker);
> > > -   struct drm_device *dev = priv->dev;
> > >     struct msm_gem_object *msm_obj;
> > >     unsigned long freed = 0;
> > > -   bool unlock;
> > > -
> > > -   if (!msm_gem_shrinker_lock(dev, &unlock))
> > > -           return SHRINK_STOP;
> > >
> > >     mutex_lock(&priv->mm_lock);
> >
> > Better if the change in behavior is documented that SHRINK_STOP will
> > no longer be needed.
>
> btw I read through this and noticed you have your own obj lock, plus
> mutex_lock_nested. I strongly recommend to just cut over to dma_resv_lock
> for all object lock needs (soc drivers have been terrible with this
> unfortuntaly), and in the shrinker just use dma_resv_trylock instead of
> trying to play clever games outsmarting lockdep.
>
> I recently wrote an entire blog length rant on why I think
> mutex_lock_nested is too dangerous to be useful:
>
> https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
>
> Not anything about this here, just general comment. The problem extends to
> shmem helpers and all that also having their own locks for everything.

the shrinker lock class has existed for a while.. and is based on the
idea that anything in the get-pages/vmap path cannot happen on a
WONTNEED bo.. although perhaps there should be a few more 'if
(WARN_ON(obj->madv != WILLNEED)) return -EBUSY'..

replacing obj->lock with dma_resv lock, might be a nice cleanup.. but
I think it will be a bit churny..

BR,
-R

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter Oct. 5, 2020, 6:18 p.m. UTC | #5
On Mon, Oct 5, 2020 at 6:49 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Mon, Oct 5, 2020 at 7:02 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Oct 05, 2020 at 05:24:19PM +0800, Hillf Danton wrote:
> > >
> > > On Sun,  4 Oct 2020 12:21:45
> > > > From: Rob Clark <robdclark@chromium.org>
> > > >
> > > > Now that the inactive_list is protected by mm_lock, and everything
> > > > else on per-obj basis is protected by obj->lock, we no longer depend
> > > > on struct_mutex.
> > > >
> > > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > > ---
> > > >  drivers/gpu/drm/msm/msm_gem.c          |  1 -
> > > >  drivers/gpu/drm/msm/msm_gem_shrinker.c | 54 --------------------------
> > > >  2 files changed, 55 deletions(-)
> > > >
> > > [...]
> > >
> > > > @@ -71,13 +33,8 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
> > > >  {
> > > >     struct msm_drm_private *priv =
> > > >             container_of(shrinker, struct msm_drm_private, shrinker);
> > > > -   struct drm_device *dev = priv->dev;
> > > >     struct msm_gem_object *msm_obj;
> > > >     unsigned long freed = 0;
> > > > -   bool unlock;
> > > > -
> > > > -   if (!msm_gem_shrinker_lock(dev, &unlock))
> > > > -           return SHRINK_STOP;
> > > >
> > > >     mutex_lock(&priv->mm_lock);
> > >
> > > Better if the change in behavior is documented that SHRINK_STOP will
> > > no longer be needed.
> >
> > btw I read through this and noticed you have your own obj lock, plus
> > mutex_lock_nested. I strongly recommend to just cut over to dma_resv_lock
> > for all object lock needs (soc drivers have been terrible with this
> > unfortuntaly), and in the shrinker just use dma_resv_trylock instead of
> > trying to play clever games outsmarting lockdep.
> >
> > I recently wrote an entire blog length rant on why I think
> > mutex_lock_nested is too dangerous to be useful:
> >
> > https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
> >
> > Not anything about this here, just general comment. The problem extends to
> > shmem helpers and all that also having their own locks for everything.
>
> the shrinker lock class has existed for a while.. and is based on the
> idea that anything in the get-pages/vmap path cannot happen on a
> WONTNEED bo.. although perhaps there should be a few more 'if
> (WARN_ON(obj->madv != WILLNEED)) return -EBUSY'..

Yeah it works, but it's the kind of really clever stuff that
eventually bites again. For pretty much no benefit, if the lock is
held then you can assume someone else is using the object and you
won't be able to shrink it anyway. So trylock is enough.

> replacing obj->lock with dma_resv lock, might be a nice cleanup.. but
> I think it will be a bit churny..

Oh fully agreed, I tried to push the helpers a bit in that direction
for shmem/cma and gave up. Just something I think we should have in
mind. And in case your gpu ever becomes discrete ... yes the churn is
terrible :-/
-Daniel
Daniel Vetter Oct. 5, 2020, 6:20 p.m. UTC | #6
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
Rob Clark Oct. 6, 2020, 3:25 a.m. UTC | #7
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
Rob Clark Oct. 6, 2020, 3:40 a.m. UTC | #8
On Mon, Oct 5, 2020 at 5:44 PM Hillf Danton <hdanton@sina.com> wrote:
>
>
> On Mon, 5 Oct 2020 18:17:01 Kristian H. Kristensen wrote:
> > On Mon, Oct 5, 2020 at 4:02 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Mon, Oct 05, 2020 at 05:24:19PM +0800, Hillf Danton wrote:
> > > >
> > > > On Sun,  4 Oct 2020 12:21:45
> > > > > From: Rob Clark <robdclark@chromium.org>
> > > > >
> > > > > Now that the inactive_list is protected by mm_lock, and everything
> > > > > else on per-obj basis is protected by obj->lock, we no longer depend
> > > > > on struct_mutex.
> > > > >
> > > > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > > > ---
> > > > >  drivers/gpu/drm/msm/msm_gem.c          |  1 -
> > > > >  drivers/gpu/drm/msm/msm_gem_shrinker.c | 54 --------------------------
> > > > >  2 files changed, 55 deletions(-)
> > > > >
> > > > [...]
> > > >
> > > > > @@ -71,13 +33,8 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
> > > > >  {
> > > > >     struct msm_drm_private *priv =
> > > > >             container_of(shrinker, struct msm_drm_private, shrinker);
> > > > > -   struct drm_device *dev = priv->dev;
> > > > >     struct msm_gem_object *msm_obj;
> > > > >     unsigned long freed = 0;
> > > > > -   bool unlock;
> > > > > -
> > > > > -   if (!msm_gem_shrinker_lock(dev, &unlock))
> > > > > -           return SHRINK_STOP;
> > > > >
> > > > >     mutex_lock(&priv->mm_lock);
> > > >
> > > > Better if the change in behavior is documented that SHRINK_STOP will
> > > > no longer be needed.
> > >
> > > btw I read through this and noticed you have your own obj lock, plus
> > > mutex_lock_nested. I strongly recommend to just cut over to dma_resv_lock
> > > for all object lock needs (soc drivers have been terrible with this
> > > unfortuntaly), and in the shrinker just use dma_resv_trylock instead of
> > > trying to play clever games outsmarting lockdep.
>
> The trylock makes page reclaimers turn to their next target e.g. inode
> cache instead of waiting for the mutex to be released. It makes sense
> for instance in scenarios of mild memory pressure.

is there some behind-the-scenes signalling for this, or is this just
down to what the shrinker callbacks return?  Generally when we get
into shrinking, there are a big set of purgable bo's to consider, so
the shrinker callback return wouldn't be considering just one
potentially lock contended bo (buffer object).  Ie failing one
trylock, we just move on to the next.

fwiw, what I've seen on the userspace bo cache vs shrinker (anything
that is shrinker potential is in userspace bo cache and
MADV(WONTNEED)) is that in steady state I see a very strong recycling
of bo's (which avoids allocating and mmap'ing or mapping to gpu a new
buffer object), so it is definitely a win in mmap/realloc bandwidth..
in steady state there is a lot of free and realloc of same-sized
buffers from frame to frame.

But in transient situations like moving to new game level when there
is a heavy memory pressure and lots of freeing old
buffers/textures/etc and then allocating new ones, I see shrinker
kicking in hard (in android situations, not so much so with
traditional linux userspace)

BR,
-R

>
> > >
> > > I recently wrote an entire blog length rant on why I think
> > > mutex_lock_nested is too dangerous to be useful:
> > >
> > > https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
> > >
> > > Not anything about this here, just general comment. The problem extends to
> > > shmem helpers and all that also having their own locks for everything.
> >
> > This is definitely a tangible improvement though - very happy to see
> > msm_gem_shrinker_lock() go.
> >
> > Reviewed-by: Kristian H. Kristensen <hoegsberg@google.com>
> >
> > > -Daniel
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Daniel Vetter Oct. 6, 2020, 9:35 a.m. UTC | #9
On Mon, Oct 05, 2020 at 08:40:12PM -0700, Rob Clark wrote:
> On Mon, Oct 5, 2020 at 5:44 PM Hillf Danton <hdanton@sina.com> wrote:
> >
> >
> > On Mon, 5 Oct 2020 18:17:01 Kristian H. Kristensen wrote:
> > > On Mon, Oct 5, 2020 at 4:02 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Mon, Oct 05, 2020 at 05:24:19PM +0800, Hillf Danton wrote:
> > > > >
> > > > > On Sun,  4 Oct 2020 12:21:45
> > > > > > From: Rob Clark <robdclark@chromium.org>
> > > > > >
> > > > > > Now that the inactive_list is protected by mm_lock, and everything
> > > > > > else on per-obj basis is protected by obj->lock, we no longer depend
> > > > > > on struct_mutex.
> > > > > >
> > > > > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > > > > ---
> > > > > >  drivers/gpu/drm/msm/msm_gem.c          |  1 -
> > > > > >  drivers/gpu/drm/msm/msm_gem_shrinker.c | 54 --------------------------
> > > > > >  2 files changed, 55 deletions(-)
> > > > > >
> > > > > [...]
> > > > >
> > > > > > @@ -71,13 +33,8 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
> > > > > >  {
> > > > > >     struct msm_drm_private *priv =
> > > > > >             container_of(shrinker, struct msm_drm_private, shrinker);
> > > > > > -   struct drm_device *dev = priv->dev;
> > > > > >     struct msm_gem_object *msm_obj;
> > > > > >     unsigned long freed = 0;
> > > > > > -   bool unlock;
> > > > > > -
> > > > > > -   if (!msm_gem_shrinker_lock(dev, &unlock))
> > > > > > -           return SHRINK_STOP;
> > > > > >
> > > > > >     mutex_lock(&priv->mm_lock);
> > > > >
> > > > > Better if the change in behavior is documented that SHRINK_STOP will
> > > > > no longer be needed.
> > > >
> > > > btw I read through this and noticed you have your own obj lock, plus
> > > > mutex_lock_nested. I strongly recommend to just cut over to dma_resv_lock
> > > > for all object lock needs (soc drivers have been terrible with this
> > > > unfortuntaly), and in the shrinker just use dma_resv_trylock instead of
> > > > trying to play clever games outsmarting lockdep.
> >
> > The trylock makes page reclaimers turn to their next target e.g. inode
> > cache instead of waiting for the mutex to be released. It makes sense
> > for instance in scenarios of mild memory pressure.
> 
> is there some behind-the-scenes signalling for this, or is this just
> down to what the shrinker callbacks return?  Generally when we get
> into shrinking, there are a big set of purgable bo's to consider, so
> the shrinker callback return wouldn't be considering just one
> potentially lock contended bo (buffer object).  Ie failing one
> trylock, we just move on to the next.
> 
> fwiw, what I've seen on the userspace bo cache vs shrinker (anything
> that is shrinker potential is in userspace bo cache and
> MADV(WONTNEED)) is that in steady state I see a very strong recycling
> of bo's (which avoids allocating and mmap'ing or mapping to gpu a new
> buffer object), so it is definitely a win in mmap/realloc bandwidth..
> in steady state there is a lot of free and realloc of same-sized
> buffers from frame to frame.
> 
> But in transient situations like moving to new game level when there
> is a heavy memory pressure and lots of freeing old
> buffers/textures/etc and then allocating new ones, I see shrinker
> kicking in hard (in android situations, not so much so with
> traditional linux userspace)

Yeah per-buffer trylock is fine. Trylock on the mm_lock (or anything else
device-global, like struct_mutex and msm_gem_shrinker_lock) I think isn't
fine, since if you're unlucky you're hogging a ton of memory and that's
the only freeable resource in the system. Going to other shrinkers won't
help when it's the gpu shrinker that has all the freeable memory.

Also other shrinkers (inode and all these) also do lots of per-object
trylocking. I think there's a canonical threshold of shrinker rounds where
you're supposed to try harder (if possible), but that doesn't apply to
dma_resv_lock.
-Daniel

> 
> BR,
> -R
> 
> >
> > > >
> > > > I recently wrote an entire blog length rant on why I think
> > > > mutex_lock_nested is too dangerous to be useful:
> > > >
> > > > https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
> > > >
> > > > Not anything about this here, just general comment. The problem extends to
> > > > shmem helpers and all that also having their own locks for everything.
> > >
> > > This is definitely a tangible improvement though - very happy to see
> > > msm_gem_shrinker_lock() go.
> > >
> > > Reviewed-by: Kristian H. Kristensen <hoegsberg@google.com>
> > >
> > > > -Daniel
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel