| Message ID | 20180711073608.20286-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > 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
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
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@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. 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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
Quoting Daniel Vetter (2018-07-11 10:36:36) > 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@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. It's vma->pin_count, so I guarded it with vm->mutex (along with the drm_mm and vm->*list, with some vigorous pruning of lists to reduce the locking surface). There's also the obj->vma_list and trees that are moved to a obj->vma_lock. > > 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. Right, the only way is to avoid us using reservation_obj->lock inside the shrinker, which is quite easy (for the moment at least, I've haven't completed the i915_request eradication of struct_mutex for ordering which is likely to require more reservation_obj or similar). I do have a branch that gets us to no struct_mutex inside intel_display and to remove struct_mutex from the shrinker within 10 more ugly patches that pass CI, which is a start. Something that I want to drip feed slowly so that we can catch the high MTBF regressions that are inevitable. -Chris
On Wed, Jul 11, 2018 at 10:49:51AM +0100, Chris Wilson wrote: > Quoting Daniel Vetter (2018-07-11 10:36:36) > > 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@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. > > It's vma->pin_count, so I guarded it with vm->mutex (along with the > drm_mm and vm->*list, with some vigorous pruning of lists to reduce the > locking surface). There's also the obj->vma_list and trees that are > moved to a obj->vma_lock. Hm, Christian König's series to wrap dma_buf_map/unmap in the reservation_obj is why I thought we'd need that. But just for the unmap of foreign objects I guess we can always punt that to some worker, or just don't bother if it's contended. > > > 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. > > Right, the only way is to avoid us using reservation_obj->lock inside > the shrinker, which is quite easy (for the moment at least, I've haven't > completed the i915_request eradication of struct_mutex for ordering > which is likely to require more reservation_obj or similar). Yeah I thihnk as long as i915 doesn't need it we can get away with trylocks for foreign objects (which I think will need it, sooner or later). > I do have a branch that gets us to no struct_mutex inside intel_display > and to remove struct_mutex from the shrinker within 10 more ugly patches > that pass CI, which is a start. Something that I want to drip feed > slowly so that we can catch the high MTBF regressions that are inevitable. Very much agreed on drip feeding :-) -Daniel
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); +}
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> --- 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(-)