[PATCH] net-thunder: One check less in nicvf_register_interrupts() after error detection

From: Markus Elfring <redacted>
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".

Signed-off-by: Markus Elfring <redacted>
---
 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;
 }
 

-- 
2.6.3

On 31.12.15 22:47:31, SF Markus Elfring wrote:

From: Markus Elfring <redacted>
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

quoted

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?

quoted

@@ -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:

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

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:

quoted

quoted

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.

quoted

* 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:

quoted

quoted

* 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:

quoted

Some prefer that source code be "templatized" regardless
of the number of exit points that any particular use of a
specific function type.

[]

quoted

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.

quoted

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.