Hi Mauro, Mauro Condarelli <mc5...@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.petazz...@bootlin.com> wrote on Thu, 1 Oct > >> 2020 09:28:41 +0200: > >> > >>> Hello, > >>> > >>> On Wed, 30 Sep 2020 17:45:11 +0200 > >>> Mauro Condarelli <mc5...@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 <mc5...@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
