Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1303482 > unrolled thread
| Started by | Robert Richter <rric@kernel.org> |
|---|---|
| First post | 2016-01-07 12:10 +0100 |
| Last post | 2016-01-07 21:10 +0100 |
| Articles | 9 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] net-thunder: One check less in nicvf_register_interrupts() after error detection Robert Richter <rric@kernel.org> - 2016-01-07 12:10 +0100
Re: [PATCH] net-thunder: One check less in nicvf_register_interrupts() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-07 20:40 +0100
Re: [PATCH] net-thunder: One check less in nicvf_register_interrupts() after error detection Joe Perches <joe@perches.com> - 2016-01-07 20:50 +0100
Re: net-thunder: One check less in nicvf_register_interrupts() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-07 21:00 +0100
Re: net-thunder: One check less in nicvf_register_interrupts() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-07 21:10 +0100
Re: net-thunder: One check less in nicvf_register_interrupts() after error detection Joe Perches <joe@perches.com> - 2016-01-07 21:30 +0100
Re: net-thunder: One check less in nicvf_register_interrupts() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2016-01-07 21:40 +0100
Re: net-thunder: One check less in nicvf_register_interrupts() after error detection Joe Perches <joe@perches.com> - 2016-01-07 21:50 +0100
Re: net-thunder: One check less in nicvf_register_interrupts() after error detection Joe Perches <joe@perches.com> - 2016-01-07 21:10 +0100
| From | Robert Richter <rric@kernel.org> |
|---|---|
| Date | 2016-01-07 12:10 +0100 |
| Subject | Re: [PATCH] net-thunder: One check less in nicvf_register_interrupts() after error detection |
| Message-ID | <qOgC5-7hc-5@gated-at.bofh.it> |
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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| Date | 2016-01-07 20:40 +0100 |
| Message-ID | <qOozE-4bb-3@gated-at.bofh.it> |
| In reply to | #1303482 |
>> 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;
>> }
[toc] | [prev] | [next] | [standalone]
| From | Joe Perches <joe@perches.com> |
|---|---|
| Date | 2016-01-07 20:50 +0100 |
| Message-ID | <qOoJk-4eE-13@gated-at.bofh.it> |
| In reply to | #1303844 |
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.
[toc] | [prev] | [next] | [standalone]
| From | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| Date | 2016-01-07 21:00 +0100 |
| Subject | Re: net-thunder: One check less in nicvf_register_interrupts() after error detection |
| Message-ID | <qOoT0-4ij-19@gated-at.bofh.it> |
| In reply to | #1303849 |
>> 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
[toc] | [prev] | [next] | [standalone]
| From | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| Date | 2016-01-07 21:10 +0100 |
| Subject | Re: net-thunder: One check less in nicvf_register_interrupts() after error detection |
| Message-ID | <qOp2F-4BD-11@gated-at.bofh.it> |
| In reply to | #1303859 |
>> * 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
[toc] | [prev] | [next] | [standalone]
| From | Joe Perches <joe@perches.com> |
|---|---|
| Date | 2016-01-07 21:30 +0100 |
| Subject | Re: net-thunder: One check less in nicvf_register_interrupts() after error detection |
| Message-ID | <qOpm3-4Jn-5@gated-at.bofh.it> |
| In reply to | #1303860 |
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.
[toc] | [prev] | [next] | [standalone]
| From | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| Date | 2016-01-07 21:40 +0100 |
| Subject | Re: net-thunder: One check less in nicvf_register_interrupts() after error detection |
| Message-ID | <qOpvH-4NZ-1@gated-at.bofh.it> |
| In reply to | #1303868 |
> 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
[toc] | [prev] | [next] | [standalone]
| From | Joe Perches <joe@perches.com> |
|---|---|
| Date | 2016-01-07 21:50 +0100 |
| Subject | Re: net-thunder: One check less in nicvf_register_interrupts() after error detection |
| Message-ID | <qOpFp-4Sh-21@gated-at.bofh.it> |
| In reply to | #1303875 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Joe Perches <joe@perches.com> |
|---|---|
| Date | 2016-01-07 21:10 +0100 |
| Subject | Re: net-thunder: One check less in nicvf_register_interrupts() after error detection |
| Message-ID | <qOp2F-4BD-13@gated-at.bofh.it> |
| In reply to | #1303859 |
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.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web