| Message ID | 20220516173930.159535-1-bh1scw@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-cgroup: Remove unnecessary rcu_read_lock/unlock() | expand |
On Tue, May 17, 2022 at 01:39:30AM +0800, bh1scw@gmail.com wrote: > From: Fanjun Kong <bh1scw@gmail.com> > > spin_lock_irq/spin_unlock_irq contains preempt_disable/enable(). > Which can serve as RCU read-side critical region, so remove > rcu_read_lock/unlock(). > > Signed-off-by: Fanjun Kong <bh1scw@gmail.com> Acked-by: Tejun Heo <tj@kernel.org> Thanks.
On Tue, May 17, 2022 at 1:39 AM <bh1scw@gmail.com> wrote: > > From: Fanjun Kong <bh1scw@gmail.com> > > spin_lock_irq/spin_unlock_irq contains preempt_disable/enable(). > Which can serve as RCU read-side critical region, so remove > rcu_read_lock/unlock(). > > Signed-off-by: Fanjun Kong <bh1scw@gmail.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com> Thanks.
On Tue, 17 May 2022 01:39:30 +0800, bh1scw@gmail.com wrote: > From: Fanjun Kong <bh1scw@gmail.com> > > spin_lock_irq/spin_unlock_irq contains preempt_disable/enable(). > Which can serve as RCU read-side critical region, so remove > rcu_read_lock/unlock(). > > > [...] Applied, thanks! [1/1] blk-cgroup: Remove unnecessary rcu_read_lock/unlock() commit: 77c570a1ea85ba4ab135c61a028420a6e9fe77f3 Best regards,
On 16.05.2022 19:39, bh1scw@gmail.com wrote: > From: Fanjun Kong <bh1scw@gmail.com> > > spin_lock_irq/spin_unlock_irq contains preempt_disable/enable(). > Which can serve as RCU read-side critical region, so remove > rcu_read_lock/unlock(). > > Signed-off-by: Fanjun Kong <bh1scw@gmail.com> This patch landed in today's linux next-20220518 as commit 77c570a1ea85 ("blk-cgroup: Remove unnecessary rcu_read_lock/unlock()"). Unfortunately it triggers the following warning on ARM64 based Raspberry Pi 4B board: ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at block/blk-cgroup.c:301 blkg_create+0x398/0x4e0 Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.18.0-rc3+ #5080 Hardware name: Raspberry Pi 4 Model B (DT) pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : blkg_create+0x398/0x4e0 ... Call trace: blkg_create+0x398/0x4e0 blkcg_init_queue+0x74/0x204 __alloc_disk_node+0xf8/0x1f0 __blk_alloc_disk+0x38/0x140 brd_alloc.part.0+0xf8/0x220 brd_init+0xe8/0x164 do_one_initcall+0x74/0x400 kernel_init_freeable+0x2f4/0x37c kernel_init+0x28/0x130 ret_from_fork+0x10/0x20 irq event stamp: 218372 hardirqs last enabled at (218371): [<ffff80000914b99c>] _raw_spin_unlock_irqrestore+0x98/0x9c hardirqs last disabled at (218372): [<ffff80000914bcbc>] _raw_spin_lock_irq+0xac/0xb0 softirqs last enabled at (216732): [<ffff800008010470>] _stext+0x470/0x5e8 softirqs last disabled at (216723): [<ffff8000080a0ec4>] __irq_exit_rcu+0x180/0x1ac ---[ end trace 0000000000000000 ]--- If this is a false positive, then the check in the code needs to be adjusted. > --- > block/blk-cgroup.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c > index a91f8ae18b49..7bdc16a36560 100644 > --- a/block/blk-cgroup.c > +++ b/block/blk-cgroup.c > @@ -1283,14 +1283,13 @@ int blkcg_init_queue(struct request_queue *q) > preloaded = !radix_tree_preload(GFP_KERNEL); > > /* Make sure the root blkg exists. */ > - rcu_read_lock(); > + /* spin_lock_irq can serve as RCU read-side critical section. */ > spin_lock_irq(&q->queue_lock); > blkg = blkg_create(&blkcg_root, q, new_blkg); > if (IS_ERR(blkg)) > goto err_unlock; > q->root_blkg = blkg; > spin_unlock_irq(&q->queue_lock); > - rcu_read_unlock(); > > if (preloaded) > radix_tree_preload_end(); > @@ -1316,7 +1315,6 @@ int blkcg_init_queue(struct request_queue *q) > return ret; > err_unlock: > spin_unlock_irq(&q->queue_lock); > - rcu_read_unlock(); > if (preloaded) > radix_tree_preload_end(); > return PTR_ERR(blkg); Best regards
On 5/18/22 1:28 PM, Marek Szyprowski wrote: > On 16.05.2022 19:39, bh1scw@gmail.com wrote: >> From: Fanjun Kong <bh1scw@gmail.com> >> >> spin_lock_irq/spin_unlock_irq contains preempt_disable/enable(). >> Which can serve as RCU read-side critical region, so remove >> rcu_read_lock/unlock(). >> >> Signed-off-by: Fanjun Kong <bh1scw@gmail.com> > > This patch landed in today's linux next-20220518 as commit 77c570a1ea85 > ("blk-cgroup: Remove unnecessary rcu_read_lock/unlock()"). > > Unfortunately it triggers the following warning on ARM64 based Raspberry > Pi 4B board:> > ------------[ cut here ]------------ > WARNING: CPU: 0 PID: 1 at block/blk-cgroup.c:301 blkg_create+0x398/0x4e0 Should this use rcu_read_lock_any_held() rather than rcu_read_lock_held()?
On 5/18/22 4:29 PM, Jens Axboe wrote: > On 5/18/22 1:28 PM, Marek Szyprowski wrote: >> On 16.05.2022 19:39, bh1scw@gmail.com wrote: >>> From: Fanjun Kong <bh1scw@gmail.com> >>> >>> spin_lock_irq/spin_unlock_irq contains preempt_disable/enable(). >>> Which can serve as RCU read-side critical region, so remove >>> rcu_read_lock/unlock(). >>> >>> Signed-off-by: Fanjun Kong <bh1scw@gmail.com> >> >> This patch landed in today's linux next-20220518 as commit 77c570a1ea85 >> ("blk-cgroup: Remove unnecessary rcu_read_lock/unlock()"). >> >> Unfortunately it triggers the following warning on ARM64 based Raspberry >> Pi 4B board:> >> ------------[ cut here ]------------ >> WARNING: CPU: 0 PID: 1 at block/blk-cgroup.c:301 blkg_create+0x398/0x4e0 > > Should this use rcu_read_lock_any_held() rather than > rcu_read_lock_held()? I think the better alternative is just to delete the WARN_ON(), we have a: lockdep_assert_held(&q->queue_lock); right after it. Since the queue_lock is IRQ disabling, having two checks serves no purpose. I'll kill the line.
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c index a91f8ae18b49..7bdc16a36560 100644 --- a/block/blk-cgroup.c +++ b/block/blk-cgroup.c @@ -1283,14 +1283,13 @@ int blkcg_init_queue(struct request_queue *q) preloaded = !radix_tree_preload(GFP_KERNEL); /* Make sure the root blkg exists. */ - rcu_read_lock(); + /* spin_lock_irq can serve as RCU read-side critical section. */ spin_lock_irq(&q->queue_lock); blkg = blkg_create(&blkcg_root, q, new_blkg); if (IS_ERR(blkg)) goto err_unlock; q->root_blkg = blkg; spin_unlock_irq(&q->queue_lock); - rcu_read_unlock(); if (preloaded) radix_tree_preload_end(); @@ -1316,7 +1315,6 @@ int blkcg_init_queue(struct request_queue *q) return ret; err_unlock: spin_unlock_irq(&q->queue_lock); - rcu_read_unlock(); if (preloaded) radix_tree_preload_end(); return PTR_ERR(blkg);