Re: [PATCH 1/7] drm/i915: Introduce i915_address_space.mutex

[Daniel Vetter <daniel@ffwll.ch>]

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@chris-wilson.co.uk>

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.
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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx