[PATCH] IB/mlx4: Use common error handling code in __mlx4_ib_create_flow()

[PATCH] IB/mlx4: Use common error handling code in __mlx4_ib_create_flow()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 26 Oct 2017 17:54:15 +0200

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/infiniband/hw/mlx4/main.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index c636842c5be0..4a598c48ea1c 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -1691,8 +1691,8 @@ static int __mlx4_ib_create_flow(struct ib_qp *qp, struct ib_flow_attr *flow_att
 				mdev, qp, default_table + default_flow,
 				mailbox->buf + size);
 		if (ret < 0) {
-			mlx4_free_cmd_mailbox(mdev->dev, mailbox);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto free_mailbox;
 		}
 		size += ret;
 	}
@@ -1700,8 +1700,8 @@ static int __mlx4_ib_create_flow(struct ib_qp *qp, struct ib_flow_attr *flow_att
 		ret = parse_flow_attr(mdev->dev, qp->qp_num, ib_flow,
 				      mailbox->buf + size);
 		if (ret < 0) {
-			mlx4_free_cmd_mailbox(mdev->dev, mailbox);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto free_mailbox;
 		}
 		ib_flow += ((union ib_flow_spec *) ib_flow)->size;
 		size += ret;
@@ -1726,7 +1726,7 @@ static int __mlx4_ib_create_flow(struct ib_qp *qp, struct ib_flow_attr *flow_att
 		pr_err("Device managed flow steering is disabled. Fail to register network rule.\n");
 	else if (ret)
 		pr_err("Invalid argument. Fail to register network rule.\n");
-
+free_mailbox:
 	mlx4_free_cmd_mailbox(mdev->dev, mailbox);
 	return ret;
 }
-- 
2.14.3

Re: [PATCH] IB/mlx4: Use common error handling code in __mlx4_ib_create_flow()

[Dennis Dalessandro]

On 10/26/2017 12:12 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 26 Oct 2017 17:54:15 +0200
> 
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function.

I'm not sure this is that big of a win. I mean you aren't really making 
the code any smaller and it's not making it any easier to read really.

> This issue was detected by using the Coccinelle software.

Assuming there was no testing done for this?

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/infiniband/hw/mlx4/main.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
> index c636842c5be0..4a598c48ea1c 100644
> --- a/drivers/infiniband/hw/mlx4/main.c
> +++ b/drivers/infiniband/hw/mlx4/main.c
> @@ -1691,8 +1691,8 @@ static int __mlx4_ib_create_flow(struct ib_qp *qp, struct ib_flow_attr *flow_att
>   				mdev, qp, default_table + default_flow,
>   				mailbox->buf + size);
>   		if (ret < 0) {
> -			mlx4_free_cmd_mailbox(mdev->dev, mailbox);
> -			return -EINVAL;
> +			ret = -EINVAL;
> +			goto free_mailbox;

This might be a good opportunity to bubble up the return value rather 
than just blindly returning -EINVAL. That's really a question for 
Mellanox though.

-Denny

Re: IB/mlx4: Use common error handling code in __mlx4_ib_create_flow()

[SF Markus Elfring]

>> Add a jump target so that a bit of exception handling can be better reused
>> at the end of this function.
> 
> I'm not sure this is that big of a win.

Such a view is appropriate because I proposed just another small adjustment
for this source code place.


> I mean you aren't really making the code any smaller

Would anybody like to check corresponding effects in more detail
after a specific function call was replaced by a goto statement?


> and it's not making it any easier to read really.

Is the code readability still good enough there?

Regards,
Markus

Re: IB/mlx4: Use common error handling code in __mlx4_ib_create_flow()

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 716 bytes --]

On Fri, Oct 27, 2017 at 09:34:18AM +0200, SF Markus Elfring wrote:
> >> Add a jump target so that a bit of exception handling can be better reused
> >> at the end of this function.
> >
> > I'm not sure this is that big of a win.
>
> Such a view is appropriate because I proposed just another small adjustment
> for this source code place.
>
>
> > I mean you aren't really making the code any smaller
>
> Would anybody like to check corresponding effects in more detail
> after a specific function call was replaced by a goto statement?

You are supposed to do it and not "anybody".

>
>
> > and it's not making it any easier to read really.
>
> Is the code readability still good enough there?
>
> Regards,
> Markus

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] IB/mlx4: Use common error handling code in __mlx4_ib_create_flow()

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 713 bytes --]

