| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 1 Apr 2024 14:23:10 -0700
From: Bart Van Assche <bvanassche@....org>
To: Zhiguo Niu <niuzhiguo84@...il.com>
Cc: Zhiguo Niu <zhiguo.niu@...soc.com>, axboe@...nel.dk,
linux-kernel@...r.kernel.org, linux-block@...r.kernel.org,
ke.wang@...soc.com, hongyu.jin@...soc.com,
Damien Le Moal <dlemoal@...nel.org>
Subject: Re: [PATCH] block/mq-deadline: Fix WARN when set async_depth by sysfs
On 3/31/24 17:58, Zhiguo Niu wrote:
> On Sat, Mar 30, 2024 at 2:08 AM Bart Van Assche <bvanassche@...org> wrote:
>>
>> On 3/28/24 7:44 PM, Zhiguo Niu wrote:
>>> diff --git a/block/mq-deadline.c b/block/mq-deadline.c
>>> index 02a916b..89c516e 100644
>>> --- a/block/mq-deadline.c
>>> +++ b/block/mq-deadline.c
>>> @@ -646,10 +646,12 @@ static void dd_depth_updated(struct blk_mq_hw_ctx *hctx)
>>> struct request_queue *q = hctx->queue;
>>> struct deadline_data *dd = q->elevator->elevator_data;
>>> struct blk_mq_tags *tags = hctx->sched_tags;
>>> + unsigned int shift = tags->bitmap_tags.sb.shift;
>>> + unsigned int dd_min_depth = max(1U, 3 * (1U << shift) / 4);
>>>
>>> dd->async_depth = max(1UL, 3 * q->nr_requests / 4);
>>>
>>> - sbitmap_queue_min_shallow_depth(&tags->bitmap_tags, dd->async_depth);
>>> + sbitmap_queue_min_shallow_depth(&tags->bitmap_tags, dd_min_depth);
>>> }
>>
>> The above patch sets min_shallow_depth to the same value as commit
>> d47f9717e5cf ("block/mq-deadline: use correct way to throttling write
>> requests"). That commit got reverted because it was causing performance
>> problems. So the above patch reintroduces the performance problem that
>> has been fixed by commit 256aab46e316 ("Revert "block/mq-deadline: use
>> correct way to throttling write requests"").
> Hi Bart Van Assche,
>
> This patch only modifies the initial minimum value of
> min_shallow_depth and does not change "dd->async_depth",
> so it will not cause performance problems like the previous patch
> (d47f9717e5cf ("block/mq-deadline: use correct way to throttling write
> requests")).
Oops, I misread your patch. After having taken another look, my
conclusions are as follows:
* sbitmap_queue_min_shallow_depth() is called. This causes
sbq->wake_batch to be modified but I don't think that it is a proper
fix for dd_limit_depth().
* dd_limit_depth() still assigns a number in the range 1..nr_requests to
data->shallow_depth while a number in the range 1..(1<<bt->sb.shift)
should be assigned.
> So what are your suggestions for fixing the warning shown in commit
> msg if dd->async_depth is set by the user from sysfs?
> thanks
How about the two untested patches below?
Thanks,
Bart.
Subject: [PATCH 1/2] block: Call .limit_depth() after .hctx has been set
Prepare for using .hctx in dd_limit_depth().
Signed-off-by: Bart Van Assche <bvanassche@....org>
---
block/blk-mq.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 34060d885c5a..d0db9252bb71 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -435,6 +435,7 @@ __blk_mq_alloc_requests_batch(struct
blk_mq_alloc_data *data)
static struct request *__blk_mq_alloc_requests(struct
blk_mq_alloc_data *data)
{
struct request_queue *q = data->q;
+ struct elevator_mq_ops *ops = NULL;
u64 alloc_time_ns = 0;
struct request *rq;
unsigned int tag;
@@ -459,13 +460,11 @@ static struct request
*__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
*/
if ((data->cmd_flags & REQ_OP_MASK) != REQ_OP_FLUSH &&
!blk_op_is_passthrough(data->cmd_flags)) {
- struct elevator_mq_ops *ops = &q->elevator->type->ops;
+ ops = &q->elevator->type->ops;
WARN_ON_ONCE(data->flags & BLK_MQ_REQ_RESERVED);
data->rq_flags |= RQF_USE_SCHED;
- if (ops->limit_depth)
- ops->limit_depth(data->cmd_flags, data);
}
}
@@ -478,6 +477,9 @@ static struct request
*__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
if (data->flags & BLK_MQ_REQ_RESERVED)
data->rq_flags |= RQF_RESV;
+ if (ops && ops->limit_depth)
+ ops->limit_depth(data->cmd_flags, data);
+
/*
* Try batched alloc if we want more than 1 tag.
*/
Subject: [PATCH 2/2] block/mq-deadline: Fix the tag reservation code
Fixes: 07757588e507 ("block/mq-deadline: Reserve 25% of scheduler tags
for synchronous requests")
---
block/mq-deadline.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/block/mq-deadline.c b/block/mq-deadline.c
index 02a916ba62ee..8e780069d91b 100644
--- a/block/mq-deadline.c
+++ b/block/mq-deadline.c
@@ -621,6 +621,20 @@ static struct request *dd_dispatch_request(struct
blk_mq_hw_ctx *hctx)
return rq;
}
+/*
+ * 'depth' is a number in the range 0..q->nr_requests. Convert it to a
number
+ * in the range 0..(1 << bt->sb.shift) since that is the range expected by
+ * sbitmap_get_shallow().
+ */
+static int dd_to_word_depth(struct blk_mq_hw_ctx *hctx, unsigned int
qdepth)
+{
+ struct sbitmap_queue *bt = &hctx->sched_tags->bitmap_tags;
+ const unsigned int nrr = hctx->queue->nr_requests;
+
+ return max(((qdepth << bt->sb.shift) + nrr - 1) / nrr,
+ bt->min_shallow_depth);
+}
+
/*
* Called by __blk_mq_alloc_request(). The shallow_depth value set by this
* function is used by __blk_mq_get_tag().
@@ -637,7 +651,7 @@ static void dd_limit_depth(blk_opf_t opf, struct
blk_mq_alloc_data *data)
* Throttle asynchronous requests and writes such that these requests
* do not block the allocation of synchronous requests.
*/
- data->shallow_depth = dd->async_depth;
+ data->shallow_depth = dd_to_word_depth(data->hctx, dd->async_depth);
}
/* Called by blk_mq_update_nr_requests(). */
@@ -649,7 +663,7 @@ static void dd_depth_updated(struct blk_mq_hw_ctx *hctx)
dd->async_depth = max(1UL, 3 * q->nr_requests / 4);
- sbitmap_queue_min_shallow_depth(&tags->bitmap_tags, dd->async_depth);
+ sbitmap_queue_min_shallow_depth(&tags->bitmap_tags, 1);
}
/* Called by blk_mq_init_hctx() and blk_mq_init_sched(). */
Powered by blists - more mailing lists