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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5695733C.1010201@emindsoft.com.cn>
Date:	Wed, 13 Jan 2016 05:42:20 +0800
From:	Chen Gang <chengang@...ndsoft.com.cn>
To:	Al Viro <viro@...IV.linux.org.uk>
CC:	dhowells@...hat.com, akpm@...ux-foundation.org,
	nicolas.iooss_linux@....org, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] fs: dcache: Use bool return value instead of int

On 1/12/16 06:51, Al Viro wrote:
> On Tue, Jan 12, 2016 at 05:30:45AM +0800, chengang@...ndsoft.com.cn wrote:
>> From: Chen Gang <gang.chen.5i5j@...il.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@...alhost 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