| Message ID | 20210722092929.244629-2-desmondcheongzx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm, drm/vmwgfx: fixes and updates related to drm_master | expand |
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.vetter@ffwll.ch> > > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@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. > The implementation is not hard but I don't understand the usage, for example, if we have a global variable x, and two locks L1 and L2, and the function void do_something_to_x(void) { lockdep_assert_held_either(L1, L2); x++; } and two call sites: void f(void) { lock(L1); do_something_to_x(); unlock(L1); } void g(void) { lock(L2); do_something_to_x(); unlock(L2); } , wouldn't it be racy if f() and g() called by two threads at the same time? Usually I would expect there exists a third synchronazition mechanism (say M), which synchronizes the calls to f() and g(), and we put M in the lockdep_assert_held() check inside do_something_to_x() like: void do_something_to_x(void) { lockdep_assert_held_once(M); x++; } But of course, M may not be a lock, so we cannot put the assert there. My cscope failed to find ->master_lookup_lock in -rc2 and seems it's not introduced in the patchset either, could you point me the branch this patchset is based on, so that I could understand this better, and maybe come up with a solution? Thanks ;-) Regards, Boqun > Adding lockdep folks, maybe they have ideas. > > On the patch: > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; > > } > > > > @@ -82,9 +83,9 @@ bool drm_is_current_master(struct drm_file *fpriv) > > { > > bool ret; > > > > - mutex_lock(&fpriv->minor->dev->master_mutex); > > + spin_lock(&fpriv->master_lookup_lock); > > ret = drm_is_current_master_locked(fpriv); > > - mutex_unlock(&fpriv->minor->dev->master_mutex); > > + spin_unlock(&fpriv->master_lookup_lock); > > > > return ret; > > } > > -- > > 2.25.1 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Thu, Jul 22, 2021 at 6:00 PM Boqun Feng <boqun.feng@gmail.com> 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.vetter@ffwll.ch> > > > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@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. > > > > The implementation is not hard but I don't understand the usage, for > example, if we have a global variable x, and two locks L1 and L2, and > the function > > void do_something_to_x(void) > { > lockdep_assert_held_either(L1, L2); > x++; > } > > and two call sites: > > void f(void) > { > lock(L1); > do_something_to_x(); > unlock(L1); > } > > void g(void) > { > lock(L2); > do_something_to_x(); > unlock(L2); > } > > , wouldn't it be racy if f() and g() called by two threads at the same > time? Usually I would expect there exists a third synchronazition > mechanism (say M), which synchronizes the calls to f() and g(), and we > put M in the lockdep_assert_held() check inside do_something_to_x() > like: > > void do_something_to_x(void) > { > lockdep_assert_held_once(M); > x++; > } > > But of course, M may not be a lock, so we cannot put the assert there. > > My cscope failed to find ->master_lookup_lock in -rc2 and seems it's not > introduced in the patchset either, could you point me the branch this > patchset is based on, so that I could understand this better, and maybe > come up with a solution? Thanks ;-) The use case is essentially 2 nesting locks, and only the innermost is used to update a field. So when you only read this field, it's safe if either of these two locks are held. Essentially this is a read/write lock type of thing, except for various reasons the two locks might not be of the same type (like here where the write lock is a mutex, but the read lock is a spinlock). It's a bit like the rcu_derefence macro where it's ok to either be in a rcu_read_lock() section, or holding the relevant lock that's used to update the value. We do _not_ have two different locks that allow writing to the same X. Does that make it clearer what's the use-case here? In an example: void * interesting_pointer. do_update_interesting_pointer() { mutex_lock(A); /* do more stuff to prepare things */ spin_lock(B); interesting_pointer = new_value; spin_unlock(B); mutex_unlock(A); } read_interesting_thing_locked() { lockdep_assert_held_either(A, B); return interesting_pointer->thing; } read_interesting_thing() { int thing; spin_lock(B); thing = interesting_pointer->thing; spin_unlock(B); return B; } spinlock might also be irqsafe here if this can be called from irq context. Cheers, Daniel > Regards, > Boqun > > > Adding lockdep folks, maybe they have ideas. > > > > On the patch: > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; > > > } > > > > > > @@ -82,9 +83,9 @@ bool drm_is_current_master(struct drm_file *fpriv) > > > { > > > bool ret; > > > > > > - mutex_lock(&fpriv->minor->dev->master_mutex); > > > + spin_lock(&fpriv->master_lookup_lock); > > > ret = drm_is_current_master_locked(fpriv); > > > - mutex_unlock(&fpriv->minor->dev->master_mutex); > > > + spin_unlock(&fpriv->master_lookup_lock); > > > > > > return ret; > > > } > > > -- > > > 2.25.1 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch
On Thu, Jul 22, 2021 at 09:02:41PM +0200, Daniel Vetter wrote: > On Thu, Jul 22, 2021 at 6:00 PM Boqun Feng <boqun.feng@gmail.com> 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.vetter@ffwll.ch> > > > > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@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. > > > > > > > The implementation is not hard but I don't understand the usage, for > > example, if we have a global variable x, and two locks L1 and L2, and > > the function > > > > void do_something_to_x(void) > > { > > lockdep_assert_held_either(L1, L2); > > x++; > > } > > > > and two call sites: > > > > void f(void) > > { > > lock(L1); > > do_something_to_x(); > > unlock(L1); > > } > > > > void g(void) > > { > > lock(L2); > > do_something_to_x(); > > unlock(L2); > > } > > > > , wouldn't it be racy if f() and g() called by two threads at the same > > time? Usually I would expect there exists a third synchronazition > > mechanism (say M), which synchronizes the calls to f() and g(), and we > > put M in the lockdep_assert_held() check inside do_something_to_x() > > like: > > > > void do_something_to_x(void) > > { > > lockdep_assert_held_once(M); > > x++; > > } > > > > But of course, M may not be a lock, so we cannot put the assert there. > > > > My cscope failed to find ->master_lookup_lock in -rc2 and seems it's not > > introduced in the patchset either, could you point me the branch this > > patchset is based on, so that I could understand this better, and maybe > > come up with a solution? Thanks ;-) > > The use case is essentially 2 nesting locks, and only the innermost is > used to update a field. So when you only read this field, it's safe if > either of these two locks are held. Essentially this is a read/write lock > type of thing, except for various reasons the two locks might not be of > the same type (like here where the write lock is a mutex, but the read > lock is a spinlock). > > It's a bit like the rcu_derefence macro where it's ok to either be in a > rcu_read_lock() section, or holding the relevant lock that's used to > update the value. We do _not_ have two different locks that allow writing > to the same X. > > Does that make it clearer what's the use-case here? > > In an example: > > void * interesting_pointer. > > do_update_interesting_pointer() > { > mutex_lock(A); > /* do more stuff to prepare things */ > spin_lock(B); > interesting_pointer = new_value; > spin_unlock(B); > mutex_unlock(A); > } > > read_interesting_thing_locked() > { > lockdep_assert_held_either(A, B); > > return interesting_pointer->thing; > } > > read_interesting_thing() > { > int thing; > spin_lock(B); > thing = interesting_pointer->thing; > spin_unlock(B); > > return B; > } > > spinlock might also be irqsafe here if this can be called from irq > context. > Make sense, so we'd better also provide lockdep_assert_held_both(), I think, to use it at the update side, something as below: /* * lockdep_assert_held_{both,either}(). * * Sometimes users can use a combination of two locks to * implement a rwlock-like lock, for example, say we have * locks L1 and L2, and we only allow updates when two locks * both held like: * * update() * { * lockdep_assert_held_both(L1, L2); * x++; // update x * } * * while for read-only accesses, either lock suffices (since * holding either lock means others cannot hold both, so readers * serialized with the updaters): * * read() * { * lockdep_assert_held_either(L1, L2); * r = x; // read x * } */ #define lockdep_assert_held_both(l1, l2) do { \ WARN_ON_ONCE(debug_locks && \ (!lockdep_is_held(l1) || \ !lockdep_is_held(l2))); \ } while (0) #define lockdep_assert_held_either(l1, l2) do { \ WARN_ON_ONCE(debug_locks && \ (!lockdep_is_held(l1) && \ !lockdep_is_held(l2))); \ } while (0) Still need sometime to think through this (e.g. on whether this it the best implementation). Regards, Boqun > Cheers, Daniel > > > Regards, > > Boqun > > > > > Adding lockdep folks, maybe they have ideas. > > > > > > On the patch: > > > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > > > return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; > > > > } > > > > > > > > @@ -82,9 +83,9 @@ bool drm_is_current_master(struct drm_file *fpriv) > > > > { > > > > bool ret; > > > > > > > > - mutex_lock(&fpriv->minor->dev->master_mutex); > > > > + spin_lock(&fpriv->master_lookup_lock); > > > > ret = drm_is_current_master_locked(fpriv); > > > > - mutex_unlock(&fpriv->minor->dev->master_mutex); > > > > + spin_unlock(&fpriv->master_lookup_lock); > > > > > > > > return ret; > > > > } > > > > -- > > > > 2.25.1 > > > > > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
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.vetter@ffwll.ch> > > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@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 ? --- 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) <peterz@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)
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.vetter@ffwll.ch> > > > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@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 > > --- > 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) <peterz@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) >
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.vetter@ffwll.ch> >>>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@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) <peterz@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) >> >
On Thu, Jul 29, 2021 at 10:32:13PM +0800, Desmond Cheong Zhi Xi wrote: > 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/ Bah, I knew I should've compile tested it :-), Thanks!
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. + */ return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; } @@ -82,9 +83,9 @@ bool drm_is_current_master(struct drm_file *fpriv) { bool ret; - mutex_lock(&fpriv->minor->dev->master_mutex); + spin_lock(&fpriv->master_lookup_lock); ret = drm_is_current_master_locked(fpriv); - mutex_unlock(&fpriv->minor->dev->master_mutex); + spin_unlock(&fpriv->master_lookup_lock); return ret; }
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.vetter@ffwll.ch> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> --- drivers/gpu/drm/drm_auth.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)