Re: [Xen-devel] [PATCH 2/3] xen-scsiback: One function call less in scsiback_device_action() after error detection

[Juergen Gross <jgross@xxxxxxxx>]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel