[RFC PATCH v2 1/1] Fix missing __udivdi3 in SquashFS implementation.

[Miquel Raynal]

Hi Mauro,

Mauro Condarelli <mc5686 at mclink.it> wrote on Thu, 1 Oct 2020 10:53:30
+0200:

> Correcting myself.
> See below.
> 
> On 10/1/20 10:41 AM, Mauro Condarelli wrote:
> > Thanks for Your review.
> >
> > On 10/1/20 9:59 AM, Miquel Raynal wrote:  
> >> Hello,
> >>
> >> Thomas Petazzoni <thomas.petazzoni at bootlin.com> wrote on Thu, 1 Oct
> >> 2020 09:28:41 +0200:
> >>  
> >>> Hello,
> >>>
> >>> On Wed, 30 Sep 2020 17:45:11 +0200
> >>> Mauro Condarelli <mc5686 at mclink.it> wrote:
> >>>  
> >>>> Use right shift to avoid 64-bit divisions.
> >>>>
> >>>> 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.
> >>>>
> >>>> Signed-off-by: Mauro Condarelli <mc5686 at mclink.it>    
> >>> Perhaps the commit log should contain an example U-Boot
> >>> configuration/platform where it fails to build. Indeed, we did test the
> >>> SquashFS code on ARM 32-bit, and it built and worked fine.  
> > This fails on mips32, specifically for the vocore2 board.
> > Problem here is __udivdi3 is not defined for this architecture
> > while it is for ARM32.
> > My (limited) understanding suggests it should be removed for ARM
> > since its usage has been (is being?) weeded out from both kernel
> > and u-boot. This is not my call, though.
> >
> > I will add a note to v3.
> >  
> >>>> ---
> >>>>
> >>>> 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/Kconfig      |  2 ++
> >>>>  fs/squashfs/sqfs.c       | 30 +++++++++++++++---------------
> >>>>  fs/squashfs/sqfs_inode.c |  6 +++---
> >>>>  3 files changed, 20 insertions(+), 18 deletions(-)
> >>>>
> >>>> diff --git a/fs/squashfs/Kconfig b/fs/squashfs/Kconfig
> >>>> index 54ab1618f1..7c3f83d007 100644
> >>>> --- a/fs/squashfs/Kconfig
> >>>> +++ b/fs/squashfs/Kconfig
> >>>> @@ -9,3 +9,5 @@ config FS_SQUASHFS
> >>>>  	  filesystem use, for archival use (i.e. in cases where a .tar.gz file
> >>>>  	  may be used), and in constrained block device/memory systems (e.g.
> >>>>  	  embedded systems) where low overhead is needed.
> >>>> +	  WARNING: if compression is enabled SquashFS needs a large amount
> >>>> +	  of dynamic memory; make sure CONFIG_SYS_MALLOC_LEN >= 0x4000.    
> >>> This change is completely unrelated, and should be in a separate patch.  
> >> I was about to tell you the same thing, this warning is useful but
> >> should definitely lay into its own commit.  
> > Will do in v3.
> >
> >  
> >>>> -		n_blks = DIV_ROUND_UP(table_size + table_offset,
> >>>> -				      ctxt.cur_dev->blksz);
> >>>> +		n_blks = (table_size + table_offset + ctxt.cur_dev->blksz - 1) >>
> >>>> +			ctxt.cur_dev->log2blksz;    
> >>> I understand why you have to add blksz - 1 before doing the shift, but
> >>> I find that it's overall a lot less readable/clear. Is there a way to
> >>> do better ?
> >>>
> >>> We could use DIV_ROUND_UP_ULL() I guess.  
> > I did not do this because DIV_ROUND_UP() is in a global (non-architecture-specific)
> > header and it unconditionally uses division; I did not know how to handle this.
> > Would a comment suffice to clarify intent? Something like:
> >
> > n_blks = (table_size + table_offset + ctxt.cur_dev->blksz - 1) >>
> > 	  ctxt.cur_dev->log2blksz; /* ROUND_UP division */
> >
> > Note: this problem stays even if we roll-back to use do_div(); see below.  
> actually include/linux/kernel.h defines both DIV_ROUND_DOWN_ULL()
> and DIV_ROUND_UP_ULL() I suppose we should use those in all cases.
> 
> 
> >>>>  		else
> >>>> -			blk_list_size = file_size / blk_size;
> >>>> +			blk_list_size = file_size > LOG2(blk_size);    
> >>> Very bad mistake here: > should be >>.  
> > Sorry, my bad.
> > Corrected for v3.
> >  
> >> I personally highly dislike replacing divisions into shifts. I think
> >> it's too much effort when trying to understand code you did not write
> >> yourself. Is it possible to use something like do_div? plus, you can
> >> check the remainder to be 0 in this case.  
> > Please make up your mind about this.
> > I personally tend to agree with Miquèl and my v1 patch used
> > do_div() exclusively (i did not use even the lldiv() wrapper), but
> > I will not insist either way... just let me know what's considered
> > better.  
> As said above: it seems using the macros is both "standard" and
> safer than using shifts.
> If I get a go-ahead I'll use those macros in v3.

I think we all agree using these macros is much nicer, readable and
probably almost as fast.

Thanks,
Miquèl