Ok, Thanks. Patch is ready, I'll send it after some extended testing on my system (vocore2).
Regards Mauro On 10/1/20 10:56 AM, Miquel Raynal wrote: > 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 >
