Hi,
On 03.12.2021 14:15, Ming Lei wrote:
> Remove hctx_lock and hctx_unlock, and add one helper of
> blk_mq_run_dispatch_ops() to run code block defined in dispatch_ops
> with rcu/srcu read held.
>
> Compared with hctx_lock()/hctx_unlock():
>
> 1) remove 2 branch to 1, so we just need to check
> (hctx->flags & BLK_MQ_F_BLOCKING) once when running one dispatch_ops
>
> 2) srcu_idx needn't to be touched in case of non-blocking
>
> 3) might_sleep_if() can be moved to the blocking branch
>
> Also put the added blk_mq_run_dispatch_ops() in private header, so that
> the following patch can use it out of blk-mq.c.
>
> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
This patch landed in linux next-20211206 as commit 2a904d00855f
("blk-mq: remove hctx_lock and hctx_unlock"). It introduces a following
'BUG' warning on my test systems (ARM/ARM64-based boards with rootfs on
SD card or eMMC):
BUG: sleeping function called from invalid context at block/blk-mq.c:2060
in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 249, name:
kworker/0:3H
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
4 locks held by kworker/0:3H/249:
#0: c1d782a8 ((wq_completion)mmc_complete){+.+.}-{0:0}, at:
process_one_work+0x21c/0x7ec
#1: c3b59f18 ((work_completion)(&mq->complete_work)){+.+.}-{0:0}, at:
process_one_work+0x21c/0x7ec
#2: c1d7858c (&mq->complete_lock){+.+.}-{3:3}, at:
mmc_blk_mq_complete_prev_req.part.3+0x2c/0x234
#3: c1f7a1b4 (&fq->mq_flush_lock){....}-{2:2}, at:
mq_flush_data_end_io+0x68/0x124
irq event stamp: 16306
hardirqs last enabled at (16305): [<c01da058>] ktime_get+0x178/0x19c
hardirqs last disabled at (16306): [<c0b7a090>]
_raw_spin_lock_irqsave+0x24/0x60
softirqs last enabled at (16300): [<c01016fc>] __do_softirq+0x4cc/0x5ec
softirqs last disabled at (16289): [<c012f95c>] do_softirq+0xb8/0xc4
Preemption disabled at:
[<00000000>] 0x0
CPU: 0 PID: 249 Comm: kworker/0:3H Not tainted
5.16.0-rc3-00080-g2a904d00855f #4254
Hardware name: Samsung Exynos (Flattened Device Tree)
Workqueue: mmc_complete mmc_blk_mq_complete_work
[<c01110d0>] (unwind_backtrace) from [<c010cab8>] (show_stack+0x10/0x14)
[<c010cab8>] (show_stack) from [<c0b6c728>] (dump_stack_lvl+0x58/0x70)
[<c0b6c728>] (dump_stack_lvl) from [<c01594d0>]
(__might_resched+0x248/0x298)
[<c01594d0>] (__might_resched) from [<c0512cec>]
(blk_mq_run_hw_queue+0xac/0x230)
[<c0512cec>] (blk_mq_run_hw_queue) from [<c050f660>]
(__blk_mq_free_request+0x84/0x90)
[<c050f660>] (__blk_mq_free_request) from [<c05086b8>]
(blk_flush_complete_seq+0x250/0x260)
[<c05086b8>] (blk_flush_complete_seq) from [<c0508b34>]
(mq_flush_data_end_io+0x80/0x124)
[<c0508b34>] (mq_flush_data_end_io) from [<c08a0c28>]
(mmc_blk_mq_post_req+0xc4/0xd8)
[<c08a0c28>] (mmc_blk_mq_post_req) from [<c08a0e68>]
(mmc_blk_mq_complete_prev_req.part.3+0x22c/0x234)
[<c08a0e68>] (mmc_blk_mq_complete_prev_req.part.3) from [<c0148980>]
(process_one_work+0x2c8/0x7ec)
[<c0148980>] (process_one_work) from [<c0148ef4>] (worker_thread+0x50/0x584)
[<c0148ef4>] (worker_thread) from [<c015118c>] (kthread+0x13c/0x19c)
[<c015118c>] (kthread) from [<c0100108>] (ret_from_fork+0x14/0x2c)
Exception stack(0xc3b59fb0 to 0xc3b59ff8)
9fa0: 00000000 00000000 00000000
00000000
9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
Please let me know if I can do something to help debugging this issue.
> ---
> block/blk-mq.c | 57 +++++++++-----------------------------------------
> block/blk-mq.h | 16 ++++++++++++++
> 2 files changed, 26 insertions(+), 47 deletions(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index fc4520e992b1..a9d69e1dea8b 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1071,26 +1071,6 @@ void blk_mq_complete_request(struct request *rq)
> }
> EXPORT_SYMBOL(blk_mq_complete_request);
>
> -static void hctx_unlock(struct blk_mq_hw_ctx *hctx, int srcu_idx)
> - __releases(hctx->srcu)
> -{
> - if (!(hctx->flags & BLK_MQ_F_BLOCKING))
> - rcu_read_unlock();
> - else
> - srcu_read_unlock(hctx->srcu, srcu_idx);
> -}
> -
> -static void hctx_lock(struct blk_mq_hw_ctx *hctx, int *srcu_idx)
> - __acquires(hctx->srcu)
> -{
> - if (!(hctx->flags & BLK_MQ_F_BLOCKING)) {
> - /* shut up gcc false positive */
> - *srcu_idx = 0;
> - rcu_read_lock();
> - } else
> - *srcu_idx = srcu_read_lock(hctx->srcu);
> -}
> -
> /**
> * blk_mq_start_request - Start processing a request
> * @rq: Pointer to request to be started
> @@ -1947,19 +1927,13 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list,
> */
> static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx)
> {
> - int srcu_idx;
> -
> /*
> * We can't run the queue inline with ints disabled. Ensure that
> * we catch bad users of this early.
> */
> WARN_ON_ONCE(in_interrupt());
>
> - might_sleep_if(hctx->flags & BLK_MQ_F_BLOCKING);
> -
> - hctx_lock(hctx, &srcu_idx);
> - blk_mq_sched_dispatch_requests(hctx);
> - hctx_unlock(hctx, srcu_idx);
> + blk_mq_run_dispatch_ops(hctx, blk_mq_sched_dispatch_requests(hctx));
> }
>
> static inline int blk_mq_first_mapped_cpu(struct blk_mq_hw_ctx *hctx)
> @@ -2071,7 +2045,6 @@ EXPORT_SYMBOL(blk_mq_delay_run_hw_queue);
> */
> void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async)
> {
> - int srcu_idx;
> bool need_run;
>
> /*
> @@ -2082,10 +2055,9 @@ void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async)
> * And queue will be rerun in blk_mq_unquiesce_queue() if it is
> * quiesced.
> */
> - hctx_lock(hctx, &srcu_idx);
> - need_run = !blk_queue_quiesced(hctx->queue) &&
> - blk_mq_hctx_has_pending(hctx);
> - hctx_unlock(hctx, srcu_idx);
> + blk_mq_run_dispatch_ops(hctx,
> + need_run = !blk_queue_quiesced(hctx->queue) &&
> + blk_mq_hctx_has_pending(hctx));
>
> if (need_run)
> __blk_mq_delay_run_hw_queue(hctx, async, 0);
> @@ -2488,32 +2460,22 @@ static blk_status_t __blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
> static void blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
> struct request *rq)
> {
> - blk_status_t ret;
> - int srcu_idx;
> -
> - might_sleep_if(hctx->flags & BLK_MQ_F_BLOCKING);
> + blk_status_t ret =
> + __blk_mq_try_issue_directly(hctx, rq, false, true);
>
> - hctx_lock(hctx, &srcu_idx);
> -
> - ret = __blk_mq_try_issue_directly(hctx, rq, false, true);
> if (ret == BLK_STS_RESOURCE || ret == BLK_STS_DEV_RESOURCE)
> blk_mq_request_bypass_insert(rq, false, true);
> else if (ret != BLK_STS_OK)
> blk_mq_end_request(rq, ret);
> -
> - hctx_unlock(hctx, srcu_idx);
> }
>
> static blk_status_t blk_mq_request_issue_directly(struct request *rq, bool last)
> {
> blk_status_t ret;
> - int srcu_idx;
> struct blk_mq_hw_ctx *hctx = rq->mq_hctx;
>
> - hctx_lock(hctx, &srcu_idx);
> - ret = __blk_mq_try_issue_directly(hctx, rq, true, last);
> - hctx_unlock(hctx, srcu_idx);
> -
> + blk_mq_run_dispatch_ops(hctx,
> + ret = __blk_mq_try_issue_directly(hctx, rq, true, last));
> return ret;
> }
>
> @@ -2826,7 +2788,8 @@ void blk_mq_submit_bio(struct bio *bio)
> (q->nr_hw_queues == 1 || !is_sync)))
> blk_mq_sched_insert_request(rq, false, true, true);
> else
> - blk_mq_try_issue_directly(rq->mq_hctx, rq);
> + blk_mq_run_dispatch_ops(rq->mq_hctx,
> + blk_mq_try_issue_directly(rq->mq_hctx, rq));
> }
>
> /**
> diff --git a/block/blk-mq.h b/block/blk-mq.h
> index d516c7a46f57..e4c396204928 100644
> --- a/block/blk-mq.h
> +++ b/block/blk-mq.h
> @@ -374,5 +374,21 @@ static inline bool hctx_may_queue(struct blk_mq_hw_ctx *hctx,
> return __blk_mq_active_requests(hctx) < depth;
> }
>
> +/* run the code block in @dispatch_ops with rcu/srcu read lock held */
> +#define blk_mq_run_dispatch_ops(hctx, dispatch_ops) \
> +do { \
> + if (!((hctx)->flags & BLK_MQ_F_BLOCKING)) { \
> + rcu_read_lock(); \
> + (dispatch_ops); \
> + rcu_read_unlock(); \
> + } else { \
> + int srcu_idx; \
> + \
> + might_sleep(); \
> + srcu_idx = srcu_read_lock((hctx)->srcu); \
> + (dispatch_ops); \
> + srcu_read_unlock((hctx)->srcu, srcu_idx); \
> + } \
> +} while (0)
>
> #endif
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
