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