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@xxxxxxxxxxxxx> >>>> --- >>>> 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
