* [PATCH 0/2] locking/lockdep, drm: apply new lockdep assert in drm_auth.c @ 2021-07-30 4:15 Desmond Cheong Zhi Xi 2021-07-30 4:15 ` [PATCH 1/2] locking/lockdep: Provide lockdep_assert{,_once}() helpers Desmond Cheong Zhi Xi ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Desmond Cheong Zhi Xi @ 2021-07-30 4:15 UTC (permalink / raw) To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, peterz, mingo, will, longman, boqun.feng Cc: Desmond Cheong Zhi Xi, dri-devel, linux-kernel, skhan, gregkh, linux-kernel-mentees 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.com/ [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(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] locking/lockdep: Provide lockdep_assert{,_once}() helpers 2021-07-30 4:15 [PATCH 0/2] locking/lockdep, drm: apply new lockdep assert in drm_auth.c Desmond Cheong Zhi Xi @ 2021-07-30 4:15 ` Desmond Cheong Zhi Xi 2021-07-30 4:15 ` [PATCH 2/2] drm: add lockdep assert to drm_is_current_master_locked Desmond Cheong Zhi Xi 2021-07-30 17:37 ` [PATCH 0/2] locking/lockdep, drm: apply new lockdep assert in drm_auth.c Waiman Long 2 siblings, 0 replies; 7+ messages in thread From: Desmond Cheong Zhi Xi @ 2021-07-30 4:15 UTC (permalink / raw) To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, peterz, mingo, will, longman, boqun.feng Cc: Desmond Cheong Zhi Xi, dri-devel, linux-kernel, skhan, gregkh, linux-kernel-mentees 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) -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] drm: add lockdep assert to drm_is_current_master_locked 2021-07-30 4:15 [PATCH 0/2] locking/lockdep, drm: apply new lockdep assert in drm_auth.c Desmond Cheong Zhi Xi 2021-07-30 4:15 ` [PATCH 1/2] locking/lockdep: Provide lockdep_assert{,_once}() helpers Desmond Cheong Zhi Xi @ 2021-07-30 4:15 ` Desmond Cheong Zhi Xi 2021-07-30 6:08 ` Boqun Feng 2021-07-30 17:37 ` [PATCH 0/2] locking/lockdep, drm: apply new lockdep assert in drm_auth.c Waiman Long 2 siblings, 1 reply; 7+ messages in thread From: Desmond Cheong Zhi Xi @ 2021-07-30 4:15 UTC (permalink / raw) To: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, peterz, mingo, will, longman, boqun.feng Cc: Desmond Cheong Zhi Xi, dri-devel, linux-kernel, skhan, gregkh, linux-kernel-mentees 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; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm: add lockdep assert to drm_is_current_master_locked 2021-07-30 4:15 ` [PATCH 2/2] drm: add lockdep assert to drm_is_current_master_locked Desmond Cheong Zhi Xi @ 2021-07-30 6:08 ` Boqun Feng 2021-07-30 8:06 ` Desmond Cheong Zhi Xi 0 siblings, 1 reply; 7+ messages in thread From: Boqun Feng @ 2021-07-30 6:08 UTC (permalink / raw) To: Desmond Cheong Zhi Xi Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, peterz, mingo, will, longman, dri-devel, linux-kernel, skhan, gregkh, linux-kernel-mentees 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm: add lockdep assert to drm_is_current_master_locked 2021-07-30 6:08 ` Boqun Feng @ 2021-07-30 8:06 ` Desmond Cheong Zhi Xi 2021-07-30 9:48 ` Boqun Feng 0 siblings, 1 reply; 7+ messages in thread From: Desmond Cheong Zhi Xi @ 2021-07-30 8:06 UTC (permalink / raw) To: Boqun Feng Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, peterz, mingo, will, longman, dri-devel, linux-kernel, skhan, gregkh, linux-kernel-mentees 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 >> ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm: add lockdep assert to drm_is_current_master_locked 2021-07-30 8:06 ` Desmond Cheong Zhi Xi @ 2021-07-30 9:48 ` Boqun Feng 0 siblings, 0 replies; 7+ messages in thread From: Boqun Feng @ 2021-07-30 9:48 UTC (permalink / raw) To: Desmond Cheong Zhi Xi Cc: maarten.lankhorst, mripard, tzimmermann, airlied, daniel, peterz, mingo, will, longman, dri-devel, linux-kernel, skhan, gregkh, linux-kernel-mentees 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 > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] locking/lockdep, drm: apply new lockdep assert in drm_auth.c 2021-07-30 4:15 [PATCH 0/2] locking/lockdep, drm: apply new lockdep assert in drm_auth.c Desmond Cheong Zhi Xi 2021-07-30 4:15 ` [PATCH 1/2] locking/lockdep: Provide lockdep_assert{,_once}() helpers Desmond Cheong Zhi Xi 2021-07-30 4:15 ` [PATCH 2/2] drm: add lockdep assert to drm_is_current_master_locked Desmond Cheong Zhi Xi @ 2021-07-30 17:37 ` Waiman Long 2 siblings, 0 replies; 7+ messages in thread From: Waiman Long @ 2021-07-30 17:37 UTC (permalink / raw) To: Desmond Cheong Zhi Xi, maarten.lankhorst, mripard, tzimmermann, airlied, daniel, peterz, mingo, will, boqun.feng Cc: dri-devel, linux-kernel, skhan, gregkh, linux-kernel-mentees 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.com/ [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! ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-07-30 17:37 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-30 4:15 [PATCH 0/2] locking/lockdep, drm: apply new lockdep assert in drm_auth.c Desmond Cheong Zhi Xi 2021-07-30 4:15 ` [PATCH 1/2] locking/lockdep: Provide lockdep_assert{,_once}() helpers Desmond Cheong Zhi Xi 2021-07-30 4:15 ` [PATCH 2/2] drm: add lockdep assert to drm_is_current_master_locked Desmond Cheong Zhi Xi 2021-07-30 6:08 ` Boqun Feng 2021-07-30 8:06 ` Desmond Cheong Zhi Xi 2021-07-30 9:48 ` Boqun Feng 2021-07-30 17:37 ` [PATCH 0/2] locking/lockdep, drm: apply new lockdep assert in drm_auth.c Waiman Long
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).