Re: [fs/squashfs PATCH v3 2/2] avoid 64-bit divisions on 32-bit
On 10/8/20 9:34 AM, Miquel Raynal wrote: > Hi Mauro, > > Mauro Condarelli 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 >> --- >> >> 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 >> #include >> #include >> -#include >> +#include >> +#include >> #include >> #include >> #include >> #include >> #include >> #include >> -#include >> >> #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 >
Re: [fs/squashfs PATCH v3 2/2] avoid 64-bit divisions on 32-bit
Hello, The commit title lacks a proper prefix, it should start with "fs/squashfs". You confused the prefix of the commit title with the "subject prefix" of format-patch. On Thu, 8 Oct 2020 00:30:21 +0200 Mauro Condarelli wrote: > 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 > #include > #include > -#include > +#include > +#include Do we need both ? > #include > #include > #include > #include > #include > #include > -#include Is this change related ? > #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; Why are you not using DIV_ROUND_UP_ULL() here ? Thanks, Thomas Petazzoni -- Thomas Petazzoni, CTO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Re: [fs/squashfs PATCH v3 2/2] avoid 64-bit divisions on 32-bit
Hi Mauro, Mauro Condarelli wrote on Thu, 8 Oct 2020 00:30:21 +0200: > Use macros in linux/kernel.h to avoid 64-bit divisions. s/in/from/? > > 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? > > Signed-off-by: Mauro Condarelli > --- > > 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 > #include > #include > -#include > +#include > +#include > #include > #include > #include > #include > #include > #include > -#include > > #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; 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? 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); Thanks, Miquèl
[fs/squashfs PATCH v3 2/2] avoid 64-bit divisions on 32-bit
Use macros in linux/kernel.h 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.
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.
Signed-off-by: Mauro Condarelli
---
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
#include
#include
-#include
+#include
+#include
#include
#include
#include
#include
#include
#include
-#include
#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;
}
/*
@@ -109,8 +109,8 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
if (inode_fragment_index >= get_unaligned_le32(>fragments))
return -EINVAL;
- start = get_unaligned_le64(>fragment_table_start) /
- ctxt.cur_dev->blksz;
+ start =
DIV_ROUND_DOWN_ULL(get_unaligned_le64(>fragment_table_start),
+ ctxt.cur_dev->blksz);
n_blks = sqfs_calc_n_blks(sblk->fragment_table_start,
sblk->export_table_start,
_offset);
@@ -135,7 +135,7 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
start_block = get_unaligned_le64(table + table_offset + block *
sizeof(u64));
- start = start_block / ctxt.cur_dev->blksz;
+ start = DIV_ROUND_DOWN_ULL(start_block, ctxt.cur_dev->blksz);
n_blks = sqfs_calc_n_blks(cpu_to_le64(start_block),
sblk->fragment_table_start, _offset);
@@ -641,8 +641,8 @@ static int sqfs_read_inode_table(unsigned char
**inode_table)
table_size = get_unaligned_le64(>directory_table_start) -
get_unaligned_le64(>inode_table_start);
- start = get_unaligned_le64(>inode_table_start) /
- ctxt.cur_dev->blksz;
+ start = DIV_ROUND_DOWN_ULL(get_unaligned_le64(>inode_table_start),
+ ctxt.cur_dev->blksz);
n_blks = sqfs_calc_n_blks(sblk->inode_table_start,
sblk->directory_table_start, _offset);
@@ -725,8 +725,8 @@ static int sqfs_read_directory_table(unsigned char
**dir_table, u32 **pos_list)
/* DIRECTORY TABLE */
table_size = get_unaligned_le64(>fragment_table_start) -
get_unaligned_le64(>directory_table_start);
- start = get_unaligned_le64(>directory_table_start) /
- ctxt.cur_dev->blksz;
+ start =
DIV_ROUND_DOWN_ULL(get_unaligned_le64(>directory_table_start),
+ ctxt.cur_dev->blksz);
n_blks = sqfs_calc_n_blks(sblk->directory_table_start,
sblk->fragment_table_start, _offset);
@@ -1334,11 +1334,11 @@ int sqfs_read(const char *filename, void *buf, loff_t
offset, loff_t len,
}
for (j = 0; j < datablk_count; j++) {
- start = data_offset / ctxt.cur_dev->blksz;
+ start = DIV_ROUND_DOWN_ULL(data_offset, ctxt.cur_dev->blksz);
table_size = SQFS_BLOCK_SIZE(finfo.blk_sizes[j]);
table_offset = data_offset - (start * ctxt.cur_dev->blksz);
- n_blks = DIV_ROUND_UP(table_size + table_offset,
- ctxt.cur_dev->blksz);
+ n_blks = DIV_ROUND_UP_ULL(table_size + table_offset,
+ ctxt.cur_dev->blksz);
data_buffer = malloc_cache_aligned(n_blks *
ctxt.cur_dev->blksz);
@@ -1388,10 +1388,10 @@ int sqfs_read(const char *filename, void *buf, loff_t
offset, loff_t len,
