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
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
