[PATCH] fs: dcache: Use bool return value instead of int

[PATCH] fs: dcache: Use bool return value instead of int

[chengang]

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.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 fs/dcache.c            | 8 ++++----
 include/linux/dcache.h | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index b4539e8..7701479 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1281,9 +1281,9 @@ rename_retry:
 
 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;
@@ -1296,9 +1296,9 @@ static enum d_walk_ret check_mount(void *data, struct dentry *dentry)
  * Return true if the parent or its subdirectories contain
  * a mount point
  */
-int have_submounts(struct dentry *parent)
+bool have_submounts(struct dentry *parent)
 {
-	int ret = 0;
+	bool ret = false;
 
 	d_walk(parent, &ret, check_mount, NULL);
 
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 7781ce11..880a41c 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -266,7 +266,7 @@ extern struct dentry *d_find_alias(struct inode *);
 extern void d_prune_aliases(struct inode *);
 
 /* test whether we have any submounts in a subdir tree */
-extern int have_submounts(struct dentry *);
+extern bool have_submounts(struct dentry *);
 
 /*
  * This adds the entry to the hash queues.
@@ -370,12 +370,12 @@ extern struct dentry *dget_parent(struct dentry *dentry);
  *	Returns true if the dentry passed is not currently hashed.
  */
  
-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);
 }
@@ -508,7 +508,7 @@ static inline bool d_really_is_positive(const struct dentry *dentry)
 	return dentry->d_inode != NULL;
 }
 
-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);
 }
-- 
1.9.3

Re: [PATCH] fs: dcache: Use bool return value instead of int

[Al Viro]

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?

>  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.

> -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...

Re: [PATCH] fs: dcache: Use bool return value instead of int

[David Howells]

Al Viro <viro@ZenIV.linux.org.uk> wrote:

> > -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...

gcc-5 does actually produce slightly smaller code when int returns are
replaced by bools under some circumstances within the kernel.

David

Re: [PATCH] fs: dcache: Use bool return value instead of int

[Al Viro]

On Tue, Jan 12, 2016 at 12:33:37AM +0000, David Howells wrote:

> gcc-5 does actually produce slightly smaller code when int returns are
> replaced by bools under some circumstances within the kernel.

For inlines?  Are you serious?

Re: [PATCH] fs: dcache: Use bool return value instead of int

[Chen Gang]

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

Re: [PATCH] fs: dcache: Use bool return value instead of int

[Al Viro]

On Wed, Jan 13, 2016 at 05:42:20AM +0800, Chen Gang wrote:

> 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.

What the hell do you mean, unaligned?  It's given an address of local
variable of type int; it _will_ be aligned, or the compiler is FUBAR.

As for the inlines... frankly, if gcc generates a different code from having
replaced int with bool in those, it's time to do something very nasty to
gcc developers.

Re: [PATCH] fs: dcache: Use bool return value instead of int

[Chen Gang]

On 1/13/16 06:21, Al Viro wrote:
> On Wed, Jan 13, 2016 at 05:42:20AM +0800, Chen Gang wrote:
> 
>> 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.
> 
> What the hell do you mean, unaligned?  It's given an address of local
> variable of type int; it _will_ be aligned, or the compiler is FUBAR.
> 

I guess you misunderstand my meaning, in our case, it should not happen,
so I say "for check_mount() individually".

For 32 bits store instruction, we need consider about the byte alignment.

> As for the inlines... frankly, if gcc generates a different code from having
> replaced int with bool in those, it's time to do something very nasty to
> gcc developers.
> 

Could you provide the related proof?

Thanks.
-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed

Re: [PATCH] fs: dcache: Use bool return value instead of int

[Al Viro]

On Thu, Jan 14, 2016 at 06:39:53AM +0800, Chen Gang wrote:

> > As for the inlines... frankly, if gcc generates a different code from having
> > replaced int with bool in those, it's time to do something very nasty to
> > gcc developers.
> > 
> 
> Could you provide the related proof?

static inline _Bool f(.....)
{
	return <int expression>;
}

...
	if (f(.....))

should generate the code identical to
	if ((_Bool)<int expression>)
which, in turn, should generate the code identical to
	if (<int expression> != 0)
and
	if (<int expression>)

Neither explicit nor implicit conversion to _Bool (the former by the explicit
cast, the latter - by declaring f() to return _Bool) matters at all when the
damn thing is inlined in a condition context.  Conversion to _Bool is
equivalent to comparison with 0, and so is the use in condition of if() and
friends.

For something not inlined you might get different code generated due to a
difference in calling sequences of _Bool(...) and int(...); for inlined
case having one of those variants produce a better code means that compiler
has managed to miss some trivial optimization in all other variants.

And I'm yet to see any proof that gcc *does* fuck up in that fashion.  It
might - dumb bugs happen to everyone, but I would not assume that they'd
managed to do something that bogys without experimental evidence.

Re: [PATCH] fs: dcache: Use bool return value instead of int

[Chen Gang]

On 1/14/16 06:54, Al Viro wrote:
> On Thu, Jan 14, 2016 at 06:39:53AM +0800, Chen Gang wrote:
> 
>>> As for the inlines... frankly, if gcc generates a different code from having
>>> replaced int with bool in those, it's time to do something very nasty to
>>> gcc developers.
>>>
>>
>> Could you provide the related proof?
> 
> static inline _Bool f(.....)
> {
> 	return <int expression>;
> }
> 
> ...
> 	if (f(.....))
> 

