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

[Chris Wilson]

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 at chris-wilson.co.uk>
> Cc: "Tang, CQ" <cq.tang at intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  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.

> 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.
-Chris