| 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
| ||
|
Message-ID: <45c0c587-59a2-1761-e175-3920669d0bfb@huawei.com>
Date: Mon, 27 Sep 2021 10:02:40 +0100
From: John Garry <john.garry@...wei.com>
To: Ming Lei <ming.lei@...hat.com>
CC: "axboe@...nel.dk" <axboe@...nel.dk>,
"linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
"hare@...e.de" <hare@...e.de>
Subject: Re: [PATCH v4 11/13] blk-mq: Refactor and rename
blk_mq_free_map_and_{requests->rqs}()
On 26/09/2021 03:05, Ming Lei wrote:
> On Fri, Sep 24, 2021 at 04:28:28PM +0800, John Garry wrote:
>> Refactor blk_mq_free_map_and_requests() such that it can be used at many
>> sites at which the tag map and rqs are freed.
>>
>> Also rename to blk_mq_free_map_and_rqs(), which is shorter and matches the
>> alloc equivalent.
>>
>> Suggested-by: Ming Lei<ming.lei@...hat.com>
>> Signed-off-by: John Garry<john.garry@...wei.com>
>> Reviewed-by: Hannes Reinecke<hare@...e.de>
>> ---
>> block/blk-mq-tag.c | 3 +--
>> block/blk-mq.c | 40 ++++++++++++++++++++++++----------------
>> block/blk-mq.h | 4 +++-
>> 3 files changed, 28 insertions(+), 19 deletions(-)
>>
>> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
>> index db99f1246795..a0ecc6d88f84 100644
>> --- a/block/blk-mq-tag.c
>> +++ b/block/blk-mq-tag.c
>> @@ -607,8 +607,7 @@ int blk_mq_tag_update_depth(struct blk_mq_hw_ctx *hctx,
>> if (!new)
>> return -ENOMEM;
>>
>> - blk_mq_free_rqs(set, *tagsptr, hctx->queue_num);
>> - blk_mq_free_rq_map(*tagsptr, set->flags);
>> + blk_mq_free_map_and_rqs(set, *tagsptr, hctx->queue_num);
>> *tagsptr = new;
>> } else {
>> /*
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 46772773b9c4..464ea20b9bcb 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -2878,15 +2878,15 @@ static bool __blk_mq_alloc_map_and_rqs(struct blk_mq_tag_set *set,
>> return set->tags[hctx_idx];
>> }
>>
>> -static void blk_mq_free_map_and_requests(struct blk_mq_tag_set *set,
>> - unsigned int hctx_idx)
>> +void blk_mq_free_map_and_rqs(struct blk_mq_tag_set *set,
>> + struct blk_mq_tags *tags,
>> + unsigned int hctx_idx)
>> {
>> unsigned int flags = set->flags;
>>
>> - if (set->tags && set->tags[hctx_idx]) {
>> - blk_mq_free_rqs(set, set->tags[hctx_idx], hctx_idx);
>> - blk_mq_free_rq_map(set->tags[hctx_idx], flags);
>> - set->tags[hctx_idx] = NULL;
>> + if (tags) {
>> + blk_mq_free_rqs(set, tags, hctx_idx);
>> + blk_mq_free_rq_map(tags, flags);
>> }
>> }
>>
>> @@ -2967,8 +2967,10 @@ static void blk_mq_map_swqueue(struct request_queue *q)
>> * fallback in case of a new remap fails
>> * allocation
>> */
>> - if (i && set->tags[i])
>> - blk_mq_free_map_and_requests(set, i);
>> + if (i && set->tags[i]) {
>> + blk_mq_free_map_and_rqs(set, set->tags[i], i);
>> + set->tags[i] = NULL;
>> + }
>>
>> hctx->tags = NULL;
>> continue;
>> @@ -3264,8 +3266,8 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
>> struct blk_mq_hw_ctx *hctx = hctxs[j];
>>
>> if (hctx) {
>> - if (hctx->tags)
>> - blk_mq_free_map_and_requests(set, j);
>> + blk_mq_free_map_and_rqs(set, set->tags[j], j);
>> + set->tags[j] = NULL;
>> blk_mq_exit_hctx(q, set, hctx, j);
>> hctxs[j] = NULL;
>> }
>> @@ -3361,8 +3363,10 @@ static int __blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set)
>> return 0;
>>
>> out_unwind:
>> - while (--i >= 0)
>> - blk_mq_free_map_and_requests(set, i);
>> + while (--i >= 0) {
>> + blk_mq_free_map_and_rqs(set, set->tags[i], i);
>> + set->tags[i] = NULL;
>> + }
>>
>> return -ENOMEM;
>> }
>> @@ -3557,8 +3561,10 @@ int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set)
>> return 0;
>>
>> out_free_mq_rq_maps:
>> - for (i = 0; i < set->nr_hw_queues; i++)
>> - blk_mq_free_map_and_requests(set, i);
>> + for (i = 0; i < set->nr_hw_queues; i++) {
>> + blk_mq_free_map_and_rqs(set, set->tags[i], i);
>> + set->tags[i] = NULL;
>> + }
>> out_free_mq_map:
>> for (i = 0; i < set->nr_maps; i++) {
>> kfree(set->map[i].mq_map);
>> @@ -3590,8 +3596,10 @@ void blk_mq_free_tag_set(struct blk_mq_tag_set *set)
>> {
>> int i, j;
>>
>> - for (i = 0; i < set->nr_hw_queues; i++)
>> - blk_mq_free_map_and_requests(set, i);
>> + for (i = 0; i < set->nr_hw_queues; i++) {
>> + blk_mq_free_map_and_rqs(set, set->tags[i], i);
>> + set->tags[i] = NULL;
>> + }
> There are 5 callers in which 'set->tags[i]' is cleared, so just
> wondering why you don't clear set->tags[i] at default in
> blk_mq_free_map_and_rqs(). And just call __blk_mq_free_map_and_rqs()
> for the only other user?
blk_mq_free_map_and_rqs() is not always passed set->tags[i]:
- blk_mq_tag_update_depth() calls blk_mq_free_map_and_rqs() for sched tags
- __blk_mq_alloc_rq_maps() calls blk_mq_free_map_and_rqs() for
shared_sbitmap_tags
Function __blk_mq_free_map_and_rqs() is not public and really only
intended for set->tags[i]
So I did consider passing the address of the tags pointer to
blk_mq_free_map_and_rqs(), like:
void blk_mq_free_map_and_rqs(struct blk_mq_tag_set *set,
struct blk_mq_tag **tags,
unsigned int hctx_idx)
{
...
*tags = NULL;
}
But then the API becomes a bit asymmetric, as we deal with tags pointer
normally:
struct blk_mq_tags *blk_mq_alloc_map_and_rqs(struct blk_mq_tag_set *set,
unsigned int hctx_idx,
unsigned int depth);
However, apart from this, I can change __blk_mq_free_map_and_rqs() to
NULLify set->tags[i], as it is always passed set->tags[i].
Do you have a preference?
Thanks,
John
Powered by blists - more mailing lists