linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] cleanups/fix for blk_mq_get_tag()
@ 2020-11-30  1:36 Pavel Begunkov
  2020-11-30  1:36 ` [PATCH 1/3] blk-mq: use right tag offset after wait Pavel Begunkov
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Pavel Begunkov @ 2020-11-30  1:36 UTC (permalink / raw)
  To: Jens Axboe, linux-block

[1/3] fixes using tag_offset of a different HW queue. May only fire
if it's allowed for queues to have different breserved_tags or a resize
happened while it was waiting (can be?). Two others are simple cleanups
catched the eye.

Pavel Begunkov (3):
  blk-mq: use right tag offset after wait
  blk-mq: deduplicate blk_mq_get_tag() bits
  blk-mq: idiomatic use of WARN_ON_ONCE

 block/blk-mq-tag.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

-- 
2.24.0


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 1/3] blk-mq: use right tag offset after wait
  2020-11-30  1:36 [PATCH 0/3] cleanups/fix for blk_mq_get_tag() Pavel Begunkov
@ 2020-11-30  1:36 ` Pavel Begunkov
  2020-12-03  6:49   ` Ming Lei
  2020-11-30  1:36 ` [PATCH 2/3] blk-mq: deduplicate blk_mq_get_tag() bits Pavel Begunkov
  2020-11-30  1:36 ` [PATCH 3/3] blk-mq: idiomatic use of WARN_ON_ONCE Pavel Begunkov
  2 siblings, 1 reply; 6+ messages in thread
From: Pavel Begunkov @ 2020-11-30  1:36 UTC (permalink / raw)
  To: Jens Axboe, linux-block

blk_mq_get_tag() selects tag_offset in the beginning and doesn't update
it even though the tag set it may have changed hw queue during waiting.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/blk-mq-tag.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index 9c92053e704d..dbbf11edf9a6 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c
@@ -101,10 +101,8 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)
 			return BLK_MQ_NO_TAG;
 		}
 		bt = tags->breserved_tags;
-		tag_offset = 0;
 	} else {
 		bt = tags->bitmap_tags;
-		tag_offset = tags->nr_reserved_tags;
 	}
 
 	tag = __blk_mq_get_tag(data, bt);
@@ -167,6 +165,11 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)
 	sbitmap_finish_wait(bt, ws, &wait);
 
 found_tag:
+	if (data->flags & BLK_MQ_REQ_RESERVED)
+		tag_offset = 0;
+	else
+		tag_offset = tags->nr_reserved_tags;
+
 	/*
 	 * Give up this allocation if the hctx is inactive.  The caller will
 	 * retry on an active hctx.
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/3] blk-mq: deduplicate blk_mq_get_tag() bits
  2020-11-30  1:36 [PATCH 0/3] cleanups/fix for blk_mq_get_tag() Pavel Begunkov
  2020-11-30  1:36 ` [PATCH 1/3] blk-mq: use right tag offset after wait Pavel Begunkov
@ 2020-11-30  1:36 ` Pavel Begunkov
  2020-11-30  1:36 ` [PATCH 3/3] blk-mq: idiomatic use of WARN_ON_ONCE Pavel Begunkov
  2 siblings, 0 replies; 6+ messages in thread
From: Pavel Begunkov @ 2020-11-30  1:36 UTC (permalink / raw)
  To: Jens Axboe, linux-block

Deduplicate bt_wait_ptr() call in blk_mq_get_tag(), condition-less while
and absence of loop controls in-between former and current call location
makes the change functionally identical.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/blk-mq-tag.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index dbbf11edf9a6..98f6ea219bb4 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c
@@ -112,10 +112,11 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)
 	if (data->flags & BLK_MQ_REQ_NOWAIT)
 		return BLK_MQ_NO_TAG;
 
-	ws = bt_wait_ptr(bt, data->hctx);
 	do {
 		struct sbitmap_queue *bt_prev;
 
+		ws = bt_wait_ptr(bt, data->hctx);
+
 		/*
 		 * We're out of tags on this hardware queue, kick any
 		 * pending IO submits before going to sleep waiting for
@@ -158,8 +159,6 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)
 		 */
 		if (bt != bt_prev)
 			sbitmap_queue_wake_up(bt_prev);
-
-		ws = bt_wait_ptr(bt, data->hctx);
 	} while (1);
 
 	sbitmap_finish_wait(bt, ws, &wait);
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 3/3] blk-mq: idiomatic use of WARN_ON_ONCE
  2020-11-30  1:36 [PATCH 0/3] cleanups/fix for blk_mq_get_tag() Pavel Begunkov
  2020-11-30  1:36 ` [PATCH 1/3] blk-mq: use right tag offset after wait Pavel Begunkov
  2020-11-30  1:36 ` [PATCH 2/3] blk-mq: deduplicate blk_mq_get_tag() bits Pavel Begunkov
