[PATCH 0/3] lib/string_helpers: fix precision issues and introduce tests

[PATCH 0/3] lib/string_helpers: fix precision issues and introduce tests

[Vitaly Kuznetsov]

Linux always lies about your storage size when it has 4k sectors and its size
is big enough. E.g. a device with 8192 4k sectors will be reported as
"32.7 MB/32 MiB" while "33.5 MB/32 MiB" is expected. This series is supposed
to fix the issue by fixing calculation precision in string_get_size() for all
possible inputs.

PATCH 1/3 is a preparatory change, PATCH 2/3 re-factors string_get_size() fixing
the issue, PATCH 3/3 introduces tests for string_get_size().

PATCH 3/3 was previously sent as part of "lib/string_helpers.c: fix infinite
loop in string_get_size()" series but afaics wasn't merged. In this submission
I add additional tests to it.

Vitaly Kuznetsov (3):
  lib/string_helpers: change blk_size to u32 for string_get_size()
    interface
  lib/string_helpers.c: don't lose precision in string_get_size()
  lib/test-string_helpers.c: add string_get_size() tests

 include/linux/string_helpers.h |  2 +-
 lib/string_helpers.c           | 48 +++++++++++++++++++++++-------------------
 lib/test-string_helpers.c      | 44 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+), 23 deletions(-)

-- 
2.4.3

[PATCH 1/3] lib/string_helpers: change blk_size to u32 for string_get_size() interface

[Vitaly Kuznetsov]

string_get_size() can't really handle huge block sizes, especially
blk_size > U32_MAX but string_get_size() interface states the opposite.
Change blk_size from u64 to u32 to reflect the reality.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 include/linux/string_helpers.h | 2 +-
 lib/string_helpers.c           | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index dabe643..1223e80 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -10,7 +10,7 @@ enum string_size_units {
 	STRING_UNITS_2,		/* use binary powers of 2^10 */
 };
 
