[v4] block: move non sync requests complete flow to softirq

Commit Message

章辉 Sept. 6, 2024, 3:14 a.m. UTC

From: zhanghui <[e-mail]>

Currently, for a controller that supports multiple queues, like UFS4.0,
the mq_ops->complete is executed in the interrupt top-half. Therefore, 
the file system's end io is executed during the request completion process,
such as f2fs_write_end_io on smartphone.

However, we found that the execution time of the file system end io
is strongly related to the size of the bio and the processing speed
of the CPU. Because the file system's end io will traverse every page
in bio, this is a very time-consuming operation.

We measured that the 80M bio write operation on the little CPU will
cause the execution time of the top-half to be greater than 100ms,
which will undoubtedly affect interrupt response latency.

Let's fixed this issue by moved non sync request completion flow to
softirq, and keep the sync request completion in the top-half.

Signed-off-by: zhanghui <[e-mail]>
---
Changes in v4:
- fix commit log from "scheduling efficiency" to "interrupt response latency"

Changes in v3:
- modify op_is_sync to rq_is_sync

Changes in v2:
- fix build warning
---
 block/blk-mq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Damien Le Moal Sept. 6, 2024, 6:18 a.m. UTC | #1

On 9/6/24 12:14, ZhangHui wrote:
> From: zhanghui <[e-mail]>
> 
> Currently, for a controller that supports multiple queues, like UFS4.0,
> the mq_ops->complete is executed in the interrupt top-half. Therefore, 
> the file system's end io is executed during the request completion process,
> such as f2fs_write_end_io on smartphone.
> 
> However, we found that the execution time of the file system end io
> is strongly related to the size of the bio and the processing speed
> of the CPU. Because the file system's end io will traverse every page
> in bio, this is a very time-consuming operation.
> 
> We measured that the 80M bio write operation on the little CPU will
> cause the execution time of the top-half to be greater than 100ms,
> which will undoubtedly affect interrupt response latency.
> 
> Let's fixed this issue by moved non sync request completion flow to
> softirq, and keep the sync request completion in the top-half.

Typos/grammar:

Let's fix this issue by moving non sync requests completion to softirq context,
and keeping sync requests completion in the irq top-half context.

> 
> Signed-off-by: zhanghui <[e-mail]>
> ---
> Changes in v4:
> - fix commit log from "scheduling efficiency" to "interrupt response latency"
> 
> Changes in v3:
> - modify op_is_sync to rq_is_sync
> 
> Changes in v2:
> - fix build warning
> ---
>  block/blk-mq.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index e3c3c0c21b55..06b232edff11 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1193,6 +1193,8 @@ static void blk_mq_raise_softirq(struct request *rq)
>  
>  bool blk_mq_complete_request_remote(struct request *rq)
>  {
> +	const bool is_sync = rq_is_sync(rq);
> +
>  	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
>  
>  	/*
> @@ -1210,7 +1212,7 @@ bool blk_mq_complete_request_remote(struct request *rq)
>  		return true;
>  	}
>  
> -	if (rq->q->nr_hw_queues == 1) {
> +	if ((rq->q->nr_hw_queues == 1) || !is_sync) {

I do not see the point of the is_sync variable, and you do not need the inner
parenthesis either. Why not:

	if (rq->q->nr_hw_queues == 1 || !rq_is_sync(rq)) {

And a comment before the "if" explaining why soft-irq is raised for non-sync
requests would be nice too.

>  		blk_mq_raise_softirq(rq);
>  		return true;
>  	}

Patch

diff --git a/block/blk-mq.c b/block/blk-mq.c
index e3c3c0c21b55..06b232edff11 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1193,6 +1193,8 @@  static void blk_mq_raise_softirq(struct request *rq)
 
 bool blk_mq_complete_request_remote(struct request *rq)
 {
+	const bool is_sync = rq_is_sync(rq);
+
 	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
 
 	/*
@@ -1210,7 +1212,7 @@  bool blk_mq_complete_request_remote(struct request *rq)
 		return true;
 	}
 
-	if (rq->q->nr_hw_queues == 1) {
+	if ((rq->q->nr_hw_queues == 1) || !is_sync) {
 		blk_mq_raise_softirq(rq);
 		return true;
 	}