[fs/squashfs PATCH v3 2/2] avoid 64-bit divisions on 32-bit

[Mauro Condarelli]

On 10/8/20 9:34 AM, Miquel Raynal wrote:
> Hi Mauro,
>
> Mauro Condarelli <mc5686 at mclink.it> wrote on Thu,  8 Oct 2020 00:30:21
> +0200:
>
>> Use macros in linux/kernel.h to avoid 64-bit divisions.
> s/in/from/?
My command of English language is far from perfect.
I meant those macros are used in Linux kernel.
Feel free to rewrite as needed.

>> These divisions are needed to convert from file length (potentially
>> over 32-bit range) to block number, so result and remainder are
>> guaranteed to fit in 32-bit integers.
>>
>> Some 32bit architectures (notably mipsel) lack an implementation of
>> __udivdi3() compiler helper function in reduced libgcc.
>>
>> Standard strategy is to use macros and do_div() inline.
> You don't use do_div(), here, is it a leftover or do you mean that this
> is a generic solution?
I meant I use the macros in SquashFS code, but macros use
do_div() inline function to do their job.
Should I rephrase to be more explicit?


>> Signed-off-by: Mauro Condarelli <mc5686 at mclink.it>
>> ---
>>
>> Changes in v3:
>> - converted to use DIV_ROUND_(UP|DOWN)_ULL() macros (Miquel Raynal).
>> - split commits to handle unrelated Kconfig warning (Thomas Petazzoni).
>>
>> Changes in v2:
>> - replace division with right shift (Daniel Schwierzeck).
>> - remove vocore2-specific change (Daniel Schwierzeck).
>> - add warning to Kconfig about CONFIG_SYS_MALLOC_LEN (Tom Rini).
>>
>>  fs/squashfs/sqfs.c       | 32 ++++++++++++++++----------------
>>  fs/squashfs/sqfs_inode.c |  7 ++++---
>>  2 files changed, 20 insertions(+), 19 deletions(-)
>>
>> diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
>> index 15208b4dab..ef9f5e3449 100644
>> --- a/fs/squashfs/sqfs.c
>> +++ b/fs/squashfs/sqfs.c
>> @@ -10,14 +10,14 @@
>>  #include <asm/unaligned.h>
>>  #include <errno.h>
>>  #include <fs.h>
>> -#include <linux/types.h>
>> +#include <linux/kernel.h>
>> +#include <div64.h>
>>  #include <linux/byteorder/little_endian.h>
>>  #include <linux/byteorder/generic.h>
>>  #include <memalign.h>
>>  #include <stdlib.h>
>>  #include <string.h>
>>  #include <squashfs.h>
>> -#include <part.h>
>>  
>>  #include "sqfs_decompressor.h"
>>  #include "sqfs_filesystem.h"
>> @@ -85,10 +85,10 @@ static int sqfs_calc_n_blks(__le64 start, __le64 end, u64 *offset)
>>  	u64 start_, table_size;
>>  
>>  	table_size = le64_to_cpu(end) - le64_to_cpu(start);
>> -	start_ = le64_to_cpu(start) / ctxt.cur_dev->blksz;
>> +	start_ = DIV_ROUND_DOWN_ULL(le64_to_cpu(start), ctxt.cur_dev->blksz);
>>  	*offset = le64_to_cpu(start) - (start_ * ctxt.cur_dev->blksz);
>>  
>> -	return DIV_ROUND_UP(table_size + *offset, ctxt.cur_dev->blksz);
>> +	return (table_size + *offset + ctxt.cur_dev->blksz - 1) >> ctxt.cur_dev->log2blksz;
This is definitely a leftover.
It should be:

          return DIV_ROUND_UP_ULL(table_size + *offset, ctxt.cur_dev->blksz);

> I don't recall Joao using this kind of structure but I might be wrong.
> Can you just verify that this is not a leftover from a previous change?
I will correct, retest and resend.

> Also, as you state in the commit message, all these divisions serve the
> same purpose: translating a file length to a block number. I think a
> helper would be very nice here, something like
>
> 	sqfs_length_to_block_id(ctxt, length);
I will consider it.

>
> Thanks,
> Miquèl
>