On Thu, Oct 26, 2017 at 06:12:31PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 26 Oct 2017 17:54:15 +0200
>
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/infiniband/hw/mlx4/main.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>

Doug,

I would like to refresh my request, please don't take ANY patches
for mlx4/mlx5/rxe from Markus without explicit acknowledge from our
side.

This change adds nothing except code churn.

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] IB/mlx4: Use common error handling code in __mlx4_ib_create_flow()

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]

On Thu, Oct 26, 2017 at 08:33:39PM -0400, Dennis Dalessandro wrote:
> On 10/26/2017 12:12 PM, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Thu, 26 Oct 2017 17:54:15 +0200
> >
> > Add a jump target so that a bit of exception handling can be better reused
> > at the end of this function.
>
> I'm not sure this is that big of a win. I mean you aren't really making the
> code any smaller and it's not making it any easier to read really.
>
> > This issue was detected by using the Coccinelle software.
>
> Assuming there was no testing done for this?
>
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >   drivers/infiniband/hw/mlx4/main.c | 10 +++++-----
> >   1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
> > index c636842c5be0..4a598c48ea1c 100644
> > --- a/drivers/infiniband/hw/mlx4/main.c
> > +++ b/drivers/infiniband/hw/mlx4/main.c
> > @@ -1691,8 +1691,8 @@ static int __mlx4_ib_create_flow(struct ib_qp *qp, struct ib_flow_attr *flow_att
> >   				mdev, qp, default_table + default_flow,
> >   				mailbox->buf + size);
> >   		if (ret < 0) {
> > -			mlx4_free_cmd_mailbox(mdev->dev, mailbox);
> > -			return -EINVAL;
> > +			ret = -EINVAL;
> > +			goto free_mailbox;
>
> This might be a good opportunity to bubble up the return value rather than
> just blindly returning -EINVAL. That's really a question for Mellanox
> though.

It is worth to check it, especially for parse_flow_attr() which can
return -ENOTSUPP (need to fix return -EOPNOTSUPP).

Thanks


>
> -Denny

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: IB/mlx4: Use common error handling code in __mlx4_ib_create_flow()

[SF Markus Elfring]

> This change adds nothing except code churn.

I guess that the shown change possibility can reduce the object code size
for the affected function.
Do you care for such an detail?

Regards,
Markus

Re: IB/mlx4: Use common error handling code in __mlx4_ib_create_flow()

[Dennis Dalessandro]

On 10/27/2017 4:53 PM, SF Markus Elfring wrote:
>> This change adds nothing except code churn.
> 
> I guess that the shown change possibility can reduce the object code size
> for the affected function.
> Do you care for such an detail?
> 
> Regards,
> Markus
> 

You guess? Well perhaps you should find out for certain. Is it an 
appreciable impact?

-Denny

Re: IB/mlx4: Use common error handling code in __mlx4_ib_create_flow()

[SF Markus Elfring]

>> I guess that the shown change possibility can reduce the object code size
>> for the affected function.
…
> You guess?

I am convinced somehow!

 
> Well perhaps you should find out for certain.

I am trying to point another general implementation detail out:
A jump to an existing call of a function like “mlx4_free_cmd_mailbox”
can be useful if you would like to optimise also this software for
smaller code size.

Are you looking for an information source which you would trust
more (than me)?


> Is it an appreciable impact?

I hope so.

But I showed only the replacement of two function calls here.
I am curious if you care for a small effect at a special place.

A similar refactoring can have a bigger influence in other
software modules, can't it?


There might be an other useful side effect. My concrete proposal
can be questionable as usual.
It seems that the software development attention was increased
a bit so that contributors started thinking about the relevance
of the error code “-EINVAL” at another source code place again.

Regards,
Markus

Re: IB/mlx4: Use common error handling code in __mlx4_ib_create_flow()

[SF Markus Elfring]

>>> I mean you aren't really making the code any smaller
>>
>> Would anybody like to check corresponding effects in more detail
>> after a specific function call was replaced by a goto statement?
> 
> You are supposed to do it and not "anybody".

I can offer another bit of information for this software development discussion.

The following build settings were active in my “Makefile” for this Linux test case.

…
HOSTCFLAGS   = -Wall -Wmissing-prototypes -Wstrict-prototypes -O0 -fomit-frame-pointer -std=gnu89
…


The affected source file can be compiled for the processor architecture “x86_64”
by a tool like “GCC 6.4.1+r251631-1.3” from the software distribution
“openSUSE Tumbleweed” with the following command example.

my_cc=/usr/bin/gcc-6 \
&& my_module=drivers/infiniband/hw/mlx4/main.o \
&& git checkout next-20171009 \
&& make -j4 CC="${my_cc}" HOSTCC="${my_cc}" allmodconfig "${my_module}" \
&& size "${my_module}" \
&& git checkout next_fine-tuning_in_mlx4_1 \
&& make -j4 CC="${my_cc}" HOSTCC="${my_cc}" allmodconfig "${my_module}" \
&& size "${my_module}"


Do you find the following details useful for further clarification?

text: -32
data: 0
bss:  0

Regards,
Markus

Re: IB/mlx4: Use common error handling code in __mlx4_ib_create_flow()

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]

