| Message ID | 20231219012833.2129540-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-cgroup: fix rcu lockdep warning in blkg_lookup() | expand |
On Tue, Dec 19, 2023 at 9:28 AM Ming Lei <ming.lei@redhat.com> wrote: > > blkg_lookup() is called with either queue_lock or rcu read lock, so > use rcu_dereference_check(lockdep_is_held(&q->queue_lock)) for > retrieving 'blkg', which way models the check exactly for covering > queue lock or rcu read lock. > > Fix lockdep warning of "block/blk-cgroup.h:254 suspicious rcu_dereference_check() usage!" > from blkg_lookup(). > > Tested-by: Changhui Zhong <czhong@redhat.com> > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > block/blk-cgroup.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h > index fd482439afbc..b927a4a0ad03 100644 > --- a/block/blk-cgroup.h > +++ b/block/blk-cgroup.h > @@ -252,7 +252,8 @@ static inline struct blkcg_gq *blkg_lookup(struct blkcg *blkcg, > if (blkcg == &blkcg_root) > return q->root_blkg; > > - blkg = rcu_dereference(blkcg->blkg_hint); > + blkg = rcu_dereference_check(blkcg->blkg_hint, > + lockdep_is_held(&q->queue_lock)); > if (blkg && blkg->q == q) > return blkg; Hello, Ping... Thanks,
Hi, 在 2023/12/19 9:28, Ming Lei 写道: > blkg_lookup() is called with either queue_lock or rcu read lock, so > use rcu_dereference_check(lockdep_is_held(&q->queue_lock)) for > retrieving 'blkg', which way models the check exactly for covering > queue lock or rcu read lock. > > Fix lockdep warning of "block/blk-cgroup.h:254 suspicious rcu_dereference_check() usage!" > from blkg_lookup(). > > Tested-by: Changhui Zhong <czhong@redhat.com> > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > block/blk-cgroup.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h > index fd482439afbc..b927a4a0ad03 100644 > --- a/block/blk-cgroup.h > +++ b/block/blk-cgroup.h > @@ -252,7 +252,8 @@ static inline struct blkcg_gq *blkg_lookup(struct blkcg *blkcg, > if (blkcg == &blkcg_root) > return q->root_blkg; > > - blkg = rcu_dereference(blkcg->blkg_hint); > + blkg = rcu_dereference_check(blkcg->blkg_hint, > + lockdep_is_held(&q->queue_lock)); This patch itself is correct, and in fact this is a false positive warning. I noticed that commit 83462a6c971c ("blkcg: Drop unnecessary RCU read [un]locks from blkg_conf_prep/finish()") drop rcu_read_lock/unlock() because 'queue_lock' is held. This is correct, however you add this back for tg_conf_updated() later in commit 27b13e209ddc ("blk-throttle: fix lockdep warning of "cgroup_mutex or RCU read lock required!"") because rcu_read_lock_held() from blkg_lookup() is triggered. And this patch is again another use case cased by commit 83462a6c971c. I just wonder, with the respect of rcu implementation, is it possible to add preemptible() check directly in rcu_read_lock_held() to bypass all this kind of false positive warning? Thanks, Kuai > if (blkg && blkg->q == q) > return blkg; > >
On Tue, Jan 02, 2024 at 06:32:13PM +0800, Yu Kuai wrote: > Hi, > > 在 2023/12/19 9:28, Ming Lei 写道: > > blkg_lookup() is called with either queue_lock or rcu read lock, so > > use rcu_dereference_check(lockdep_is_held(&q->queue_lock)) for > > retrieving 'blkg', which way models the check exactly for covering > > queue lock or rcu read lock. > > > > Fix lockdep warning of "block/blk-cgroup.h:254 suspicious rcu_dereference_check() usage!" > > from blkg_lookup(). > > > > Tested-by: Changhui Zhong <czhong@redhat.com> > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > --- > > block/blk-cgroup.h | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h > > index fd482439afbc..b927a4a0ad03 100644 > > --- a/block/blk-cgroup.h > > +++ b/block/blk-cgroup.h > > @@ -252,7 +252,8 @@ static inline struct blkcg_gq *blkg_lookup(struct blkcg *blkcg, > > if (blkcg == &blkcg_root) > > return q->root_blkg; > > - blkg = rcu_dereference(blkcg->blkg_hint); > > + blkg = rcu_dereference_check(blkcg->blkg_hint, > > + lockdep_is_held(&q->queue_lock)); > > This patch itself is correct, and in fact this is a false positive > warning. Yeah, it is, but we always teach lockdep to not trigger warning, > > I noticed that commit 83462a6c971c ("blkcg: Drop unnecessary RCU read > [un]locks from blkg_conf_prep/finish()") drop rcu_read_lock/unlock() > because 'queue_lock' is held. This is correct, however you add this back > for tg_conf_updated() later in commit 27b13e209ddc ("blk-throttle: fix > lockdep warning of "cgroup_mutex or RCU read lock required!"") because > rcu_read_lock_held() from blkg_lookup() is triggered. And this patch is > again another use case cased by commit 83462a6c971c. We should add: Fixes: 83462a6c971c ("blkcg: Drop unnecessary RCU read [un]locks from blkg_conf_prep/finish()") > > I just wonder, with the respect of rcu implementation, is it possible to > add preemptible() check directly in rcu_read_lock_held() to bypass all > this kind of false positive warning? It isn't related with rcu_read_lock_held(), and the check is done in RCU_LOCKDEP_WARN(). rcu_dereference_check() does cover this situation, and no need to invent wheel for avoiding the warning. Thanks, Ming
在 2024/01/02 19:27, Ming Lei 写道: > On Tue, Jan 02, 2024 at 06:32:13PM +0800, Yu Kuai wrote: >> Hi, >> >> 在 2023/12/19 9:28, Ming Lei 写道: >>> blkg_lookup() is called with either queue_lock or rcu read lock, so >>> use rcu_dereference_check(lockdep_is_held(&q->queue_lock)) for >>> retrieving 'blkg', which way models the check exactly for covering >>> queue lock or rcu read lock. >>> >>> Fix lockdep warning of "block/blk-cgroup.h:254 suspicious rcu_dereference_check() usage!" >>> from blkg_lookup(). >>> >>> Tested-by: Changhui Zhong <czhong@redhat.com> >>> Signed-off-by: Ming Lei <ming.lei@redhat.com> >>> --- >>> block/blk-cgroup.h | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h >>> index fd482439afbc..b927a4a0ad03 100644 >>> --- a/block/blk-cgroup.h >>> +++ b/block/blk-cgroup.h >>> @@ -252,7 +252,8 @@ static inline struct blkcg_gq *blkg_lookup(struct blkcg *blkcg, >>> if (blkcg == &blkcg_root) >>> return q->root_blkg; >>> - blkg = rcu_dereference(blkcg->blkg_hint); >>> + blkg = rcu_dereference_check(blkcg->blkg_hint, >>> + lockdep_is_held(&q->queue_lock)); >> >> This patch itself is correct, and in fact this is a false positive >> warning. > > Yeah, it is, but we always teach lockdep to not trigger warning, > >> >> I noticed that commit 83462a6c971c ("blkcg: Drop unnecessary RCU read >> [un]locks from blkg_conf_prep/finish()") drop rcu_read_lock/unlock() >> because 'queue_lock' is held. This is correct, however you add this back >> for tg_conf_updated() later in commit 27b13e209ddc ("blk-throttle: fix >> lockdep warning of "cgroup_mutex or RCU read lock required!"") because >> rcu_read_lock_held() from blkg_lookup() is triggered. And this patch is >> again another use case cased by commit 83462a6c971c. > > We should add: > > Fixes: 83462a6c971c ("blkcg: Drop unnecessary RCU read [un]locks from blkg_conf_prep/finish()") With the above fix tag, Reviewed-by: Yu Kuai <yukuai3@huawei.com> > >> >> I just wonder, with the respect of rcu implementation, is it possible to >> add preemptible() check directly in rcu_read_lock_held() to bypass all >> this kind of false positive warning? > > It isn't related with rcu_read_lock_held(), and the check is done in > RCU_LOCKDEP_WARN(). rcu_dereference_check() does cover this situation, > and no need to invent wheel for avoiding the warning. > > Thanks, > Ming > > > . >
On Tue, Jan 02, 2024 at 03:38:10PM +0800, Ming Lei wrote: > On Tue, Dec 19, 2023 at 9:28 AM Ming Lei <ming.lei@redhat.com> wrote: > > > > blkg_lookup() is called with either queue_lock or rcu read lock, so > > use rcu_dereference_check(lockdep_is_held(&q->queue_lock)) for > > retrieving 'blkg', which way models the check exactly for covering > > queue lock or rcu read lock. > > > > Fix lockdep warning of "block/blk-cgroup.h:254 suspicious rcu_dereference_check() usage!" > > from blkg_lookup(). > > > > Tested-by: Changhui Zhong <czhong@redhat.com> > > Signed-off-by: Ming Lei <ming.lei@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> Thanks.
On Tue, 19 Dec 2023 09:28:33 +0800, Ming Lei wrote: > blkg_lookup() is called with either queue_lock or rcu read lock, so > use rcu_dereference_check(lockdep_is_held(&q->queue_lock)) for > retrieving 'blkg', which way models the check exactly for covering > queue lock or rcu read lock. > > Fix lockdep warning of "block/blk-cgroup.h:254 suspicious rcu_dereference_check() usage!" > from blkg_lookup(). > > [...] Applied, thanks! [1/1] blk-cgroup: fix rcu lockdep warning in blkg_lookup() commit: 393cd8ffd832f23eec3a105553eff622e8198918 Best regards,
diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h index fd482439afbc..b927a4a0ad03 100644 --- a/block/blk-cgroup.h +++ b/block/blk-cgroup.h @@ -252,7 +252,8 @@ static inline struct blkcg_gq *blkg_lookup(struct blkcg *blkcg, if (blkcg == &blkcg_root) return q->root_blkg; - blkg = rcu_dereference(blkcg->blkg_hint); + blkg = rcu_dereference_check(blkcg->blkg_hint, + lockdep_is_held(&q->queue_lock)); if (blkg && blkg->q == q) return blkg;