| Message ID | 47f88a11-b949-28ed-5589-925888a37574@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> From: Markus Elfring <elfring@users.sourceforge.net> > Date: Wed, 17 Aug 2016 19:25:50 +0200 > > The kfree() function was called in a few cases by the > kvm_s390_import_bp_data() function during error handling > even if a passed variable contained a null pointer. > > Adjust jump targets according to the Linux coding style convention. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > arch/s390/kvm/guestdbg.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > Nack, we don't need micro optimization for error handling code. Adding more jump labels is never a good idea, it just increases complexity. David -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 17 Aug 2016 20:10:37 +0200 SF Markus Elfring <elfring@users.sourceforge.net> wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Wed, 17 Aug 2016 19:25:50 +0200 > > The kfree() function was called in a few cases by the > kvm_s390_import_bp_data() function during error handling > even if a passed variable contained a null pointer. > > Adjust jump targets according to the Linux coding style convention. NACK. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > arch/s390/kvm/guestdbg.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, > vcpu->arch.guestdbg.nr_hw_wp = nr_wp; > vcpu->arch.guestdbg.hw_wp_info = wp_info; > return 0; > -error: > - kfree(bp_data); > - kfree(wp_info); > +free_bp_info: > kfree(bp_info); > +free_wp_info: > + kfree(wp_info); > +free_bp_data: > + kfree(bp_data); > return ret; > } > This replaces a perfectly fine fallthrough with some horrible labels. Please don't do that. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >> vcpu->arch.guestdbg.nr_hw_wp = nr_wp; >> vcpu->arch.guestdbg.hw_wp_info = wp_info; >> return 0; >> -error: >> - kfree(bp_data); >> - kfree(wp_info); >> +free_bp_info: >> kfree(bp_info); >> +free_wp_info: >> + kfree(wp_info); >> +free_bp_data: >> + kfree(bp_data); >> return ret; >> } >> > > This replaces a perfectly fine fallthrough The usage of a single goto label like "error" seems to be convenient. But how do these habits fit to the current Linux coding style convention? > with some horrible labels. Do they explain better which processing steps should be performed for an efficient exception handling in this function implementation? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 22 Aug 2016 18:56:47 +0200 SF Markus Elfring <elfring@users.sourceforge.net> wrote: > >> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, > >> vcpu->arch.guestdbg.nr_hw_wp = nr_wp; > >> vcpu->arch.guestdbg.hw_wp_info = wp_info; > >> return 0; > >> -error: > >> - kfree(bp_data); > >> - kfree(wp_info); > >> +free_bp_info: > >> kfree(bp_info); > >> +free_wp_info: > >> + kfree(wp_info); > >> +free_bp_data: > >> + kfree(bp_data); > >> return ret; > >> } > >> > > > > This replaces a perfectly fine fallthrough > > The usage of a single goto label like "error" seems to be convenient. > But how do these habits fit to the current Linux coding style convention? > > > > with some horrible labels. > > Do they explain better which processing steps should be performed > for an efficient exception handling in this function implementation? *sigh* It's _exception handling_. It does not need to be "efficient", it needs to be easily parsable by humans. If in doubt, the compiler will be _much_ better at optimizing that kind of stuff anyway. So still NACK. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >>>> vcpu->arch.guestdbg.nr_hw_wp = nr_wp; >>>> vcpu->arch.guestdbg.hw_wp_info = wp_info; >>>> return 0; >>>> -error: >>>> - kfree(bp_data); >>>> - kfree(wp_info); >>>> +free_bp_info: >>>> kfree(bp_info); >>>> +free_wp_info: >>>> + kfree(wp_info); >>>> +free_bp_data: >>>> + kfree(bp_data); >>>> return ret; >>>> } >>>> >>> >>> This replaces a perfectly fine fallthrough >> >> The usage of a single goto label like "error" seems to be convenient. >> But how do these habits fit to the current Linux coding style convention? >> >> >>> with some horrible labels. >> >> Do they explain better which processing steps should be performed >> for an efficient exception handling in this function implementation? > > *sigh* > > It's _exception handling_. It does not need to be "efficient", I imagine that run time situations could evolve where software efficiency will also matter for this purpose. > it needs to be easily parsable by humans. I guess that we have got different preferences for this detail. > If in doubt, the compiler will be _much_ better at optimizing > that kind of stuff anyway. Which compiler (or optimizer) implementation is capable to restructure the jump targets for you automatically in the way I propose here? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 22 Aug 2016 23:17:26 +0200 SF Markus Elfring <elfring@users.sourceforge.net> wrote: > >>>> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, > >>>> vcpu->arch.guestdbg.nr_hw_wp = nr_wp; > >>>> vcpu->arch.guestdbg.hw_wp_info = wp_info; > >>>> return 0; > >>>> -error: > >>>> - kfree(bp_data); > >>>> - kfree(wp_info); > >>>> +free_bp_info: > >>>> kfree(bp_info); > >>>> +free_wp_info: > >>>> + kfree(wp_info); > >>>> +free_bp_data: > >>>> + kfree(bp_data); > >>>> return ret; > >>>> } > >>>> > >>> > >>> This replaces a perfectly fine fallthrough > >> > >> The usage of a single goto label like "error" seems to be convenient. > >> But how do these habits fit to the current Linux coding style convention? > >> > >> > >>> with some horrible labels. > >> > >> Do they explain better which processing steps should be performed > >> for an efficient exception handling in this function implementation? > > > > *sigh* > > > > It's _exception handling_. It does not need to be "efficient", > > I imagine that run time situations could evolve where software efficiency > will also matter for this purpose. *major sigh* We can start to optimize error handling that should never run after we fixed every other performance problem that we have. Not earlier. > > > > it needs to be easily parsable by humans. > > I guess that we have got different preferences for this detail. And I'm maintainer for this code. > > > > If in doubt, the compiler will be _much_ better at optimizing > > that kind of stuff anyway. > > Which compiler (or optimizer) implementation is capable to restructure > the jump targets for you automatically in the way I propose here? No, please stop right here. NACK. EOD. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/17/2016 02:10 PM, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Wed, 17 Aug 2016 19:25:50 +0200 > > The kfree() function was called in a few cases by the > kvm_s390_import_bp_data() function during error handling > even if a passed variable contained a null pointer. > > Adjust jump targets according to the Linux coding style convention. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > arch/s390/kvm/guestdbg.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c > index 8f886ee..f2514af 100644 > --- a/arch/s390/kvm/guestdbg.c > +++ b/arch/s390/kvm/guestdbg.c > @@ -239,7 +239,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, > wp_info = kmalloc(size, GFP_KERNEL); > if (!wp_info) { > ret = -ENOMEM; > - goto error; > + goto free_bp_data; > } > } > size = nr_bp * sizeof(*bp_info); > @@ -247,7 +247,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, > bp_info = kmalloc(size, GFP_KERNEL); > if (!bp_info) { > ret = -ENOMEM; > - goto error; > + goto free_wp_info; > } > } > > @@ -257,7 +257,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, > ret = __import_wp_info(vcpu, &bp_data[i], > &wp_info[nr_wp]); > if (ret) > - goto error; > + goto free_bp_info; > nr_wp++; > break; > case KVM_HW_BP: > @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, > vcpu->arch.guestdbg.nr_hw_wp = nr_wp; > vcpu->arch.guestdbg.hw_wp_info = wp_info; > return 0; > -error: > - kfree(bp_data); > - kfree(wp_info); > +free_bp_info: > kfree(bp_info); > +free_wp_info: > + kfree(wp_info); > +free_bp_data: > + kfree(bp_data); > return ret; > } I agree with Cornelia, while it seems correct from a technical point of view, it will make the code harder to maintain. For example if we ever add a new malloc and remove another one over time we would need to reshuffle the labels and this did went wrong several times in the past. Christian -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, >> vcpu->arch.guestdbg.nr_hw_wp = nr_wp; >> vcpu->arch.guestdbg.hw_wp_info = wp_info; >> return 0; >> -error: >> - kfree(bp_data); >> - kfree(wp_info); >> +free_bp_info: >> kfree(bp_info); >> +free_wp_info: >> + kfree(wp_info); >> +free_bp_data: >> + kfree(bp_data); >> return ret; >> } > > I agree with Cornelia, This is generally fine. > while it seems correct from a technical point of view, Thanks for another bit of acknowledgement. > it will make the code harder to maintain. I agree that there some efforts and challenges involved. > For example if we ever add a new malloc and remove another one Do you see any changes coming from this direction? > over time we would need to reshuffle the labels This can occasionally happen, can't it? > and this did went wrong several times in the past. Would you like to add any corresponding software development experiences to discussions around a topic like "CodingStyle: add some more error handling guidelines"? https://www.spinics.net/lists/linux-doc/msg39307.html http://marc.info/?l=linux-doc&m=147187538413914 Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/08/2016 23:17, SF Markus Elfring wrote: >> If in doubt, the compiler will be _much_ better at optimizing >> that kind of stuff anyway. > > Which compiler (or optimizer) implementation is capable to restructure > the jump targets for you automatically in the way I propose here? If kfree were implemented as if (p) really_kfree(p); then the compiler would be able to jump over the NULL test. In principle one could also add a "does nothing if NULL" attribute to GCC and annotate kfree with it. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c index 8f886ee..f2514af 100644 --- a/arch/s390/kvm/guestdbg.c +++ b/arch/s390/kvm/guestdbg.c @@ -239,7 +239,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, wp_info = kmalloc(size, GFP_KERNEL); if (!wp_info) { ret = -ENOMEM; - goto error; + goto free_bp_data; } } size = nr_bp * sizeof(*bp_info); @@ -247,7 +247,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, bp_info = kmalloc(size, GFP_KERNEL); if (!bp_info) { ret = -ENOMEM; - goto error; + goto free_wp_info; } } @@ -257,7 +257,7 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, ret = __import_wp_info(vcpu, &bp_data[i], &wp_info[nr_wp]); if (ret) - goto error; + goto free_bp_info; nr_wp++; break; case KVM_HW_BP: @@ -273,10 +273,12 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu, vcpu->arch.guestdbg.nr_hw_wp = nr_wp; vcpu->arch.guestdbg.hw_wp_info = wp_info; return 0; -error: - kfree(bp_data); - kfree(wp_info); +free_bp_info: kfree(bp_info); +free_wp_info: + kfree(wp_info); +free_bp_data: + kfree(bp_data); return ret; }