On Wed, Jul 11, 2018 at 11:33:26AM +0200, Daniel Vetter wrote:
> On Wed, Jul 11, 2018 at 08:36:02AM +0100, Chris Wilson wrote:
> > Add a mutex into struct i915_address_space to be used while operating on
> > the vma and their lists for a particular vm. As this may be called from
> > the shrinker, we taint the mutex with fs_reclaim so that from the start
> > lockdep warns us if we are caught holding the mutex across an
> > allocation. (With such small steps we will eventually rid ourselves of
> > struct_mutex recursion!)
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>
> Not sure it exists in a branch of yours already, but here's my thoughts on
> extending this to the address_space lrus and the shrinker callback (which
> I think would be the next step with good pay-off):
>
> 1. make sure pin_count is protected by reservation_obj.
> 2. grab the vm.mutex when walking LRUs everywhere. This is going to be
> tricky for ggtt because of runtime PM. Between lock-dropping, carefully
> avoiding rpm when cleaning up objects and just grabbing an rpm wakeref
> when walking the ggtt vm this should be possible to work around (since for
> the fences we clearly need to be able to nest the vm.mutex within rpm or
> we're busted).
> 3. In the shrinker trylock the reservation_obj and treat a failure to get
> the lock as if pin_count is elevated. If we can't shrink enough then grab
> a temporary reference to the bo using kref_get_unless_zero, drop the
> vm.mutex (since that's what gave us the weak ref) and do a blocking
> reservation_obj lock.
Ok this doesn't work, because reservation_obj needs to allow allocations.
But compared to our current lock stealing trickery the above scheme
reduces possibilities to shrink, or at least rate-limit command submission
somewhat. Not sure how to best tackle that.
Either way a fs_reclaim_*() trick to annotate reservation_obj would be
good ...
-Daniel
> 4. Audit the obj/vma unbind paths and apply reservation_obj or vm.mutex
> locking as needed until we can drop the struct_mutex from the shrinker.
> Biggest trouble is probably ggtt mmap (but I think ordering of pte
> shootdown takes care of that) and drm_mm ("just" needs to be protected by
> vm.mutex too I think).
>
> Plan is probably the underestimation of the decade, at least :-)
> -Daniel
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > drivers/gpu/drm/i915/i915_gem_gtt.c | 10 ++++++++++
> > drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++
> > drivers/gpu/drm/i915/i915_gem_shrinker.c | 12 ++++++++++++
> > 4 files changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index eeb002a47032..01dd29837233 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3304,7 +3304,7 @@ unsigned long i915_gem_shrink(struct drm_i915_private *i915,
> > unsigned long i915_gem_shrink_all(struct drm_i915_private *i915);
> > void i915_gem_shrinker_register(struct drm_i915_private *i915);
> > void i915_gem_shrinker_unregister(struct drm_i915_private *i915);
> > -
> > +void i915_gem_shrinker_taints_mutex(struct mutex *mutex);
> >
> > /* i915_gem_tiling.c */
> > static inline bool i915_gem_object_needs_bit17_swizzle(struct drm_i915_gem_object *obj)
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index abd81fb9b0b6..d0acef299b9c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -531,6 +531,14 @@ static void vm_free_page(struct i915_address_space *vm, struct page *page)
> > static void i915_address_space_init(struct i915_address_space *vm,
> > struct drm_i915_private *dev_priv)
> > {
> > + /*
> > + * The vm->mutex must be reclaim safe (for use in the shrinker).
> > + * Do a dummy acquire now under fs_reclaim so that any allocation
> > + * attempt holding the lock is immediately reported by lockdep.
> > + */
> > + mutex_init(&vm->mutex);
> > + i915_gem_shrinker_taints_mutex(&vm->mutex);
> > +
> > GEM_BUG_ON(!vm->total);
> > drm_mm_init(&vm->mm, 0, vm->total);
> > vm->mm.head_node.color = I915_COLOR_UNEVICTABLE;
> > @@ -551,6 +559,8 @@ static void i915_address_space_fini(struct i915_address_space *vm)
> > spin_unlock(&vm->free_pages.lock);
> >
> > drm_mm_takedown(&vm->mm);
> > +
> > + mutex_destroy(&vm->mutex);
> > }
> >
> > static int __setup_page_dma(struct i915_address_space *vm,
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > index feda45dfd481..14e62651010b 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > @@ -293,6 +293,8 @@ struct i915_address_space {
> >
> > bool closed;
> >
> > + struct mutex mutex; /* protects vma and our lists */
> > +
> > struct i915_page_dma scratch_page;
> > struct i915_page_table *scratch_pt;
> > struct i915_page_directory *scratch_pd;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > index c61f5b80fee3..ea90d3a0d511 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> > @@ -23,6 +23,7 @@
> > */
> >
> > #include <linux/oom.h>
> > +#include <linux/sched/mm.h>
> > #include <linux/shmem_fs.h>
> > #include <linux/slab.h>
> > #include <linux/swap.h>
> > @@ -531,3 +532,14 @@ void i915_gem_shrinker_unregister(struct drm_i915_private *i915)
> > WARN_ON(unregister_oom_notifier(&i915->mm.oom_notifier));
> > unregister_shrinker(&i915->mm.shrinker);
> > }
> > +
> > +void i915_gem_shrinker_taints_mutex(struct mutex *mutex)
> > +{
> > + if (!IS_ENABLED(CONFIG_LOCKDEP))
> > + return;
> > +
> > + fs_reclaim_acquire(GFP_KERNEL);
> > + mutex_lock(mutex);
> > + mutex_unlock(mutex);
> > + fs_reclaim_release(GFP_KERNEL);
> > +}
> > --
> > 2.18.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
