[PATCH] drm/i915/gvt: Use common error handling code in shadow_workload_ring_buffer()

[PATCH] drm/i915/gvt: Use common error handling code in shadow_workload_ring_buffer()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 24 Oct 2017 14:20:06 +0200

Add a jump target so that a call of the function "gvt_vgpu_err" is stored
only once at the end of this function implementation.
Replace two calls by goto statements.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/i915/gvt/cmd_parser.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
index 2c0ccbb817dc..caa181380958 100644
--- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
+++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
@@ -2640,10 +2640,9 @@ static int shadow_workload_ring_buffer(struct intel_vgpu_workload *workload)
 	if (gma_head > gma_tail) {
 		ret = copy_gma_to_hva(vgpu, vgpu->gtt.ggtt_mm,
 				      gma_head, gma_top, shadow_ring_buffer_va);
-		if (ret < 0) {
-			gvt_vgpu_err("fail to copy guest ring buffer\n");
-			return ret;
-		}
+		if (ret < 0)
+			goto report_failure;
+
 		shadow_ring_buffer_va += ret;
 		gma_head = workload->rb_start;
 	}
@@ -2651,11 +2650,14 @@ static int shadow_workload_ring_buffer(struct intel_vgpu_workload *workload)
 	/* copy head or start <-> tail */
 	ret = copy_gma_to_hva(vgpu, vgpu->gtt.ggtt_mm, gma_head, gma_tail,
 				shadow_ring_buffer_va);
-	if (ret < 0) {
-		gvt_vgpu_err("fail to copy guest ring buffer\n");
-		return ret;
-	}
+	if (ret < 0)
+		goto report_failure;
+
 	return 0;
+
+report_failure:
+	gvt_vgpu_err("fail to copy guest ring buffer\n");
+	return ret;
 }
 
 int intel_gvt_scan_and_shadow_ringbuffer(struct intel_vgpu_workload *workload)
-- 
2.14.3

Re: [PATCH] drm/i915/gvt: Use common error handling code in shadow_workload_ring_buffer()

[Jani Nikula]

On Tue, 24 Oct 2017, SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 24 Oct 2017 14:20:06 +0200
>
> Add a jump target so that a call of the function "gvt_vgpu_err" is stored
> only once at the end of this function implementation.
> Replace two calls by goto statements.
>
> This issue was detected by using the Coccinelle software.

I don't think this is an issue or an improvement.

BR,
Jani.

>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/i915/gvt/cmd_parser.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> index 2c0ccbb817dc..caa181380958 100644
> --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> @@ -2640,10 +2640,9 @@ static int shadow_workload_ring_buffer(struct intel_vgpu_workload *workload)
>  	if (gma_head > gma_tail) {
>  		ret = copy_gma_to_hva(vgpu, vgpu->gtt.ggtt_mm,
>  				      gma_head, gma_top, shadow_ring_buffer_va);
> -		if (ret < 0) {
> -			gvt_vgpu_err("fail to copy guest ring buffer\n");
> -			return ret;
> -		}
> +		if (ret < 0)
> +			goto report_failure;
> +
>  		shadow_ring_buffer_va += ret;
>  		gma_head = workload->rb_start;
>  	}
> @@ -2651,11 +2650,14 @@ static int shadow_workload_ring_buffer(struct intel_vgpu_workload *workload)
>  	/* copy head or start <-> tail */
>  	ret = copy_gma_to_hva(vgpu, vgpu->gtt.ggtt_mm, gma_head, gma_tail,
>  				shadow_ring_buffer_va);
> -	if (ret < 0) {
> -		gvt_vgpu_err("fail to copy guest ring buffer\n");
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto report_failure;
> +
>  	return 0;
> +
> +report_failure:
> +	gvt_vgpu_err("fail to copy guest ring buffer\n");
> +	return ret;
>  }
>  
>  int intel_gvt_scan_and_shadow_ringbuffer(struct intel_vgpu_workload *workload)

-- 
Jani Nikula, Intel Open Source Technology Center

Re: drm/i915/gvt: Use common error handling code in shadow_workload_ring_buffer()

[SF Markus Elfring]

>> Add a jump target so that a call of the function "gvt_vgpu_err" is stored
>> only once at the end of this function implementation.
>> Replace two calls by goto statements.
>>
>> This issue was detected by using the Coccinelle software.
> 
> I don't think this is an issue or an improvement.

Do you care for the detail how often an error message is stored in the code?

Regards,
Markus

Re: drm/i915/gvt: Use common error handling code in shadow_workload_ring_buffer()

[Garry Hurley]

Markus, 

I normally keep quiet on threads like this. I half agree with you. Yes, perhaps a reportError function would be a good idea, but it seems that what you are suggesting is trading an inline subroutine call for a spaghetti-code vondition. The goto is used only if you do not have any code that follows, and what you are taking out is a function call that DOES return execution flow to the calling block. You are replacing it with a goto call which does not return execution flow. The risks of process termination during a goto call make it a bad idea. In this case, the subroutine call is the right move. If the reuse of the error message bothers you, perhaps it is a good candidate for a static string definition, so that if it is changed it can be changed in one location in the code. The tradeoff there is that the static string definition will eat a few bytes of memory from the variable storage. Just my perspective. I could be wrong. 

Sent from my iPhone

On Oct 24, 2017, at 9:17 AM, SF Markus Elfring <elfring@users.sourceforge.net> wrote:

>>> Add a jump target so that a call of the function "gvt_vgpu_err" is stored
>>> only once at the end of this function implementation.
>>> Replace two calls by goto statements.
>>> 
>>> This issue was detected by using the Coccinelle software.
>> 
>> I don't think this is an issue or an improvement.
> 
> Do you care for the detail how often an error message is stored in the code?
> 
> Regards,
> Markus
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: drm/i915/gvt: Use common error handling code in shadow_workload_ring_buffer()

[Dan Carpenter]

The point of unwind code is to undo what was done earlier.  If a
function allocates a list of things, using standard unwind style makes
it simpler, safer and more readable.

This isn't the case here.  Instead of making the code more readable,
we're making it more convoluted.  It's just that two out of three error
messages happened to be the same and Markus wants to save a bit of
memory by using the same string.  The memory savings is not so big that
it's worth making the code less readable.

regards,
dan carpenter

Re: drm/i915/gvt: Use common error handling code in shadow_workload_ring_buffer()

[SF Markus Elfring]

> This isn't the case here.

I find your view interesting for further clarification somehow.


> Instead of making the code more readable, we're making it more convoluted.

Can the shown software refactoring usually help here?


> It's just that two out of three error messages happened to be the same

This is true.


> and Markus wants to save a bit of memory by using the same string.

And also the same executable code (besides an identical error message).


> The memory savings is not so big that it's worth making the code less readable.

How does such a feedback fit to information for the deletion of questionable
messages at other source code places?

Regards,
Markus

Re: drm/i915/gvt: Use common error handling code in shadow_workload_ring_buffer()

[Joe Perches]

On Tue, 2017-10-24 at 17:26 +0300, Dan Carpenter wrote:
> The point of unwind code is to undo what was done earlier.  If a
> function allocates a list of things, using standard unwind style makes
> it simpler, safer and more readable.
> 
> This isn't the case here.  Instead of making the code more readable,
> we're making it more convoluted.  It's just that two out of three error
> messages happened to be the same and Markus wants to save a bit of
> memory by using the same string.  The memory savings is not so big that
> it's worth making the code less readable.

I agree with Dan.

It doesn't save any real memory either as the compiler/linker
reuses the repeated string.

It might, depending on the compiler, save a few bytes of
object code as the compiler may not optimize the repeated
call away though.  But a good compiler could do that too.

Re: drm/i915/gvt: Use common error handling code in shadow_workload_ring_buffer()

[SF Markus Elfring]

>> …  It's just that two out of three error
>> messages happened to be the same and Markus wants to save a bit of
>> memory by using the same string.  The memory savings is not so big that
>> it's worth making the code less readable.
> 
> I agree with Dan.
> 
> It doesn't save any real memory either as the compiler/linker
> reuses the repeated string.

It might depend on passing appropriate parameters.


> It might, depending on the compiler, save a few bytes of
> object code as the compiler may not optimize the repeated
> call away though.

I am trying to show corresponding change possibilities.


> But a good compiler could do that too.

Do you prefer to delegate the proposed software refactoring
only to a corresponding optimiser?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: drm/i915/gvt: Use common error handling code in shadow_workload_ring_buffer()

[Joe Perches]

On Tue, 2017-10-24 at 16:51 +0200, SF Markus Elfring wrote:
> Do you prefer to delegate the proposed software refactoring
> only to a corresponding optimiser?

yes.

Re: drm/i915/gvt: Use common error handling code in shadow_workload_ring_buffer()

[SF Markus Elfring]

>> Do you prefer to delegate the proposed software refactoring
>> only to a corresponding optimiser?
> 
> yes.

Will any applications around the semantic patch language
(Coccinelle software) fit also in the preferred tool category?

Regards,
Markus

Re: drm/i915/gvt: Use common error handling code in shadow_workload_ring_buffer()

[Daniele Nicolodi]

On 10/24/17 9:01 AM, SF Markus Elfring wrote:
>>> Do you prefer to delegate the proposed software refactoring
>>> only to a corresponding optimiser?
>>
>> yes.
> 
> Will any applications around the semantic patch language
> (Coccinelle software) fit also in the preferred tool category?

What do you think of quantum computing as a solution instead?

Cheers,
Dan

RE: [PATCH] drm/i915/gvt: Use common error handling code in shadow_workload_ring_buffer()

[Wang, Zhi A]

VGhhbmtzIGZvciB0aGUgcGF0Y2ghIEFjdHVhbGx5IEkgd291bGQgcHJlZmVyIHRoYXQgd2UgY2Fu
IGhhdmUgZGlmZmVyZW50IGVycm9yIG1lc3NhZ2VzIGhlcmUuIEUuZy4gbGlrZSBjb3B5X2dtYV90
b19odmEgZmFpbCBpbiB3aGljaCBwYXJ0IG9mIHJpbmcgYnVmZmVyLiBPciB3ZSBjYW4gZGlyZWN0
bHkgcmVtb3ZlIHRoZSBlcnJvciBtZXNzYWdlIHNpbmNlIHVzdWFsbHkgaWYgaXQgZmFpbHMgaXQg
bWVhbnMgYSBidWcgb2YgTVBUIG1vZHVsZSB3aGVuIEdWVC1nIGlzIGFjY2Vzc2luZyB0aGUgZ3Vl
c3QgbWVtb3J5IGJ5IFhlbi9LVk0gZnVuY3Rpb25zIGFuZCBpdCBzaG91bGRu4oCZdCBoYXBwZW4g
dW5sZXNzIE1QVCBtb2R1bGVzIGhhcyBhIGJ1ZywgOiggc28gSSB3aXNoIHRoZSBlcnJvciBtZXNz
YWdlIGNhbiBiZSByZXBvcnRlZCBpbiBNUFQgbW9kdWxlLiA6KQ0KDQpUaGFua3MgZm9yIHRoZSBw
YXRjaC4gIDopDQoNClRoYW5rcywNClpoaS4NCg0KLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0N
CkZyb206IFNGIE1hcmt1cyBFbGZyaW5nIFttYWlsdG86ZWxmcmluZ0B1c2Vycy5zb3VyY2Vmb3Jn
ZS5uZXRdIA0KU2VudDogVHVlc2RheSwgT2N0b2JlciAyNCwgMjAxNyAzOjI2IFBNDQpUbzogZHJp
LWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZzsgaW50ZWwtZ2Z4QGxpc3RzLmZyZWVkZXNrdG9w
Lm9yZzsgaW50ZWwtZ3Z0LWRldkBsaXN0cy5mcmVlZGVza3RvcC5vcmc7IERhdmlkIEFpcmxpZSA8
YWlybGllZEBsaW51eC5pZT47IEphbmkgTmlrdWxhIDxqYW5pLm5pa3VsYUBsaW51eC5pbnRlbC5j
b20+OyBKb29uYXMgTGFodGluZW4gPGpvb25hcy5sYWh0aW5lbkBsaW51eC5pbnRlbC5jb20+OyBW
aXZpLCBSb2RyaWdvIDxyb2RyaWdvLnZpdmlAaW50ZWwuY29tPjsgWmhlbnl1IFdhbmcgPHpoZW55
dXdAbGludXguaW50ZWwuY29tPjsgV2FuZywgWmhpIEEgPHpoaS5hLndhbmdAaW50ZWwuY29tPg0K
Q2M6IExLTUwgPGxpbnV4LWtlcm5lbEB2Z2VyLmtlcm5lbC5vcmc+OyBrZXJuZWwtamFuaXRvcnNA
dmdlci5rZXJuZWwub3JnDQpTdWJqZWN0OiBbUEFUQ0hdIGRybS9pOTE1L2d2dDogVXNlIGNvbW1v
biBlcnJvciBoYW5kbGluZyBjb2RlIGluIHNoYWRvd193b3JrbG9hZF9yaW5nX2J1ZmZlcigpDQoN
CkZyb206IE1hcmt1cyBFbGZyaW5nIDxlbGZyaW5nQHVzZXJzLnNvdXJjZWZvcmdlLm5ldD4NCkRh
dGU6IFR1ZSwgMjQgT2N0IDIwMTcgMTQ6MjA6MDYgKzAyMDANCg0KQWRkIGEganVtcCB0YXJnZXQg
c28gdGhhdCBhIGNhbGwgb2YgdGhlIGZ1bmN0aW9uICJndnRfdmdwdV9lcnIiIGlzIHN0b3JlZCBv
bmx5IG9uY2UgYXQgdGhlIGVuZCBvZiB0aGlzIGZ1bmN0aW9uIGltcGxlbWVudGF0aW9uLg0KUmVw
bGFjZSB0d28gY2FsbHMgYnkgZ290byBzdGF0ZW1lbnRzLg0KDQpUaGlzIGlzc3VlIHdhcyBkZXRl
Y3RlZCBieSB1c2luZyB0aGUgQ29jY2luZWxsZSBzb2Z0d2FyZS4NCg0KU2lnbmVkLW9mZi1ieTog
TWFya3VzIEVsZnJpbmcgPGVsZnJpbmdAdXNlcnMuc291cmNlZm9yZ2UubmV0Pg0KLS0tDQogZHJp
dmVycy9ncHUvZHJtL2k5MTUvZ3Z0L2NtZF9wYXJzZXIuYyB8IDE4ICsrKysrKysrKystLS0tLS0t
LQ0KIDEgZmlsZSBjaGFuZ2VkLCAxMCBpbnNlcnRpb25zKCspLCA4IGRlbGV0aW9ucygtKQ0KDQpk
aWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvZ3Z0L2NtZF9wYXJzZXIuYyBiL2RyaXZl
cnMvZ3B1L2RybS9pOTE1L2d2dC9jbWRfcGFyc2VyLmMNCmluZGV4IDJjMGNjYmI4MTdkYy4uY2Fh
MTgxMzgwOTU4IDEwMDY0NA0KLS0tIGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvZ3Z0L2NtZF9wYXJz
ZXIuYw0KKysrIGIvZHJpdmVycy9ncHUvZHJtL2k5MTUvZ3Z0L2NtZF9wYXJzZXIuYw0KQEAgLTI2
NDAsMTAgKzI2NDAsOSBAQCBzdGF0aWMgaW50IHNoYWRvd193b3JrbG9hZF9yaW5nX2J1ZmZlcihz
dHJ1Y3QgaW50ZWxfdmdwdV93b3JrbG9hZCAqd29ya2xvYWQpDQogCWlmIChnbWFfaGVhZCA+IGdt
YV90YWlsKSB7DQogCQlyZXQgPSBjb3B5X2dtYV90b19odmEodmdwdSwgdmdwdS0+Z3R0LmdndHRf
bW0sDQogCQkJCSAgICAgIGdtYV9oZWFkLCBnbWFfdG9wLCBzaGFkb3dfcmluZ19idWZmZXJfdmEp
Ow0KLQkJaWYgKHJldCA8IDApIHsNCi0JCQlndnRfdmdwdV9lcnIoImZhaWwgdG8gY29weSBndWVz
dCByaW5nIGJ1ZmZlclxuIik7DQotCQkJcmV0dXJuIHJldDsNCi0JCX0NCisJCWlmIChyZXQgPCAw
KQ0KKwkJCWdvdG8gcmVwb3J0X2ZhaWx1cmU7DQorDQogCQlzaGFkb3dfcmluZ19idWZmZXJfdmEg
Kz0gcmV0Ow0KIAkJZ21hX2hlYWQgPSB3b3JrbG9hZC0+cmJfc3RhcnQ7DQogCX0NCkBAIC0yNjUx
LDExICsyNjUwLDE0IEBAIHN0YXRpYyBpbnQgc2hhZG93X3dvcmtsb2FkX3JpbmdfYnVmZmVyKHN0
cnVjdCBpbnRlbF92Z3B1X3dvcmtsb2FkICp3b3JrbG9hZCkNCiAJLyogY29weSBoZWFkIG9yIHN0
YXJ0IDwtPiB0YWlsICovDQogCXJldCA9IGNvcHlfZ21hX3RvX2h2YSh2Z3B1LCB2Z3B1LT5ndHQu
Z2d0dF9tbSwgZ21hX2hlYWQsIGdtYV90YWlsLA0KIAkJCQlzaGFkb3dfcmluZ19idWZmZXJfdmEp
Ow0KLQlpZiAocmV0IDwgMCkgew0KLQkJZ3Z0X3ZncHVfZXJyKCJmYWlsIHRvIGNvcHkgZ3Vlc3Qg
cmluZyBidWZmZXJcbiIpOw0KLQkJcmV0dXJuIHJldDsNCi0JfQ0KKwlpZiAocmV0IDwgMCkNCisJ
CWdvdG8gcmVwb3J0X2ZhaWx1cmU7DQorDQogCXJldHVybiAwOw0KKw0KK3JlcG9ydF9mYWlsdXJl
Og0KKwlndnRfdmdwdV9lcnIoImZhaWwgdG8gY29weSBndWVzdCByaW5nIGJ1ZmZlclxuIik7DQor
CXJldHVybiByZXQ7DQogfQ0KIA0KIGludCBpbnRlbF9ndnRfc2Nhbl9hbmRfc2hhZG93X3Jpbmdi
dWZmZXIoc3RydWN0IGludGVsX3ZncHVfd29ya2xvYWQgKndvcmtsb2FkKQ0KLS0NCjIuMTQuMw0K
DQo