From: Markus Elfring <[email protected]>
Date: Tue, 7 Nov 2017 08:51:00 +0100
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 <[email protected]>
---
v1 - Request for comments:
I can offer another bit of information for a software development discussion. ????
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=fs/nfs/write.o \
&& for XYZ in 0 s 3; do echo " _____ $XYZ _____" \
&& my_extra="-O$XYZ" \
&& git checkout next-20171102 \
&& make -j4 CC="${my_cc}" HOSTCC="${my_cc}" EXTRA_CFLAGS="${my_extra}" allmodconfig "${my_module}" \
&& size "${my_module}" \
&& git checkout ':/^nfs/write: Use common error handling code in nfs_lock_and_join_requests' \
&& make -j4 CC="${my_cc}" HOSTCC="${my_cc}" EXTRA_CFLAGS="${my_extra}" allmodconfig "${my_module}" \
&& size "${my_module}"; done
???? Do you find the following differences worth for further clarification?
╔═════════╤══════╗
║ setting │ text ║
╠═════════╪══════╣
║ O0 │ ??? ║ Compilation failure?
║ Os │ -17 ║
║ O3 │ -16 ║
╚═════════╧══════╝
fs/nfs/write.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index babebbccae2a..5b5f464f6f2a 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -487,10 +487,8 @@ nfs_lock_and_join_requests(struct page *page)
}
ret = nfs_page_group_lock(head);
- if (ret < 0) {
- nfs_unlock_and_release_request(head);
- return ERR_PTR(ret);
- }
+ if (ret < 0)
+ goto release_request;
/* lock each request in the page group */
total_bytes = head->wb_bytes;
@@ -515,8 +513,7 @@ nfs_lock_and_join_requests(struct page *page)
if (ret < 0) {
nfs_unroll_locks(inode, head, subreq);
nfs_release_request(subreq);
- nfs_unlock_and_release_request(head);
- return ERR_PTR(ret);
+ goto release_request;
}
}
/*
@@ -532,8 +529,8 @@ nfs_lock_and_join_requests(struct page *page)
nfs_page_group_unlock(head);
nfs_unroll_locks(inode, head, subreq);
nfs_unlock_and_release_request(subreq);
- nfs_unlock_and_release_request(head);
- return ERR_PTR(-EIO);
+ ret = -EIO;
+ goto release_request;
}
}
@@ -576,6 +573,10 @@ nfs_lock_and_join_requests(struct page *page)
/* still holds ref on head from nfs_page_find_head_request
* and still has lock on head from lock loop */
return head;
+
+release_request:
+ nfs_unlock_and_release_request(head);
+ return ERR_PTR(ret);
}
static void nfs_write_error_remove_page(struct nfs_page *req)
--
2.15.0
Hello,
I came along some software modules where I suggested source code adjustments.
Example:
nfs/write: Use common error handling code in nfs_lock_and_join_requests()
https://lkml.org/lkml/2017/11/7/599
https://patchwork.kernel.org/patch/10047013/
https://lkml.kernel.org/r/<[email protected]>
I would like to check corresponding build results then without extra
optimisation applied by the compiler.
But I got surprised by error messages for a command like the following.
elfring@Sonne:~/Projekte/Linux/next-patched> my_cc=/usr/bin/gcc-7 && LANG=C make -j4 CC="${my_cc}" HOSTCC="${my_cc}" EXTRA_CFLAGS='-O0' allmodconfig fs/nfs/write.o
…
In file included from ./include/linux/compiler.h:58:0,
from ./include/uapi/linux/stddef.h:1,
from ./include/linux/stddef.h:4,
from ./include/uapi/linux/posix_types.h:4,
from ./include/uapi/linux/types.h:13,
from ./include/linux/types.h:5,
from fs/nfs/write.c:9:
./arch/x86/include/asm/jump_label.h: In function ‘trace_nfs_writeback_page_enter’:
./include/linux/compiler-gcc.h:275:38: warning: asm operand 0 probably doesn’t match constraints
#define asm_volatile_goto(x...) do { asm goto(x); asm (""); } while (0)
…
How do you think about to improve this software situation anyhow?
Regards,
Markus
T24gU3VuLCAyMDE3LTEyLTAzIGF0IDE1OjE1ICswMTAwLCBTRiBNYXJrdXMgRWxmcmluZyB3cm90
ZToNCj4gSGVsbG8sDQo+IA0KPiBJIGNhbWUgYWxvbmcgc29tZSBzb2Z0d2FyZSBtb2R1bGVzIHdo
ZXJlIEkgc3VnZ2VzdGVkIHNvdXJjZSBjb2RlDQo+IGFkanVzdG1lbnRzLg0KPiANCj4gRXhhbXBs
ZToNCj4gbmZzL3dyaXRlOiBVc2UgY29tbW9uIGVycm9yIGhhbmRsaW5nIGNvZGUgaW4NCj4gbmZz
X2xvY2tfYW5kX2pvaW5fcmVxdWVzdHMoKQ0KPiANCj4gaHR0cHM6Ly9sa21sLm9yZy9sa21sLzIw
MTcvMTEvNy81OTkNCj4gaHR0cHM6Ly9wYXRjaHdvcmsua2VybmVsLm9yZy9wYXRjaC8xMDA0NzAx
My8NCj4gaHR0cHM6Ly9sa21sLmtlcm5lbC5vcmcvci88N2YwNzJmNzgtZWVmNC02ZDg3LWQyMzMt
Y2VlNzFkYWM1YTMyQHVzZXJzDQo+IC5zb3VyY2Vmb3JnZS5uZXQ+Ow0KPiANCj4gSSB3b3VsZCBs
aWtlIHRvIGNoZWNrIGNvcnJlc3BvbmRpbmcgYnVpbGQgcmVzdWx0cyB0aGVuIHdpdGhvdXQgZXh0
cmENCj4gb3B0aW1pc2F0aW9uIGFwcGxpZWQgYnkgdGhlIGNvbXBpbGVyLg0KPiBCdXQgSSBnb3Qg
c3VycHJpc2VkIGJ5IGVycm9yIG1lc3NhZ2VzIGZvciBhIGNvbW1hbmQgbGlrZSB0aGUNCj4gZm9s
bG93aW5nLg0KPiANCj4gZWxmcmluZ0BTb25uZTp+L1Byb2pla3RlL0xpbnV4L25leHQtcGF0Y2hl
ZD4gbXlfY2M9L3Vzci9iaW4vZ2NjLTcgJiYNCj4gTEFORz1DIG1ha2UgLWo0IENDPSIke215X2Nj
fSIgSE9TVENDPSIke215X2NjfSIgRVhUUkFfQ0ZMQUdTPSctTzAnDQo+IGFsbG1vZGNvbmZpZyBm
cy9uZnMvd3JpdGUubw0KPiDigKYNCj4gSW4gZmlsZSBpbmNsdWRlZCBmcm9tIC4vaW5jbHVkZS9s
aW51eC9jb21waWxlci5oOjU4OjAsDQo+ICAgICAgICAgICAgICAgICAgZnJvbSAuL2luY2x1ZGUv
dWFwaS9saW51eC9zdGRkZWYuaDoxLA0KPiAgICAgICAgICAgICAgICAgIGZyb20gLi9pbmNsdWRl
L2xpbnV4L3N0ZGRlZi5oOjQsDQo+ICAgICAgICAgICAgICAgICAgZnJvbSAuL2luY2x1ZGUvdWFw
aS9saW51eC9wb3NpeF90eXBlcy5oOjQsDQo+ICAgICAgICAgICAgICAgICAgZnJvbSAuL2luY2x1
ZGUvdWFwaS9saW51eC90eXBlcy5oOjEzLA0KPiAgICAgICAgICAgICAgICAgIGZyb20gLi9pbmNs
dWRlL2xpbnV4L3R5cGVzLmg6NSwNCj4gICAgICAgICAgICAgICAgICBmcm9tIGZzL25mcy93cml0
ZS5jOjk6DQo+IC4vYXJjaC94ODYvaW5jbHVkZS9hc20vanVtcF9sYWJlbC5oOiBJbiBmdW5jdGlv
bg0KPiDigJh0cmFjZV9uZnNfd3JpdGViYWNrX3BhZ2VfZW50ZXLigJk6DQo+IC4vaW5jbHVkZS9s
aW51eC9jb21waWxlci1nY2MuaDoyNzU6Mzg6IHdhcm5pbmc6IGFzbSBvcGVyYW5kIDANCj4gcHJv
YmFibHkgZG9lc27igJl0IG1hdGNoIGNvbnN0cmFpbnRzDQo+ICAjZGVmaW5lIGFzbV92b2xhdGls
ZV9nb3RvKHguLi4pIGRvIHsgYXNtIGdvdG8oeCk7IGFzbSAoIiIpOyB9IHdoaWxlDQo+ICgwKQ0K
PiDigKYNCj4gDQo+IA0KPiBIb3cgZG8geW91IHRoaW5rIGFib3V0IHRvIGltcHJvdmUgdGhpcyBz
b2Z0d2FyZSBzaXR1YXRpb24gYW55aG93Pw0KDQpJJ20gbm90IHNlZWluZyBhbnl0aGluZyBvYnZp
b3VzbHkgd3Jvbmcgd2l0aCB0aGUgTkZTIHVzZSBvZiB0cmFjZXBvaW50cw0KdGhlcmUsIGFuZCB0
aGUgd2FybmluZyBzdWdnZXN0cyByYXRoZXIgdGhhdCBnY2MgaGFzIGFuIGlzc3VlIHdpdGggdGhl
DQppbmxpbmVkIGFzc2VtYmx5IGNvZGUgaW4ganVtcF9sYWJlbC5oLg0KDQpDY2luZyBQZXRlciBa
aWpsc3RyYSAod2hvIGFwcGVhcnMgdG8gaGF2ZSBiZWVuIHRoZSBsYXN0IHBlcnNvbiB0byB0b3Vj
aA0KdGhhdCBhc3NlbWJseSBjb2RlKSBhbmQgU3RldmVuIFJvc3RlZHQuDQoNCi0tIA0KVHJvbmQg
TXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXIsIFByaW1hcnlEYXRhDQp0cm9u
ZC5teWtsZWJ1c3RAcHJpbWFyeWRhdGEuY29tDQo=
On Sun, 3 Dec 2017 15:17:32 +0000
Trond Myklebust <[email protected]> wrote:
> > I would like to check corresponding build results then without extra
> > optimisation applied by the compiler.
> > But I got surprised by error messages for a command like the
> > following.
> >
> > elfring@Sonne:~/Projekte/Linux/next-patched> my_cc=/usr/bin/gcc-7 &&
> > LANG=C make -j4 CC="${my_cc}" HOSTCC="${my_cc}" EXTRA_CFLAGS='-O0'
> > allmodconfig fs/nfs/write.o
Why would you compile the kernel without optimization? There's many
places in the kernel that WILL NOT BUILD without optimization. In fact,
we do a lot of tricks to make sure that things work the way we expect
it to, because we add broken code that only gets compiled out when gcc
optimizes the code the way we expect it to be, and the kernel build
will break otherwise.
-- Steve
> Why would you compile the kernel without optimization?
I would like to see how big an effect finally is in such a build configuration
after specific source code adjustments.
> There's many places in the kernel that WILL NOT BUILD without optimization.
I did not really know this detail so far.
I noticed that the optimised build variants worked during my test comparisons.
> In fact, we do a lot of tricks to make sure that things work the way
> we expect it to, because we add broken code that only gets compiled out
> when gcc optimizes the code the way we expect it to be,
> and the kernel build will break otherwise.
Thanks for your information.
Can the software areas distinguished where such special handling matters?
Regards,
Markus
On Sun, 3 Dec 2017 22:56:51 +0100
SF Markus Elfring <[email protected]> wrote:
> Can the software areas distinguished where such special handling matters?
No idea. That's something you are going to have to figure out on your
own.
-- Steve
> Why would you compile the kernel without optimization?
Can another reason be occasionally still relevant?
Will the compilation be a bit quicker when extra data processing
could be omitted?
> There's many places in the kernel that WILL NOT BUILD without optimization.
Would you like to keep the software situation in this way?
> In fact, we do a lot of tricks to make sure that things work the way
> we expect it to, because we add broken code that only gets compiled out
> when gcc optimizes the code the way we expect it to be,
> and the kernel build will break otherwise.
* Can this goal be also achieved without the addition of “broken code”?
* How do you think about to improve the error handling there?
Regards,
Markus
On Mon, 4 Dec 2017 10:00:54 +0100
SF Markus Elfring <[email protected]> wrote:
> > Why would you compile the kernel without optimization?
>
> Can another reason be occasionally still relevant?
No.
>
> Will the compilation be a bit quicker when extra data processing
> could be omitted?
Why would you care more about the time it takes to compile the kernel,
than the time it takes for executing it? Benchmarks are all about
performance of a running kernel, nobody compares benchmarks of the time
it takes to compile it. Sure, we like to make the compile times quicker
(heck, I wrote "make localmodconfig" for just that purpose), but we
never favor compiler time over execution time. In fact, if we can
improve the execution performance by sacrificing compile time, we are
happy to do that.
>
>
> > There's many places in the kernel that WILL NOT BUILD without optimization.
>
> Would you like to keep the software situation in this way?
Yes.
>
>
> > In fact, we do a lot of tricks to make sure that things work the way
> > we expect it to, because we add broken code that only gets compiled out
> > when gcc optimizes the code the way we expect it to be,
> > and the kernel build will break otherwise.
>
> * Can this goal be also achieved without the addition of “broken code”?
No.
>
> * How do you think about to improve the error handling there?
It works just fine as is. Errors that can be detected at build time are
100 times better than detecting them at execution time.
-- Steve
>> Can the software areas distinguished where such special handling matters?
>
> No idea.
I would like to point another example out.
> That's something you are going to have to figure out on your own.
How do you think about information from an other clarification request
for the topic “caif: Use common error handling code in cfdgml_receive()”?
https://lkml.org/lkml/2017/11/7/486
https://patchwork.kernel.org/patch/10046849/
https://lkml.kernel.org/r/<[email protected]>
Regards,
Markus
>> Will the compilation be a bit quicker when extra data processing
>> could be omitted?
>
> Why would you care more about the time it takes to compile the kernel,
> than the time it takes for executing it?
I am also interested in the evolution of compilation time frames.
> Benchmarks are all about performance of a running kernel,
This is generally reasonable.
> nobody compares benchmarks of the time it takes to compile it.
I guess that the situation can be occasionally different there.
> Sure, we like to make the compile times quicker
Good to know …
> (heck, I wrote "make localmodconfig" for just that purpose),
Thanks.
> but we never favor compiler time over execution time.
I imagine that the speed expectations could be adjusted during software development,
couldn't they?
> In fact, if we can improve the execution performance by sacrificing compile time,
> we are happy to do that.
I guess that you would like to consider some constraints there.
>>> In fact, we do a lot of tricks to make sure that things work the way
>>> we expect it to, because we add broken code that only gets compiled out
>>> when gcc optimizes the code the way we expect it to be,
>>> and the kernel build will break otherwise.
>>
>> * Can this goal be also achieved without the addition of “broken code”?
>
> No.
Will any other contributors take another look?
>> * How do you think about to improve the error handling there?
>
> It works just fine as is.
I hope that further software improvements can be achieved also for this use case.
> Errors that can be detected at build time are 100 times better
> than detecting them at execution time.
I agree to such a general view.
Will an other (or no) error message be more appropriate?
Regards,
Markus