Messages in this thread | | | Subject | Re: [PATCH 2/3] xen-scsiback: One function call less in scsiback_device_action() after error detection | From | Juergen Gross <> | Date | Wed, 20 Jul 2016 07:26:31 +0200 |
| |
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
| |