Hi,
Following a discussion on the patch ("drm: use the lookup lock in drm_is_current_master") [1], Peter Zijlstra proposed new lockdep_assert helpers to make it convenient to compose lockdep checks together.
This series includes the patch that introduces the new lockdep helpers, then utilizes these helpers in drm_is_current_master_locked in the following patch.
Link: https://lore.kernel.org/lkml/20210722092929.244629-2-desmondcheongzx@gmail.c... [1]
Best wishes, Desmond
Desmond Cheong Zhi Xi (1): drm: add lockdep assert to drm_is_current_master_locked
Peter Zijlstra (1): locking/lockdep: Provide lockdep_assert{,_once}() helpers
drivers/gpu/drm/drm_auth.c | 6 +++--- include/linux/lockdep.h | 41 +++++++++++++++++++------------------- 2 files changed, 24 insertions(+), 23 deletions(-)
From: Peter Zijlstra peterz@infradead.org
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 Signed-off-by: Desmond Cheong Zhi Xi desmondcheongzx@gmail.com --- include/linux/lockdep.h | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h index 5cf387813754..9fe165beb0f9 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_STATE_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_STATE_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)
In drm_is_current_master_locked, accessing drm_file.master should be protected by either drm_file.master_lookup_lock or drm_device.master_mutex. This was previously awkward to assert with lockdep.
Following patch ("locking/lockdep: Provide lockdep_assert{,_once}() helpers"), this assertion is now convenient so we add it in.
Signed-off-by: Desmond Cheong Zhi Xi desmondcheongzx@gmail.com --- drivers/gpu/drm/drm_auth.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index 9c24b8cc8e36..6f4d7ff23c80 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -63,9 +63,9 @@
static bool drm_is_current_master_locked(struct drm_file *fpriv) { - /* Either drm_device.master_mutex or drm_file.master_lookup_lock - * should be held here. - */ + lockdep_assert_once(lockdep_is_held(&fpriv->master_lookup_lock) || + lockdep_is_held(&fpriv->minor->dev->master_mutex)); + return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; }
On Fri, Jul 30, 2021 at 12:15:15PM +0800, Desmond Cheong Zhi Xi wrote:
In drm_is_current_master_locked, accessing drm_file.master should be protected by either drm_file.master_lookup_lock or drm_device.master_mutex. This was previously awkward to assert with lockdep.
Following patch ("locking/lockdep: Provide lockdep_assert{,_once}() helpers"), this assertion is now convenient so we add it in.
Signed-off-by: Desmond Cheong Zhi Xi desmondcheongzx@gmail.com
drivers/gpu/drm/drm_auth.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index 9c24b8cc8e36..6f4d7ff23c80 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -63,9 +63,9 @@
static bool drm_is_current_master_locked(struct drm_file *fpriv) {
- /* Either drm_device.master_mutex or drm_file.master_lookup_lock
* should be held here.*/
- lockdep_assert_once(lockdep_is_held(&fpriv->master_lookup_lock) ||
lockdep_is_held(&fpriv->minor->dev->master_mutex));
I think it's better to also add the lockdep_assert() of & (i.e. both held) in the updater side, and have comments pointing to each other.
Is it convenient to do in this patchset? If the updater side doesn't need to put the lockdep_assert() (maybe the lock acquire code and the update code are in the same function), it's still better to add some comments like:
/* * To update drm_file.master, both drm_file.master_lookup_lock * and drm_device.master_mutex are needed, therefore holding * either of them is safe and enough for the read side. */
Just feel it's better to explain the lock design either in the lockdep_assert() or comments.
Regards, Boqun
return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; }
-- 2.25.1
On 30/7/21 2:08 pm, Boqun Feng wrote:
On Fri, Jul 30, 2021 at 12:15:15PM +0800, Desmond Cheong Zhi Xi wrote:
In drm_is_current_master_locked, accessing drm_file.master should be protected by either drm_file.master_lookup_lock or drm_device.master_mutex. This was previously awkward to assert with lockdep.
Following patch ("locking/lockdep: Provide lockdep_assert{,_once}() helpers"), this assertion is now convenient so we add it in.
Signed-off-by: Desmond Cheong Zhi Xi desmondcheongzx@gmail.com
drivers/gpu/drm/drm_auth.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index 9c24b8cc8e36..6f4d7ff23c80 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -63,9 +63,9 @@
static bool drm_is_current_master_locked(struct drm_file *fpriv) {
- /* Either drm_device.master_mutex or drm_file.master_lookup_lock
* should be held here.*/
- lockdep_assert_once(lockdep_is_held(&fpriv->master_lookup_lock) ||
lockdep_is_held(&fpriv->minor->dev->master_mutex));I think it's better to also add the lockdep_assert() of & (i.e. both held) in the updater side, and have comments pointing to each other.
Is it convenient to do in this patchset? If the updater side doesn't need to put the lockdep_assert() (maybe the lock acquire code and the update code are in the same function), it's still better to add some
Thanks for the feedback, Boqun.
Yeah, I think the updater side maybe doesn't need new lockdep_assert() because what currently happens is either
lockdep_assert_held_once(&dev->master_mutex); /* 6 lines of prep */ spin_lock(&fpriv->master_lookup_lock); fpriv->master = new_value; or mutex_lock(&dev->master_mutex); /* 3 lines of checks */ spin_lock(&file_priv->master_lookup_lock); file_priv->master = new_value;
comments like:
/* * To update drm_file.master, both drm_file.master_lookup_lock * and drm_device.master_mutex are needed, therefore holding * either of them is safe and enough for the read side. */
Just feel it's better to explain the lock design either in the lockdep_assert() or comments.
But clarifying the lock design in the documentation sounds like a really good idea.
Probably a good place for this would be in the kerneldoc where we also explain the lifetime rules and usage of the pointer outside drm_auth.c:
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index 726cfe0ff5f5..a3acb7ac3550 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -233,6 +233,10 @@ struct drm_file { * this only matches &drm_device.master if the master is the currently * active one. * + * To update @master, both &drm_device.master_mutex and + * @master_lookup_lock need to be held, therefore holding either of + * them is safe and enough for the read side. + * * When dereferencing this pointer, either hold struct * &drm_device.master_mutex for the duration of the pointer's use, or * use drm_file_get_master() if struct &drm_device.master_mutex is not
Best wishes, Desmond
Regards, Boqun
return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; }
-- 2.25.1
On Fri, Jul 30, 2021 at 04:06:44PM +0800, Desmond Cheong Zhi Xi wrote:
On 30/7/21 2:08 pm, Boqun Feng wrote:
On Fri, Jul 30, 2021 at 12:15:15PM +0800, Desmond Cheong Zhi Xi wrote:
In drm_is_current_master_locked, accessing drm_file.master should be protected by either drm_file.master_lookup_lock or drm_device.master_mutex. This was previously awkward to assert with lockdep.
Following patch ("locking/lockdep: Provide lockdep_assert{,_once}() helpers"), this assertion is now convenient so we add it in.
Signed-off-by: Desmond Cheong Zhi Xi desmondcheongzx@gmail.com
drivers/gpu/drm/drm_auth.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index 9c24b8cc8e36..6f4d7ff23c80 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -63,9 +63,9 @@ static bool drm_is_current_master_locked(struct drm_file *fpriv) {
- /* Either drm_device.master_mutex or drm_file.master_lookup_lock
* should be held here.*/
- lockdep_assert_once(lockdep_is_held(&fpriv->master_lookup_lock) ||
lockdep_is_held(&fpriv->minor->dev->master_mutex));I think it's better to also add the lockdep_assert() of & (i.e. both held) in the updater side, and have comments pointing to each other.
Is it convenient to do in this patchset? If the updater side doesn't need to put the lockdep_assert() (maybe the lock acquire code and the update code are in the same function), it's still better to add some
Thanks for the feedback, Boqun.
Yeah, I think the updater side maybe doesn't need new lockdep_assert() because what currently happens is either
lockdep_assert_held_once(&dev->master_mutex); /* 6 lines of prep */ spin_lock(&fpriv->master_lookup_lock); fpriv->master = new_value; or mutex_lock(&dev->master_mutex); /* 3 lines of checks */ spin_lock(&file_priv->master_lookup_lock); file_priv->master = new_value;
comments like:
/* * To update drm_file.master, both drm_file.master_lookup_lock * and drm_device.master_mutex are needed, therefore holding * either of them is safe and enough for the read side. */
Just feel it's better to explain the lock design either in the lockdep_assert() or comments.
But clarifying the lock design in the documentation sounds like a really good idea.
Probably a good place for this would be in the kerneldoc where we also explain the lifetime rules and usage of the pointer outside drm_auth.c:
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index 726cfe0ff5f5..a3acb7ac3550 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -233,6 +233,10 @@ struct drm_file { * this only matches &drm_device.master if the master is the currently * active one. *
* To update @master, both &drm_device.master_mutex and* @master_lookup_lock need to be held, therefore holding either of* them is safe and enough for the read side.*
- When dereferencing this pointer, either hold struct
- &drm_device.master_mutex for the duration of the pointer's use, or
- use drm_file_get_master() if struct &drm_device.master_mutex is not
Works for me ;-)
For the whole series, feel free to add:
Acked-by: Boqun Feng boqun.feng@gmail.com
Regards, Boqun
Best wishes, Desmond
Regards, Boqun
return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; } -- 2.25.1
On 7/30/21 12:15 AM, Desmond Cheong Zhi Xi wrote:
Hi,
Following a discussion on the patch ("drm: use the lookup lock in drm_is_current_master") [1], Peter Zijlstra proposed new lockdep_assert helpers to make it convenient to compose lockdep checks together.
This series includes the patch that introduces the new lockdep helpers, then utilizes these helpers in drm_is_current_master_locked in the following patch.
Link: https://lore.kernel.org/lkml/20210722092929.244629-2-desmondcheongzx@gmail.c... [1]
Best wishes, Desmond
Desmond Cheong Zhi Xi (1): drm: add lockdep assert to drm_is_current_master_locked
Peter Zijlstra (1): locking/lockdep: Provide lockdep_assert{,_once}() helpers
drivers/gpu/drm/drm_auth.c | 6 +++--- include/linux/lockdep.h | 41 +++++++++++++++++++------------------- 2 files changed, 24 insertions(+), 23 deletions(-)
This patch series looks good to me.
Acked-by: Waiman Long longman@redhat.com
Thanks!
dri-devel@lists.freedesktop.org
-
Boqun Feng -
Desmond Cheong Zhi Xi -
Waiman Long