| Message ID | 5685A273.6070607@users.sourceforge.net |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On 31.12.15 22:47:31, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Thu, 31 Dec 2015 22:40:39 +0100 > > Adjust a jump target to eliminate a check before error logging. > Use the identifier "report_failure" instead of "err". I don't see much value in those changes. Using the 'err' label is ok as it is not misleading and common use. And, there is no need to optimize the check since this is not the fast path and will be compiler optimized anyway. So let's keep the code as it is with the flavor of the original author. -Robert > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/net/ethernet/cavium/thunder/nicvf_main.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c > index c24cb2a..21e1579 100644 > --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c > +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c > @@ -922,7 +922,7 @@ static int nicvf_register_interrupts(struct nicvf *nic) > ret = request_irq(vector, nicvf_intr_handler, > 0, nic->irq_name[irq], nic->napi[irq]); > if (ret) > - goto err; > + goto report_failure; > nic->irq_allocated[irq] = true; > } > > @@ -933,7 +933,7 @@ static int nicvf_register_interrupts(struct nicvf *nic) > ret = request_irq(vector, nicvf_rbdr_intr_handler, > 0, nic->irq_name[irq], nic); > if (ret) > - goto err; > + goto report_failure; > nic->irq_allocated[irq] = true; > } > > @@ -944,13 +944,12 @@ static int nicvf_register_interrupts(struct nicvf *nic) > ret = request_irq(nic->msix_entries[irq].vector, > nicvf_qs_err_intr_handler, > 0, nic->irq_name[irq], nic); > - if (!ret) > + if (!ret) { > nic->irq_allocated[irq] = true; > - > -err: > - if (ret) > - netdev_err(nic->netdev, "request_irq failed, vector %d\n", irq); > - > + return 0; > + } > +report_failure: > + netdev_err(nic->netdev, "request_irq failed, vector %d\n", irq); > return ret; > } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Adjust a jump target to eliminate a check before error logging. >> Use the identifier "report_failure" instead of "err". > > I don't see much value in those changes. Thanks for your feedback. > Using the 'err' label is ok as it is not misleading and common use. Is such a short jump label enough explanation for the information "what" and "why"? > And, there is no need to optimize the check since this is not the fast path Really? - Is it a bit more efficient to avoid a double check for the variable "ret" at the end of the current implementation for the discussed function? https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/net/ethernet/cavium/thunder/nicvf_main.c?id=40fb5f8a60f33133d36afde35a9ad865d35e4423#n940 > and will be compiler optimized anyway. How sure are you about automatic software optimisations? Can it occasionally help to jump to the really intended source code location directly? >> @@ -944,13 +944,12 @@ static int nicvf_register_interrupts(struct nicvf *nic) >> ret = request_irq(nic->msix_entries[irq].vector, >> nicvf_qs_err_intr_handler, >> 0, nic->irq_name[irq], nic); >> - if (!ret) >> + if (!ret) { >> nic->irq_allocated[irq] = true; >> - >> -err: >> - if (ret) >> - netdev_err(nic->netdev, "request_irq failed, vector %d\n", irq); >> - >> + return 0; >> + } >> +report_failure: >> + netdev_err(nic->netdev, "request_irq failed, vector %d\n", irq); >> return ret; >> }
On Thu, 2016-01-07 at 20:30 +0100, SF Markus Elfring wrote: > > > Adjust a jump target to eliminate a check before error logging. > > > Use the identifier "report_failure" instead of "err". > > I don't see much value in those changes > Thanks for your feedback. > > Using the 'err' label is ok as it is not misleading and common use. > Is such a short jump label enough explanation for the information > "what" and "why"? When there is only one type of error possible, yes. > > And, there is no need to optimize the check since this is not the > > fast path > Really? - Is it a bit more efficient to avoid a double check for the > variable "ret" at the end of the current implementation for the > discussed function? Before asking questions you could answer yourself, please look at object code produced by the compiler before and after your proposed changes.
>> Is it a bit more efficient to avoid a double check for the >> variable "ret" at the end of the current implementation for the >> discussed function? > > Before asking questions you could answer yourself, > please look at object code produced by the compiler > before and after your proposed changes. * Do any more source code reviewers wonder about the need for such a double check? * Which object code representations would you find representative for a further constructive discussion around this software component? Regards, Markus
On Thu, 2016-01-07 at 20:56 +0100, SF Markus Elfring wrote: > > > Is it a bit more efficient to avoid a double check for the > > > variable "ret" at the end of the current implementation for the > > > discussed function? > > > > Before asking questions you could answer yourself, > > please look at object code produced by the compiler > > before and after your proposed changes. > > * Do any more source code reviewers wonder about the need > for such a double check? Given the feedback you've already received, it seems so. > * Which object code representations would you find representative > for a further constructive discussion around this > software component? Evidence of actual object code improvement when with compiled with optimizations.
>> * Which object code representations would you find representative >> for a further constructive discussion around this >> software component? > > Evidence of actual object code improvement How do you think about to provide a function implementation which looks a bit more efficient by default? > when with compiled with optimizations. Which combinations of hardware and software would you recommend for corresponding system checks? Regards, Markus
On Thu, 2016-01-07 at 21:07 +0100, SF Markus Elfring wrote: > > > * Which object code representations would you find representative > > > for a further constructive discussion around this > > > software component? > > > > Evidence of actual object code improvement > > How do you think about to provide a function implementation > which looks a bit more efficient by default? It's not a matter of "looks a bit more efficient". it's taste, style, and repetition for various functions. Some prefer that source code be "templatized" regardless of the number of exit points that any particular use of a specific function type. Some of your patches are converting these templatized functions to a different form for no added value. These patches make the local source code inconsistent and generally goes against the authors preferred style.
> Some prefer that source code be "templatized" regardless > of the number of exit points that any particular use of a > specific function type. This is another interesting view on involved implementation details. > Some of your patches are converting these templatized > functions to a different form for no added value. Would you like to distinguish a bit more between my evolving collection of update suggestions and the concrete proposal for the function "nicvf_register_interrupts"? > These patches make the local source code inconsistent > and generally goes against the authors preferred style. Which programming approach will be the leading one here finally? Regards, Markus
On Thu, 2016-01-07 at 21:38 +0100, SF Markus Elfring wrote: > > Some prefer that source code be "templatized" regardless > > of the number of exit points that any particular use of a > > specific function type. [] > > Some of your patches are converting these templatized > > functions to a different form for no added value. > > Would you like to distinguish a bit more between my evolving > collection of update suggestions and the concrete proposal > for the function "nicvf_register_interrupts"? No. > > These patches make the local source code inconsistent > > and generally goes against the authors preferred style. > > Which programming approach will be the leading one here finally? Whatever the developer wants. There is no _best_ or _only_ style for this.
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c index c24cb2a..21e1579 100644 --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c @@ -922,7 +922,7 @@ static int nicvf_register_interrupts(struct nicvf *nic) ret = request_irq(vector, nicvf_intr_handler, 0, nic->irq_name[irq], nic->napi[irq]); if (ret) - goto err; + goto report_failure; nic->irq_allocated[irq] = true; } @@ -933,7 +933,7 @@ static int nicvf_register_interrupts(struct nicvf *nic) ret = request_irq(vector, nicvf_rbdr_intr_handler, 0, nic->irq_name[irq], nic); if (ret) - goto err; + goto report_failure; nic->irq_allocated[irq] = true; } @@ -944,13 +944,12 @@ static int nicvf_register_interrupts(struct nicvf *nic) ret = request_irq(nic->msix_entries[irq].vector, nicvf_qs_err_intr_handler, 0, nic->irq_name[irq], nic); - if (!ret) + if (!ret) { nic->irq_allocated[irq] = true; - -err: - if (ret) - netdev_err(nic->netdev, "request_irq failed, vector %d\n", irq); - + return 0; + } +report_failure: + netdev_err(nic->netdev, "request_irq failed, vector %d\n", irq); return ret; }