Re: [PATCH] drm/i915: Switch obj->mm.lock lockdep annotations on its head

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx]
> Sent: Friday, August 16, 2019 3:03 PM
> To: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Intel Graphics Development <intel-gfx@xxxxxxxxxxxxxxxxxxxxx>; Tang, CQ
> <cq.tang@xxxxxxxxx>; Ursulin, Tvrtko <tvrtko.ursulin@xxxxxxxxx>; Joonas
> Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>; Vetter, Daniel
> <daniel.vetter@xxxxxxxxx>
> Subject: Re: [PATCH] drm/i915: Switch obj->mm.lock lockdep annotations on
> its head
> 
> On Fri, Aug 16, 2019 at 8:46 PM Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> wrote:
> >
> > Quoting Daniel Vetter (2019-08-16 19:23:36)
> > > 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
> > >   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).
> > >
> > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > Cc: "Tang, CQ" <cq.tang@xxxxxxxxx>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/i915/gem/i915_gem_object.c       | 13 ++++++++++++-
> > >  drivers/gpu/drm/i915/gem/i915_gem_object.h       | 16
> +++++++++++++---
> > >  drivers/gpu/drm/i915/gem/i915_gem_object_types.h |  6 +++++-
> > >  drivers/gpu/drm/i915/gem/i915_gem_pages.c        |  9 ++++-----
> > >  drivers/gpu/drm/i915/gem/i915_gem_phys.c         |  2 +-
> > >  drivers/gpu/drm/i915/gem/i915_gem_shrinker.c     |  5 ++---
> > >  drivers/gpu/drm/i915/gem/i915_gem_userptr.c      |  4 ++--
> > >  drivers/gpu/drm/i915/gem/selftests/huge_pages.c  | 12 ++++++------
> > >  8 files changed, 45 insertions(+), 22 deletions(-)
> >
> > static inline int __must_check
> > i915_gem_object_pin_pages(struct drm_i915_gem_object *obj) {
> >         might_lock(&obj->mm.lock);
> >
> >         if (atomic_inc_not_zero(&obj->mm.pages_pin_count))
> >                 return 0;
> >
> >         return __i915_gem_object_get_pages(obj); }
> >
> > is now testing the wrong lock class.
> 
> Unfortunately there's no might_lock_nested.
> 
> But then, this is the best kind of wrong, because of the nesting we have:
> 
> obj->mm.lock#I915_MM_GET_PAGES -> fs_reclaim -> obj->mm.lock
> 
> So the might_lock we have actually checks for way more than just the "more
> correct" annotation. I think I'll just add the above as a comment and leave
> the code as-is. Thoughts?

I believe we should allow recursive call to i915_gem_object_pin_pages(),  if the object is already pinned, the next call just bump up the pin count and return. Otherwise, you only allow paired call:
                  I915_gem_object_pin_pages(obj);
	  I915_gem_object_unpin_pages(obj);

Sometimes we need do this:
	I915_gem_object_pin_pages(obj);
	.....
	I915_gem_object_pin_pages(obj);
	I915_gem_object_unpin_pages(obj);
	.....
	I915_gem_object_unpin_pages(obj);

The nested call is deep in the calling stack.  For example, we pin an object when doing put_pages(),  in put_pages() if we do swapping out, the blitter copying function will pin this object again, even though it is already pinned.

--CQ

> 
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > > b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > > index 3929c3a6b281..d01258b175f5 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"
> > > @@ -61,6 +63,15 @@ void i915_gem_object_init(struct
> > > drm_i915_gem_object *obj,  {
> > >         mutex_init(&obj->mm.lock);
> > >
> > > +       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);
> > > +       }
> >
> > This is very powerful and sells a lot of churn.
> 
> Yeah that was the idea here. Plus I hope it's the easier to understand the
> annotations and lock nesting rules for obj->mm.lock this way - I freaked out
> quite a bit about the current one until you convinced me (which took it's
> sweet time) that it's all fine. Maybe explicitly annotating get_pages and it's
> special rule will help others (I can't play guinea pig twice unfortunately, so we
> can't test that theory).
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux