[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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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
_______________________________________________
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()

[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
_______________________________________________
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
_______________________________________________
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()

[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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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]

Thanks for the patch! Actually I would prefer that we can have different error messages here. E.g. like copy_gma_to_hva fail in which part of ring buffer. Or we can directly remove the error message since usually if it fails it means a bug of MPT module when GVT-g is accessing the guest memory by Xen/KVM functions and it shouldn’t happen unless MPT modules has a bug, :( so I wish the error message can be reported in MPT module. :)

Thanks for the patch.  :)

Thanks,
Zhi.

-----Original Message-----
From: SF Markus Elfring [mailto:elfring@users.sourceforge.net] 
Sent: Tuesday, October 24, 2017 3:26 PM
To: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org; David Airlie <airlied@linux.ie>; Jani Nikula <jani.nikula@linux.intel.com>; Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Zhenyu Wang <zhenyuw@linux.intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>; kernel-janitors@vger.kernel.org
Subject: [PATCH] drm/i915/gvt: Use common error handling code in shadow_workload_ring_buffer()

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx