Re: [PATCH] btrfs: fix build warning due to u64 devided by u32 for 32bit arch

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3154 bytes --]



On 2020/11/2 下午9:54, David Sterba wrote:
> On Mon, Nov 02, 2020 at 03:31:14PM +0800, Qu Wenruo wrote:
>> [BUG]
>> When building the kernel with subpage preparation patches, 32bit arches
>> will complain about the following linking error:
>>
>>    ld: fs/btrfs/extent_io.o: in function `release_extent_buffer':
>>    fs/btrfs/extent_io.c:5340: undefined reference to `__udivdi3'
>>
>> [CAUSE]
>> For 32bits, dividing u64 with u32 need to call div_u64(), not directly
>> call u64 / u32.
>>
>> [FIX]
>> Instead of calling the div_u64() macros, here we introduce a helper,
>> btrfs_sector_shift(), to calculate the sector shift, and we just do bit
>> shift to avoid executing the expensive division instruction.
> 
> Division is expensive but ilog2 does not come without a cost either.
> It's implemented as bsrl+cmov, which can be also considered expensive
> for frequent use.

You're right, ilog2() is also expensive.
For our usage, what we really want is ffs(), which can be done with
hardware support to reduce the cost.

Thanks,
Qu
> 
>> The sector_shift may be better cached in btrfs_fs_info, but so far there
>> are only very limited callers for that, thus the fs_info::sector_shift
>> can be there for further cleanup.
>>
>> David, can this patch be folded into the offending commit?
>> The patch is small enough, and doesn't change btrfs_fs_info.
>> Thus should be OK to fold.
> 
> I have sent my series cleaning up the simple shifts, for the sectorsize
> shift in particular see
> 
> https://lore.kernel.org/linux-btrfs/b38721840b8d703a29807b71460464134b9ca7e1.1603981453.git.dsterba@suse.com/
> 
>> Fixes: ef57afc454fb ("btrfs: extent_io: make btrfs_fs_info::buffer_radix to take sector size devided values")
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/ctree.h     |  5 +++++
>>  fs/btrfs/extent_io.c | 14 +++++++++-----
>>  2 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index 8a83bce3225c..eb282af985f5 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -3489,6 +3489,11 @@ static inline int __btrfs_fs_compat_ro(struct btrfs_fs_info *fs_info, u64 flag)
>>  	return !!(btrfs_super_compat_ro_flags(disk_super) & flag);
>>  }
>>  
>> +static inline u8 btrfs_sector_shift(struct btrfs_fs_info *fs_info)
>> +{
>> +	return ilog2(fs_info->sectorsize);
> 
> This has a runtime cost of calculating the the ilog2 each time we use
> it.
> 
>> +}
>> +
>>  /* acl.c */
>>  #ifdef CONFIG_BTRFS_FS_POSIX_ACL
>>  struct posix_acl *btrfs_get_acl(struct inode *inode, int type);
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 80b35885004a..3452019aef79 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -5129,10 +5129,10 @@ struct extent_buffer *find_extent_buffer(struct btrfs_fs_info *fs_info,
>>  					 u64 start)
>>  {
>>  	struct extent_buffer *eb;
>> +	u8 sector_shift = btrfs_sector_shift(fs_info);
> 
> And each use needs a temporary variable, where u8 generates worse
> assembly and also potentially needs stack space.
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]