Re: [PATCH 2/3] xen-scsiback: One function call less in scsiback_device_action() after error detection
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
Re: [PATCH 2/3] xen-scsiback: One function call less in scsiback_device_action() after error detection
@@ -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
Re: [PATCH 2/3] xen-scsiback: One function call less in scsiback_device_action() after error detection
On 19/07/16 16:56, 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).
>> +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.
I like a linear error path containing all the needed cleanups best.
Juergen
Re: [PATCH 2/3] xen-scsiback: One function call less in scsiback_device_action() after error detection
>> @@ -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?
> +free_tmr:
> kfree(tmr);
How do you think about to skip this function call after a memory
allocation failure?
Regards,
Markus
Re: [PATCH 2/3] xen-scsiback: One function call less in scsiback_device_action() after error detection
On 16/07/16 22:23, SF Markus Elfring wrote:
> From: Markus Elfring
> Date: Sat, 16 Jul 2016 21:42:42 +0200
>
> The kfree() function was called in one case by the
> scsiback_device_action() function during error handling
> even if the passed variable "tmr" contained a null pointer.
>
> Adjust jump targets according to the Linux coding style convention.
>
> Signed-off-by: Markus Elfring
> ---
> drivers/xen/xen-scsiback.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> index 4a48c06..7612bc9 100644
> --- a/drivers/xen/xen-scsiback.c
> +++ b/drivers/xen/xen-scsiback.c
> @@ -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);
+free_tmr:
kfree(tmr);
Juergen
>
> init_waitqueue_head(&tmr->tmr_wait);
> @@ -616,7 +616,7 @@ static void scsiback_device_action(struct vscsibk_pend
> *pending_req,
> unpacked_lun, tmr, act, GFP_KERNEL,
> tag, TARGET_SCF_ACK_KREF);
> if (rc)
> - goto err;
> + goto free_tmr;
>
> wait_event(tmr->tmr_wait, atomic_read(&tmr->tmr_complete));
>
> @@ -626,8 +626,9 @@ static void scsiback_device_action(struct vscsibk_pend
> *pending_req,
> scsiback_do_resp_with_sense(NULL, err, 0, pending_req);
> transport_generic_free_cmd(&pending_req->se_cmd, 1);
> return;
> -err:
> +free_tmr:
> kfree(tmr);
> +do_resp:
> scsiback_do_resp_with_sense(NULL, err, 0, pending_req);
> }
>
>