On Mon, Oct 30, 2017 at 09:04:36AM +0100, SF Markus Elfring wrote:
> >>> I mean you aren't really making the code any smaller
> >>
> >> Would anybody like to check corresponding effects in more detail
> >> after a specific function call was replaced by a goto statement?
> >
> > You are supposed to do it and not "anybody".
>
> I can offer another bit of information for this software development discussion.
>
> The following build settings were active in my “Makefile” for this Linux test case.
>
> …
> HOSTCFLAGS   = -Wall -Wmissing-prototypes -Wstrict-prototypes -O0 -fomit-frame-pointer -std=gnu89
> …
>
>
> The affected source file can be compiled for the processor architecture “x86_64”
> by a tool like “GCC 6.4.1+r251631-1.3” from the software distribution
> “openSUSE Tumbleweed” with the following command example.
>
> my_cc=/usr/bin/gcc-6 \
> && my_module=drivers/infiniband/hw/mlx4/main.o \
> && git checkout next-20171009 \
> && make -j4 CC="${my_cc}" HOSTCC="${my_cc}" allmodconfig "${my_module}" \
> && size "${my_module}" \
> && git checkout next_fine-tuning_in_mlx4_1 \
> && make -j4 CC="${my_cc}" HOSTCC="${my_cc}" allmodconfig "${my_module}" \
> && size "${my_module}"
>
>
> Do you find the following details useful for further clarification?

Almost, you should compare sizes of mlx4_ib.ko and not drivers/infiniband/hw/mlx4/main.o.

Thanks

>
> text: -32
> data: 0
> bss:  0
>
> Regards,
> Markus

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: IB/mlx4: Use common error handling code in __mlx4_ib_create_flow()

[SF Markus Elfring]

>> Do you find the following details useful for further clarification?
> 
> Almost, you should compare sizes of mlx4_ib.ko and not drivers/infiniband/hw/mlx4/main.o.

text: -32
data: 0
bss:  0


The determined difference is the same finally.

Regards,
Markus