-void string_get_size(u64 size, u64 blk_size, enum string_size_units units,
+void string_get_size(u64 size, u32 blk_size, enum string_size_units units,
 		     char *buf, int len);
 
 #define UNESCAPE_SPACE		0x01
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 5939f63..f6c27dc 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -26,7 +26,7 @@
  * at least 9 bytes and will always be zero terminated.
  *
  */
-void string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
+void string_get_size(u64 size, u32 blk_size, const enum string_size_units units,
 		     char *buf, int len)
 {
 	static const char *const units_10[] = {
@@ -58,7 +58,7 @@ void string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
 		i++;
 	}
 
-	exp = divisor[units] / (u32)blk_size;
+	exp = divisor[units] / blk_size;
 	/*
 	 * size must be strictly greater than exp here to ensure that remainder
 	 * is greater than divisor[units] coming out of the if below.
-- 
2.4.3

[PATCH 2/3] lib/string_helpers.c: don't lose precision in string_get_size()

[Vitaly Kuznetsov]

string_get_size() loses precision when there is a remainder for
blk_size / divisor[units] and size is big enough. E.g
string_get_size(8192, 4096, STRING_UNITS_10, ...) returns "32.7 MB"
while it is supposed to return "33.5 MB". For some artificial inputs
the result can be ridiculously wrong, e.g.
string_get_size(3000, 1900, STRING_UNITS_10, ...) returns "3.00 MB"
when "5.70 MB" is expected.

The issues comes from the fact than we through away
blk_size / divisor[units] remainder when size is > exp. This can be fixed
by saving it and doing some non-trivial calculations later to fix the error
but that would make this function even more cumbersome. Slightly re-factor
the function to not lose the precision for all inputs.

The overall complexity of this function comes from the fact that size can
be huge and we don't want to do size * blk_size as it can overflow. Do the
math in two steps:
1) Reduce size to something < blk_size * divisor[units]
2) Multiply the result (and the remainder) by blk_size and do final
   calculations.

Reported-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 lib/string_helpers.c | 46 +++++++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index f6c27dc..0658994 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -43,41 +43,43 @@ void string_get_size(u64 size, u32 blk_size, const enum string_size_units units,
 		[STRING_UNITS_10] = 1000,
 		[STRING_UNITS_2] = 1024,
 	};
-	int i, j;
-	u32 remainder = 0, sf_cap, exp;
+	int order = 0, j;
+	u64 remainder = 0;
+	u32 sf_cap;
 	char tmp[8];
 	const char *unit;
 
 	tmp[0] = '\0';
-	i = 0;
 	if (!size)
 		goto out;
 
-	while (blk_size >= divisor[units]) {
-		remainder = do_div(blk_size, divisor[units]);
-		i++;
-	}
-
-	exp = divisor[units] / blk_size;
 	/*
-	 * size must be strictly greater than exp here to ensure that remainder
-	 * is greater than divisor[units] coming out of the if below.
+	 * size can be huge and doing size * blk_size right away can overflow.
+	 * As a first step reduce huge size to something less than
+	 * blk_size * divisor[units].
 	 */
-	if (size > exp) {
+	while (size > (u64)blk_size * divisor[units]) {
 		remainder = do_div(size, divisor[units]);
-		remainder *= blk_size;
-		i++;
-	} else {
-		remainder *= size;
+		order++;
 	}
 
+	/* Now we're OK with doing size * blk_size, it won't overflow. */
 	size *= blk_size;
+	remainder *= blk_size;
+	/*
+	 * We were doing partial multiplication by blk_size.
+	 * remainder >= divisor[units] here means size should be increased.
+	 */
 	size += remainder / divisor[units];
-	remainder %= divisor[units];
+	remainder -= (remainder / divisor[units]) * divisor[units];
 
+	/*
+	 * Normalize. size >= divisor[units] means we still have enough
+	 * precision and dropping remainder is fine.
+	 */
 	while (size >= divisor[units]) {
 		remainder = do_div(size, divisor[units]);
-		i++;
+		order++;
 	}
 
 	sf_cap = size;
@@ -87,16 +89,18 @@ void string_get_size(u64 size, u32 blk_size, const enum string_size_units units,
 	if (j) {
 		remainder *= 1000;
 		remainder /= divisor[units];
-		snprintf(tmp, sizeof(tmp), ".%03u", remainder);
+		/* remainder is < divisor[units] here, (u32) is legit */
+		snprintf(tmp, sizeof(tmp), ".%03u", (u32)remainder);
 		tmp[j+1] = '\0';
 	}
 
  out:
-	if (i >= ARRAY_SIZE(units_2))
+	if (order >= ARRAY_SIZE(units_2))
 		unit = "UNK";
 	else
-		unit = units_str[units][i];
+		unit = units_str[units][order];
 
+	/* size is < divisor[units] here, (u32) is legit */
 	snprintf(buf, len, "%u%s %s", (u32)size,
 		 tmp, unit);
 }
-- 
2.4.3

[PATCH 3/3] lib/test-string_helpers.c: add string_get_size() tests

[Vitaly Kuznetsov]

Add a couple of simple tests for string_get_size().

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 lib/test-string_helpers.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c
index 8e376ef..a158cb3 100644
--- a/lib/test-string_helpers.c
+++ b/lib/test-string_helpers.c
@@ -326,6 +326,47 @@ out:
 	kfree(out_test);
 }
 
+#define string_get_size_maxbuf 16
+#define test_string_get_size_one(size, blk_size, units, exp_result)            \
+	do {                                                                   \
+		BUILD_BUG_ON(sizeof(exp_result) >= string_get_size_maxbuf);    \
+		__test_string_get_size((size), (blk_size), (units),            \
+				       (exp_result));                          \
+	} while (0)
+
+
+static __init void __test_string_get_size(const u64 size, const u32 blk_size,
+					  const enum string_size_units units,
+					  const char *exp_result)
+{
+	char buf[string_get_size_maxbuf];
+
+	string_get_size(size, blk_size, units, buf, sizeof(buf));
+	if (!memcmp(buf, exp_result, strlen(exp_result) + 1))
+		return;
+
+	buf[sizeof(buf) - 1] = '\0';
+	pr_warn("Test 'test_string_get_size_one' failed!\n");
+	pr_warn("string_get_size(size = %llu, blk_size = %u, units = %d\n",
+		size, blk_size, units);
+	pr_warn("expected: '%s', got '%s'\n", exp_result, buf);
+}
+
+static __init void test_string_get_size(void)
+{
+	test_string_get_size_one(16384, 512, STRING_UNITS_2, "8.00 MiB");
+	test_string_get_size_one(500118192, 512, STRING_UNITS_2, "238 GiB");
+	test_string_get_size_one(8192, 4096, STRING_UNITS_10, "33.5 MB");
+	test_string_get_size_one(1100, 1, STRING_UNITS_10, "1.10 kB");
+	test_string_get_size_one(3000, 1900, STRING_UNITS_10, "5.70 MB");
+	test_string_get_size_one(151234561234657, 3456789, STRING_UNITS_10,
+				 "522 EB");
+	test_string_get_size_one(1999, U32_MAX - 1, STRING_UNITS_10,
+				 "8.58 TB");
+	test_string_get_size_one(1, 512, STRING_UNITS_10, "512 B");
+	test_string_get_size_one(0, 512, STRING_UNITS_10, "0 B");
+}
+
 static int __init test_string_helpers_init(void)
 {
 	unsigned int i;
@@ -344,6 +385,9 @@ static int __init test_string_helpers_init(void)
 	for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++)
 		test_string_escape("escape 1", escape1, i, TEST_STRING_2_DICT_1);
 
+	/* Test string_get_size() */
+	test_string_get_size();
+
 	return -EINVAL;
 }
 module_init(test_string_helpers_init);
-- 
2.4.3

Re: [PATCH 3/3] lib/test-string_helpers.c: add string_get_size() tests

[Andy Shevchenko]

On Mon, 2015-10-26 at 14:55 +0100, Vitaly Kuznetsov wrote:
> Add a couple of simple tests for string_get_size().
> 

In linux-next this one (or similar?) is commit 29f3d140.
I don't think you need it in the series since it's in Andrew's patch
set already.


> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  lib/test-string_helpers.c | 44
> ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c
> index 8e376ef..a158cb3 100644
> --- a/lib/test-string_helpers.c
> +++ b/lib/test-string_helpers.c
> @@ -326,6 +326,47 @@ out:
>  	kfree(out_test);
>  }
>  
> +#define string_get_size_maxbuf 16
> +#define test_string_get_size_one(size, blk_size, units,
> exp_result)            \
> +	do
> {                                                                   \
> +		BUILD_BUG_ON(sizeof(exp_result) >=
> string_get_size_maxbuf);    \
> +		__test_string_get_size((size), (blk_size),
> (units),            \
> +				       (exp_result));               
>            \
> +	} while (0)
> +
> +
> +static __init void __test_string_get_size(const u64 size, const u32
> blk_size,
> +					  const enum
> string_size_units units,
> +					  const char *exp_result)
> +{
> +	char buf[string_get_size_maxbuf];
> +
> +	string_get_size(size, blk_size, units, buf, sizeof(buf));
> +	if (!memcmp(buf, exp_result, strlen(exp_result) + 1))
> +		return;
> +
> +	buf[sizeof(buf) - 1] = '\0';
> +	pr_warn("Test 'test_string_get_size_one' failed!\n");
> +	pr_warn("string_get_size(size = %llu, blk_size = %u, units =
> %d\n",
> +		size, blk_size, units);
> +	pr_warn("expected: '%s', got '%s'\n", exp_result, buf);
> +}
> +
> +static __init void test_string_get_size(void)
> +{
> +	test_string_get_size_one(16384, 512, STRING_UNITS_2, "8.00
> MiB");
> +	test_string_get_size_one(500118192, 512, STRING_UNITS_2,
> "238 GiB");
> +	test_string_get_size_one(8192, 4096, STRING_UNITS_10, "33.5
> MB");
> +	test_string_get_size_one(1100, 1, STRING_UNITS_10, "1.10
> kB");
> +	test_string_get_size_one(3000, 1900, STRING_UNITS_10, "5.70
> MB");
> +	test_string_get_size_one(151234561234657, 3456789,
> STRING_UNITS_10,
> +				 "522 EB");
> +	test_string_get_size_one(1999, U32_MAX - 1, STRING_UNITS_10,
> +				 "8.58 TB");
> +	test_string_get_size_one(1, 512, STRING_UNITS_10, "512 B");
> +	test_string_get_size_one(0, 512, STRING_UNITS_10, "0 B");
> +}
> +
>  static int __init test_string_helpers_init(void)
>  {
>  	unsigned int i;
> @@ -344,6 +385,9 @@ static int __init test_string_helpers_init(void)
>  	for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++)
>  		test_string_escape("escape 1", escape1, i,
> TEST_STRING_2_DICT_1);
>  
> +	/* Test string_get_size() */
> +	test_string_get_size();
> +
>  	return -EINVAL;
>  }
>  module_init(test_string_helpers_init);

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/3] lib/test-string_helpers.c: add string_get_size() tests

[Vitaly Kuznetsov]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Mon, 2015-10-26 at 14:55 +0100, Vitaly Kuznetsov wrote:
>> Add a couple of simple tests for string_get_size().
>> 
>
> In linux-next this one (or similar?) is commit 29f3d140.
> I don't think you need it in the series since it's in Andrew's patch
> set already.
>

Ah, sorry, missed that. In case it was already merged I'll just add a
couple of additional tests here.

>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  lib/test-string_helpers.c | 44
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 44 insertions(+)
>> 
>> diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c
>> index 8e376ef..a158cb3 100644
>> --- a/lib/test-string_helpers.c
>> +++ b/lib/test-string_helpers.c
>> @@ -326,6 +326,47 @@ out:
>>  	kfree(out_test);
>>  }
>>  
>> +#define string_get_size_maxbuf 16
>> +#define test_string_get_size_one(size, blk_size, units,
>> exp_result)            \
>> +	do
>> {                                                                   \
>> +		BUILD_BUG_ON(sizeof(exp_result) >=
>> string_get_size_maxbuf);    \
>> +		__test_string_get_size((size), (blk_size),
>> (units),            \
>> +				       (exp_result));               
>>            \
>> +	} while (0)
>> +
>> +
>> +static __init void __test_string_get_size(const u64 size, const u32
>> blk_size,
>> +					  const enum
>> string_size_units units,
>> +					  const char *exp_result)
>> +{
>> +	char buf[string_get_size_maxbuf];
>> +
>> +	string_get_size(size, blk_size, units, buf, sizeof(buf));
>> +	if (!memcmp(buf, exp_result, strlen(exp_result) + 1))
>> +		return;
>> +
>> +	buf[sizeof(buf) - 1] = '\0';
>> +	pr_warn("Test 'test_string_get_size_one' failed!\n");
>> +	pr_warn("string_get_size(size = %llu, blk_size = %u, units =
>> %d\n",
>> +		size, blk_size, units);
>> +	pr_warn("expected: '%s', got '%s'\n", exp_result, buf);
>> +}
>> +
>> +static __init void test_string_get_size(void)
>> +{
>> +	test_string_get_size_one(16384, 512, STRING_UNITS_2, "8.00
>> MiB");
>> +	test_string_get_size_one(500118192, 512, STRING_UNITS_2,
>> "238 GiB");
>> +	test_string_get_size_one(8192, 4096, STRING_UNITS_10, "33.5
>> MB");
>> +	test_string_get_size_one(1100, 1, STRING_UNITS_10, "1.10
>> kB");
>> +	test_string_get_size_one(3000, 1900, STRING_UNITS_10, "5.70
>> MB");
>> +	test_string_get_size_one(151234561234657, 3456789,
>> STRING_UNITS_10,
>> +				 "522 EB");
>> +	test_string_get_size_one(1999, U32_MAX - 1, STRING_UNITS_10,
>> +				 "8.58 TB");
>> +	test_string_get_size_one(1, 512, STRING_UNITS_10, "512 B");
>> +	test_string_get_size_one(0, 512, STRING_UNITS_10, "0 B");
>> +}
>> +
>>  static int __init test_string_helpers_init(void)
>>  {
>>  	unsigned int i;
>> @@ -344,6 +385,9 @@ static int __init test_string_helpers_init(void)
>>  	for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++)
>>  		test_string_escape("escape 1", escape1, i,
>> TEST_STRING_2_DICT_1);
>>  
>> +	/* Test string_get_size() */
>> +	test_string_get_size();
>> +
>>  	return -EINVAL;
>>  }
>>  module_init(test_string_helpers_init);

-- 
  Vitaly

Re: [PATCH 3/3] lib/test-string_helpers.c: add string_get_size() tests

[Andy Shevchenko]

On Mon, 2015-10-26 at 16:18 +0100, Vitaly Kuznetsov wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> 
> > On Mon, 2015-10-26 at 14:55 +0100, Vitaly Kuznetsov wrote:
> > > Add a couple of simple tests for string_get_size().
> > > 
> > 
> > In linux-next this one (or similar?) is commit 29f3d140.
> > I don't think you need it in the series since it's in Andrew's
> > patch
> > set already.
> > 
> 
> Ah, sorry, missed that. In case it was already merged

It's not yet in upstream afaiuc.


>  I'll just add a
> couple of additional tests here.

Then perhaps send it as a separate patch first?

> 
> > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > ---
> > >  lib/test-string_helpers.c | 44
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 44 insertions(+)
> > > 
> > > diff --git a/lib/test-string_helpers.c b/lib/test-
> > > string_helpers.c
> > > index 8e376ef..a158cb3 100644
> > > --- a/lib/test-string_helpers.c
> > > +++ b/lib/test-string_helpers.c
> > > @@ -326,6 +326,47 @@ out:
> > >  	kfree(out_test);
> > >  }
> > >  
> > > +#define string_get_size_maxbuf 16
> > > +#define test_string_get_size_one(size, blk_size, units,
> > > exp_result)            \
> > > +	do
> > > {                                                                
> > >    \
> > > +		BUILD_BUG_ON(sizeof(exp_result) >=
> > > string_get_size_maxbuf);    \
> > > +		__test_string_get_size((size), (blk_size),
> > > (units),            \
> > > +				       (exp_result));           
> > >     
> > >            \
> > > +	} while (0)
> > > +
> > > +
> > > +static __init void __test_string_get_size(const u64 size, const
> > > u32
> > > blk_size,
> > > +					  const enum
> > > string_size_units units,
> > > +					  const char
> > > *exp_result)
> > > +{
> > > +	char buf[string_get_size_maxbuf];
> > > +
> > > +	string_get_size(size, blk_size, units, buf,
> > > sizeof(buf));
> > > +	if (!memcmp(buf, exp_result, strlen(exp_result) + 1))
> > > +		return;
> > > +
> > > +	buf[sizeof(buf) - 1] = '\0';
> > > +	pr_warn("Test 'test_string_get_size_one' failed!\n");
> > > +	pr_warn("string_get_size(size = %llu, blk_size = %u,
> > > units =
> > > %d\n",
> > > +		size, blk_size, units);
> > > +	pr_warn("expected: '%s', got '%s'\n", exp_result, buf);
> > > +}
> > > +
> > > +static __init void test_string_get_size(void)
> > > +{
> > > +	test_string_get_size_one(16384, 512, STRING_UNITS_2,
> > > "8.00
> > > MiB");
> > > +	test_string_get_size_one(500118192, 512, STRING_UNITS_2,
> > > "238 GiB");
> > > +	test_string_get_size_one(8192, 4096, STRING_UNITS_10,
> > > "33.5
> > > MB");
> > > +	test_string_get_size_one(1100, 1, STRING_UNITS_10, "1.10
> > > kB");
> > > +	test_string_get_size_one(3000, 1900, STRING_UNITS_10,
> > > "5.70
> > > MB");
> > > +	test_string_get_size_one(151234561234657, 3456789,
> > > STRING_UNITS_10,
> > > +				 "522 EB");
> > > +	test_string_get_size_one(1999, U32_MAX - 1,
> > > STRING_UNITS_10,
> > > +				 "8.58 TB");
> > > +	test_string_get_size_one(1, 512, STRING_UNITS_10, "512
> > > B");
> > > +	test_string_get_size_one(0, 512, STRING_UNITS_10, "0
> > > B");
> > > +}
> > > +
> > >  static int __init test_string_helpers_init(void)
> > >  {
> > >  	unsigned int i;
> > > @@ -344,6 +385,9 @@ static int __init
> > > test_string_helpers_init(void)
> > >  	for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++)
> > >  		test_string_escape("escape 1", escape1, i,
> > > TEST_STRING_2_DICT_1);
> > >  
> > > +	/* Test string_get_size() */
> > > +	test_string_get_size();
> > > +
> > >  	return -EINVAL;
> > >  }
> > >  module_init(test_string_helpers_init);
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/3] lib/test-string_helpers.c: add string_get_size() tests

[Vitaly Kuznetsov]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Mon, 2015-10-26 at 16:18 +0100, Vitaly Kuznetsov wrote:
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> 
>> > On Mon, 2015-10-26 at 14:55 +0100, Vitaly Kuznetsov wrote:
>> > > Add a couple of simple tests for string_get_size().
>> > > 
>> > 
>> > In linux-next this one (or similar?) is commit 29f3d140.
>> > I don't think you need it in the series since it's in Andrew's
>> > patch
>> > set already.
>> > 
>> 
>> Ah, sorry, missed that. In case it was already merged
>
> It's not yet in upstream afaiuc.
>
>>  I'll just add a
>> couple of additional tests here.
>
> Then perhaps send it as a separate patch first?
>

Oh, actually 29f3d140 (linux-next commit id) is some old version of my
patch, there were later submissions (I think 'v5' was the last one:
https://lkml.org/lkml/2015/9/17/172 and that's where Rasmus
found the issue I'm fixing here). So I think it would be the best to
throw away 29f3d140 from Andrew's tree and take this one instead.

[skip]

-- 
  Vitaly

Re: [PATCH 2/3] lib/string_helpers.c: don't lose precision in string_get_size()

[Andy Shevchenko]

On Mon, 2015-10-26 at 14:55 +0100, Vitaly Kuznetsov wrote:
> string_get_size() loses precision when there is a remainder for
> blk_size / divisor[units] and size is big enough. E.g
> string_get_size(8192, 4096, STRING_UNITS_10, ...) returns "32.7 MB"
> while it is supposed to return "33.5 MB". For some artificial inputs
> the result can be ridiculously wrong, e.g.
> string_get_size(3000, 1900, STRING_UNITS_10, ...) returns "3.00 MB"
> when "5.70 MB" is expected.
> 
> The issues comes from the fact than we through away
> blk_size / divisor[units] remainder when size is > exp. This can be
> fixed
> by saving it and doing some non-trivial calculations later to fix the
> error
> but that would make this function even more cumbersome. Slightly re-
> factor
> the function to not lose the precision for all inputs.
> 
> The overall complexity of this function comes from the fact that size
> can
> be huge and we don't want to do size * blk_size as it can overflow.
> Do the
> math in two steps:
> 1) Reduce size to something < blk_size * divisor[units]
> 2) Multiply the result (and the remainder) by blk_size and do final
>    calculations.
> 
> Reported-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  lib/string_helpers.c | 46 +++++++++++++++++++++++++-----------------
> ----
>  1 file changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index f6c27dc..0658994 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -43,41 +43,43 @@ void string_get_size(u64 size, u32 blk_size,
> const enum string_size_units units,
>  		[STRING_UNITS_10] = 1000,
>  		[STRING_UNITS_2] = 1024,
>  	};
> -	int i, j;
> -	u32 remainder = 0, sf_cap, exp;
> +	int order = 0, j;
> +	u64 remainder = 0;
> +	u32 sf_cap;
>  	char tmp[8];
>  	const char *unit;
>  
>  	tmp[0] = '\0';
> -	i = 0;

Maybe leave i naming as is. Your order is not strictly speaking an
order, rather 3x order. I will make patch neater.

>  	if (!size)
>  		goto out;
>  
> -	while (blk_size >= divisor[units]) {
> -		remainder = do_div(blk_size, divisor[units]);
> -		i++;
> -	}
> -
> -	exp = divisor[units] / blk_size;
>  	/*
> -	 * size must be strictly greater than exp here to ensure
> that remainder
> -	 * is greater than divisor[units] coming out of the if
> below.
> +	 * size can be huge and doing size * blk_size right away can
> overflow.
> +	 * As a first step reduce huge size to something less than
> +	 * blk_size * divisor[units].
>  	 */
> -	if (size > exp) {
> +	while (size > (u64)blk_size * divisor[units]) {
>  		remainder = do_div(size, divisor[units]);
> -		remainder *= blk_size;
> -		i++;
> -	} else {
> -		remainder *= size;
> +		order++;
>  	}
>  
> +	/* Now we're OK with doing size * blk_size, it won't
> overflow. */
>  	size *= blk_size;
> +	remainder *= blk_size;
> +	/*
> +	 * We were doing partial multiplication by blk_size.
> +	 * remainder >= divisor[units] here means size should be
> increased.
> +	 */
>  	size += remainder / divisor[units];
> -	remainder %= divisor[units];
> +	remainder -= (remainder / divisor[units]) * divisor[units];



>  
> +	/*
> +	 * Normalize. size >= divisor[units] means we still have
> enough
> +	 * precision and dropping remainder is fine.
> +	 */
>  	while (size >= divisor[units]) {
>  		remainder = do_div(size, divisor[units]);
> -		i++;
> +		order++;
>  	}
>  
>  	sf_cap = size;
> @@ -87,16 +89,18 @@ void string_get_size(u64 size, u32 blk_size,
> const enum string_size_units units,
>  	if (j) {
>  		remainder *= 1000;
>  		remainder /= divisor[units];
> -		snprintf(tmp, sizeof(tmp), ".%03u", remainder);
> +		/* remainder is < divisor[units] here, (u32) is
> legit */
> +		snprintf(tmp, sizeof(tmp), ".%03u", (u32)remainder);
>  		tmp[j+1] = '\0';
>  	}
>  
>   out:
> -	if (i >= ARRAY_SIZE(units_2))
> +	if (order >= ARRAY_SIZE(units_2))
>  		unit = "UNK";
>  	else
> -		unit = units_str[units][i];
> +		unit = units_str[units][order];
>  
> +	/* size is < divisor[units] here, (u32) is legit */
>  	snprintf(buf, len, "%u%s %s", (u32)size,
>  		 tmp, unit);
>  }

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/3] lib/string_helpers.c: don't lose precision in string_get_size()

[Rasmus Villemoes]

So I also played with this over the weekend, and also threw together a
stupid script to check the output. I see you have more or less the same
idea I used, namely to combine size and blk_size earlier.

I put some code on github, https://github.com/Villemoes/get_size. All
versions still fail a very simply case, size=1594323, blk_size=1, for
which the correct answer is "1.52 MiB", but we get "1.51 MiB". But both
your version and my two attempts seem to have the property that they are
always at most one ULP (unit in the last place) too low, and never too
high. ATM, my version 2 fails 70 of the 13598 test cases, while yours
fail 86 (the former being a strict subset of the latter), so they're
very similar.

Regardless of which algorithm we go with, I have some cleanups I'd like
to do, but they're mostly independent and can wait.

Rasmus

Re: [PATCH 3/3] lib/test-string_helpers.c: add string_get_size() tests

[Rasmus Villemoes]

On Mon, Oct 26 2015, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Add a couple of simple tests for string_get_size().
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  lib/test-string_helpers.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>
> diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c
> index 8e376ef..a158cb3 100644
> --- a/lib/test-string_helpers.c
> +++ b/lib/test-string_helpers.c
> @@ -326,6 +326,47 @@ out:
>  	kfree(out_test);
>  }
>  
> +#define string_get_size_maxbuf 16
> +#define test_string_get_size_one(size, blk_size, units, exp_result)            \
> +	do {                                                                   \
> +		BUILD_BUG_ON(sizeof(exp_result) >= string_get_size_maxbuf);    \
> +		__test_string_get_size((size), (blk_size), (units),            \
> +				       (exp_result));                          \
> +	} while (0)
> +
> +
> +static __init void __test_string_get_size(const u64 size, const u32 blk_size,
> +					  const enum string_size_units units,
> +					  const char *exp_result)
> +{
> +	char buf[string_get_size_maxbuf];
> +
> +	string_get_size(size, blk_size, units, buf, sizeof(buf));
> +	if (!memcmp(buf, exp_result, strlen(exp_result) + 1))
> +		return;
> +
> +	buf[sizeof(buf) - 1] = '\0';
> +	pr_warn("Test 'test_string_get_size_one' failed!\n");
> +	pr_warn("string_get_size(size = %llu, blk_size = %u, units = %d\n",
> +		size, blk_size, units);
> +	pr_warn("expected: '%s', got '%s'\n", exp_result, buf);
> +}
> +
> +static __init void test_string_get_size(void)
> +{
> +	test_string_get_size_one(16384, 512, STRING_UNITS_2, "8.00 MiB");
> +	test_string_get_size_one(500118192, 512, STRING_UNITS_2, "238 GiB");
> +	test_string_get_size_one(8192, 4096, STRING_UNITS_10, "33.5 MB");
> +	test_string_get_size_one(1100, 1, STRING_UNITS_10, "1.10 kB");
> +	test_string_get_size_one(3000, 1900, STRING_UNITS_10, "5.70 MB");
> +	test_string_get_size_one(151234561234657, 3456789, STRING_UNITS_10,
> +				 "522 EB");

Since we're changing this anyway, can't we test every pair of
(size,blk_size) with both units? That'll be twice the number of tests
for less horizontal real estate. E.g.

  test_string_get_size_one(8192, 4096, "32.0 MiB", "33.5 MB");

Do we really care how and if string_get_size works for a non-power-of-2
blk_size? I certainly assume that we're passed a non-zero value.

Rasmus

Re: [PATCH 2/3] lib/string_helpers.c: don't lose precision in string_get_size()

[Vitaly Kuznetsov]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Mon, 2015-10-26 at 14:55 +0100, Vitaly Kuznetsov wrote:
>> string_get_size() loses precision when there is a remainder for
>> blk_size / divisor[units] and size is big enough. E.g
>> string_get_size(8192, 4096, STRING_UNITS_10, ...) returns "32.7 MB"
>> while it is supposed to return "33.5 MB". For some artificial inputs
>> the result can be ridiculously wrong, e.g.
>> string_get_size(3000, 1900, STRING_UNITS_10, ...) returns "3.00 MB"
>> when "5.70 MB" is expected.
>> 
>> The issues comes from the fact than we through away
>> blk_size / divisor[units] remainder when size is > exp. This can be
>> fixed
>> by saving it and doing some non-trivial calculations later to fix the
>> error
>> but that would make this function even more cumbersome. Slightly re-
>> factor
>> the function to not lose the precision for all inputs.
>> 
>> The overall complexity of this function comes from the fact that size
>> can
>> be huge and we don't want to do size * blk_size as it can overflow.
>> Do the
>> math in two steps:
>> 1) Reduce size to something < blk_size * divisor[units]
>> 2) Multiply the result (and the remainder) by blk_size and do final
>>    calculations.
>> 
>> Reported-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  lib/string_helpers.c | 46 +++++++++++++++++++++++++-----------------
>> ----
>>  1 file changed, 25 insertions(+), 21 deletions(-)
>> 
>> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
>> index f6c27dc..0658994 100644
>> --- a/lib/string_helpers.c
>> +++ b/lib/string_helpers.c
>> @@ -43,41 +43,43 @@ void string_get_size(u64 size, u32 blk_size,
>> const enum string_size_units units,
>>  		[STRING_UNITS_10] = 1000,
>>  		[STRING_UNITS_2] = 1024,
>>  	};
>> -	int i, j;
>> -	u32 remainder = 0, sf_cap, exp;
>> +	int order = 0, j;
>> +	u64 remainder = 0;
>> +	u32 sf_cap;
>>  	char tmp[8];
>>  	const char *unit;
>>  
>>  	tmp[0] = '\0';
>> -	i = 0;
>
> Maybe leave i naming as is. Your order is not strictly speaking an
> order, rather 3x order. I will make patch neater.
>

While reading the original function I found meaningless 'i' and 'j' here
a bit consufing but yes, strictly speaking 'i' is a power of
divisor[units], not 'order' and I don't have a good name for it
(div_power?). I'll revert back to 'i' in v2.


>>  	if (!size)
>>  		goto out;
>>  
>> -	while (blk_size >= divisor[units]) {
>> -		remainder = do_div(blk_size, divisor[units]);
>> -		i++;
>> -	}
>> -
>> -	exp = divisor[units] / blk_size;
>>  	/*
>> -	 * size must be strictly greater than exp here to ensure
>> that remainder
>> -	 * is greater than divisor[units] coming out of the if
>> below.
>> +	 * size can be huge and doing size * blk_size right away can
>> overflow.
>> +	 * As a first step reduce huge size to something less than
>> +	 * blk_size * divisor[units].
>>  	 */
>> -	if (size > exp) {
>> +	while (size > (u64)blk_size * divisor[units]) {
>>  		remainder = do_div(size, divisor[units]);
>> -		remainder *= blk_size;
>> -		i++;
>> -	} else {
>> -		remainder *= size;
>> +		order++;
>>  	}
>>  
>> +	/* Now we're OK with doing size * blk_size, it won't
>> overflow. */
>>  	size *= blk_size;
>> +	remainder *= blk_size;
>> +	/*
>> +	 * We were doing partial multiplication by blk_size.
>> +	 * remainder >= divisor[units] here means size should be
>> increased.
>> +	 */
>>  	size += remainder / divisor[units];
>> -	remainder %= divisor[units];
>> +	remainder -= (remainder / divisor[units]) * divisor[units];
>
>>  
>> +	/*
>> +	 * Normalize. size >= divisor[units] means we still have
>> enough
>> +	 * precision and dropping remainder is fine.
>> +	 */
>>  	while (size >= divisor[units]) {
>>  		remainder = do_div(size, divisor[units]);
>> -		i++;
>> +		order++;
>>  	}
>>  
>>  	sf_cap = size;
>> @@ -87,16 +89,18 @@ void string_get_size(u64 size, u32 blk_size,
>> const enum string_size_units units,
>>  	if (j) {
>>  		remainder *= 1000;
>>  		remainder /= divisor[units];
>> -		snprintf(tmp, sizeof(tmp), ".%03u", remainder);
>> +		/* remainder is < divisor[units] here, (u32) is
>> legit */
>> +		snprintf(tmp, sizeof(tmp), ".%03u", (u32)remainder);
>>  		tmp[j+1] = '\0';
>>  	}
>>  
>>   out:
>> -	if (i >= ARRAY_SIZE(units_2))
>> +	if (order >= ARRAY_SIZE(units_2))
>>  		unit = "UNK";
>>  	else
>> -		unit = units_str[units][i];
>> +		unit = units_str[units][order];
>>  
>> +	/* size is < divisor[units] here, (u32) is legit */
>>  	snprintf(buf, len, "%u%s %s", (u32)size,
>>  		 tmp, unit);
>>  }

-- 
  Vitaly

Re: [PATCH 2/3] lib/string_helpers.c: don't lose precision in string_get_size()

[Vitaly Kuznetsov]

Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:

> So I also played with this over the weekend, and also threw together a
> stupid script to check the output. I see you have more or less the same
> idea I used, namely to combine size and blk_size earlier.
>
> I put some code on github, https://github.com/Villemoes/get_size. All
> versions still fail a very simply case, size=1594323, blk_size=1, for
> which the correct answer is "1.52 MiB", but we get "1.51 MiB".

Yes, the algorithm still has minor glitches (e.g. rounding issues) but
I'd say keeping it simple enough here is worth it.

> But both
> your version and my two attempts seem to have the property that they are
> always at most one ULP (unit in the last place) too low, and never too
> high. ATM, my version 2 fails 70 of the 13598 test cases, while yours
> fail 86 (the former being a strict subset of the latter), so they're
> very similar.
>
> Regardless of which algorithm we go with, I have some cleanups I'd like
> to do, but they're mostly independent and can wait.
>
> Rasmus

-- 
  Vitaly

Re: [PATCH 3/3] lib/test-string_helpers.c: add string_get_size() tests

[Vitaly Kuznetsov]

Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:

> On Mon, Oct 26 2015, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
>> Add a couple of simple tests for string_get_size().
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  lib/test-string_helpers.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 44 insertions(+)
>>
>> diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c
>> index 8e376ef..a158cb3 100644
>> --- a/lib/test-string_helpers.c
>> +++ b/lib/test-string_helpers.c
>> @@ -326,6 +326,47 @@ out:
>>  	kfree(out_test);
>>  }
>>  
>> +#define string_get_size_maxbuf 16
>> +#define test_string_get_size_one(size, blk_size, units, exp_result)            \
>> +	do {                                                                   \
>> +		BUILD_BUG_ON(sizeof(exp_result) >= string_get_size_maxbuf);    \
>> +		__test_string_get_size((size), (blk_size), (units),            \
>> +				       (exp_result));                          \
>> +	} while (0)
>> +
>> +
>> +static __init void __test_string_get_size(const u64 size, const u32 blk_size,
>> +					  const enum string_size_units units,
>> +					  const char *exp_result)
>> +{
>> +	char buf[string_get_size_maxbuf];
>> +
>> +	string_get_size(size, blk_size, units, buf, sizeof(buf));
>> +	if (!memcmp(buf, exp_result, strlen(exp_result) + 1))
>> +		return;
>> +
>> +	buf[sizeof(buf) - 1] = '\0';
>> +	pr_warn("Test 'test_string_get_size_one' failed!\n");
>> +	pr_warn("string_get_size(size = %llu, blk_size = %u, units = %d\n",
>> +		size, blk_size, units);
>> +	pr_warn("expected: '%s', got '%s'\n", exp_result, buf);
>> +}
>> +
>> +static __init void test_string_get_size(void)
>> +{
>> +	test_string_get_size_one(16384, 512, STRING_UNITS_2, "8.00 MiB");
>> +	test_string_get_size_one(500118192, 512, STRING_UNITS_2, "238 GiB");
>> +	test_string_get_size_one(8192, 4096, STRING_UNITS_10, "33.5 MB");
>> +	test_string_get_size_one(1100, 1, STRING_UNITS_10, "1.10 kB");
>> +	test_string_get_size_one(3000, 1900, STRING_UNITS_10, "5.70 MB");
>> +	test_string_get_size_one(151234561234657, 3456789, STRING_UNITS_10,
>> +				 "522 EB");
>
> Since we're changing this anyway, can't we test every pair of
> (size,blk_size) with both units? That'll be twice the number of tests
> for less horizontal real estate. E.g.
>
>   test_string_get_size_one(8192, 4096, "32.0 MiB", "33.5 MB");
>

Nice idea, will do.

> Do we really care how and if string_get_size works for a non-power-of-2
> blk_size? 

Probably not but there is nothing in current algorithm which prevents it
from working correctly with a non-power-of-2 blk_sizes. The issue you
found is even more visible on (3000, 1900) test.

> I certainly assume that we're passed a non-zero value.

Oh crap, we don't have a check for blk_size = 0 and this leads to an
infinite loop now... will do something in v2.

>
> Rasmus

-- 
  Vitaly

Re: [PATCH 2/3] lib/string_helpers.c: don't lose precision in string_get_size()

[Andy Shevchenko]

On Tue, 2015-10-27 at 09:36 +0100, Vitaly Kuznetsov wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> 
> > On Mon, 2015-10-26 at 14:55 +0100, Vitaly Kuznetsov wrote:
> > > string_get_size() loses precision when there is a remainder for
> > > blk_size / divisor[units] and size is big enough. E.g
> > > string_get_size(8192, 4096, STRING_UNITS_10, ...) returns "32.7
> > > MB"
> > > while it is supposed to return "33.5 MB". For some artificial
> > > inputs
> > > the result can be ridiculously wrong, e.g.
> > > string_get_size(3000, 1900, STRING_UNITS_10, ...) returns "3.00
> > > MB"
> > > when "5.70 MB" is expected.

[]

> > > -	int i, j;
> > > -	u32 remainder = 0, sf_cap, exp;
> > > +	int order = 0, j;
> > > +	u64 remainder = 0;
> > > +	u32 sf_cap;
> > >  	char tmp[8];
> > >  	const char *unit;
> > >  
> > >  	tmp[0] = '\0';
> > > -	i = 0;
> > 
> > Maybe leave i naming as is. Your order is not strictly speaking an
> > order, rather 3x order. I will make patch neater.
> > 
> 
> While reading the original function I found meaningless 'i' and 'j'
> here
> a bit consufing but yes, strictly speaking 'i' is a power of
> divisor[units], not 'order' and I don't have a good name for it
> (div_power?). I'll revert back to 'i' in v2.

I agree that names suck, however it might be better to have separate
patch to fix naming.

Rasmus?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy