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

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

[ZhangHui]

From: zhanghui <zhanghui31@xiaomi.com>

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 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 <zhanghui31@xiaomi.com>
---
Changes in v6:
- remove unnecessary inner parenthesis

Changes in v5:
- modify the commit log
- remove unnecessary variable and add comment

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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index e3c3c0c21b55..45e4d255ea3b 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1210,7 +1210,11 @@ bool blk_mq_complete_request_remote(struct request *rq)
 		return true;
 	}
 
-	if (rq->q->nr_hw_queues == 1) {
+	/*
+	 * To reduce the execution time in the IRQ top-half,
+	 * move non-sync request completions to softirq context.
+	 */
+	if (rq->q->nr_hw_queues == 1 || !rq_is_sync(rq)) {
 		blk_mq_raise_softirq(rq);
 		return true;
 	}
-- 
2.43.0

Re: [PATCH v6] block: move non sync requests complete flow to softirq

[Jens Axboe]

On 9/6/24 8:49 PM, ZhangHui wrote:
> From: zhanghui <zhanghui31@xiaomi.com>
> 
> 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 fix this issue by moving non sync requests completion to softirq
> context, and keeping sync requests completion in the IRQ top-half context.

You keep ignoring the feedback, and hence I too shall be ignoring this
patch going forward then.

The key issue here is that the completion takes so long, and adding a
heuristic that equates not-sync with latency-not-important is pretty
bogus and not a good way to attempt to work around it.

-- 
Jens Axboe

Re: [External Mail]Re: [PATCH v6] block: move non sync requests complete flow to softirq

[章辉]

On 2024/9/7 21:46, Jens Axboe wrote:
> On 9/6/24 8:49 PM, ZhangHui wrote:
>> From: zhanghui <zhanghui31@xiaomi.com>
>>
>> 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 fix this issue by moving non sync requests completion to softirq
>> context, and keeping sync requests completion in the IRQ top-half context.
> You keep ignoring the feedback, and hence I too shall be ignoring this
> patch going forward then.
>
> The key issue here is that the completion takes so long, and adding a
> heuristic that equates not-sync with latency-not-important is pretty
> bogus and not a good way to attempt to work around it.
>
> --
> Jens Axboe
>
hi Jens,

Sorry for not replying in time.

We have basically determined the plan for the f2fs side. The short-term
plan is to limit the size of a single bio, and the long-term plan is to
change f2fs from page to folio to reduce the pagecache traversal time.

However, I think it also makes sense to move less urgent work out of the
IRQ top-half.

Thanks
Zhang

Re: [External Mail]Re: [PATCH v6] block: move non sync requests complete flow to softirq

[Jens Axboe]

On 9/8/24 8:17 PM, ?? wrote:
> On 2024/9/7 21:46, Jens Axboe wrote:
>> On 9/6/24 8:49 PM, ZhangHui wrote:
>>> From: zhanghui <zhanghui31@xiaomi.com>
>>>
>>> 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 fix this issue by moving non sync requests completion to softirq
>>> context, and keeping sync requests completion in the IRQ top-half context.
>> You keep ignoring the feedback, and hence I too shall be ignoring this
>> patch going forward then.
>>
>> The key issue here is that the completion takes so long, and adding a
>> heuristic that equates not-sync with latency-not-important is pretty
>> bogus and not a good way to attempt to work around it.
>>
>> --
>> Jens Axboe
>>
> hi Jens,
> 
> Sorry for not replying in time.
> 
> We have basically determined the plan for the f2fs side. The short-term
> plan is to limit the size of a single bio, and the long-term plan is to
> change f2fs from page to folio to reduce the pagecache traversal time.
> 
> However, I think it also makes sense to move less urgent work out of the
> IRQ top-half.

What you are missing is that what you think is less urgent, may be just
as urgent as other requests to others. !rq_is_sync() doesn't mean that
it's necessarily a background or low priority request. So no, I'm not
interested in merging an odd work-around for what is really a different
issue.

Fixing f2fs is indeed the right way, and I'd suggest in the mean time
you just limit the per-request size to something a lot more reasonable.
If you see high latencies with 80MB requests, then perhaps don't be
doing 80MB requests. That should be well beyond the diminishing returns
point for bandwidth anyway, there's no reason why anyone should be doing
requests that huge and not expect longer processing times.

-- 
Jens Axboe