* [PATCH] blk-mq: remove needless goto from blk_mq_get_driver_tag
@ 2019-10-22 17:41 André Almeida
2019-10-24 1:34 ` Bart Van Assche
0 siblings, 1 reply; 5+ messages in thread
From: André Almeida @ 2019-10-22 17:41 UTC (permalink / raw)
To: linux-block, linux-kernel; +Cc: axboe, kernel, krisman, André Almeida
The only usage of the label "done" is when (rq->tag != -1) at the
begging of the function. Rather than jumping to label, we can just
remove this label and execute the code at the "if". Besides that,
the code that would be executed after the label "done" is the return of
the logical expression (rq->tag != -1) but since we are already inside
the if, we now that this is true. Remove the label and replace the goto
with the proper result of the label.
Signed-off-by: André Almeida <andrealmeid@collabora.com>
---
Hello,
I've used `blktest` to check if this change add any regression. I have
used `./check block` and I got the same results with and without this
patch (a bunch of "passed" and three "not run" because of the virtual
scsi capabilities). Please let me know if there would be a better way to
test changes at block stack.
This commit was rebase at linux-block/for-5.5/block.
Thanks,
André
---
block/blk-mq.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 8538dc415499..1e067b78ab97 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1036,7 +1036,7 @@ bool blk_mq_get_driver_tag(struct request *rq)
bool shared;
if (rq->tag != -1)
- goto done;
+ return true;
if (blk_mq_tag_is_reserved(data.hctx->sched_tags, rq->internal_tag))
data.flags |= BLK_MQ_REQ_RESERVED;
@@ -1051,7 +1051,6 @@ bool blk_mq_get_driver_tag(struct request *rq)
data.hctx->tags->rqs[rq->tag] = rq;
}
-done:
return rq->tag != -1;
}
--
2.23.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] blk-mq: remove needless goto from blk_mq_get_driver_tag
2019-10-22 17:41 [PATCH] blk-mq: remove needless goto from blk_mq_get_driver_tag André Almeida
@ 2019-10-24 1:34 ` Bart Van Assche
2019-10-24 3:32 ` Jens Axboe
0 siblings, 1 reply; 5+ messages in thread
From: Bart Van Assche @ 2019-10-24 1:34 UTC (permalink / raw)
To: André Almeida, linux-block, linux-kernel; +Cc: axboe, kernel, krisman
On 2019-10-22 10:41, André Almeida wrote:
> The only usage of the label "done" is when (rq->tag != -1) at the
> begging of the function. Rather than jumping to label, we can just
> remove this label and execute the code at the "if". Besides that,
> the code that would be executed after the label "done" is the return of
> the logical expression (rq->tag != -1) but since we are already inside
> the if, we now that this is true. Remove the label and replace the goto
> with the proper result of the label.
>
> Signed-off-by: André Almeida <andrealmeid@collabora.com>
> ---
> Hello,
>
> I've used `blktest` to check if this change add any regression. I have
> used `./check block` and I got the same results with and without this
> patch (a bunch of "passed" and three "not run" because of the virtual
> scsi capabilities). Please let me know if there would be a better way to
> test changes at block stack.
>
> This commit was rebase at linux-block/for-5.5/block.
>
> Thanks,
> André
> ---
> block/blk-mq.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 8538dc415499..1e067b78ab97 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1036,7 +1036,7 @@ bool blk_mq_get_driver_tag(struct request *rq)
> bool shared;
>
> if (rq->tag != -1)
> - goto done;
> + return true;
>
> if (blk_mq_tag_is_reserved(data.hctx->sched_tags, rq->internal_tag))
> data.flags |= BLK_MQ_REQ_RESERVED;
> @@ -1051,7 +1051,6 @@ bool blk_mq_get_driver_tag(struct request *rq)
> data.hctx->tags->rqs[rq->tag] = rq;
> }
>
> -done:
> return rq->tag != -1;
> }
Do we really need code changes like the above? I'm not aware of any text
in the Documentation/ directory that forbids the use of goto statements.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] blk-mq: remove needless goto from blk_mq_get_driver_tag
2019-10-24 1:34 ` Bart Van Assche
@ 2019-10-24 3:32 ` Jens Axboe
2019-10-24 18:10 ` André Almeida
0 siblings, 1 reply; 5+ messages in thread
From: Jens Axboe @ 2019-10-24 3:32 UTC (permalink / raw)
To: Bart Van Assche, André Almeida, linux-block, linux-kernel
Cc: kernel, krisman
On 10/23/19 7:34 PM, Bart Van Assche wrote:
> On 2019-10-22 10:41, André Almeida wrote:
>> The only usage of the label "done" is when (rq->tag != -1) at the
>> begging of the function. Rather than jumping to label, we can just
>> remove this label and execute the code at the "if". Besides that,
>> the code that would be executed after the label "done" is the return of
>> the logical expression (rq->tag != -1) but since we are already inside
>> the if, we now that this is true. Remove the label and replace the goto
>> with the proper result of the label.
>>
>> Signed-off-by: André Almeida <andrealmeid@collabora.com>
>> ---
>> Hello,
>>
>> I've used `blktest` to check if this change add any regression. I have
>> used `./check block` and I got the same results with and without this
>> patch (a bunch of "passed" and three "not run" because of the virtual
>> scsi capabilities). Please let me know if there would be a better way to
>> test changes at block stack.
>>
>> This commit was rebase at linux-block/for-5.5/block.
>>
>> Thanks,
>> André
>> ---
>> block/blk-mq.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 8538dc415499..1e067b78ab97 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -1036,7 +1036,7 @@ bool blk_mq_get_driver_tag(struct request *rq)
>> bool shared;
>>
>> if (rq->tag != -1)
>> - goto done;
>> + return true;
>>
>> if (blk_mq_tag_is_reserved(data.hctx->sched_tags, rq->internal_tag))
>> data.flags |= BLK_MQ_REQ_RESERVED;
>> @@ -1051,7 +1051,6 @@ bool blk_mq_get_driver_tag(struct request *rq)
>> data.hctx->tags->rqs[rq->tag] = rq;
>> }
>>
>> -done:
>> return rq->tag != -1;
>> }
>
> Do we really need code changes like the above? I'm not aware of any text
> in the Documentation/ directory that forbids the use of goto statements.
Agree, it looks fine as-is. It's also a fast path, so I'd never get rid
of that without looking at the generated code.
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] blk-mq: remove needless goto from blk_mq_get_driver_tag
2019-10-24 3:32 ` Jens Axboe
@ 2019-10-24 18:10 ` André Almeida
2019-10-25 20:15 ` Jens Axboe
0 siblings, 1 reply; 5+ messages in thread
From: André Almeida @ 2019-10-24 18:10 UTC (permalink / raw)
To: Jens Axboe, Bart Van Assche, linux-block, linux-kernel; +Cc: kernel, krisman
On 10/24/19 12:32 AM, Jens Axboe wrote:
> On 10/23/19 7:34 PM, Bart Van Assche wrote:
>> On 2019-10-22 10:41, André Almeida wrote:
>>> The only usage of the label "done" is when (rq->tag != -1) at the
>>> begging of the function. Rather than jumping to label, we can just
>>> remove this label and execute the code at the "if". Besides that,
>>> the code that would be executed after the label "done" is the return of
>>> the logical expression (rq->tag != -1) but since we are already inside
>>> the if, we now that this is true. Remove the label and replace the goto
>>> with the proper result of the label.
>>>
>>> Signed-off-by: André Almeida <andrealmeid@collabora.com>
>>> ---
>>> Hello,
>>>
>>> I've used `blktest` to check if this change add any regression. I have
>>> used `./check block` and I got the same results with and without this
>>> patch (a bunch of "passed" and three "not run" because of the virtual
>>> scsi capabilities). Please let me know if there would be a better way to
>>> test changes at block stack.
>>>
>>> This commit was rebase at linux-block/for-5.5/block.
>>>
>>> Thanks,
>>> André
>>> ---
>>> block/blk-mq.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>> index 8538dc415499..1e067b78ab97 100644
>>> --- a/block/blk-mq.c
>>> +++ b/block/blk-mq.c
>>> @@ -1036,7 +1036,7 @@ bool blk_mq_get_driver_tag(struct request *rq)
>>> bool shared;
>>>
>>> if (rq->tag != -1)
>>> - goto done;
>>> + return true;
>>>
>>> if (blk_mq_tag_is_reserved(data.hctx->sched_tags, rq->internal_tag))
>>> data.flags |= BLK_MQ_REQ_RESERVED;
>>> @@ -1051,7 +1051,6 @@ bool blk_mq_get_driver_tag(struct request *rq)
>>> data.hctx->tags->rqs[rq->tag] = rq;
>>> }
>>>
>>> -done:
>>> return rq->tag != -1;
>>> }
>>
>> Do we really need code changes like the above? I'm not aware of any text
>> in the Documentation/ directory that forbids the use of goto statements.
goto are allowed, but the coding style document[1] provides some
rationale for using goto, including that "If there is no cleanup needed
then just return directly". Seems like this code used to do some stuff
in the the past, but since 8ab6bb9ee8d0 "blk-mq: cleanup
blk_mq_get_driver_tag()" it is just a return.
>
> Agree, it looks fine as-is. It's also a fast path, so I'd never get rid
> of that without looking at the generated code.
>
You can have a look at the generated code for x86, here's the
original[2] and here is the modified[3]. The only improvement at the
assembly is that we get rid of this duplicated cmp instruction:
2736: 83 f8 ff cmp eax,0xffffffff
2739: 75 4b jne 2786
...
2786: 83 f8 ff cmp eax,0xffffffff
2789: 0f 95 c0 setne al
Thanks,
André
[1]
https://www.kernel.org/doc/html/latest/process/coding-style.html#centralized-exiting-of-functions
[2] https://pastebin.com/5eV8c2mK
[3] https://pastebin.com/Gwf2Z9FA
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] blk-mq: remove needless goto from blk_mq_get_driver_tag
2019-10-24 18:10 ` André Almeida
@ 2019-10-25 20:15 ` Jens Axboe
0 siblings, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2019-10-25 20:15 UTC (permalink / raw)
To: André Almeida, Bart Van Assche, linux-block, linux-kernel
Cc: kernel, krisman
On 10/24/19 12:10 PM, André Almeida wrote:
> On 10/24/19 12:32 AM, Jens Axboe wrote:
>> On 10/23/19 7:34 PM, Bart Van Assche wrote:
>>> On 2019-10-22 10:41, André Almeida wrote:
>>>> The only usage of the label "done" is when (rq->tag != -1) at the
>>>> begging of the function. Rather than jumping to label, we can just
>>>> remove this label and execute the code at the "if". Besides that,
>>>> the code that would be executed after the label "done" is the return of
>>>> the logical expression (rq->tag != -1) but since we are already inside
>>>> the if, we now that this is true. Remove the label and replace the goto
>>>> with the proper result of the label.
>>>>
>>>> Signed-off-by: André Almeida <andrealmeid@collabora.com>
>>>> ---
>>>> Hello,
>>>>
>>>> I've used `blktest` to check if this change add any regression. I have
>>>> used `./check block` and I got the same results with and without this
>>>> patch (a bunch of "passed" and three "not run" because of the virtual
>>>> scsi capabilities). Please let me know if there would be a better way to
>>>> test changes at block stack.
>>>>
>>>> This commit was rebase at linux-block/for-5.5/block.
>>>>
>>>> Thanks,
>>>> André
>>>> ---
>>>> block/blk-mq.c | 3 +--
>>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>>> index 8538dc415499..1e067b78ab97 100644
>>>> --- a/block/blk-mq.c
>>>> +++ b/block/blk-mq.c
>>>> @@ -1036,7 +1036,7 @@ bool blk_mq_get_driver_tag(struct request *rq)
>>>> bool shared;
>>>>
>>>> if (rq->tag != -1)
>>>> - goto done;
>>>> + return true;
>>>>
>>>> if (blk_mq_tag_is_reserved(data.hctx->sched_tags, rq->internal_tag))
>>>> data.flags |= BLK_MQ_REQ_RESERVED;
>>>> @@ -1051,7 +1051,6 @@ bool blk_mq_get_driver_tag(struct request *rq)
>>>> data.hctx->tags->rqs[rq->tag] = rq;
>>>> }
>>>>
>>>> -done:
>>>> return rq->tag != -1;
>>>> }
>>>
>>> Do we really need code changes like the above? I'm not aware of any text
>>> in the Documentation/ directory that forbids the use of goto statements.
>
> goto are allowed, but the coding style document[1] provides some
> rationale for using goto, including that "If there is no cleanup needed
> then just return directly". Seems like this code used to do some stuff
> in the the past, but since 8ab6bb9ee8d0 "blk-mq: cleanup
> blk_mq_get_driver_tag()" it is just a return.
>
>>
>> Agree, it looks fine as-is. It's also a fast path, so I'd never get rid
>> of that without looking at the generated code.
>>
>
> You can have a look at the generated code for x86, here's the
> original[2] and here is the modified[3]. The only improvement at the
> assembly is that we get rid of this duplicated cmp instruction:
>
> 2736: 83 f8 ff cmp eax,0xffffffff
> 2739: 75 4b jne 2786
> ...
> 2786: 83 f8 ff cmp eax,0xffffffff
> 2789: 0f 95 c0 setne al
Well, that's a win. I'll apply it.
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-10-25 20:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-22 17:41 [PATCH] blk-mq: remove needless goto from blk_mq_get_driver_tag André Almeida
2019-10-24 1:34 ` Bart Van Assche
2019-10-24 3:32 ` Jens Axboe
2019-10-24 18:10 ` André Almeida
2019-10-25 20:15 ` Jens Axboe
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).