For me, your case above isn't suitable for using bool. Please check this
patch, there is no any cases like you said above.

 - For d_unhashed() which return hlist_bl_unhashed(), it seems like your
   case, but in fact hlist_bl_unhashed() also need return bool (which I
   shall send another patch for, next).

 - And all the other changes of this patch are all for real, pure bool
   functions.

Thanks.

> should generate the code identical to
> 	if ((_Bool)<int expression>)
> which, in turn, should generate the code identical to
> 	if (<int expression> != 0)
> and
> 	if (<int expression>)
> 
> Neither explicit nor implicit conversion to _Bool (the former by the explicit
> cast, the latter - by declaring f() to return _Bool) matters at all when the
> damn thing is inlined in a condition context.  Conversion to _Bool is
> equivalent to comparison with 0, and so is the use in condition of if() and
> friends.
> 
> For something not inlined you might get different code generated due to a
> difference in calling sequences of _Bool(...) and int(...); for inlined
> case having one of those variants produce a better code means that compiler
> has managed to miss some trivial optimization in all other variants.
> 
> And I'm yet to see any proof that gcc *does* fuck up in that fashion.  It
> might - dumb bugs happen to everyone, but I would not assume that they'd
> managed to do something that bogys without experimental evidence.
> 

For your cases, what you said sounds OK to me (although I am not quite
sure what you said above whether precise or not).

Thanks.
-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed

Re: [PATCH] fs: dcache: Use bool return value instead of int

[Chen Gang]

Hello all:

Is this patch OK? shall I send the other patch based on this one? (the
other patch is v3 trivial patch for include/linux/dcache.h).

And sorry for replying late: the last week, I was not in Beijing, had to
be busy for analyzing a Linux kernel usb related issue for my company's
customer in Guangzhou (but at last, I guess, it is not kernel issue).


Thanks.

On 1/14/16 23:39, Chen Gang wrote:
> 
> On 1/14/16 06:54, Al Viro wrote:
>> On Thu, Jan 14, 2016 at 06:39:53AM +0800, Chen Gang wrote:
>>
>>>> As for the inlines... frankly, if gcc generates a different code from having
>>>> replaced int with bool in those, it's time to do something very nasty to
>>>> gcc developers.
>>>>
>>>
>>> Could you provide the related proof?
>>
>> static inline _Bool f(.....)
>> {
>> 	return <int expression>;
>> }
>>
>> ...
>> 	if (f(.....))
>>
> 
> For me, your case above isn't suitable for using bool. Please check this
> patch, there is no any cases like you said above.
> 
>  - For d_unhashed() which return hlist_bl_unhashed(), it seems like your
>    case, but in fact hlist_bl_unhashed() also need return bool (which I
>    shall send another patch for, next).
> 
>  - And all the other changes of this patch are all for real, pure bool
>    functions.
> 
> Thanks.
> 
>> should generate the code identical to
>> 	if ((_Bool)<int expression>)
>> which, in turn, should generate the code identical to
>> 	if (<int expression> != 0)
>> and
>> 	if (<int expression>)
>>
>> Neither explicit nor implicit conversion to _Bool (the former by the explicit
>> cast, the latter - by declaring f() to return _Bool) matters at all when the
>> damn thing is inlined in a condition context.  Conversion to _Bool is
>> equivalent to comparison with 0, and so is the use in condition of if() and
>> friends.
>>
>> For something not inlined you might get different code generated due to a
>> difference in calling sequences of _Bool(...) and int(...); for inlined
>> case having one of those variants produce a better code means that compiler
>> has managed to miss some trivial optimization in all other variants.
>>
>> And I'm yet to see any proof that gcc *does* fuck up in that fashion.  It
>> might - dumb bugs happen to everyone, but I would not assume that they'd
>> managed to do something that bogys without experimental evidence.
>>
> 
> For your cases, what you said sounds OK to me (although I am not quite
> sure what you said above whether precise or not).
> 
> Thanks.
> 

-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed

Re: [PATCH] fs: dcache: Use bool return value instead of int

[Al Viro]

On Mon, Jan 25, 2016 at 05:19:43AM +0800, Chen Gang wrote:
> Hello all:
> 
> Is this patch OK? shall I send the other patch based on this one? (the
> other patch is v3 trivial patch for include/linux/dcache.h).
> 
> And sorry for replying late: the last week, I was not in Beijing, had to
> be busy for analyzing a Linux kernel usb related issue for my company's
> customer in Guangzhou (but at last, I guess, it is not kernel issue).

Again, do you have _any_ evidence of improved code generation with that
patch?  Because if you do, I would really like to see it, so I could file
bugs against gcc optimizer.

Your impression of what _Bool is and what semantics does it have appears
to be rather different from that described in C99, but that's a secondary
issue - first and foremost, on which .config and with which gcc version
do you see improvements from that change?

Re: [PATCH] fs: dcache: Use bool return value instead of int

[Chen Gang]

On 1/25/16 05:27, Al Viro wrote:
> 
> Again, do you have _any_ evidence of improved code generation with that
> patch?  Because if you do, I would really like to see it, so I could file
> bugs against gcc optimizer.
> 
> Your impression of what _Bool is and what semantics does it have appears
> to be rather different from that described in C99, but that's a secondary
> issue - first and foremost, on which .config and with which gcc version
> do you see improvements from that change?
> 

For our case, the check_mount function have smaller size under x86_64
(movl for int, movb for bool, movl is longer than movb).

The related objdump is below, welcome any ideas, suggestions, and
discussions for it.

  origin (for int):

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 (for bool):

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) 


-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed