Re: [Intel-gfx] [PATCH 1/3] drm: use the lookup lock in drm_is_current_master

[Desmond Cheong Zhi Xi]

On 29/7/21 3:00 pm, Daniel Vetter wrote:

On Tue, Jul 27, 2021 at 04:37:22PM +0200, Peter Zijlstra wrote:

On Thu, Jul 22, 2021 at 12:38:10PM +0200, Daniel Vetter wrote:

On Thu, Jul 22, 2021 at 05:29:27PM +0800, Desmond Cheong Zhi Xi wrote:

Inside drm_is_current_master, using the outer drm_device.master_mutex
to protect reads of drm_file.master makes the function prone to creating
lock hierarchy inversions. Instead, we can use the
drm_file.master_lookup_lock that sits at the bottom of the lock
hierarchy.

Reported-by: Daniel Vetter <daniel.vet...@ffwll.ch>
Signed-off-by: Desmond Cheong Zhi Xi <desmondcheon...@gmail.com>
---
  drivers/gpu/drm/drm_auth.c | 9 +++++----
  1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index f00354bec3fb..9c24b8cc8e36 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -63,8 +63,9 @@

static bool drm_is_current_master_locked(struct drm_file *fpriv)

  {
-       lockdep_assert_held_once(&fpriv->minor->dev->master_mutex);
-
+       /* Either drm_device.master_mutex or drm_file.master_lookup_lock
+        * should be held here.
+        */

Disappointing that lockdep can't check or conditions for us, a
lockdep_assert_held_either would be really neat in some cases.

Adding lockdep folks, maybe they have ideas.

#ifdef CONFIG_LOCKDEP
        WARN_ON_ONCE(debug_locks && !(lockdep_is_held(&drm_device.master_mutex) 
||
                                      
lockdep_is_held(&drm_file.master_lookup_lock)));
#endif

doesn't exactly roll off the tongue, but should do as you want I
suppose.

Would something like:

#define lockdep_assert(cond)    WARN_ON_ONCE(debug_locks && !(cond))

Such that we can write:

        lockdep_assert(lockdep_is_held(&drm_device.master_mutex) ||
                       lockdep_is_held(&drm_file.master_lookup_lock));

make it better ?

Yeah I think that's pretty tidy and flexible.

Desmond, can you pls give this a shot with Peter's patch below?
-Daniel

Sounds good, will do. Thanks for the patch, Peter.

Just going to make a small edit:
s/LOCK_STAT_NOT_HELD/LOCK_STATE_NOT_HELD/

Best wishes,
Desmond

---
Subject: locking/lockdep: Provide lockdep_assert{,_once}() helpers

Extract lockdep_assert{,_once}() helpers to more easily write composite
assertions like, for example:

        lockdep_assert(lockdep_is_held(&drm_device.master_mutex) ||
                       lockdep_is_held(&drm_file.master_lookup_lock));

Signed-off-by: Peter Zijlstra (Intel) <pet...@infradead.org>
---
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 5cf387813754..0da67341c1fb 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -306,31 +306,29 @@ extern void lock_unpin_lock(struct lockdep_map *lock, 
struct pin_cookie);

#define lockdep_depth(tsk) (debug_locks ? (tsk)->lockdep_depth : 0) -#define lockdep_assert_held(l) do { \

-               WARN_ON(debug_locks &&                                  \
-                       lockdep_is_held(l) == LOCK_STATE_NOT_HELD);     \
-       } while (0)
+#define lockdep_assert(cond)           \
+       do { WARN_ON(debug_locks && !(cond)); } while (0)

-#define lockdep_assert_not_held(l) do { \

-               WARN_ON(debug_locks &&                                  \
-                       lockdep_is_held(l) == LOCK_STATE_HELD);         \
-       } while (0)
+#define lockdep_assert_once(cond)      \
+       do { WARN_ON_ONCE(debug_locks && !(cond)); } while (0)

-#define lockdep_assert_held_write(l) do { \

-               WARN_ON(debug_locks && !lockdep_is_held_type(l, 0));    \
-       } while (0)
+#define lockdep_assert_held(l)         \
+       lockdep_assert(lockdep_is_held(l) != LOCK_STAT_NOT_HELD)

-#define lockdep_assert_held_read(l) do { \

-               WARN_ON(debug_locks && !lockdep_is_held_type(l, 1));    \
-       } while (0)
+#define lockdep_assert_not_held(l)     \
+       lockdep_assert(lockdep_is_held(l) != LOCK_STATE_HELD)

-#define lockdep_assert_held_once(l) do { \

-               WARN_ON_ONCE(debug_locks && !lockdep_is_held(l));       \
-       } while (0)
+#define lockdep_assert_held_write(l)   \
+       lockdep_assert(lockdep_is_held_type(l, 0))

-#define lockdep_assert_none_held_once() do { \

-               WARN_ON_ONCE(debug_locks && current->lockdep_depth); \
-       } while (0)
+#define lockdep_assert_held_read(l)    \
+       lockdep_assert(lockdep_is_held_type(l, 1))
+
+#define lockdep_assert_held_once(l)            \
+       lockdep_assert_once(lockdep_is_held(l) != LOCK_STAT_NOT_HELD)
+
+#define lockdep_assert_none_held_once()                \
+       lockdep_assert_once(!current->lockdep_depth)

#define lockdep_recursing(tsk) ((tsk)->lockdep_recursion) @@ -407,6 +405,9 @@ extern int lock_is_held(const void *);

  extern int lockdep_is_held(const void *);
  #define lockdep_is_held_type(l, r)            (1)

+#define lockdep_assert(c) do { } while (0)

+#define lockdep_assert_once(c)                 do { } while (0)
+
  #define lockdep_assert_held(l)                        do { (void)(l); } while 
(0)
  #define lockdep_assert_not_held(l)            do { (void)(l); } while (0)
  #define lockdep_assert_held_write(l)          do { (void)(l); } while (0)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx