From: Chen Gang <chengang@emindsoft.com.cn>
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: dhowells@redhat.com, akpm@linux-foundation.org,
nicolas.iooss_linux@m4x.org, linux-kernel@vger.kernel.org,
linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] fs: dcache: Use bool return value instead of int
Date: Wed, 13 Jan 2016 05:42:20 +0800 [thread overview]
Message-ID: <5695733C.1010201@emindsoft.com.cn> (raw)
In-Reply-To: <20160111225104.GO17997@ZenIV.linux.org.uk>
On 1/12/16 06:51, Al Viro wrote:
> On Tue, Jan 12, 2016 at 05:30:45AM +0800, chengang@emindsoft.com.cn wrote:
>> From: Chen Gang <gang.chen.5i5j@gmail.com>
>>
>> Use bool type for all functions which return boolean value. It will not
>> only let code clearer, but also sometimes let gcc produce better code.
>
> What's the point of this chunk?
>
I'll explain it below this mail, please check.
>> static enum d_walk_ret check_mount(void *data, struct dentry *dentry)
>> {
>> - int *ret = data;
>> + bool *ret = data;
>> if (d_mountpoint(dentry)) {
>> - *ret = 1;
>> + *ret = true;
>> return D_WALK_QUIT;
>> }
>> return D_WALK_CONTINUE;
>
> You are replacing a 1-word store with 1-byte store; if anything, that's more
> likely to yield _worse_ code, not better one.
>
For me, it really generates a little better code:
- Both 1-word store and 1-byte store are 1 instruction, normally, they
have the same execution speed (although it is not quite precise).
- But 1-byte store instruction has short length under CISC archs, which
can generate a little better code globally.
- For most of archs, 1-word store can process bytes nonalignment cases,
for check_mount() individually, the parameter data may be not word
alignment, which may cause the 1-word store slower than 1-byte store.
The related objdump is below:
origin:
00000000 <check_mount>:
0: 8b 12 mov (%edx),%edx
2: 81 e2 00 00 01 00 and $0x10000,%edx
8: 74 16 je 20 <check_mount+0x20>
a: c7 00 01 00 00 00 movl $0x1,(%eax)
10: b8 01 00 00 00 mov $0x1,%eax
15: c3 ret
16: 8d 76 00 lea 0x0(%esi),%esi
19: 8d bc 27 00 00 00 00 lea 0x0(%edi,%eiz,1),%edi
20: 31 c0 xor %eax,%eax
22: c3 ret
23: 8d b6 00 00 00 00 lea 0x0(%esi),%esi
29: 8d bc 27 00 00 00 00 lea 0x0(%edi,%eiz,1),%edi
new:
00000000 <check_mount>:
0: 8b 12 mov (%edx),%edx
2: 81 e2 00 00 01 00 and $0x10000,%edx
8: 74 0e je 18 <check_mount+0x18>
a: c6 00 01 movb $0x1,(%eax)
d: b8 01 00 00 00 mov $0x1,%eax
12: c3 ret
13: 90 nop
14: 8d 74 26 00 lea 0x0(%esi,%eiz,1),%esi
18: 31 c0 xor %eax,%eax
1a: c3 ret
1b: 90 nop
1c: 8d 74 26 00 lea 0x0(%esi,%eiz,1),%esi
[root@localhost fs]# gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/local/libexec/gcc/x86_64-pc-linux-gnu/6.0.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ../gcc-ana/configure
Thread model: posix
gcc version 6.0.0 20151121 (experimental) (GCC)
>> -static inline int d_unhashed(const struct dentry *dentry)
>> +static inline bool d_unhashed(const struct dentry *dentry)
>> {
>> return hlist_bl_unhashed(&dentry->d_hash);
>> }
>>
>> -static inline int d_unlinked(const struct dentry *dentry)
>> +static inline bool d_unlinked(const struct dentry *dentry)
>> {
>> return d_unhashed(dentry) && !IS_ROOT(dentry);
>> }
>
>> -static inline int simple_positive(struct dentry *dentry)
>> +static inline bool simple_positive(struct dentry *dentry)
>> {
>> return d_really_is_positive(dentry) && !d_unhashed(dentry);
>> }
>
> And these three are harmless, but completely pointless...
>
For performance, please check the original reply above this mail.
For me, bool can make the code a little simpler and clearer:
- int can express more things: error code, handler, count ... So if we
really only use one boolean variable, bool type is more clearer (it
is only for boolean).
- The old ANSI C compiler may not support bool type, so we have to use
int type instead of. But if one header/source file has already used
bool type in some part, the whole file need use bool type too.
Thanks.
--
Chen Gang (陈刚)
Open, share, and attitude like air, water, and life which God blessed
next prev parent reply other threads:[~2016-01-12 21:39 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-11 21:30 [PATCH] fs: dcache: Use bool return value instead of int chengang
2016-01-11 22:51 ` Al Viro
2016-01-12 21:42 ` Chen Gang [this message]
2016-01-12 22:21 ` Al Viro
2016-01-13 22:39 ` Chen Gang
2016-01-13 22:54 ` Al Viro
2016-01-14 15:39 ` Chen Gang
2016-01-24 21:19 ` Chen Gang
2016-01-24 21:27 ` Al Viro
2016-01-25 21:24 ` Chen Gang
2016-01-12 0:33 ` David Howells
2016-01-12 1:02 ` Al Viro
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5695733C.1010201@emindsoft.com.cn \
--to=chengang@emindsoft.com.cn \
--cc=akpm@linux-foundation.org \
--cc=dhowells@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nicolas.iooss_linux@m4x.org \
--cc=viro@ZenIV.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.