lkml.org 
[lkml]   [2016]   [Jul]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/3] xen-scsiback: One function call less in scsiback_device_action() after error detection
From
Date
On 20/07/16 07:10, SF Markus Elfring wrote:
>>>>> @@ -606,7 +606,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
>>>>> tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL);
>>>>> if (!tmr) {
>>>>> target_put_sess_cmd(se_cmd);
>>>>> - goto err;
>>>>> + goto do_resp;
>>>>> }
>>>>
>>>> Hmm, I'm not convinced this is an improvement.
>>>>
>>>> I'd rather rename the new error label to "put_cmd" and get rid of the
>>>> braces in above if statement:
>>>>
>>>> - if (!tmr) {
>>>> - target_put_sess_cmd(se_cmd);
>>>> - goto err;
>>>> - }
>>>> + if (!tmr)
>>>> + goto put_cmd;
>>>>
>>>> and then in the error path:
>>>>
>>>> -err:
>>>> +put_cmd:
>>>> + target_put_sess_cmd(se_cmd);
>>>
>>> I am unsure on the relevance of this function on such a source position.
>>> Would it make sense to move it further down at the end?
>>
>> You only want to call it in the first error case (allocation failure).
>
> Thanks for your clarification.
>
> I find that my update suggestion (from Saturday) is still appropriate
> in this case.
> https://lkml.org/lkml/2016/7/16/172

And I still think it isn't an improvement: Nack

>>>> +free_tmr:
>>>> kfree(tmr);
>>>
>>> How do you think about to skip this function call after a memory
>>> allocation failure?
>>
>> I think this just doesn't matter. If it were a hot path, yes. But trying
>> to do micro-optimizations in an error path is just not worth the effort.
>
> Would you like to reduce also the amount of function calls in such special
> run-time situations?

I just don't care for the extra 2 or 3 nsecs. Readability is more
important here.

>> I like a linear error path containing all the needed cleanups best.
>
> I would prefer to keep the discussed single function call within
> the basic block of the if statement.
>
> Have we got different opinions about the shown implementation details?

Yes.


Juergen

\
 
 \ /
  Last update: 2016-07-20 08:01    [W:0.583 / U:0.628 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site