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