| Message ID | 0bcea3cf-91e3-01d5-8d80-34cd6b611fb1@users.sourceforge.net (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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/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; }