@ 2020-11-30  1:36 ` Pavel Begunkov
  2 siblings, 0 replies; 6+ messages in thread
From: Pavel Begunkov @ 2020-11-30  1:36 UTC (permalink / raw)
  To: Jens Axboe, linux-block

Let WARN_ON_ONCE() to be more clear by passing the warn condition
directly, so it'll be printed.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/blk-mq-tag.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index 98f6ea219bb4..f2743a9159ca 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c
@@ -96,10 +96,8 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)
 	int tag;
 
 	if (data->flags & BLK_MQ_REQ_RESERVED) {
-		if (unlikely(!tags->nr_reserved_tags)) {
-			WARN_ON_ONCE(1);
+		if (WARN_ON_ONCE(!tags->nr_reserved_tags))
 			return BLK_MQ_NO_TAG;
-		}
 		bt = tags->breserved_tags;
 	} else {
 		bt = tags->bitmap_tags;
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] blk-mq: use right tag offset after wait
  2020-11-30  1:36 ` [PATCH 1/3] blk-mq: use right tag offset after wait Pavel Begunkov
@ 2020-12-03  6:49   ` Ming Lei
  2020-12-03 12:06     ` Pavel Begunkov
  0 siblings, 1 reply; 6+ messages in thread
From: Ming Lei @ 2020-12-03  6:49 UTC (permalink / raw)
  To: Pavel Begunkov; +Cc: Jens Axboe, linux-block

On Mon, Nov 30, 2020 at 01:36:25AM +0000, Pavel Begunkov wrote:
> blk_mq_get_tag() selects tag_offset in the beginning and doesn't update
> it even though the tag set it may have changed hw queue during waiting.
> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  block/blk-mq-tag.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
> index 9c92053e704d..dbbf11edf9a6 100644
> --- a/block/blk-mq-tag.c
> +++ b/block/blk-mq-tag.c
> @@ -101,10 +101,8 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)
>  			return BLK_MQ_NO_TAG;
>  		}
>  		bt = tags->breserved_tags;
> -		tag_offset = 0;
>  	} else {
>  		bt = tags->bitmap_tags;
> -		tag_offset = tags->nr_reserved_tags;
>  	}
>  
>  	tag = __blk_mq_get_tag(data, bt);
> @@ -167,6 +165,11 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)
>  	sbitmap_finish_wait(bt, ws, &wait);
>  
>  found_tag:
> +	if (data->flags & BLK_MQ_REQ_RESERVED)
> +		tag_offset = 0;
> +	else
> +		tag_offset = tags->nr_reserved_tags;
> +

So far, the tag offset is tag-set wide, and 'tag_offset' is same for all tags,
looks no need to add the extra check.


thanks,
Ming


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] blk-mq: use right tag offset after wait
  2020-12-03  6:49   ` Ming Lei
@ 2020-12-03 12:06     ` Pavel Begunkov
  0 siblings, 0 replies; 6+ messages in thread
From: Pavel Begunkov @ 2020-12-03 12:06 UTC (permalink / raw)
  To: Ming Lei; +Cc: Jens Axboe, linux-block

On 03/12/2020 06:49, Ming Lei wrote:
> On Mon, Nov 30, 2020 at 01:36:25AM +0000, Pavel Begunkov wrote:
>> blk_mq_get_tag() selects tag_offset in the beginning and doesn't update
>> it even though the tag set it may have changed hw queue during waiting.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>  block/blk-mq-tag.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
>> index 9c92053e704d..dbbf11edf9a6 100644
>> --- a/block/blk-mq-tag.c
>> +++ b/block/blk-mq-tag.c
>> @@ -101,10 +101,8 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)
>>  			return BLK_MQ_NO_TAG;
>>  		}
>>  		bt = tags->breserved_tags;
>> -		tag_offset = 0;
>>  	} else {
>>  		bt = tags->bitmap_tags;
>> -		tag_offset = tags->nr_reserved_tags;
>>  	}
>>  
>>  	tag = __blk_mq_get_tag(data, bt);
>> @@ -167,6 +165,11 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)
>>  	sbitmap_finish_wait(bt, ws, &wait);
>>  
>>  found_tag:
>> +	if (data->flags & BLK_MQ_REQ_RESERVED)
>> +		tag_offset = 0;
>> +	else
>> +		tag_offset = tags->nr_reserved_tags;
>> +
> 
> So far, the tag offset is tag-set wide, and 'tag_offset' is same for all tags,
> looks no need to add the extra check.

Thanks for clearing that. I still think that's a good thing to do as more
apparent to me and shouldn't be slower -- it unconditionally stores offset
on-stack, even for no-wait path (gcc 9.2). With it's optimised more like

if (!(data->flags & BLK_MQ_REQ_RESERVED))
	tag += tags->nr_reserved_tags;

I like assembly a bit more, but not like that matters much.

-- 
Pavel Begunkov

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-03 12:10 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-30  1:36 [PATCH 0/3] cleanups/fix for blk_mq_get_tag() Pavel Begunkov
2020-11-30  1:36 ` [PATCH 1/3] blk-mq: use right tag offset after wait Pavel Begunkov
2020-12-03  6:49   ` Ming Lei
2020-12-03 12:06     ` Pavel Begunkov
2020-11-30  1:36 ` [PATCH 2/3] blk-mq: deduplicate blk_mq_get_tag() bits Pavel Begunkov
2020-11-30  1:36 ` [PATCH 3/3] blk-mq: idiomatic use of WARN_ON_ONCE Pavel Begunkov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).