| Message ID | 20191104173720.2696-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915: Switch obj->mm.lock lockdep annotations on its head | expand |
Quoting Daniel Vetter (2019-11-04 19:37:18) > The trouble with having a plain nesting flag for locks which do not > naturally nest (unlike block devices and their partitions, which is > the original motivation for nesting levels) is that lockdep will > never spot a true deadlock if you screw up. > > This patch is an attempt at trying better, by highlighting a bit more > the actual nature of the nesting that's going on. Essentially we have > two kinds of objects: > > - objects without pages allocated, which cannot be on any lru and are > hence inaccessible to the shrinker. > > - objects which have pages allocated, which are on an lru, and which > the shrinker can decide to throw out. > > For the former type of object, memory allcoations while holding > obj->mm.lock are permissible. For the latter they are not. And > get/put_pages transitions between the two types of objects. > > This is still not entirely fool-proof since the rules might chance. > But as long as we run such a code ever at runtime lockdep should be > able to observe the inconsistency and complain (like with any other > lockdep class that we've split up in multiple classes). But there are > a few clear benefits: > > - We can drop the nesting flag parameter from > __i915_gem_object_put_pages, because that function by definition is > never going allocate memory, and calling it on an object which > doesn't have its pages allocated would be a bug. > > - We strictly catch more bugs, since there's not only one place in the > entire tree which is annotated with the special class. All the > other places that had explicit lockdep nesting annotations we're now > going to leave up to lockdep again. > > - Specifically this catches stuff like calling get_pages from > put_pages (which isn't really a good idea, if we can call put_pages get_pages? > so could the shrinker). I've seen patches do exactly that. > > Of course I fully expect CI will show me for the fool I am with this > one here :-) > > v2: There can only be one (lockdep only has a cache for the first > subclass, not for deeper ones, and we don't want to make these locks > even slower). Still separate enums for better documentation. > > Real fix: don forget about phys objs and pin_map(), and fix the > shrinker to have the right annotations ... silly me. > > v3: Forgot usertptr too ... > > v4: Improve comment for pages_pin_count, drop the IMPORTANT comment > and instead prime lockdep (Chris). > > v5: Appease checkpatch, no double empty lines (Chris) > > v6: More rebasing over selftest changes. Also somehow I forgot to > push this patch :-/ > > Also format comments consistently while at it. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: "Tang, CQ" <cq.tang@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v5) > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Other than the below comment; Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Regards, Joonas
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > @@ -22,6 +22,8 @@ > * > */ > > +#include <linux/sched/mm.h> > + > #include "display/intel_frontbuffer.h" > #include "gt/intel_gt.h" > #include "i915_drv.h" > @@ -52,6 +54,14 @@ void i915_gem_object_init(struct > drm_i915_gem_object *obj, { > __mutex_init(&obj->mm.lock, "obj->mm.lock", key); > > + if (IS_ENABLED(CONFIG_LOCKDEP)) { > + mutex_lock_nested(&obj->mm.lock, > I915_MM_GET_PAGES); > + fs_reclaim_acquire(GFP_KERNEL); > + might_lock(&obj->mm.lock); > + fs_reclaim_release(GFP_KERNEL); > + mutex_unlock(&obj->mm.lock); > + } > + I looked the upstream code in drm-tip, I see other changes but not above. Is this correct? --CQ > spin_lock_init(&obj->vma.lock); > INIT_LIST_HEAD(&obj->vma.list); > > @@ -186,7 +196,7 @@ static void __i915_gem_free_objects(struct > drm_i915_private *i915, > GEM_BUG_ON(!list_empty(&obj->lut_list)); > > atomic_set(&obj->mm.pages_pin_count, 0); > - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); > + __i915_gem_object_put_pages(obj); > GEM_BUG_ON(i915_gem_object_has_pages(obj)); > bitmap_free(obj->bit_17); > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h > b/drivers/gpu/drm/i915/gem/i915_gem_object.h > index 458cd51331f1..edaf7126a84d 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > @@ -319,11 +319,22 @@ i915_gem_object_unpin_pages(struct > drm_i915_gem_object *obj) > > enum i915_mm_subclass { /* lockdep subclass for obj- > >mm.lock/struct_mutex */ > I915_MM_NORMAL = 0, > - I915_MM_SHRINKER /* called "recursively" from direct-reclaim- > esque */ > + /* > + * Only used by struct_mutex, when called "recursively" from > + * direct-reclaim-esque. Safe because there is only every one > + * struct_mutex in the entire system. > + */ > + I915_MM_SHRINKER = 1, > + /* > + * Used for obj->mm.lock when allocating pages. Safe because the > object > + * isn't yet on any LRU, and therefore the shrinker can't deadlock on > + * it. As soon as the object has pages, obj->mm.lock nests within > + * fs_reclaim. > + */ > + I915_MM_GET_PAGES = 1, > }; > > -int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj, > - enum i915_mm_subclass subclass); > +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj); > void i915_gem_object_truncate(struct drm_i915_gem_object *obj); void > i915_gem_object_writeback(struct drm_i915_gem_object *obj); > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > index 96008374a412..15f8297dc34e 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > @@ -162,7 +162,11 @@ struct drm_i915_gem_object { > atomic_t bind_count; > > struct { > - struct mutex lock; /* protects the pages and their use */ > + /* > + * Protects the pages and their use. Do not use directly, but > + * instead go through the pin/unpin interfaces. > + */ > + struct mutex lock; > atomic_t pages_pin_count; > atomic_t shrink_pin; > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c > b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > index 29f4c2850745..f402c2c415c2 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > @@ -106,7 +106,7 @@ int __i915_gem_object_get_pages(struct > drm_i915_gem_object *obj) { > int err; > > - err = mutex_lock_interruptible(&obj->mm.lock); > + err = mutex_lock_interruptible_nested(&obj->mm.lock, > +I915_MM_GET_PAGES); > if (err) > return err; > > @@ -190,8 +190,7 @@ __i915_gem_object_unset_pages(struct > drm_i915_gem_object *obj) > return pages; > } > > -int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj, > - enum i915_mm_subclass subclass) > +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj) > { > struct sg_table *pages; > int err; > @@ -202,7 +201,7 @@ int __i915_gem_object_put_pages(struct > drm_i915_gem_object *obj, > GEM_BUG_ON(atomic_read(&obj->bind_count)); > > /* May be called by shrinker from within get_pages() (on another bo) > */ > - mutex_lock_nested(&obj->mm.lock, subclass); > + mutex_lock(&obj->mm.lock); > if (unlikely(atomic_read(&obj->mm.pages_pin_count))) { > err = -EBUSY; > goto unlock; > @@ -308,7 +307,7 @@ void *i915_gem_object_pin_map(struct > drm_i915_gem_object *obj, > if (!i915_gem_object_type_has(obj, flags)) > return ERR_PTR(-ENXIO); > > - err = mutex_lock_interruptible(&obj->mm.lock); > + err = mutex_lock_interruptible_nested(&obj->mm.lock, > +I915_MM_GET_PAGES); > if (err) > return ERR_PTR(err); > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c > b/drivers/gpu/drm/i915/gem/i915_gem_phys.c > index 8043ff63d73f..b1b7c1b3038a 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c > @@ -164,7 +164,7 @@ int i915_gem_object_attach_phys(struct > drm_i915_gem_object *obj, int align) > if (err) > return err; > > - mutex_lock(&obj->mm.lock); > + mutex_lock_nested(&obj->mm.lock, I915_MM_GET_PAGES); > > if (obj->mm.madv != I915_MADV_WILLNEED) { > err = -EFAULT; > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > index fd3ce6da8497..066b3df677e8 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > @@ -57,7 +57,7 @@ static bool unsafe_drop_pages(struct > drm_i915_gem_object *obj, > flags = I915_GEM_OBJECT_UNBIND_ACTIVE; > > if (i915_gem_object_unbind(obj, flags) == 0) > - __i915_gem_object_put_pages(obj, I915_MM_SHRINKER); > + __i915_gem_object_put_pages(obj); > > return !i915_gem_object_has_pages(obj); } @@ -209,8 +209,7 @@ > i915_gem_shrink(struct drm_i915_private *i915, > > if (unsafe_drop_pages(obj, shrink)) { > /* May arrive from get_pages on another bo > */ > - mutex_lock_nested(&obj->mm.lock, > - I915_MM_SHRINKER); > + mutex_lock(&obj->mm.lock); > if (!i915_gem_object_has_pages(obj)) { > try_to_writeback(obj, shrink); > count += obj->base.size >> > PAGE_SHIFT; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > index 1e045c337044..ee65c6acf0e2 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > @@ -131,7 +131,7 @@ userptr_mn_invalidate_range_start(struct > mmu_notifier *_mn, > ret = i915_gem_object_unbind(obj, > > I915_GEM_OBJECT_UNBIND_ACTIVE); > if (ret == 0) > - ret = __i915_gem_object_put_pages(obj, > I915_MM_SHRINKER); > + ret = __i915_gem_object_put_pages(obj); > i915_gem_object_put(obj); > if (ret) > return ret; > @@ -483,7 +483,7 @@ __i915_gem_userptr_get_pages_worker(struct > work_struct *_work) > } > } > > - mutex_lock(&obj->mm.lock); > + mutex_lock_nested(&obj->mm.lock, I915_MM_GET_PAGES); > if (obj->userptr.work == &work->work) { > struct sg_table *pages = ERR_PTR(ret); > > diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c > b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c > index 688c49a24f32..5c9583349077 100644 > --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c > +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c > @@ -517,7 +517,7 @@ static int > igt_mock_memory_region_huge_pages(void *arg) > i915_vma_unpin(vma); > i915_vma_close(vma); > > - __i915_gem_object_put_pages(obj, > I915_MM_NORMAL); > + __i915_gem_object_put_pages(obj); > i915_gem_object_put(obj); > } > } > @@ -650,7 +650,7 @@ static int igt_mock_ppgtt_misaligned_dma(void *arg) > i915_vma_close(vma); > > i915_gem_object_unpin_pages(obj); > - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); > + __i915_gem_object_put_pages(obj); > i915_gem_object_put(obj); > } > > @@ -678,7 +678,7 @@ static void close_object_list(struct list_head *objects, > > list_del(&obj->st_link); > i915_gem_object_unpin_pages(obj); > - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); > + __i915_gem_object_put_pages(obj); > i915_gem_object_put(obj); > } > } > @@ -948,7 +948,7 @@ static int igt_mock_ppgtt_64K(void *arg) > i915_vma_close(vma); > > i915_gem_object_unpin_pages(obj); > - __i915_gem_object_put_pages(obj, > I915_MM_NORMAL); > + __i915_gem_object_put_pages(obj); > i915_gem_object_put(obj); > } > } > @@ -1301,7 +1301,7 @@ static int igt_ppgtt_exhaust_huge(void *arg) > } > > i915_gem_object_unpin_pages(obj); > - __i915_gem_object_put_pages(obj, > I915_MM_NORMAL); > + __i915_gem_object_put_pages(obj); > i915_gem_object_put(obj); > } > } > @@ -1442,7 +1442,7 @@ static int igt_ppgtt_smoke_huge(void *arg) > } > out_unpin: > i915_gem_object_unpin_pages(obj); > - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); > + __i915_gem_object_put_pages(obj); > out_put: > i915_gem_object_put(obj); > > @@ -1530,7 +1530,7 @@ static int igt_ppgtt_sanity_check(void *arg) > err = igt_write_huge(ctx, obj); > > i915_gem_object_unpin_pages(obj); > - __i915_gem_object_put_pages(obj, > I915_MM_NORMAL); > + __i915_gem_object_put_pages(obj); > i915_gem_object_put(obj); > > if (err) { > diff --git a/drivers/gpu/drm/i915/selftests/intel_memory_region.c > b/drivers/gpu/drm/i915/selftests/intel_memory_region.c > index 19e1cca8f143..95d609abd39b 100644 > --- a/drivers/gpu/drm/i915/selftests/intel_memory_region.c > +++ b/drivers/gpu/drm/i915/selftests/intel_memory_region.c > @@ -32,7 +32,7 @@ static void close_objects(struct intel_memory_region > *mem, > if (i915_gem_object_has_pinned_pages(obj)) > i915_gem_object_unpin_pages(obj); > /* No polluting the memory region between tests */ > - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); > + __i915_gem_object_put_pages(obj); > list_del(&obj->st_link); > i915_gem_object_put(obj); > } > @@ -122,7 +122,7 @@ igt_object_create(struct intel_memory_region *mem, > static void igt_object_release(struct drm_i915_gem_object *obj) { > i915_gem_object_unpin_pages(obj); > - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); > + __i915_gem_object_put_pages(obj); > list_del(&obj->st_link); > i915_gem_object_put(obj); > } > -- > 2.24.0.rc2
On Thu, Nov 7, 2019 at 8:57 PM Tang, CQ <cq.tang@intel.com> wrote: > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > > @@ -22,6 +22,8 @@ > > * > > */ > > > > +#include <linux/sched/mm.h> > > + > > #include "display/intel_frontbuffer.h" > > #include "gt/intel_gt.h" > > #include "i915_drv.h" > > @@ -52,6 +54,14 @@ void i915_gem_object_init(struct > > drm_i915_gem_object *obj, { > > __mutex_init(&obj->mm.lock, "obj->mm.lock", key); > > > > + if (IS_ENABLED(CONFIG_LOCKDEP)) { > > + mutex_lock_nested(&obj->mm.lock, > > I915_MM_GET_PAGES); > > + fs_reclaim_acquire(GFP_KERNEL); > > + might_lock(&obj->mm.lock); > > + fs_reclaim_release(GFP_KERNEL); > > + mutex_unlock(&obj->mm.lock); > > + } > > + > > I looked the upstream code in drm-tip, I see other changes but not above. Is this correct? Yeah I had to drop this because the lmem code breaks this. It allocates memory while holding memory manager locks, which in turn depend upon obj->mm.lock. That already blew up a bit, and got papered over by splitting up the lock classes. As a temporary measure only (I hope at least). I still have this as a patch locally, so once the lmem locking is sorted I can submit it, so it's not lost. -Daniel > > --CQ > > > > spin_lock_init(&obj->vma.lock); > > INIT_LIST_HEAD(&obj->vma.list); > > > > @@ -186,7 +196,7 @@ static void __i915_gem_free_objects(struct > > drm_i915_private *i915, > > GEM_BUG_ON(!list_empty(&obj->lut_list)); > > > > atomic_set(&obj->mm.pages_pin_count, 0); > > - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); > > + __i915_gem_object_put_pages(obj); > > GEM_BUG_ON(i915_gem_object_has_pages(obj)); > > bitmap_free(obj->bit_17); > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h > > b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > index 458cd51331f1..edaf7126a84d 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > @@ -319,11 +319,22 @@ i915_gem_object_unpin_pages(struct > > drm_i915_gem_object *obj) > > > > enum i915_mm_subclass { /* lockdep subclass for obj- > > >mm.lock/struct_mutex */ > > I915_MM_NORMAL = 0, > > - I915_MM_SHRINKER /* called "recursively" from direct-reclaim- > > esque */ > > + /* > > + * Only used by struct_mutex, when called "recursively" from > > + * direct-reclaim-esque. Safe because there is only every one > > + * struct_mutex in the entire system. > > + */ > > + I915_MM_SHRINKER = 1, > > + /* > > + * Used for obj->mm.lock when allocating pages. Safe because the > > object > > + * isn't yet on any LRU, and therefore the shrinker can't deadlock on > > + * it. As soon as the object has pages, obj->mm.lock nests within > > + * fs_reclaim. > > + */ > > + I915_MM_GET_PAGES = 1, > > }; > > > > -int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj, > > - enum i915_mm_subclass subclass); > > +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj); > > void i915_gem_object_truncate(struct drm_i915_gem_object *obj); void > > i915_gem_object_writeback(struct drm_i915_gem_object *obj); > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > index 96008374a412..15f8297dc34e 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > @@ -162,7 +162,11 @@ struct drm_i915_gem_object { > > atomic_t bind_count; > > > > struct { > > - struct mutex lock; /* protects the pages and their use */ > > + /* > > + * Protects the pages and their use. Do not use directly, but > > + * instead go through the pin/unpin interfaces. > > + */ > > + struct mutex lock; > > atomic_t pages_pin_count; > > atomic_t shrink_pin; > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c > > b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > > index 29f4c2850745..f402c2c415c2 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > > @@ -106,7 +106,7 @@ int __i915_gem_object_get_pages(struct > > drm_i915_gem_object *obj) { > > int err; > > > > - err = mutex_lock_interruptible(&obj->mm.lock); > > + err = mutex_lock_interruptible_nested(&obj->mm.lock, > > +I915_MM_GET_PAGES); > > if (err) > > return err; > > > > @@ -190,8 +190,7 @@ __i915_gem_object_unset_pages(struct > > drm_i915_gem_object *obj) > > return pages; > > } > > > > -int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj, > > - enum i915_mm_subclass subclass) > > +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj) > > { > > struct sg_table *pages; > > int err; > > @@ -202,7 +201,7 @@ int __i915_gem_object_put_pages(struct > > drm_i915_gem_object *obj, > > GEM_BUG_ON(atomic_read(&obj->bind_count)); > > > > /* May be called by shrinker from within get_pages() (on another bo) > > */ > > - mutex_lock_nested(&obj->mm.lock, subclass); > > + mutex_lock(&obj->mm.lock); > > if (unlikely(atomic_read(&obj->mm.pages_pin_count))) { > > err = -EBUSY; > > goto unlock; > > @@ -308,7 +307,7 @@ void *i915_gem_object_pin_map(struct > > drm_i915_gem_object *obj, > > if (!i915_gem_object_type_has(obj, flags)) > > return ERR_PTR(-ENXIO); > > > > - err = mutex_lock_interruptible(&obj->mm.lock); > > + err = mutex_lock_interruptible_nested(&obj->mm.lock, > > +I915_MM_GET_PAGES); > > if (err) > > return ERR_PTR(err); > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c > > b/drivers/gpu/drm/i915/gem/i915_gem_phys.c > > index 8043ff63d73f..b1b7c1b3038a 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c > > @@ -164,7 +164,7 @@ int i915_gem_object_attach_phys(struct > > drm_i915_gem_object *obj, int align) > > if (err) > > return err; > > > > - mutex_lock(&obj->mm.lock); > > + mutex_lock_nested(&obj->mm.lock, I915_MM_GET_PAGES); > > > > if (obj->mm.madv != I915_MADV_WILLNEED) { > > err = -EFAULT; > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > > b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > > index fd3ce6da8497..066b3df677e8 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > > @@ -57,7 +57,7 @@ static bool unsafe_drop_pages(struct > > drm_i915_gem_object *obj, > > flags = I915_GEM_OBJECT_UNBIND_ACTIVE; > > > > if (i915_gem_object_unbind(obj, flags) == 0) > > - __i915_gem_object_put_pages(obj, I915_MM_SHRINKER); > > + __i915_gem_object_put_pages(obj); > > > > return !i915_gem_object_has_pages(obj); } @@ -209,8 +209,7 @@ > > i915_gem_shrink(struct drm_i915_private *i915, > > > > if (unsafe_drop_pages(obj, shrink)) { > > /* May arrive from get_pages on another bo > > */ > > - mutex_lock_nested(&obj->mm.lock, > > - I915_MM_SHRINKER); > > + mutex_lock(&obj->mm.lock); > > if (!i915_gem_object_has_pages(obj)) { > > try_to_writeback(obj, shrink); > > count += obj->base.size >> > > PAGE_SHIFT; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > index 1e045c337044..ee65c6acf0e2 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > @@ -131,7 +131,7 @@ userptr_mn_invalidate_range_start(struct > > mmu_notifier *_mn, > > ret = i915_gem_object_unbind(obj, > > > > I915_GEM_OBJECT_UNBIND_ACTIVE); > > if (ret == 0) > > - ret = __i915_gem_object_put_pages(obj, > > I915_MM_SHRINKER); > > + ret = __i915_gem_object_put_pages(obj); > > i915_gem_object_put(obj); > > if (ret) > > return ret; > > @@ -483,7 +483,7 @@ __i915_gem_userptr_get_pages_worker(struct > > work_struct *_work) > > } > > } > > > > - mutex_lock(&obj->mm.lock); > > + mutex_lock_nested(&obj->mm.lock, I915_MM_GET_PAGES); > > if (obj->userptr.work == &work->work) { > > struct sg_table *pages = ERR_PTR(ret); > > > > diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c > > b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c > > index 688c49a24f32..5c9583349077 100644 > > --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c > > +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c > > @@ -517,7 +517,7 @@ static int > > igt_mock_memory_region_huge_pages(void *arg) > > i915_vma_unpin(vma); > > i915_vma_close(vma); > > > > - __i915_gem_object_put_pages(obj, > > I915_MM_NORMAL); > > + __i915_gem_object_put_pages(obj); > > i915_gem_object_put(obj); > > } > > } > > @@ -650,7 +650,7 @@ static int igt_mock_ppgtt_misaligned_dma(void *arg) > > i915_vma_close(vma); > > > > i915_gem_object_unpin_pages(obj); > > - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); > > + __i915_gem_object_put_pages(obj); > > i915_gem_object_put(obj); > > } > > > > @@ -678,7 +678,7 @@ static void close_object_list(struct list_head *objects, > > > > list_del(&obj->st_link); > > i915_gem_object_unpin_pages(obj); > > - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); > > + __i915_gem_object_put_pages(obj); > > i915_gem_object_put(obj); > > } > > } > > @@ -948,7 +948,7 @@ static int igt_mock_ppgtt_64K(void *arg) > > i915_vma_close(vma); > > > > i915_gem_object_unpin_pages(obj); > > - __i915_gem_object_put_pages(obj, > > I915_MM_NORMAL); > > + __i915_gem_object_put_pages(obj); > > i915_gem_object_put(obj); > > } > > } > > @@ -1301,7 +1301,7 @@ static int igt_ppgtt_exhaust_huge(void *arg) > > } > > > > i915_gem_object_unpin_pages(obj); > > - __i915_gem_object_put_pages(obj, > > I915_MM_NORMAL); > > + __i915_gem_object_put_pages(obj); > > i915_gem_object_put(obj); > > } > > } > > @@ -1442,7 +1442,7 @@ static int igt_ppgtt_smoke_huge(void *arg) > > } > > out_unpin: > > i915_gem_object_unpin_pages(obj); > > - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); > > + __i915_gem_object_put_pages(obj); > > out_put: > > i915_gem_object_put(obj); > > > > @@ -1530,7 +1530,7 @@ static int igt_ppgtt_sanity_check(void *arg) > > err = igt_write_huge(ctx, obj); > > > > i915_gem_object_unpin_pages(obj); > > - __i915_gem_object_put_pages(obj, > > I915_MM_NORMAL); > > + __i915_gem_object_put_pages(obj); > > i915_gem_object_put(obj); > > > > if (err) { > > diff --git a/drivers/gpu/drm/i915/selftests/intel_memory_region.c > > b/drivers/gpu/drm/i915/selftests/intel_memory_region.c > > index 19e1cca8f143..95d609abd39b 100644 > > --- a/drivers/gpu/drm/i915/selftests/intel_memory_region.c > > +++ b/drivers/gpu/drm/i915/selftests/intel_memory_region.c > > @@ -32,7 +32,7 @@ static void close_objects(struct intel_memory_region > > *mem, > > if (i915_gem_object_has_pinned_pages(obj)) > > i915_gem_object_unpin_pages(obj); > > /* No polluting the memory region between tests */ > > - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); > > + __i915_gem_object_put_pages(obj); > > list_del(&obj->st_link); > > i915_gem_object_put(obj); > > } > > @@ -122,7 +122,7 @@ igt_object_create(struct intel_memory_region *mem, > > static void igt_object_release(struct drm_i915_gem_object *obj) { > > i915_gem_object_unpin_pages(obj); > > - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); > > + __i915_gem_object_put_pages(obj); > > list_del(&obj->st_link); > > i915_gem_object_put(obj); > > } > > -- > > 2.24.0.rc2 >
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c index a50296cce0d8..078d515d72c0 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c @@ -22,6 +22,8 @@ * */ +#include <linux/sched/mm.h> + #include "display/intel_frontbuffer.h" #include "gt/intel_gt.h" #include "i915_drv.h" @@ -52,6 +54,14 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, { __mutex_init(&obj->mm.lock, "obj->mm.lock", key); + if (IS_ENABLED(CONFIG_LOCKDEP)) { + mutex_lock_nested(&obj->mm.lock, I915_MM_GET_PAGES); + fs_reclaim_acquire(GFP_KERNEL); + might_lock(&obj->mm.lock); + fs_reclaim_release(GFP_KERNEL); + mutex_unlock(&obj->mm.lock); + } + spin_lock_init(&obj->vma.lock); INIT_LIST_HEAD(&obj->vma.list); @@ -186,7 +196,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915, GEM_BUG_ON(!list_empty(&obj->lut_list)); atomic_set(&obj->mm.pages_pin_count, 0); - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); + __i915_gem_object_put_pages(obj); GEM_BUG_ON(i915_gem_object_has_pages(obj)); bitmap_free(obj->bit_17); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 458cd51331f1..edaf7126a84d 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -319,11 +319,22 @@ i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj) enum i915_mm_subclass { /* lockdep subclass for obj->mm.lock/struct_mutex */ I915_MM_NORMAL = 0, - I915_MM_SHRINKER /* called "recursively" from direct-reclaim-esque */ + /* + * Only used by struct_mutex, when called "recursively" from + * direct-reclaim-esque. Safe because there is only every one + * struct_mutex in the entire system. + */ + I915_MM_SHRINKER = 1, + /* + * Used for obj->mm.lock when allocating pages. Safe because the object + * isn't yet on any LRU, and therefore the shrinker can't deadlock on + * it. As soon as the object has pages, obj->mm.lock nests within + * fs_reclaim. + */ + I915_MM_GET_PAGES = 1, }; -int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj, - enum i915_mm_subclass subclass); +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj); void i915_gem_object_truncate(struct drm_i915_gem_object *obj); void i915_gem_object_writeback(struct drm_i915_gem_object *obj); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h index 96008374a412..15f8297dc34e 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h @@ -162,7 +162,11 @@ struct drm_i915_gem_object { atomic_t bind_count; struct { - struct mutex lock; /* protects the pages and their use */ + /* + * Protects the pages and their use. Do not use directly, but + * instead go through the pin/unpin interfaces. + */ + struct mutex lock; atomic_t pages_pin_count; atomic_t shrink_pin; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c index 29f4c2850745..f402c2c415c2 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c @@ -106,7 +106,7 @@ int __i915_gem_object_get_pages(struct drm_i915_gem_object *obj) { int err; - err = mutex_lock_interruptible(&obj->mm.lock); + err = mutex_lock_interruptible_nested(&obj->mm.lock, I915_MM_GET_PAGES); if (err) return err; @@ -190,8 +190,7 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj) return pages; } -int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj, - enum i915_mm_subclass subclass) +int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj) { struct sg_table *pages; int err; @@ -202,7 +201,7 @@ int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj, GEM_BUG_ON(atomic_read(&obj->bind_count)); /* May be called by shrinker from within get_pages() (on another bo) */ - mutex_lock_nested(&obj->mm.lock, subclass); + mutex_lock(&obj->mm.lock); if (unlikely(atomic_read(&obj->mm.pages_pin_count))) { err = -EBUSY; goto unlock; @@ -308,7 +307,7 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj, if (!i915_gem_object_type_has(obj, flags)) return ERR_PTR(-ENXIO); - err = mutex_lock_interruptible(&obj->mm.lock); + err = mutex_lock_interruptible_nested(&obj->mm.lock, I915_MM_GET_PAGES); if (err) return ERR_PTR(err); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c index 8043ff63d73f..b1b7c1b3038a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c @@ -164,7 +164,7 @@ int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj, int align) if (err) return err; - mutex_lock(&obj->mm.lock); + mutex_lock_nested(&obj->mm.lock, I915_MM_GET_PAGES); if (obj->mm.madv != I915_MADV_WILLNEED) { err = -EFAULT; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c index fd3ce6da8497..066b3df677e8 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c @@ -57,7 +57,7 @@ static bool unsafe_drop_pages(struct drm_i915_gem_object *obj, flags = I915_GEM_OBJECT_UNBIND_ACTIVE; if (i915_gem_object_unbind(obj, flags) == 0) - __i915_gem_object_put_pages(obj, I915_MM_SHRINKER); + __i915_gem_object_put_pages(obj); return !i915_gem_object_has_pages(obj); } @@ -209,8 +209,7 @@ i915_gem_shrink(struct drm_i915_private *i915, if (unsafe_drop_pages(obj, shrink)) { /* May arrive from get_pages on another bo */ - mutex_lock_nested(&obj->mm.lock, - I915_MM_SHRINKER); + mutex_lock(&obj->mm.lock); if (!i915_gem_object_has_pages(obj)) { try_to_writeback(obj, shrink); count += obj->base.size >> PAGE_SHIFT; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c index 1e045c337044..ee65c6acf0e2 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c @@ -131,7 +131,7 @@ userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, ret = i915_gem_object_unbind(obj, I915_GEM_OBJECT_UNBIND_ACTIVE); if (ret == 0) - ret = __i915_gem_object_put_pages(obj, I915_MM_SHRINKER); + ret = __i915_gem_object_put_pages(obj); i915_gem_object_put(obj); if (ret) return ret; @@ -483,7 +483,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) } } - mutex_lock(&obj->mm.lock); + mutex_lock_nested(&obj->mm.lock, I915_MM_GET_PAGES); if (obj->userptr.work == &work->work) { struct sg_table *pages = ERR_PTR(ret); diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c index 688c49a24f32..5c9583349077 100644 --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c @@ -517,7 +517,7 @@ static int igt_mock_memory_region_huge_pages(void *arg) i915_vma_unpin(vma); i915_vma_close(vma); - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); + __i915_gem_object_put_pages(obj); i915_gem_object_put(obj); } } @@ -650,7 +650,7 @@ static int igt_mock_ppgtt_misaligned_dma(void *arg) i915_vma_close(vma); i915_gem_object_unpin_pages(obj); - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); + __i915_gem_object_put_pages(obj); i915_gem_object_put(obj); } @@ -678,7 +678,7 @@ static void close_object_list(struct list_head *objects, list_del(&obj->st_link); i915_gem_object_unpin_pages(obj); - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); + __i915_gem_object_put_pages(obj); i915_gem_object_put(obj); } } @@ -948,7 +948,7 @@ static int igt_mock_ppgtt_64K(void *arg) i915_vma_close(vma); i915_gem_object_unpin_pages(obj); - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); + __i915_gem_object_put_pages(obj); i915_gem_object_put(obj); } } @@ -1301,7 +1301,7 @@ static int igt_ppgtt_exhaust_huge(void *arg) } i915_gem_object_unpin_pages(obj); - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); + __i915_gem_object_put_pages(obj); i915_gem_object_put(obj); } } @@ -1442,7 +1442,7 @@ static int igt_ppgtt_smoke_huge(void *arg) } out_unpin: i915_gem_object_unpin_pages(obj); - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); + __i915_gem_object_put_pages(obj); out_put: i915_gem_object_put(obj); @@ -1530,7 +1530,7 @@ static int igt_ppgtt_sanity_check(void *arg) err = igt_write_huge(ctx, obj); i915_gem_object_unpin_pages(obj); - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); + __i915_gem_object_put_pages(obj); i915_gem_object_put(obj); if (err) { diff --git a/drivers/gpu/drm/i915/selftests/intel_memory_region.c b/drivers/gpu/drm/i915/selftests/intel_memory_region.c index 19e1cca8f143..95d609abd39b 100644 --- a/drivers/gpu/drm/i915/selftests/intel_memory_region.c +++ b/drivers/gpu/drm/i915/selftests/intel_memory_region.c @@ -32,7 +32,7 @@ static void close_objects(struct intel_memory_region *mem, if (i915_gem_object_has_pinned_pages(obj)) i915_gem_object_unpin_pages(obj); /* No polluting the memory region between tests */ - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); + __i915_gem_object_put_pages(obj); list_del(&obj->st_link); i915_gem_object_put(obj); } @@ -122,7 +122,7 @@ igt_object_create(struct intel_memory_region *mem, static void igt_object_release(struct drm_i915_gem_object *obj) { i915_gem_object_unpin_pages(obj); - __i915_gem_object_put_pages(obj, I915_MM_NORMAL); + __i915_gem_object_put_pages(obj); list_del(&obj->st_link); i915_gem_object_put(obj); }