| Message ID | f563de94-afaa-ecde-2907-d6baaad65939@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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)
>> 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
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
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
> 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
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.
>> … 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
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.
>> 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
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
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)