>>>> @@ -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
>>> +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 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?
Regards,
Markus
--
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
