Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1258906 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/4] lib/string_helpers: fix precision issues and introduce tests Vitaly Kuznetsov <vkuznets@redhat.com> - 2015-10-29 17:40 +0100
    [PATCH v3 4/4] lib/test-string_helpers.c: add string_get_size() tests Vitaly Kuznetsov <vkuznets@redhat.com> - 2015-10-29 17:40 +0100
      Re: [PATCH v3 4/4] lib/test-string_helpers.c: add string_get_size() tests Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-10-29 22:40 +0100
    [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface Vitaly Kuznetsov <vkuznets@redhat.com> - 2015-10-29 17:40 +0100
      Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for  string_get_size() interface James Bottomley <jbottomley@odin.com> - 2015-10-29 23:30 +0100
        Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-30 00:20 +0100
          Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-30 00:30 +0100
          Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for  string_get_size() interface James Bottomley <jbottomley@odin.com> - 2015-10-30 04:40 +0100
        Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface Vitaly Kuznetsov <vkuznets@redhat.com> - 2015-10-30 11:50 +0100
          Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for  string_get_size() interface James Bottomley <jbottomley@odin.com> - 2015-10-31 01:30 +0100
            Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface Vitaly Kuznetsov <vkuznets@redhat.com> - 2015-11-02 17:00 +0100
              Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for  string_get_size() interface James Bottomley <jbottomley@odin.com> - 2015-11-03 04:50 +0100
                Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface Vitaly Kuznetsov <vkuznets@redhat.com> - 2015-11-03 14:20 +0100
                  Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for  string_get_size() interface James Bottomley <jbottomley@odin.com> - 2015-11-03 18:10 +0100
                    Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-03 22:00 +0100
                      Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for  string_get_size() interface James Bottomley <jbottomley@odin.com> - 2015-11-03 22:20 +0100
    [PATCH v3 2/4] lib/string_helpers.c: protect string_get_size() against blk_size=0 Vitaly Kuznetsov <vkuznets@redhat.com> - 2015-10-29 17:40 +0100
      Re: [PATCH v3 2/4] lib/string_helpers.c: protect string_get_size()  against blk_size=0 Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-10-29 22:30 +0100
      Re: [PATCH v3 2/4] lib/string_helpers.c: protect string_get_size()  against blk_size=0 James Bottomley <jbottomley@odin.com> - 2015-10-30 00:10 +0100
        Re: [PATCH v3 2/4] lib/string_helpers.c: protect string_get_size()  against blk_size=0 Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-10-30 00:40 +0100
          Re: [PATCH v3 2/4] lib/string_helpers.c: protect string_get_size()  against blk_size=0 James Bottomley <jbottomley@odin.com> - 2015-10-30 04:40 +0100
            Re: [PATCH v3 2/4] lib/string_helpers.c: protect string_get_size() against blk_size=0 Vitaly Kuznetsov <vkuznets@redhat.com> - 2015-10-30 11:50 +0100
              Re: [PATCH v3 2/4] lib/string_helpers.c: protect string_get_size()  against blk_size=0 James Bottomley <jbottomley@odin.com> - 2015-10-31 01:10 +0100

Page 1 of 2  [1] 2  Next page →

#1258906 — [PATCH v3 0/4] lib/string_helpers: fix precision issues and introduce tests

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/4 is a preparatory change, PATCH 2/4 adds additional protection
against blk_size=0 (nobody is supposed to call string_get_size() with
with blk_size=0, but better safe than sorry), PATCH 3/4 re-factors
string_get_size() fixing the issue, PATCH 4/4 introduces tests for
string_get_size().

PATCH 4/4 was previously sent as part of "lib/string_helpers.c: fix
infinite loop in string_get_size()" series but it is still not merged
upstream. In this submission I improve it and add additional tests to it.

Changes since v2:
- Separate blk_size check from Patch 3/4 to new Patch 2/4 [Andy Shevchenko]
- Slightly change the algorithm in Patch 3/4 [Rasmus Villemoes]

Vitaly Kuznetsov (4):
  lib/string_helpers: change blk_size to u32 for string_get_size()
    interface
  lib/string_helpers.c: protect string_get_size() against blk_size=0
  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           | 38 ++++++++++++++-------------
 lib/test-string_helpers.c      | 58 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 19 deletions(-)

-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1258907 — [PATCH v3 4/4] lib/test-string_helpers.c: add string_get_size() tests

Add a couple of simple tests for string_get_size().

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

diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c
index 8e376ef..4c77b54 100644
--- a/lib/test-string_helpers.c
+++ b/lib/test-string_helpers.c
@@ -326,6 +326,61 @@ out:
 	kfree(out_test);
 }
 
+#define string_get_size_maxbuf 16
+#define test_string_get_size_one(size, blk_size, exp_result10, exp_result2)    \
+	do {                                                                   \
+		BUILD_BUG_ON(sizeof(exp_result10) >= string_get_size_maxbuf);  \
+		BUILD_BUG_ON(sizeof(exp_result2) >= string_get_size_maxbuf);   \
+		__test_string_get_size((size), (blk_size), (exp_result10),     \
+				       (exp_result2));                         \
+	} while (0)
+
+
+static __init void __test_string_get_size(const u64 size, const u32 blk_size,
+					  const char *exp_result10,
+					  const char *exp_result2)
+{
+	char buf10[string_get_size_maxbuf];
+	char buf2[string_get_size_maxbuf];
+
+	string_get_size(size, blk_size, STRING_UNITS_10, buf10, sizeof(buf10));
+	string_get_size(size, blk_size, STRING_UNITS_2, buf2, sizeof(buf2));
+
+	if (!memcmp(buf10, exp_result10, strlen(exp_result10) + 1))
+		goto check_stringunits_2;
+
+	buf10[sizeof(buf10) - 1] = '\0';
+
+	pr_warn("Test 'test_string_get_size' failed!\n");
+	pr_warn("string_get_size(size = %llu, blk_size = %u, units = %s)\n",
+		size, blk_size, "STRING_UNITS_10");
+	pr_warn("expected: '%s', got '%s'\n", exp_result10, buf10);
+
+check_stringunits_2:
+	if (!memcmp(buf2, exp_result2, strlen(exp_result2) + 1))
+		return;
+
+	buf2[sizeof(buf2) - 1] = '\0';
+
+	pr_warn("Test 'test_string_get_size' failed!\n");
+	pr_warn("string_get_size(size = %llu, blk_size = %u, units = %s)\n",
+		size, blk_size, "STRING_UNITS_2");
+	pr_warn("expected: '%s', got '%s'\n", exp_result2, buf2);
+}
+
+static __init void test_string_get_size(void)
+{
+	test_string_get_size_one(16384, 512, "8.38 MB", "8.00 MiB");
+	test_string_get_size_one(500118192, 512, "256 GB", "238 GiB");
+	test_string_get_size_one(8192, 4096, "33.5 MB", "32.0 MiB");
+	test_string_get_size_one(1100, 1, "1.10 kB", "1.07 KiB");
+	test_string_get_size_one(3000, 1900, "5.70 MB", "5.43 MiB");
+	test_string_get_size_one(U64_MAX, 4096, "75.5 ZB", "63.9 ZiB");
+	test_string_get_size_one(1999, U32_MAX, "8.58 TB", "7.80 TiB");
+	test_string_get_size_one(1, 512, "512 B", "512 B");
+	test_string_get_size_one(0, 512, "0 B", "0 B");
+}
+
 static int __init test_string_helpers_init(void)
 {
 	unsigned int i;
@@ -344,6 +399,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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259027 — Re: [PATCH v3 4/4] lib/test-string_helpers.c: add string_get_size() tests

On Thu, Oct 29, 2015 at 6:30 PM, 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
>
> diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c
> index 8e376ef..4c77b54 100644
> --- a/lib/test-string_helpers.c
> +++ b/lib/test-string_helpers.c
> @@ -326,6 +326,61 @@ out:
>         kfree(out_test);
>  }
>
> +#define string_get_size_maxbuf 16
> +#define test_string_get_size_one(size, blk_size, exp_result10, exp_result2)    \
> +       do {                                                                   \
> +               BUILD_BUG_ON(sizeof(exp_result10) >= string_get_size_maxbuf);  \
> +               BUILD_BUG_ON(sizeof(exp_result2) >= string_get_size_maxbuf);   \
> +               __test_string_get_size((size), (blk_size), (exp_result10),     \
> +                                      (exp_result2));                         \
> +       } while (0)
> +
> +
> +static __init void __test_string_get_size(const u64 size, const u32 blk_size,
> +                                         const char *exp_result10,
> +                                         const char *exp_result2)
> +{
> +       char buf10[string_get_size_maxbuf];
> +       char buf2[string_get_size_maxbuf];
> +
> +       string_get_size(size, blk_size, STRING_UNITS_10, buf10, sizeof(buf10));
> +       string_get_size(size, blk_size, STRING_UNITS_2, buf2, sizeof(buf2));
> +
> +       if (!memcmp(buf10, exp_result10, strlen(exp_result10) + 1))
> +               goto check_stringunits_2;
> +

> +       buf10[sizeof(buf10) - 1] = '\0';
> +
> +       pr_warn("Test 'test_string_get_size' failed!\n");
> +       pr_warn("string_get_size(size = %llu, blk_size = %u, units = %s)\n",
> +               size, blk_size, "STRING_UNITS_10");
> +       pr_warn("expected: '%s', got '%s'\n", exp_result10, buf10);

Looks to me as a helper function

test_string_get_size_pr_err(size, blk_size, units, exp_result, buf, buflen) {}

if (memcmp(buf10, exp_result10, strlen(exp_result10) + 1))
 _pr_err(...);

> +
> +check_stringunits_2:
> +       if (!memcmp(buf2, exp_result2, strlen(exp_result2) + 1))
> +               return;
> +
> +       buf2[sizeof(buf2) - 1] = '\0';
> +
> +       pr_warn("Test 'test_string_get_size' failed!\n");
> +       pr_warn("string_get_size(size = %llu, blk_size = %u, units = %s)\n",
> +               size, blk_size, "STRING_UNITS_2");
> +       pr_warn("expected: '%s', got '%s'\n", exp_result2, buf2);
> +}
> +
> +static __init void test_string_get_size(void)
> +{
> +       test_string_get_size_one(16384, 512, "8.38 MB", "8.00 MiB");
> +       test_string_get_size_one(500118192, 512, "256 GB", "238 GiB");
> +       test_string_get_size_one(8192, 4096, "33.5 MB", "32.0 MiB");
> +       test_string_get_size_one(1100, 1, "1.10 kB", "1.07 KiB");
> +       test_string_get_size_one(3000, 1900, "5.70 MB", "5.43 MiB");
> +       test_string_get_size_one(U64_MAX, 4096, "75.5 ZB", "63.9 ZiB");
> +       test_string_get_size_one(1999, U32_MAX, "8.58 TB", "7.80 TiB");
> +       test_string_get_size_one(1, 512, "512 B", "512 B");
> +       test_string_get_size_one(0, 512, "0 B", "0 B");

> +}
> +
>  static int __init test_string_helpers_init(void)
>  {
>         unsigned int i;
> @@ -344,6 +399,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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258910 — [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259054 — Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface

T24gVGh1LCAyMDE1LTEwLTI5IGF0IDE3OjMwICswMTAwLCBWaXRhbHkgS3V6bmV0c292IHdyb3Rl
Og0KPiBzdHJpbmdfZ2V0X3NpemUoKSBjYW4ndCByZWFsbHkgaGFuZGxlIGh1Z2UgYmxvY2sgc2l6
ZXMsIGVzcGVjaWFsbHkNCj4gYmxrX3NpemUgPiBVMzJfTUFYIGJ1dCBzdHJpbmdfZ2V0X3NpemUo
KSBpbnRlcmZhY2Ugc3RhdGVzIHRoZSBvcHBvc2l0ZS4NCj4gQ2hhbmdlIGJsa19zaXplIGZyb20g
dTY0IHRvIHUzMiB0byByZWZsZWN0IHRoZSByZWFsaXR5Lg0KDQpXaGF0IGlzIHRoZSBhY3R1YWwg
ZXZpZGVuY2UgZm9yIHRoaXM/ICBUaGUgY2FsY3VsYXRpb24gaXMgZGVzaWduZWQgdG8gYmUNCmEg
c3ltbWV0cmljIDEyOCBiaXQgbXVsdGlwbHkuICBXaGVuIEkgd3JvdGUgYW5kIHRlc3RlZCBpdCwg
aXQgd29ya2VkDQpmaW5lIGZvciBodWdlIGJsb2NrIHNpemVzLg0KDQpKYW1lcw0KDQo+IFNpZ25l
ZC1vZmYtYnk6IFZpdGFseSBLdXpuZXRzb3YgPHZrdXpuZXRzQHJlZGhhdC5jb20+DQo+IC0tLQ0K
PiAgaW5jbHVkZS9saW51eC9zdHJpbmdfaGVscGVycy5oIHwgMiArLQ0KPiAgbGliL3N0cmluZ19o
ZWxwZXJzLmMgICAgICAgICAgIHwgNCArKy0tDQo+ICAyIGZpbGVzIGNoYW5nZWQsIDMgaW5zZXJ0
aW9ucygrKSwgMyBkZWxldGlvbnMoLSkNCj4gDQo+IGRpZmYgLS1naXQgYS9pbmNsdWRlL2xpbnV4
L3N0cmluZ19oZWxwZXJzLmggYi9pbmNsdWRlL2xpbnV4L3N0cmluZ19oZWxwZXJzLmgNCj4gaW5k
ZXggZGFiZTY0My4uMTIyM2U4MCAxMDA2NDQNCj4gLS0tIGEvaW5jbHVkZS9saW51eC9zdHJpbmdf
aGVscGVycy5oDQo+ICsrKyBiL2luY2x1ZGUvbGludXgvc3RyaW5nX2hlbHBlcnMuaA0KPiBAQCAt
MTAsNyArMTAsNyBAQCBlbnVtIHN0cmluZ19zaXplX3VuaXRzIHsNCj4gIAlTVFJJTkdfVU5JVFNf
MiwJCS8qIHVzZSBiaW5hcnkgcG93ZXJzIG9mIDJeMTAgKi8NCj4gIH07DQo+ICANCj4gLXZvaWQg
c3RyaW5nX2dldF9zaXplKHU2NCBzaXplLCB1NjQgYmxrX3NpemUsIGVudW0gc3RyaW5nX3NpemVf
dW5pdHMgdW5pdHMsDQo+ICt2b2lkIHN0cmluZ19nZXRfc2l6ZSh1NjQgc2l6ZSwgdTMyIGJsa19z
aXplLCBlbnVtIHN0cmluZ19zaXplX3VuaXRzIHVuaXRzLA0KPiAgCQkgICAgIGNoYXIgKmJ1Ziwg
aW50IGxlbik7DQo+ICANCj4gICNkZWZpbmUgVU5FU0NBUEVfU1BBQ0UJCTB4MDENCj4gZGlmZiAt
LWdpdCBhL2xpYi9zdHJpbmdfaGVscGVycy5jIGIvbGliL3N0cmluZ19oZWxwZXJzLmMNCj4gaW5k
ZXggNTkzOWY2My4uZjZjMjdkYyAxMDA2NDQNCj4gLS0tIGEvbGliL3N0cmluZ19oZWxwZXJzLmMN
Cj4gKysrIGIvbGliL3N0cmluZ19oZWxwZXJzLmMNCj4gQEAgLTI2LDcgKzI2LDcgQEANCj4gICAq
IGF0IGxlYXN0IDkgYnl0ZXMgYW5kIHdpbGwgYWx3YXlzIGJlIHplcm8gdGVybWluYXRlZC4NCj4g
ICAqDQo+ICAgKi8NCj4gLXZvaWQgc3RyaW5nX2dldF9zaXplKHU2NCBzaXplLCB1NjQgYmxrX3Np
emUsIGNvbnN0IGVudW0gc3RyaW5nX3NpemVfdW5pdHMgdW5pdHMsDQo+ICt2b2lkIHN0cmluZ19n
ZXRfc2l6ZSh1NjQgc2l6ZSwgdTMyIGJsa19zaXplLCBjb25zdCBlbnVtIHN0cmluZ19zaXplX3Vu
aXRzIHVuaXRzLA0KPiAgCQkgICAgIGNoYXIgKmJ1ZiwgaW50IGxlbikNCj4gIHsNCj4gIAlzdGF0
aWMgY29uc3QgY2hhciAqY29uc3QgdW5pdHNfMTBbXSA9IHsNCj4gQEAgLTU4LDcgKzU4LDcgQEAg
dm9pZCBzdHJpbmdfZ2V0X3NpemUodTY0IHNpemUsIHU2NCBibGtfc2l6ZSwgY29uc3QgZW51bSBz
dHJpbmdfc2l6ZV91bml0cyB1bml0cywNCj4gIAkJaSsrOw0KPiAgCX0NCj4gIA0KPiAtCWV4cCA9
IGRpdmlzb3JbdW5pdHNdIC8gKHUzMilibGtfc2l6ZTsNCj4gKwlleHAgPSBkaXZpc29yW3VuaXRz
XSAvIGJsa19zaXplOw0KPiAgCS8qDQo+ICAJICogc2l6ZSBtdXN0IGJlIHN0cmljdGx5IGdyZWF0
ZXIgdGhhbiBleHAgaGVyZSB0byBlbnN1cmUgdGhhdCByZW1haW5kZXINCj4gIAkgKiBpcyBncmVh
dGVyIHRoYW4gZGl2aXNvclt1bml0c10gY29taW5nIG91dCBvZiB0aGUgaWYgYmVsb3cuDQoNCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259092 — Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface

On Thu, Oct 29 2015, James Bottomley <jbottomley@odin.com> wrote:

> On Thu, 2015-10-29 at 17:30 +0100, Vitaly Kuznetsov wrote:
>> 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.
>
> What is the actual evidence for this?  The calculation is designed to be
> a symmetric 128 bit multiply.  When I wrote and tested it, it worked
> fine for huge block sizes.
>

May I politely ask how you tested it, and what you mean by "worked"? The
bug I reported last week was particularly concerning block sizes >= 1024
(e.g. the 32768, 1024 pair giving 32.7 MB where the correct output would
be 33.5 MB). Now it turns out that it was actually broken for smaller
block sizes as well. For ~13000 semirandom size,blk_size pairs, the
current code produces the wrong result in ~2100 cases. The new code
reduces that to 122 cases, all of which are off by one in the last
digit.

And I don't buy the symmetry argument either. Mathematically, it should
give the same, but your algorithm produces 2.04 MB for 512,4096 and 2.09
MB for 4096,512.

Maybe the commit message could be better, but I think it makes a lot of
sense to make blk_size u32. Breaking the symmetry between size and
blk_size is good (less likely that the arguments get swapped). It
allows a simpler implementation. It makes the generated code
smaller.

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259099 — Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface

On Fri, Oct 30 2015, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> block sizes as well. For ~13000 semirandom size,blk_size pairs,

Sorry, that should have been ~20000.

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259189 — Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface

T24gRnJpLCAyMDE1LTEwLTMwIGF0IDAwOjE5ICswMTAwLCBSYXNtdXMgVmlsbGVtb2VzIHdyb3Rl
Og0KPiBPbiBUaHUsIE9jdCAyOSAyMDE1LCBKYW1lcyBCb3R0b21sZXkgPGpib3R0b21sZXlAb2Rp
bi5jb20+IHdyb3RlOg0KPiANCj4gPiBPbiBUaHUsIDIwMTUtMTAtMjkgYXQgMTc6MzAgKzAxMDAs
IFZpdGFseSBLdXpuZXRzb3Ygd3JvdGU6DQo+ID4+IHN0cmluZ19nZXRfc2l6ZSgpIGNhbid0IHJl
YWxseSBoYW5kbGUgaHVnZSBibG9jayBzaXplcywgZXNwZWNpYWxseQ0KPiA+PiBibGtfc2l6ZSA+
IFUzMl9NQVggYnV0IHN0cmluZ19nZXRfc2l6ZSgpIGludGVyZmFjZSBzdGF0ZXMgdGhlIG9wcG9z
aXRlLg0KPiA+PiBDaGFuZ2UgYmxrX3NpemUgZnJvbSB1NjQgdG8gdTMyIHRvIHJlZmxlY3QgdGhl
IHJlYWxpdHkuDQo+ID4NCj4gPiBXaGF0IGlzIHRoZSBhY3R1YWwgZXZpZGVuY2UgZm9yIHRoaXM/
ICBUaGUgY2FsY3VsYXRpb24gaXMgZGVzaWduZWQgdG8gYmUNCj4gPiBhIHN5bW1ldHJpYyAxMjgg
Yml0IG11bHRpcGx5LiAgV2hlbiBJIHdyb3RlIGFuZCB0ZXN0ZWQgaXQsIGl0IHdvcmtlZA0KPiA+
IGZpbmUgZm9yIGh1Z2UgYmxvY2sgc2l6ZXMuDQo+ID4NCj4gDQo+IE1heSBJIHBvbGl0ZWx5IGFz
ayBob3cgeW91IHRlc3RlZCBpdCwgYW5kIHdoYXQgeW91IG1lYW4gYnkgIndvcmtlZCI/IFRoZQ0K
PiBidWcgSSByZXBvcnRlZCBsYXN0IHdlZWsgd2FzIHBhcnRpY3VsYXJseSBjb25jZXJuaW5nIGJs
b2NrIHNpemVzID49IDEwMjQNCj4gKGUuZy4gdGhlIDMyNzY4LCAxMDI0IHBhaXIgZ2l2aW5nIDMy
LjcgTUIgd2hlcmUgdGhlIGNvcnJlY3Qgb3V0cHV0IHdvdWxkDQo+IGJlIDMzLjUgTUIpLg0KDQpU
aGUgdGVzdCB3YXMgYmFzaWNhbGx5IGEgdXNlcnNwYWNlIHZlcnNpb24gcmV2ZXJzaW5nIHRoZSBs
YXJnZSBzaXplDQpzbWFsbGVyIGJsb2NrIHNpemUgbnVtYmVycyBhbmQgdmVyaWZ5aW5nIHRoZXkg
cHJvZHVjZSB0aGUgc2FtZSBvdXRwdXQuDQoNCj4gIE5vdyBpdCB0dXJucyBvdXQgdGhhdCBpdCB3
YXMgYWN0dWFsbHkgYnJva2VuIGZvciBzbWFsbGVyDQo+IGJsb2NrIHNpemVzIGFzIHdlbGwuIEZv
ciB+MTMwMDAgc2VtaXJhbmRvbSBzaXplLGJsa19zaXplIHBhaXJzLCB0aGUNCj4gY3VycmVudCBj
b2RlIHByb2R1Y2VzIHRoZSB3cm9uZyByZXN1bHQgaW4gfjIxMDAgY2FzZXMuIFRoZSBuZXcgY29k
ZQ0KPiByZWR1Y2VzIHRoYXQgdG8gMTIyIGNhc2VzLCBhbGwgb2Ygd2hpY2ggYXJlIG9mZiBieSBv
bmUgaW4gdGhlIGxhc3QNCj4gZGlnaXQuDQoNCkkgd2Fzbid0IG1ha2luZyB0aGUgcG9pbnQgdGhh
dCB0aGVyZSBpc24ndCBhIHBvdGVudGlhbCBvZmYgYnkgYSBjb3VwbGUNCm9mIHBlcmNlbnQgcHJv
YmxlbSBpbiB0aGUgYWxnb3JpdGhtIEkgd2FzIG1ha2luZyB0aGUgcG9pbnQgdGhhdCBpdA0Kc2hv
dWxkIHdvcmsgYXMgYSBtdWx0aXBsaWVyIG9mIHR3byB1NjQgbnVtYmVycywgc28gSSBjYW4ndCB1
bmRlcnN0YW5kDQp0aGUgcmF0aW9uYWwgYmFzaXMgZm9yIHJlZHVjaW5nIHRoZSBibG9jayBzaXpl
IHRvIHUzMi4NCg0KPiBBbmQgSSBkb24ndCBidXkgdGhlIHN5bW1ldHJ5IGFyZ3VtZW50IGVpdGhl
ci4gTWF0aGVtYXRpY2FsbHksIGl0IHNob3VsZA0KPiBnaXZlIHRoZSBzYW1lLCBidXQgeW91ciBh
bGdvcml0aG0gcHJvZHVjZXMgMi4wNCBNQiBmb3IgNTEyLDQwOTYgYW5kIDIuMDkNCj4gTUIgZm9y
IDQwOTYsNTEyLg0KDQpUaGF0J3MgYW4gb2ZmIGJ5IDIuNSU7IGl0IG1lYW5zIHRoZXJlJ3MgYSBz
bGlnaHQgZXJyb3IgaW4gb25lIG9mIHRoZQ0KY2FycmllcyBpdCBkb2Vzbid0IG1lYW4gdGhlcmUn
cyBhIGZ1bmRhbWVudGFsIHByb2JsZW0gaW4gdGhlIGFsZ29yaXRobS4NCg0KPiBNYXliZSB0aGUg
Y29tbWl0IG1lc3NhZ2UgY291bGQgYmUgYmV0dGVyLCBidXQgSSB0aGluayBpdCBtYWtlcyBhIGxv
dCBvZg0KPiBzZW5zZSB0byBtYWtlIGJsa19zaXplIHUzMi4gQnJlYWtpbmcgdGhlIHN5bW1ldHJ5
IGJldHdlZW4gc2l6ZSBhbmQNCj4gYmxrX3NpemUgaXMgZ29vZCAobGVzcyBsaWtlbHkgdGhhdCB0
aGUgYXJndW1lbnRzIGdldCBzd2FwcGVkKS4gSXQNCj4gYWxsb3dzIGEgc2ltcGxlciBpbXBsZW1l
bnRhdGlvbi4gSXQgbWFrZXMgdGhlIGdlbmVyYXRlZCBjb2RlDQo+IHNtYWxsZXIuDQoNClRoZSBk
cml2ZSB2ZW5kb3JzIGFyZSBhbHJlYWR5IHB1c2hpbmcgaHVnZSBibG9jayBzaXplIHN5c3RlbXMg
Zm9yIFpCQy4NClRoZXkncmUgYWxyZWFkeSB0YWxraW5nIGFib3V0IDJHQiBzZWN0b3JzLCB3aGlj
aCBpcyAzMSBiaXRzIC4uLiB0aGV5J2xsDQpiZSBvdmVyIHRoZSAzMiBiaXQgbGltaXQgZmFpcmx5
IHNob3J0bHksIEkgcHJlZGljdCwgc28gaXQgbWFrZXMgbm8gc2Vuc2UNCnRvIGhhdmUgdG8gaGF2
ZSB0aGUgc3RvcmFnZSBsYXllciBkbyBzaWxseSBiaXQgc2hpZnRpbmcgYmVjYXVzZSB3ZSB3ZXJl
DQpzaG9ydCBzaWdodGVkIGVub3VnaCB0byBjYXAgYmxvY2sgc2l6ZSB0byBhIHUzMi4NCg0KSmFt
ZXMNCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259390 — Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface

James Bottomley <jbottomley@odin.com> writes:

> On Thu, 2015-10-29 at 17:30 +0100, Vitaly Kuznetsov wrote:
>> 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.
>
> What is the actual evidence for this?  The calculation is designed to be
> a symmetric 128 bit multiply.  When I wrote and tested it, it worked
> fine for huge block sizes.

We have 'u32 remainder' and then we do:

exp = divisor[units] / (u32)blk_size;
...
remainder = do_div(size, divisor[units]);
remainder *= blk_size;

I'm pretty sure it will overflow for some inputs.

>
> James
>
>> 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.

-- 
  Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259875 — Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface

T24gRnJpLCAyMDE1LTEwLTMwIGF0IDExOjQ2ICswMTAwLCBWaXRhbHkgS3V6bmV0c292IHdyb3Rl
Og0KPiBKYW1lcyBCb3R0b21sZXkgPGpib3R0b21sZXlAb2Rpbi5jb20+IHdyaXRlczoNCj4gDQo+
ID4gT24gVGh1LCAyMDE1LTEwLTI5IGF0IDE3OjMwICswMTAwLCBWaXRhbHkgS3V6bmV0c292IHdy
b3RlOg0KPiA+PiBzdHJpbmdfZ2V0X3NpemUoKSBjYW4ndCByZWFsbHkgaGFuZGxlIGh1Z2UgYmxv
Y2sgc2l6ZXMsIGVzcGVjaWFsbHkNCj4gPj4gYmxrX3NpemUgPiBVMzJfTUFYIGJ1dCBzdHJpbmdf
Z2V0X3NpemUoKSBpbnRlcmZhY2Ugc3RhdGVzIHRoZSBvcHBvc2l0ZS4NCj4gPj4gQ2hhbmdlIGJs
a19zaXplIGZyb20gdTY0IHRvIHUzMiB0byByZWZsZWN0IHRoZSByZWFsaXR5Lg0KPiA+DQo+ID4g
V2hhdCBpcyB0aGUgYWN0dWFsIGV2aWRlbmNlIGZvciB0aGlzPyAgVGhlIGNhbGN1bGF0aW9uIGlz
IGRlc2lnbmVkIHRvIGJlDQo+ID4gYSBzeW1tZXRyaWMgMTI4IGJpdCBtdWx0aXBseS4gIFdoZW4g
SSB3cm90ZSBhbmQgdGVzdGVkIGl0LCBpdCB3b3JrZWQNCj4gPiBmaW5lIGZvciBodWdlIGJsb2Nr
IHNpemVzLg0KPiANCj4gV2UgaGF2ZSAndTMyIHJlbWFpbmRlcicgYW5kIHRoZW4gd2UgZG86DQo+
IA0KPiBleHAgPSBkaXZpc29yW3VuaXRzXSAvICh1MzIpYmxrX3NpemU7DQo+IC4uLg0KPiByZW1h
aW5kZXIgPSBkb19kaXYoc2l6ZSwgZGl2aXNvclt1bml0c10pOw0KPiByZW1haW5kZXIgKj0gYmxr
X3NpemU7DQo+IA0KPiBJJ20gcHJldHR5IHN1cmUgaXQgd2lsbCBvdmVyZmxvdyBmb3Igc29tZSBp
bnB1dHMuDQoNCkl0IHNob3VsZG4ndDsgdGhlIGZ1bGwgY29kZSBzbmlwcGV0IGRvZXMgdGhpczoN
Cg0KICAgICAgICAJd2hpbGUgKGJsa19zaXplID49IGRpdmlzb3JbdW5pdHNdKSB7DQogICAgICAg
IAkJcmVtYWluZGVyID0gZG9fZGl2KGJsa19zaXplLCBkaXZpc29yW3VuaXRzXSk7DQogICAgICAg
IAkJaSsrOw0KICAgICAgICAJfQ0KICAgICAgICANCiAgICAgICAgCWV4cCA9IGRpdmlzb3JbdW5p
dHNdIC8gKHUzMilibGtfc2l6ZTsNCg0KU28gYnkgdGhlIHRpbWUgaXQgcmVhY2hlcyB0aGUgc3Rh
dGVtZW50IHlvdSBjb21wbGFpbiBhYm91dCwgYmxrX3NpemUgaXMNCmFscmVhZHkgbGVzcyB0aGFu
IG9yIGVxdWFsIHRvIHRoZSBkaXZpc29yICh3aGljaCBpcyAxMDAwIG9yIDEwMjQpIHNvDQp0cnVu
Y2F0aW5nIHRvIDMyIGJpdHMgaXMgYWx3YXlzIGNvcnJlY3QuDQoNCkknbSBzb3J0IG9mIGdldHRp
bmcgdGhlIGltcHJlc3Npb24geW91IGRvbid0IHF1aXRlIHVuZGVyc3RhbmQgdGhlDQptYXRoZW1h
dGljczogIGkgaXMgdGhlIGxvZ2FyaXRobSB0byB0aGUgYmFzZSBkaXZpc29yW3VuaXRzXS4gIFdl
IHJlZHVjZQ0KYm90aCBvcGVyYW5kcyB0byBleHBvbmVudHMgb2YgdGhlIGxvZ2FyaXRobSBiYXNl
IChhZGRpbmcgdGhlIHR3byBiYXNlcw0KdG9nZXRoZXIgaW4gaSksIHdoaWNoIG1lYW5zIHRoZXkg
YXJlIGJ5IGRlZmluaXRpb24gaW4gYSByYW5nZSBiZXR3ZWVuDQp6ZXJvIGFuZCB0aGUgYmFzZSBh
bmQgdGhlbiBtdWx0aXBseSB0aGUgcmVtYWluaW5nIGV4cG9uZW50cyBjb3JyZWN0aW5nDQp0aGUg
cmVzdWx0IGZvciBhIGJhc2Ugb3ZlcmZsb3cgKHNvIHRoZSByZXN1bHQgaXMgYWx3YXlzIGEgY29y
cmVjdA0KZXhwb25lbnQgYW5kIGkgaXMgdGhlIGxvZ2FyaXRobSB0byB0aGUgYmFzZSkuICBJdCdz
IGFjdHVhbGx5IHNpbXBseQ0KTmFwaWVyJ3MgYWxnb3JpdGhtLg0KDQpUaGUgcmVhc29uIHdlJ3Jl
IGdldHRpbmcgdGhlIHVwIHRvIDIuNSUgcm91bmRpbmcgZXJyb3JzIHlvdSBjb21wbGFpbg0KYWJv
dXQgaXMgYmVjYXVzZSBhdCBlYWNoIGxvZ2FyaXRobSB1bnRpbCB0aGUgbGFzdCBvbmUsIHdlIHRo
cm93IGF3YXkgdGhlDQpyZW1haW5kZXIgKGl0J3MgbGVnaXRpbWF0ZSBiZWNhdXNlIGl0J3MgYWx3
YXlzIDEwMDB4IHNtYWxsZXIgdGhhbiB0aGUNCmV4cG9uZW50KSwgYnV0IGluIHRoZSBjYXNlIG9m
IGEgbGFyZ2UgcmVtYWluZGVyIGl0IHByb3ZpZGVzIGEgc21hbGwNCmNvcnJlY3Rpb24gdG8gdGhl
IGZpbmFsIG9wZXJhdGlvbiB3aGljaCB3ZSBkb24ndCBhY2NvdW50IGZvci4gIElmIHlvdQ0Kd2Fu
dCB0byBtYWtlIGEgdHJ1ZSBjb3JyZWN0aW9uLCB5b3Ugc2F2ZSB0aGUgcGVudWx0aW1hdGUgcmVz
aWR1ZSBpbiBlYWNoDQpjYXNlLCBtdWx0aXBseSBlYWNoIGJ5IHRoZSAqb3RoZXIqIGV4cG9uZW50
IGFkZCB0aGVtIHRvZ2V0aGVyLCBkaXZpZGUgYnkNCnRoZSBiYXNlIGFuZCBpbmNyZW1lbnQgdGhl
IGZpbmFsIHJlc3VsdCBieSB0aGUgcmVtYWluZGVyLg0KDQpIb3dldmVyLCBmb3IgMi41JSB0aGUg
cGh5c2ljaXN0IGluIG1lIHNheXMgdGhlIGFib3ZlIGlzIHdheSBvdmVya2lsbC4NCg0KSmFtZXMN
Cg0KPiA+DQo+ID4gSmFtZXMNCj4gPg0KPiA+PiBTaWduZWQtb2ZmLWJ5OiBWaXRhbHkgS3V6bmV0
c292IDx2a3V6bmV0c0ByZWRoYXQuY29tPg0KPiA+PiAtLS0NCj4gPj4gIGluY2x1ZGUvbGludXgv
c3RyaW5nX2hlbHBlcnMuaCB8IDIgKy0NCj4gPj4gIGxpYi9zdHJpbmdfaGVscGVycy5jICAgICAg
ICAgICB8IDQgKystLQ0KPiA+PiAgMiBmaWxlcyBjaGFuZ2VkLCAzIGluc2VydGlvbnMoKyksIDMg
ZGVsZXRpb25zKC0pDQo+ID4+IA0KPiA+PiBkaWZmIC0tZ2l0IGEvaW5jbHVkZS9saW51eC9zdHJp
bmdfaGVscGVycy5oIGIvaW5jbHVkZS9saW51eC9zdHJpbmdfaGVscGVycy5oDQo+ID4+IGluZGV4
IGRhYmU2NDMuLjEyMjNlODAgMTAwNjQ0DQo+ID4+IC0tLSBhL2luY2x1ZGUvbGludXgvc3RyaW5n
X2hlbHBlcnMuaA0KPiA+PiArKysgYi9pbmNsdWRlL2xpbnV4L3N0cmluZ19oZWxwZXJzLmgNCj4g
Pj4gQEAgLTEwLDcgKzEwLDcgQEAgZW51bSBzdHJpbmdfc2l6ZV91bml0cyB7DQo+ID4+ICAJU1RS
SU5HX1VOSVRTXzIsCQkvKiB1c2UgYmluYXJ5IHBvd2VycyBvZiAyXjEwICovDQo+ID4+ICB9Ow0K
PiA+PiAgDQo+ID4+IC12b2lkIHN0cmluZ19nZXRfc2l6ZSh1NjQgc2l6ZSwgdTY0IGJsa19zaXpl
LCBlbnVtIHN0cmluZ19zaXplX3VuaXRzIHVuaXRzLA0KPiA+PiArdm9pZCBzdHJpbmdfZ2V0X3Np
emUodTY0IHNpemUsIHUzMiBibGtfc2l6ZSwgZW51bSBzdHJpbmdfc2l6ZV91bml0cyB1bml0cywN
Cj4gPj4gIAkJICAgICBjaGFyICpidWYsIGludCBsZW4pOw0KPiA+PiAgDQo+ID4+ICAjZGVmaW5l
IFVORVNDQVBFX1NQQUNFCQkweDAxDQo+ID4+IGRpZmYgLS1naXQgYS9saWIvc3RyaW5nX2hlbHBl
cnMuYyBiL2xpYi9zdHJpbmdfaGVscGVycy5jDQo+ID4+IGluZGV4IDU5MzlmNjMuLmY2YzI3ZGMg
MTAwNjQ0DQo+ID4+IC0tLSBhL2xpYi9zdHJpbmdfaGVscGVycy5jDQo+ID4+ICsrKyBiL2xpYi9z
dHJpbmdfaGVscGVycy5jDQo+ID4+IEBAIC0yNiw3ICsyNiw3IEBADQo+ID4+ICAgKiBhdCBsZWFz
dCA5IGJ5dGVzIGFuZCB3aWxsIGFsd2F5cyBiZSB6ZXJvIHRlcm1pbmF0ZWQuDQo+ID4+ICAgKg0K
PiA+PiAgICovDQo+ID4+IC12b2lkIHN0cmluZ19nZXRfc2l6ZSh1NjQgc2l6ZSwgdTY0IGJsa19z
aXplLCBjb25zdCBlbnVtIHN0cmluZ19zaXplX3VuaXRzIHVuaXRzLA0KPiA+PiArdm9pZCBzdHJp
bmdfZ2V0X3NpemUodTY0IHNpemUsIHUzMiBibGtfc2l6ZSwgY29uc3QgZW51bSBzdHJpbmdfc2l6
ZV91bml0cyB1bml0cywNCj4gPj4gIAkJICAgICBjaGFyICpidWYsIGludCBsZW4pDQo+ID4+ICB7
DQo+ID4+ICAJc3RhdGljIGNvbnN0IGNoYXIgKmNvbnN0IHVuaXRzXzEwW10gPSB7DQo+ID4+IEBA
IC01OCw3ICs1OCw3IEBAIHZvaWQgc3RyaW5nX2dldF9zaXplKHU2NCBzaXplLCB1NjQgYmxrX3Np
emUsIGNvbnN0IGVudW0gc3RyaW5nX3NpemVfdW5pdHMgdW5pdHMsDQo+ID4+ICAJCWkrKzsNCj4g
Pj4gIAl9DQo+ID4+ICANCj4gPj4gLQlleHAgPSBkaXZpc29yW3VuaXRzXSAvICh1MzIpYmxrX3Np
emU7DQo+ID4+ICsJZXhwID0gZGl2aXNvclt1bml0c10gLyBibGtfc2l6ZTsNCj4gPj4gIAkvKg0K
PiA+PiAgCSAqIHNpemUgbXVzdCBiZSBzdHJpY3RseSBncmVhdGVyIHRoYW4gZXhwIGhlcmUgdG8g
ZW5zdXJlIHRoYXQgcmVtYWluZGVyDQo+ID4+ICAJICogaXMgZ3JlYXRlciB0aGFuIGRpdmlzb3Jb
dW5pdHNdIGNvbWluZyBvdXQgb2YgdGhlIGlmIGJlbG93Lg0KPiANCg0KDQo=
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1260744 — Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface

James Bottomley <jbottomley@odin.com> writes:

> On Fri, 2015-10-30 at 11:46 +0100, Vitaly Kuznetsov wrote:
>> James Bottomley <jbottomley@odin.com> writes:
>> 
>> > On Thu, 2015-10-29 at 17:30 +0100, Vitaly Kuznetsov wrote:
>> >> 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.
>> >
>> > What is the actual evidence for this?  The calculation is designed to be
>> > a symmetric 128 bit multiply.  When I wrote and tested it, it worked
>> > fine for huge block sizes.
>> 
>> We have 'u32 remainder' and then we do:
>> 
>> exp = divisor[units] / (u32)blk_size;
>> ...
>> remainder = do_div(size, divisor[units]);
>> remainder *= blk_size;
>> 
>> I'm pretty sure it will overflow for some inputs.
>
> It shouldn't; the full code snippet does this:
>
>         	while (blk_size >= divisor[units]) {
>         		remainder = do_div(blk_size, divisor[units]);
>         		i++;
>         	}
>
>         	exp = divisor[units] / (u32)blk_size;
>
> So by the time it reaches the statement you complain about, blk_size is
> already less than or equal to the divisor (which is 1000 or 1024) so
> truncating to 32 bits is always correct.
>

I overlooked, sorry!

> I'm sort of getting the impression you don't quite understand the
> mathematics:  i is the logarithm to the base divisor[units].  We reduce
> both operands to exponents of the logarithm base (adding the two bases
> together in i), which means they are by definition in a range between
> zero and the base and then multiply the remaining exponents correcting
> the result for a base overflow (so the result is always a correct
> exponent and i is the logarithm to the base).  It's actually simply
> Napier's algorithm.
>
> The reason we're getting the up to 2.5% rounding errors you complain
> about is because at each logarithm until the last one, we throw away the
> remainder (it's legitimate because it's always 1000x smaller than the
> exponent), but in the case of a large remainder it provides a small
> correction to the final operation which we don't account for.  If you
> want to make a true correction, you save the penultimate residue in each
> case, multiply each by the *other* exponent add them together, divide by
> the base and increment the final result by the remainder.

My assumption was that we don't really need to support blk_sizes >
U32_MAX and we can simplify string_get_size() instead of adding
additional complexity. Apparently, the assumption was wrong.

>
> However, for 2.5% the physicist in me says the above is way overkill.
>

It is getting was over 2.5% if blk_size is not a power of 2. While it is
probably never the case for block subsystem the function is in lib and
pretends to be general-enough. I'll try to make proper correction and
let's see if it's worth the effort. 

Thanks,

> James
>
>> >
>> > James
>> >
>> >> 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.
>> 

-- 
  Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261179 — Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface

T24gTW9uLCAyMDE1LTExLTAyIGF0IDE2OjU4ICswMTAwLCBWaXRhbHkgS3V6bmV0c292IHdyb3Rl
Og0KPiBKYW1lcyBCb3R0b21sZXkgPGpib3R0b21sZXlAb2Rpbi5jb20+IHdyaXRlczoNCj4gDQo+
ID4gT24gRnJpLCAyMDE1LTEwLTMwIGF0IDExOjQ2ICswMTAwLCBWaXRhbHkgS3V6bmV0c292IHdy
b3RlOg0KPiA+PiBKYW1lcyBCb3R0b21sZXkgPGpib3R0b21sZXlAb2Rpbi5jb20+IHdyaXRlczoN
Cj4gPj4gDQo+ID4+ID4gT24gVGh1LCAyMDE1LTEwLTI5IGF0IDE3OjMwICswMTAwLCBWaXRhbHkg
S3V6bmV0c292IHdyb3RlOg0KPiA+PiA+PiBzdHJpbmdfZ2V0X3NpemUoKSBjYW4ndCByZWFsbHkg
aGFuZGxlIGh1Z2UgYmxvY2sgc2l6ZXMsIGVzcGVjaWFsbHkNCj4gPj4gPj4gYmxrX3NpemUgPiBV
MzJfTUFYIGJ1dCBzdHJpbmdfZ2V0X3NpemUoKSBpbnRlcmZhY2Ugc3RhdGVzIHRoZSBvcHBvc2l0
ZS4NCj4gPj4gPj4gQ2hhbmdlIGJsa19zaXplIGZyb20gdTY0IHRvIHUzMiB0byByZWZsZWN0IHRo
ZSByZWFsaXR5Lg0KPiA+PiA+DQo+ID4+ID4gV2hhdCBpcyB0aGUgYWN0dWFsIGV2aWRlbmNlIGZv
ciB0aGlzPyAgVGhlIGNhbGN1bGF0aW9uIGlzIGRlc2lnbmVkIHRvIGJlDQo+ID4+ID4gYSBzeW1t
ZXRyaWMgMTI4IGJpdCBtdWx0aXBseS4gIFdoZW4gSSB3cm90ZSBhbmQgdGVzdGVkIGl0LCBpdCB3
b3JrZWQNCj4gPj4gPiBmaW5lIGZvciBodWdlIGJsb2NrIHNpemVzLg0KPiA+PiANCj4gPj4gV2Ug
aGF2ZSAndTMyIHJlbWFpbmRlcicgYW5kIHRoZW4gd2UgZG86DQo+ID4+IA0KPiA+PiBleHAgPSBk
aXZpc29yW3VuaXRzXSAvICh1MzIpYmxrX3NpemU7DQo+ID4+IC4uLg0KPiA+PiByZW1haW5kZXIg
PSBkb19kaXYoc2l6ZSwgZGl2aXNvclt1bml0c10pOw0KPiA+PiByZW1haW5kZXIgKj0gYmxrX3Np
emU7DQo+ID4+IA0KPiA+PiBJJ20gcHJldHR5IHN1cmUgaXQgd2lsbCBvdmVyZmxvdyBmb3Igc29t
ZSBpbnB1dHMuDQo+ID4NCj4gPiBJdCBzaG91bGRuJ3Q7IHRoZSBmdWxsIGNvZGUgc25pcHBldCBk
b2VzIHRoaXM6DQo+ID4NCj4gPiAgICAgICAgIAl3aGlsZSAoYmxrX3NpemUgPj0gZGl2aXNvclt1
bml0c10pIHsNCj4gPiAgICAgICAgIAkJcmVtYWluZGVyID0gZG9fZGl2KGJsa19zaXplLCBkaXZp
c29yW3VuaXRzXSk7DQo+ID4gICAgICAgICAJCWkrKzsNCj4gPiAgICAgICAgIAl9DQo+ID4NCj4g
PiAgICAgICAgIAlleHAgPSBkaXZpc29yW3VuaXRzXSAvICh1MzIpYmxrX3NpemU7DQo+ID4NCj4g
PiBTbyBieSB0aGUgdGltZSBpdCByZWFjaGVzIHRoZSBzdGF0ZW1lbnQgeW91IGNvbXBsYWluIGFi
b3V0LCBibGtfc2l6ZSBpcw0KPiA+IGFscmVhZHkgbGVzcyB0aGFuIG9yIGVxdWFsIHRvIHRoZSBk
aXZpc29yICh3aGljaCBpcyAxMDAwIG9yIDEwMjQpIHNvDQo+ID4gdHJ1bmNhdGluZyB0byAzMiBi
aXRzIGlzIGFsd2F5cyBjb3JyZWN0Lg0KPiA+DQo+IA0KPiBJIG92ZXJsb29rZWQsIHNvcnJ5IQ0K
PiANCj4gPiBJJ20gc29ydCBvZiBnZXR0aW5nIHRoZSBpbXByZXNzaW9uIHlvdSBkb24ndCBxdWl0
ZSB1bmRlcnN0YW5kIHRoZQ0KPiA+IG1hdGhlbWF0aWNzOiAgaSBpcyB0aGUgbG9nYXJpdGhtIHRv
IHRoZSBiYXNlIGRpdmlzb3JbdW5pdHNdLiAgV2UgcmVkdWNlDQo+ID4gYm90aCBvcGVyYW5kcyB0
byBleHBvbmVudHMgb2YgdGhlIGxvZ2FyaXRobSBiYXNlIChhZGRpbmcgdGhlIHR3byBiYXNlcw0K
PiA+IHRvZ2V0aGVyIGluIGkpLCB3aGljaCBtZWFucyB0aGV5IGFyZSBieSBkZWZpbml0aW9uIGlu
IGEgcmFuZ2UgYmV0d2Vlbg0KPiA+IHplcm8gYW5kIHRoZSBiYXNlIGFuZCB0aGVuIG11bHRpcGx5
IHRoZSByZW1haW5pbmcgZXhwb25lbnRzIGNvcnJlY3RpbmcNCj4gPiB0aGUgcmVzdWx0IGZvciBh
IGJhc2Ugb3ZlcmZsb3cgKHNvIHRoZSByZXN1bHQgaXMgYWx3YXlzIGEgY29ycmVjdA0KPiA+IGV4
cG9uZW50IGFuZCBpIGlzIHRoZSBsb2dhcml0aG0gdG8gdGhlIGJhc2UpLiAgSXQncyBhY3R1YWxs
eSBzaW1wbHkNCj4gPiBOYXBpZXIncyBhbGdvcml0aG0uDQo+ID4NCj4gPiBUaGUgcmVhc29uIHdl
J3JlIGdldHRpbmcgdGhlIHVwIHRvIDIuNSUgcm91bmRpbmcgZXJyb3JzIHlvdSBjb21wbGFpbg0K
PiA+IGFib3V0IGlzIGJlY2F1c2UgYXQgZWFjaCBsb2dhcml0aG0gdW50aWwgdGhlIGxhc3Qgb25l
LCB3ZSB0aHJvdyBhd2F5IHRoZQ0KPiA+IHJlbWFpbmRlciAoaXQncyBsZWdpdGltYXRlIGJlY2F1
c2UgaXQncyBhbHdheXMgMTAwMHggc21hbGxlciB0aGFuIHRoZQ0KPiA+IGV4cG9uZW50KSwgYnV0
IGluIHRoZSBjYXNlIG9mIGEgbGFyZ2UgcmVtYWluZGVyIGl0IHByb3ZpZGVzIGEgc21hbGwNCj4g
PiBjb3JyZWN0aW9uIHRvIHRoZSBmaW5hbCBvcGVyYXRpb24gd2hpY2ggd2UgZG9uJ3QgYWNjb3Vu
dCBmb3IuICBJZiB5b3UNCj4gPiB3YW50IHRvIG1ha2UgYSB0cnVlIGNvcnJlY3Rpb24sIHlvdSBz
YXZlIHRoZSBwZW51bHRpbWF0ZSByZXNpZHVlIGluIGVhY2gNCj4gPiBjYXNlLCBtdWx0aXBseSBl
YWNoIGJ5IHRoZSAqb3RoZXIqIGV4cG9uZW50IGFkZCB0aGVtIHRvZ2V0aGVyLCBkaXZpZGUgYnkN
Cj4gPiB0aGUgYmFzZSBhbmQgaW5jcmVtZW50IHRoZSBmaW5hbCByZXN1bHQgYnkgdGhlIHJlbWFp
bmRlci4NCj4gDQo+IE15IGFzc3VtcHRpb24gd2FzIHRoYXQgd2UgZG9uJ3QgcmVhbGx5IG5lZWQg
dG8gc3VwcG9ydCBibGtfc2l6ZXMgPg0KPiBVMzJfTUFYIGFuZCB3ZSBjYW4gc2ltcGxpZnkgc3Ry
aW5nX2dldF9zaXplKCkgaW5zdGVhZCBvZiBhZGRpbmcNCj4gYWRkaXRpb25hbCBjb21wbGV4aXR5
LiBBcHBhcmVudGx5LCB0aGUgYXNzdW1wdGlvbiB3YXMgd3JvbmcuDQo+IA0KPiA+DQo+ID4gSG93
ZXZlciwgZm9yIDIuNSUgdGhlIHBoeXNpY2lzdCBpbiBtZSBzYXlzIHRoZSBhYm92ZSBpcyB3YXkg
b3ZlcmtpbGwuDQo+ID4NCj4gDQo+IEl0IGlzIGdldHRpbmcgd2FzIG92ZXIgMi41JSBpZiBibGtf
c2l6ZSBpcyBub3QgYSBwb3dlciBvZiAyLiBXaGlsZSBpdCBpcw0KPiBwcm9iYWJseSBuZXZlciB0
aGUgY2FzZSBmb3IgYmxvY2sgc3Vic3lzdGVtIHRoZSBmdW5jdGlvbiBpcyBpbiBsaWIgYW5kDQo+
IHByZXRlbmRzIHRvIGJlIGdlbmVyYWwtZW5vdWdoLiBJJ2xsIHRyeSB0byBtYWtlIHByb3BlciBj
b3JyZWN0aW9uIGFuZA0KPiBsZXQncyBzZWUgaWYgaXQncyB3b3J0aCB0aGUgZWZmb3J0LiANCg0K
T0ssIHRoaXMgaXMgdGhlIGZ1bGwgY2FsY3VsYXRpb24uICBJdCBhbHNvIGluY2x1ZGVzIGFuIGFy
aXRobWV0aWMNCnJvdW5kaW5nIHRvIHRoZSBmaW5hbCBmaWd1cmUgcHJpbnQuICBJIHN1cHBvc2Ug
aXQncyBub3QgdGhhdCBtdWNoIG1vcmUNCmNvbXBsZXhpdHkgdGhhbiB0aGUgb3JpZ2luYWwsIGFu
ZCBpdCBkb2VzIG1ha2UgdGhlIGFsZ29yaXRobSBlYXNpZXIgdG8NCnVuZGVyc3RhbmQuDQoNCldl
IGNvdWxkIGRvIHdpdGggcnVubmluZyB0aGUgY29tbWVudHMgYnkgc29tZSBvdGhlciBub24tbWF0
aGVtYXRpY2lhbiwNCm5vdyBJJ3ZlIGV4cGxhaW5lZCBpdCBpbiBkZXRhaWwgdG8geW91IHR3bywg
dG8gc2VlIGlmIHRoZXkgYWN0dWFsbHkgZ2l2ZQ0KYW4gdW5kZXJzdGFuZGluZyBvZiB0aGUgYWxn
b3JpdGhtLg0KDQpKYW1lcw0KDQotLS0NCg0KZGlmZiAtLWdpdCBhL2xpYi9zdHJpbmdfaGVscGVy
cy5jIGIvbGliL3N0cmluZ19oZWxwZXJzLmMNCmluZGV4IDU5MzlmNjMuLjFlYzdlNzdhIDEwMDY0
NA0KLS0tIGEvbGliL3N0cmluZ19oZWxwZXJzLmMNCisrKyBiL2xpYi9zdHJpbmdfaGVscGVycy5j
DQpAQCAtNDQsNyArNDQsNyBAQCB2b2lkIHN0cmluZ19nZXRfc2l6ZSh1NjQgc2l6ZSwgdTY0IGJs
a19zaXplLCBjb25zdCBlbnVtIHN0cmluZ19zaXplX3VuaXRzIHVuaXRzLA0KIAkJW1NUUklOR19V
TklUU18yXSA9IDEwMjQsDQogCX07DQogCWludCBpLCBqOw0KLQl1MzIgcmVtYWluZGVyID0gMCwg
c2ZfY2FwLCBleHA7DQorCXUzMiByZW1haW5kZXIgPSAwLCBzZl9jYXAsIHIxID0gMCwgcjIgPSAw
LCByb3VuZDsNCiAJY2hhciB0bXBbOF07DQogCWNvbnN0IGNoYXIgKnVuaXQ7DQogDQpAQCAtNTMs
MjcgKzUzLDQ2IEBAIHZvaWQgc3RyaW5nX2dldF9zaXplKHU2NCBzaXplLCB1NjQgYmxrX3NpemUs
IGNvbnN0IGVudW0gc3RyaW5nX3NpemVfdW5pdHMgdW5pdHMsDQogCWlmICghc2l6ZSkNCiAJCWdv
dG8gb3V0Ow0KIA0KKwkvKiBUaGlzIGlzIG5hcGllcidzIGFsZ29yaXRobS4gIFJlZHVjZSB0aGUg
b3JpZ2luYWwgYmxvY2sgc2l6ZSB0bw0KKwkgKg0KKwkgKiBjbyAqIGRpdmlzb3JbdW5pdHNdXmkN
CisJICoNCisJICogd2hlcmUgY28gPSBibGtfc2l6ZSArIHIxL2Rpdmlzb3JbdW5pdHNdOw0KKwkg
Kg0KKwkgKiBhbmQgdGhlIHNhbWUgZm9yIHNpemUuICBXZSBzaW1wbHkgYWRkIHRvIHRoZSBleHBv
bmVudCBpLCBiZWNhdXNlDQorCSAqIHRoZSBmaW5hbCBjYWxjdWxhdGlvbiB3ZSdyZSBsb29raW5n
IGZvciBpcw0KKwkgKg0KKwkgKiAoY28xICogY28yKSAqIGRpdmlzb3JbdW5pdHNdXmkNCisJICov
DQorDQorDQogCXdoaWxlIChibGtfc2l6ZSA+PSBkaXZpc29yW3VuaXRzXSkgew0KLQkJcmVtYWlu
ZGVyID0gZG9fZGl2KGJsa19zaXplLCBkaXZpc29yW3VuaXRzXSk7DQorCQlyMSA9IGRvX2Rpdihi
bGtfc2l6ZSwgZGl2aXNvclt1bml0c10pOw0KIAkJaSsrOw0KIAl9DQogDQotCWV4cCA9IGRpdmlz
b3JbdW5pdHNdIC8gKHUzMilibGtfc2l6ZTsNCi0JLyoNCi0JICogc2l6ZSBtdXN0IGJlIHN0cmlj
dGx5IGdyZWF0ZXIgdGhhbiBleHAgaGVyZSB0byBlbnN1cmUgdGhhdCByZW1haW5kZXINCi0JICog
aXMgZ3JlYXRlciB0aGFuIGRpdmlzb3JbdW5pdHNdIGNvbWluZyBvdXQgb2YgdGhlIGlmIGJlbG93
Lg0KLQkgKi8NCi0JaWYgKHNpemUgPiBleHApIHsNCi0JCXJlbWFpbmRlciA9IGRvX2RpdihzaXpl
LCBkaXZpc29yW3VuaXRzXSk7DQotCQlyZW1haW5kZXIgKj0gYmxrX3NpemU7DQorCXdoaWxlIChz
aXplID49IGRpdmlzb3JbdW5pdHNdKSB7DQorCQlyMiA9IGRvX2RpdihzaXplLCBkaXZpc29yW3Vu
aXRzXSk7DQogCQlpKys7DQotCX0gZWxzZSB7DQotCQlyZW1haW5kZXIgKj0gc2l6ZTsNCiAJfQ0K
IA0KLQlzaXplICo9IGJsa19zaXplOw0KLQlzaXplICs9IHJlbWFpbmRlciAvIGRpdmlzb3JbdW5p
dHNdOw0KLQlyZW1haW5kZXIgJT0gZGl2aXNvclt1bml0c107DQorCS8qIGhlcmUncyB0aGUgbWFn
aWMuICBjbzEgKiBjbzIgbWF5IGJlID4gZGl2aXNvcltpXSwgc28gY29ycmVjdCBmb3INCisJICog
dGhhdCBpbiB0aGUgZXhwb25lbnQgYW5kIG1ha2Ugc3VyZSB0aGF0IHRoZSBhZGRpdGlvbmFsIGNv
cnJlY3Rpb25zDQorCSAqIGZyb20gdGhlIHJlbWFpbmRlcnMgaXMgYWRkZWQgaW4uDQorCSAqDQor
CSAqIGNvMSAqY28yID0gKGJsa19zaXplICsgcjEvZGl2aXNvclt1bml0c10pKihzaXplICsgcjIv
ZGl2aXNvclt1bml0c10pDQorCSAqDQorCSAqIHRoZXJlZm9yZQ0KKwkgKg0KKwkgKiBjbzEqY28y
KmRpdmlzb3JbdW5pdHNdID0gYmxrX3NpemUqc2l6ZSpkaXZpc29yW3VuaXRzXSArDQorCSAqICAg
ICAgICAgIHIxKnNpemUgKyByMipzaXplICsgcjEqcjIvZGl2aXNvclt1bml0c10NCisJICoNCisJ
ICogZHJvcCB0aGUgbGFzdCB0ZXJtIGJlY2F1c2UgaXQncyB0b28gc21hbGwgYW5kIHBlcmZvcm0g
dGhlDQorCSAqIGNhbGN1bGF0aW9uIGNsZXZlcmx5IGJ5IGRlY3JlbWV0aW5nIGkgdG8gYmUgYXV0
b21hdGljYWxseSBkZWFsaW5nDQorCSAqIHdpdGggZXZlcnl0aGluZyBtdWx0aXBsaWVkIGJ5IGRp
dmlzb3JbdW5pdHNdICovDQorDQorCS0taTsNCisJc2l6ZSA9IHNpemUgKiBibGtfc2l6ZSAqIGRp
dmlzb3JbdW5pdHNdICsgcjEgKiBzaXplICsgcjIgKiBibGtfc2l6ZTsNCiANCiAJd2hpbGUgKHNp
emUgPj0gZGl2aXNvclt1bml0c10pIHsNCiAJCXJlbWFpbmRlciA9IGRvX2RpdihzaXplLCBkaXZp
c29yW3VuaXRzXSk7DQpAQCAtODEsOCArMTAwLDE1IEBAIHZvaWQgc3RyaW5nX2dldF9zaXplKHU2
NCBzaXplLCB1NjQgYmxrX3NpemUsIGNvbnN0IGVudW0gc3RyaW5nX3NpemVfdW5pdHMgdW5pdHMs
DQogCX0NCiANCiAJc2ZfY2FwID0gc2l6ZTsNCi0JZm9yIChqID0gMDsgc2ZfY2FwKjEwIDwgMTAw
MDsgaisrKQ0KKwlyb3VuZCA9IDUwMDsNCisJZm9yIChqID0gMDsgc2ZfY2FwKjEwIDwgMTAwMDsg
aisrKSB7DQogCQlzZl9jYXAgKj0gMTA7DQorCQlyb3VuZCAvPSAxMDsNCisJfQ0KKw0KKwkvKiBh
ZGQgYSA1IHRvIHRoZSBkaWdpdCBiZWxvdyB3aGF0IHdpbGwgYmUgcHJpbnRlZCB0byBlbnN1cmUN
CisJICogYW4gYXJpdGhtZXRpY2FsIHJvdW5kIHVwICovDQorCXJlbWFpbmRlciArPSByb3VuZDsN
CiANCiAJaWYgKGopIHsNCiAJCXJlbWFpbmRlciAqPSAxMDAwOw0KDQo=
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261490 — Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface

James Bottomley <jbottomley@odin.com> writes:

> On Mon, 2015-11-02 at 16:58 +0100, Vitaly Kuznetsov wrote:
>> James Bottomley <jbottomley@odin.com> writes:
>> 
>> > On Fri, 2015-10-30 at 11:46 +0100, Vitaly Kuznetsov wrote:
>> >> James Bottomley <jbottomley@odin.com> writes:
>> >> 
>> >> > On Thu, 2015-10-29 at 17:30 +0100, Vitaly Kuznetsov wrote:
>> >> >> 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.
>> >> >
>> >> > What is the actual evidence for this?  The calculation is designed to be
>> >> > a symmetric 128 bit multiply.  When I wrote and tested it, it worked
>> >> > fine for huge block sizes.
>> >> 
>> >> We have 'u32 remainder' and then we do:
>> >> 
>> >> exp = divisor[units] / (u32)blk_size;
>> >> ...
>> >> remainder = do_div(size, divisor[units]);
>> >> remainder *= blk_size;
>> >> 
>> >> I'm pretty sure it will overflow for some inputs.
>> >
>> > It shouldn't; the full code snippet does this:
>> >
>> >         	while (blk_size >= divisor[units]) {
>> >         		remainder = do_div(blk_size, divisor[units]);
>> >         		i++;
>> >         	}
>> >
>> >         	exp = divisor[units] / (u32)blk_size;
>> >
>> > So by the time it reaches the statement you complain about, blk_size is
>> > already less than or equal to the divisor (which is 1000 or 1024) so
>> > truncating to 32 bits is always correct.
>> >
>> 
>> I overlooked, sorry!
>> 
>> > I'm sort of getting the impression you don't quite understand the
>> > mathematics:  i is the logarithm to the base divisor[units].  We reduce
>> > both operands to exponents of the logarithm base (adding the two bases
>> > together in i), which means they are by definition in a range between
>> > zero and the base and then multiply the remaining exponents correcting
>> > the result for a base overflow (so the result is always a correct
>> > exponent and i is the logarithm to the base).  It's actually simply
>> > Napier's algorithm.
>> >
>> > The reason we're getting the up to 2.5% rounding errors you complain
>> > about is because at each logarithm until the last one, we throw away the
>> > remainder (it's legitimate because it's always 1000x smaller than the
>> > exponent), but in the case of a large remainder it provides a small
>> > correction to the final operation which we don't account for.  If you
>> > want to make a true correction, you save the penultimate residue in each
>> > case, multiply each by the *other* exponent add them together, divide by
>> > the base and increment the final result by the remainder.
>> 
>> My assumption was that we don't really need to support blk_sizes >
>> U32_MAX and we can simplify string_get_size() instead of adding
>> additional complexity. Apparently, the assumption was wrong.
>> 
>> >
>> > However, for 2.5% the physicist in me says the above is way overkill.
>> >
>> 
>> It is getting was over 2.5% if blk_size is not a power of 2. While it is
>> probably never the case for block subsystem the function is in lib and
>> pretends to be general-enough. I'll try to make proper correction and
>> let's see if it's worth the effort. 
>
> OK, this is the full calculation.  It also includes an arithmetic
> rounding to the final figure print.  I suppose it's not that much more
> complexity than the original, and it does make the algorithm easier to
> understand.
>
> We could do with running the comments by some other non-mathematician,
> now I've explained it in detail to you two, to see if they actually give
> an understanding of the algorithm.

Thanks, to me they look great! One nitpick below ...

>
> James
>
> ---
>
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index 5939f63..1ec7e77a 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -44,7 +44,7 @@ void string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
>  		[STRING_UNITS_2] = 1024,
>  	};
>  	int i, j;
> -	u32 remainder = 0, sf_cap, exp;
> +	u32 remainder = 0, sf_cap, r1 = 0, r2 = 0, round;
>  	char tmp[8];
>  	const char *unit;
>
> @@ -53,27 +53,46 @@ void string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
>  	if (!size)
>  		goto out;
>
> +	/* This is napier's algorithm.  Reduce the original block size to
> +	 *
> +	 * co * divisor[units]^i
> +	 *
> +	 * where co = blk_size + r1/divisor[units];
> +	 *
> +	 * and the same for size.  We simply add to the exponent i, because
> +	 * the final calculation we're looking for is
> +	 *
> +	 * (co1 * co2) * divisor[units]^i
> +	 */
> +
> +
>  	while (blk_size >= divisor[units]) {
> -		remainder = do_div(blk_size, divisor[units]);
> +		r1 = do_div(blk_size, divisor[units]);
>  		i++;
>  	}
>
> -	exp = divisor[units] / (u32)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.
> -	 */
> -	if (size > exp) {
> -		remainder = do_div(size, divisor[units]);
> -		remainder *= blk_size;
> +	while (size >= divisor[units]) {
> +		r2 = do_div(size, divisor[units]);
>  		i++;
> -	} else {
> -		remainder *= size;
>  	}
>
> -	size *= blk_size;
> -	size += remainder / divisor[units];
> -	remainder %= divisor[units];
> +	/* here's the magic.  co1 * co2 may be > divisor[i], so correct for
> +	 * that in the exponent and make sure that the additional corrections
> +	 * from the remainders is added in.
> +	 *
> +	 * co1 *co2 = (blk_size + r1/divisor[units])*(size + r2/divisor[units])
> +	 *
> +	 * therefore
> +	 *
> +	 * co1*co2*divisor[units] = blk_size*size*divisor[units] +
> +	 *          r1*size + r2*size + r1*r2/divisor[units]
> +	 *
> +	 * drop the last term because it's too small and perform the
> +	 * calculation cleverly by decremeting i to be automatically dealing
> +	 * with everything multiplied by divisor[units] */
> +
> +	--i;
> +	size = size * blk_size * divisor[units] + r1 * size + r2 *
> blk_size;

The last term is actually not that small. Here is an example:

size = 8192  blk_size = 1024

'As is' the algorithm gives us '8.38 MB', and if we add "+ r1 * r1 /
divisor[units]" we get '8.39 MB' (the correct answer is 8192 * 1024 =
8388608 which is 8.39).

Both r1 and r2 are < divisor[units] here so r1 * r2 won't overflow u32,
I suggest we add this term.

>
>  	while (size >= divisor[units]) {
>  		remainder = do_div(size, divisor[units]);
> @@ -81,8 +100,15 @@ void string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
>  	}
>
>  	sf_cap = size;
> -	for (j = 0; sf_cap*10 < 1000; j++)
> +	round = 500;
> +	for (j = 0; sf_cap*10 < 1000; j++) {
>  		sf_cap *= 10;
> +		round /= 10;
> +	}
> +
> +	/* add a 5 to the digit below what will be printed to ensure
> +	 * an arithmetical round up */
> +	remainder += round;
>
>  	if (j) {
>  		remainder *= 1000;

Can I post this solution with your Suggested-by or do you plan to do it
yourself?

Thanks,

-- 
  Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261726 — Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface

T24gVHVlLCAyMDE1LTExLTAzIGF0IDE0OjEzICswMTAwLCBWaXRhbHkgS3V6bmV0c292IHdyb3Rl
Og0KPiBKYW1lcyBCb3R0b21sZXkgPGpib3R0b21sZXlAb2Rpbi5jb20+IHdyaXRlczoNCj4gDQo+
ID4gT24gTW9uLCAyMDE1LTExLTAyIGF0IDE2OjU4ICswMTAwLCBWaXRhbHkgS3V6bmV0c292IHdy
b3RlOg0KPiA+PiBKYW1lcyBCb3R0b21sZXkgPGpib3R0b21sZXlAb2Rpbi5jb20+IHdyaXRlczoN
Cj4gPj4gDQo+ID4+ID4gT24gRnJpLCAyMDE1LTEwLTMwIGF0IDExOjQ2ICswMTAwLCBWaXRhbHkg
S3V6bmV0c292IHdyb3RlOg0KPiA+PiA+PiBKYW1lcyBCb3R0b21sZXkgPGpib3R0b21sZXlAb2Rp
bi5jb20+IHdyaXRlczoNCj4gPj4gPj4gDQo+ID4+ID4+ID4gT24gVGh1LCAyMDE1LTEwLTI5IGF0
IDE3OjMwICswMTAwLCBWaXRhbHkgS3V6bmV0c292IHdyb3RlOg0KPiA+PiA+PiA+PiBzdHJpbmdf
Z2V0X3NpemUoKSBjYW4ndCByZWFsbHkgaGFuZGxlIGh1Z2UgYmxvY2sgc2l6ZXMsIGVzcGVjaWFs
bHkNCj4gPj4gPj4gPj4gYmxrX3NpemUgPiBVMzJfTUFYIGJ1dCBzdHJpbmdfZ2V0X3NpemUoKSBp
bnRlcmZhY2Ugc3RhdGVzIHRoZSBvcHBvc2l0ZS4NCj4gPj4gPj4gPj4gQ2hhbmdlIGJsa19zaXpl
IGZyb20gdTY0IHRvIHUzMiB0byByZWZsZWN0IHRoZSByZWFsaXR5Lg0KPiA+PiA+PiA+DQo+ID4+
ID4+ID4gV2hhdCBpcyB0aGUgYWN0dWFsIGV2aWRlbmNlIGZvciB0aGlzPyAgVGhlIGNhbGN1bGF0
aW9uIGlzIGRlc2lnbmVkIHRvIGJlDQo+ID4+ID4+ID4gYSBzeW1tZXRyaWMgMTI4IGJpdCBtdWx0
aXBseS4gIFdoZW4gSSB3cm90ZSBhbmQgdGVzdGVkIGl0LCBpdCB3b3JrZWQNCj4gPj4gPj4gPiBm
aW5lIGZvciBodWdlIGJsb2NrIHNpemVzLg0KPiA+PiA+PiANCj4gPj4gPj4gV2UgaGF2ZSAndTMy
IHJlbWFpbmRlcicgYW5kIHRoZW4gd2UgZG86DQo+ID4+ID4+IA0KPiA+PiA+PiBleHAgPSBkaXZp
c29yW3VuaXRzXSAvICh1MzIpYmxrX3NpemU7DQo+ID4+ID4+IC4uLg0KPiA+PiA+PiByZW1haW5k
ZXIgPSBkb19kaXYoc2l6ZSwgZGl2aXNvclt1bml0c10pOw0KPiA+PiA+PiByZW1haW5kZXIgKj0g
YmxrX3NpemU7DQo+ID4+ID4+IA0KPiA+PiA+PiBJJ20gcHJldHR5IHN1cmUgaXQgd2lsbCBvdmVy
ZmxvdyBmb3Igc29tZSBpbnB1dHMuDQo+ID4+ID4NCj4gPj4gPiBJdCBzaG91bGRuJ3Q7IHRoZSBm
dWxsIGNvZGUgc25pcHBldCBkb2VzIHRoaXM6DQo+ID4+ID4NCj4gPj4gPiAgICAgICAgIAl3aGls
ZSAoYmxrX3NpemUgPj0gZGl2aXNvclt1bml0c10pIHsNCj4gPj4gPiAgICAgICAgIAkJcmVtYWlu
ZGVyID0gZG9fZGl2KGJsa19zaXplLCBkaXZpc29yW3VuaXRzXSk7DQo+ID4+ID4gICAgICAgICAJ
CWkrKzsNCj4gPj4gPiAgICAgICAgIAl9DQo+ID4+ID4NCj4gPj4gPiAgICAgICAgIAlleHAgPSBk
aXZpc29yW3VuaXRzXSAvICh1MzIpYmxrX3NpemU7DQo+ID4+ID4NCj4gPj4gPiBTbyBieSB0aGUg
dGltZSBpdCByZWFjaGVzIHRoZSBzdGF0ZW1lbnQgeW91IGNvbXBsYWluIGFib3V0LCBibGtfc2l6
ZSBpcw0KPiA+PiA+IGFscmVhZHkgbGVzcyB0aGFuIG9yIGVxdWFsIHRvIHRoZSBkaXZpc29yICh3
aGljaCBpcyAxMDAwIG9yIDEwMjQpIHNvDQo+ID4+ID4gdHJ1bmNhdGluZyB0byAzMiBiaXRzIGlz
IGFsd2F5cyBjb3JyZWN0Lg0KPiA+PiA+DQo+ID4+IA0KPiA+PiBJIG92ZXJsb29rZWQsIHNvcnJ5
IQ0KPiA+PiANCj4gPj4gPiBJJ20gc29ydCBvZiBnZXR0aW5nIHRoZSBpbXByZXNzaW9uIHlvdSBk
b24ndCBxdWl0ZSB1bmRlcnN0YW5kIHRoZQ0KPiA+PiA+IG1hdGhlbWF0aWNzOiAgaSBpcyB0aGUg
bG9nYXJpdGhtIHRvIHRoZSBiYXNlIGRpdmlzb3JbdW5pdHNdLiAgV2UgcmVkdWNlDQo+ID4+ID4g
Ym90aCBvcGVyYW5kcyB0byBleHBvbmVudHMgb2YgdGhlIGxvZ2FyaXRobSBiYXNlIChhZGRpbmcg
dGhlIHR3byBiYXNlcw0KPiA+PiA+IHRvZ2V0aGVyIGluIGkpLCB3aGljaCBtZWFucyB0aGV5IGFy
ZSBieSBkZWZpbml0aW9uIGluIGEgcmFuZ2UgYmV0d2Vlbg0KPiA+PiA+IHplcm8gYW5kIHRoZSBi
YXNlIGFuZCB0aGVuIG11bHRpcGx5IHRoZSByZW1haW5pbmcgZXhwb25lbnRzIGNvcnJlY3RpbmcN
Cj4gPj4gPiB0aGUgcmVzdWx0IGZvciBhIGJhc2Ugb3ZlcmZsb3cgKHNvIHRoZSByZXN1bHQgaXMg
YWx3YXlzIGEgY29ycmVjdA0KPiA+PiA+IGV4cG9uZW50IGFuZCBpIGlzIHRoZSBsb2dhcml0aG0g
dG8gdGhlIGJhc2UpLiAgSXQncyBhY3R1YWxseSBzaW1wbHkNCj4gPj4gPiBOYXBpZXIncyBhbGdv
cml0aG0uDQo+ID4+ID4NCj4gPj4gPiBUaGUgcmVhc29uIHdlJ3JlIGdldHRpbmcgdGhlIHVwIHRv
IDIuNSUgcm91bmRpbmcgZXJyb3JzIHlvdSBjb21wbGFpbg0KPiA+PiA+IGFib3V0IGlzIGJlY2F1
c2UgYXQgZWFjaCBsb2dhcml0aG0gdW50aWwgdGhlIGxhc3Qgb25lLCB3ZSB0aHJvdyBhd2F5IHRo
ZQ0KPiA+PiA+IHJlbWFpbmRlciAoaXQncyBsZWdpdGltYXRlIGJlY2F1c2UgaXQncyBhbHdheXMg
MTAwMHggc21hbGxlciB0aGFuIHRoZQ0KPiA+PiA+IGV4cG9uZW50KSwgYnV0IGluIHRoZSBjYXNl
IG9mIGEgbGFyZ2UgcmVtYWluZGVyIGl0IHByb3ZpZGVzIGEgc21hbGwNCj4gPj4gPiBjb3JyZWN0
aW9uIHRvIHRoZSBmaW5hbCBvcGVyYXRpb24gd2hpY2ggd2UgZG9uJ3QgYWNjb3VudCBmb3IuICBJ
ZiB5b3UNCj4gPj4gPiB3YW50IHRvIG1ha2UgYSB0cnVlIGNvcnJlY3Rpb24sIHlvdSBzYXZlIHRo
ZSBwZW51bHRpbWF0ZSByZXNpZHVlIGluIGVhY2gNCj4gPj4gPiBjYXNlLCBtdWx0aXBseSBlYWNo
IGJ5IHRoZSAqb3RoZXIqIGV4cG9uZW50IGFkZCB0aGVtIHRvZ2V0aGVyLCBkaXZpZGUgYnkNCj4g
Pj4gPiB0aGUgYmFzZSBhbmQgaW5jcmVtZW50IHRoZSBmaW5hbCByZXN1bHQgYnkgdGhlIHJlbWFp
bmRlci4NCj4gPj4gDQo+ID4+IE15IGFzc3VtcHRpb24gd2FzIHRoYXQgd2UgZG9uJ3QgcmVhbGx5
IG5lZWQgdG8gc3VwcG9ydCBibGtfc2l6ZXMgPg0KPiA+PiBVMzJfTUFYIGFuZCB3ZSBjYW4gc2lt
cGxpZnkgc3RyaW5nX2dldF9zaXplKCkgaW5zdGVhZCBvZiBhZGRpbmcNCj4gPj4gYWRkaXRpb25h
bCBjb21wbGV4aXR5LiBBcHBhcmVudGx5LCB0aGUgYXNzdW1wdGlvbiB3YXMgd3JvbmcuDQo+ID4+
IA0KPiA+PiA+DQo+ID4+ID4gSG93ZXZlciwgZm9yIDIuNSUgdGhlIHBoeXNpY2lzdCBpbiBtZSBz
YXlzIHRoZSBhYm92ZSBpcyB3YXkgb3ZlcmtpbGwuDQo+ID4+ID4NCj4gPj4gDQo+ID4+IEl0IGlz
IGdldHRpbmcgd2FzIG92ZXIgMi41JSBpZiBibGtfc2l6ZSBpcyBub3QgYSBwb3dlciBvZiAyLiBX
aGlsZSBpdCBpcw0KPiA+PiBwcm9iYWJseSBuZXZlciB0aGUgY2FzZSBmb3IgYmxvY2sgc3Vic3lz
dGVtIHRoZSBmdW5jdGlvbiBpcyBpbiBsaWIgYW5kDQo+ID4+IHByZXRlbmRzIHRvIGJlIGdlbmVy
YWwtZW5vdWdoLiBJJ2xsIHRyeSB0byBtYWtlIHByb3BlciBjb3JyZWN0aW9uIGFuZA0KPiA+PiBs
ZXQncyBzZWUgaWYgaXQncyB3b3J0aCB0aGUgZWZmb3J0LiANCj4gPg0KPiA+IE9LLCB0aGlzIGlz
IHRoZSBmdWxsIGNhbGN1bGF0aW9uLiAgSXQgYWxzbyBpbmNsdWRlcyBhbiBhcml0aG1ldGljDQo+
ID4gcm91bmRpbmcgdG8gdGhlIGZpbmFsIGZpZ3VyZSBwcmludC4gIEkgc3VwcG9zZSBpdCdzIG5v
dCB0aGF0IG11Y2ggbW9yZQ0KPiA+IGNvbXBsZXhpdHkgdGhhbiB0aGUgb3JpZ2luYWwsIGFuZCBp
dCBkb2VzIG1ha2UgdGhlIGFsZ29yaXRobSBlYXNpZXIgdG8NCj4gPiB1bmRlcnN0YW5kLg0KPiA+
DQo+ID4gV2UgY291bGQgZG8gd2l0aCBydW5uaW5nIHRoZSBjb21tZW50cyBieSBzb21lIG90aGVy
IG5vbi1tYXRoZW1hdGljaWFuLA0KPiA+IG5vdyBJJ3ZlIGV4cGxhaW5lZCBpdCBpbiBkZXRhaWwg
dG8geW91IHR3bywgdG8gc2VlIGlmIHRoZXkgYWN0dWFsbHkgZ2l2ZQ0KPiA+IGFuIHVuZGVyc3Rh
bmRpbmcgb2YgdGhlIGFsZ29yaXRobS4NCj4gDQo+IFRoYW5rcywgdG8gbWUgdGhleSBsb29rIGdy
ZWF0ISBPbmUgbml0cGljayBiZWxvdyAuLi4NCj4gDQo+ID4NCj4gPiBKYW1lcw0KPiA+DQo+ID4g
LS0tDQo+ID4NCj4gPiBkaWZmIC0tZ2l0IGEvbGliL3N0cmluZ19oZWxwZXJzLmMgYi9saWIvc3Ry
aW5nX2hlbHBlcnMuYw0KPiA+IGluZGV4IDU5MzlmNjMuLjFlYzdlNzdhIDEwMDY0NA0KPiA+IC0t
LSBhL2xpYi9zdHJpbmdfaGVscGVycy5jDQo+ID4gKysrIGIvbGliL3N0cmluZ19oZWxwZXJzLmMN
Cj4gPiBAQCAtNDQsNyArNDQsNyBAQCB2b2lkIHN0cmluZ19nZXRfc2l6ZSh1NjQgc2l6ZSwgdTY0
IGJsa19zaXplLCBjb25zdCBlbnVtIHN0cmluZ19zaXplX3VuaXRzIHVuaXRzLA0KPiA+ICAJCVtT
VFJJTkdfVU5JVFNfMl0gPSAxMDI0LA0KPiA+ICAJfTsNCj4gPiAgCWludCBpLCBqOw0KPiA+IC0J
dTMyIHJlbWFpbmRlciA9IDAsIHNmX2NhcCwgZXhwOw0KPiA+ICsJdTMyIHJlbWFpbmRlciA9IDAs
IHNmX2NhcCwgcjEgPSAwLCByMiA9IDAsIHJvdW5kOw0KPiA+ICAJY2hhciB0bXBbOF07DQo+ID4g
IAljb25zdCBjaGFyICp1bml0Ow0KPiA+DQo+ID4gQEAgLTUzLDI3ICs1Myw0NiBAQCB2b2lkIHN0
cmluZ19nZXRfc2l6ZSh1NjQgc2l6ZSwgdTY0IGJsa19zaXplLCBjb25zdCBlbnVtIHN0cmluZ19z
aXplX3VuaXRzIHVuaXRzLA0KPiA+ICAJaWYgKCFzaXplKQ0KPiA+ICAJCWdvdG8gb3V0Ow0KPiA+
DQo+ID4gKwkvKiBUaGlzIGlzIG5hcGllcidzIGFsZ29yaXRobS4gIFJlZHVjZSB0aGUgb3JpZ2lu
YWwgYmxvY2sgc2l6ZSB0bw0KPiA+ICsJICoNCj4gPiArCSAqIGNvICogZGl2aXNvclt1bml0c11e
aQ0KPiA+ICsJICoNCj4gPiArCSAqIHdoZXJlIGNvID0gYmxrX3NpemUgKyByMS9kaXZpc29yW3Vu
aXRzXTsNCj4gPiArCSAqDQo+ID4gKwkgKiBhbmQgdGhlIHNhbWUgZm9yIHNpemUuICBXZSBzaW1w
bHkgYWRkIHRvIHRoZSBleHBvbmVudCBpLCBiZWNhdXNlDQo+ID4gKwkgKiB0aGUgZmluYWwgY2Fs
Y3VsYXRpb24gd2UncmUgbG9va2luZyBmb3IgaXMNCj4gPiArCSAqDQo+ID4gKwkgKiAoY28xICog
Y28yKSAqIGRpdmlzb3JbdW5pdHNdXmkNCj4gPiArCSAqLw0KPiA+ICsNCj4gPiArDQo+ID4gIAl3
aGlsZSAoYmxrX3NpemUgPj0gZGl2aXNvclt1bml0c10pIHsNCj4gPiAtCQlyZW1haW5kZXIgPSBk
b19kaXYoYmxrX3NpemUsIGRpdmlzb3JbdW5pdHNdKTsNCj4gPiArCQlyMSA9IGRvX2RpdihibGtf
c2l6ZSwgZGl2aXNvclt1bml0c10pOw0KPiA+ICAJCWkrKzsNCj4gPiAgCX0NCj4gPg0KPiA+IC0J
ZXhwID0gZGl2aXNvclt1bml0c10gLyAodTMyKWJsa19zaXplOw0KPiA+IC0JLyoNCj4gPiAtCSAq
IHNpemUgbXVzdCBiZSBzdHJpY3RseSBncmVhdGVyIHRoYW4gZXhwIGhlcmUgdG8gZW5zdXJlIHRo
YXQgcmVtYWluZGVyDQo+ID4gLQkgKiBpcyBncmVhdGVyIHRoYW4gZGl2aXNvclt1bml0c10gY29t
aW5nIG91dCBvZiB0aGUgaWYgYmVsb3cuDQo+ID4gLQkgKi8NCj4gPiAtCWlmIChzaXplID4gZXhw
KSB7DQo+ID4gLQkJcmVtYWluZGVyID0gZG9fZGl2KHNpemUsIGRpdmlzb3JbdW5pdHNdKTsNCj4g
PiAtCQlyZW1haW5kZXIgKj0gYmxrX3NpemU7DQo+ID4gKwl3aGlsZSAoc2l6ZSA+PSBkaXZpc29y
W3VuaXRzXSkgew0KPiA+ICsJCXIyID0gZG9fZGl2KHNpemUsIGRpdmlzb3JbdW5pdHNdKTsNCj4g
PiAgCQlpKys7DQo+ID4gLQl9IGVsc2Ugew0KPiA+IC0JCXJlbWFpbmRlciAqPSBzaXplOw0KPiA+
ICAJfQ0KPiA+DQo+ID4gLQlzaXplICo9IGJsa19zaXplOw0KPiA+IC0Jc2l6ZSArPSByZW1haW5k
ZXIgLyBkaXZpc29yW3VuaXRzXTsNCj4gPiAtCXJlbWFpbmRlciAlPSBkaXZpc29yW3VuaXRzXTsN
Cj4gPiArCS8qIGhlcmUncyB0aGUgbWFnaWMuICBjbzEgKiBjbzIgbWF5IGJlID4gZGl2aXNvcltp
XSwgc28gY29ycmVjdCBmb3INCj4gPiArCSAqIHRoYXQgaW4gdGhlIGV4cG9uZW50IGFuZCBtYWtl
IHN1cmUgdGhhdCB0aGUgYWRkaXRpb25hbCBjb3JyZWN0aW9ucw0KPiA+ICsJICogZnJvbSB0aGUg
cmVtYWluZGVycyBpcyBhZGRlZCBpbi4NCj4gPiArCSAqDQo+ID4gKwkgKiBjbzEgKmNvMiA9IChi
bGtfc2l6ZSArIHIxL2Rpdmlzb3JbdW5pdHNdKSooc2l6ZSArIHIyL2Rpdmlzb3JbdW5pdHNdKQ0K
PiA+ICsJICoNCj4gPiArCSAqIHRoZXJlZm9yZQ0KPiA+ICsJICoNCj4gPiArCSAqIGNvMSpjbzIq
ZGl2aXNvclt1bml0c10gPSBibGtfc2l6ZSpzaXplKmRpdmlzb3JbdW5pdHNdICsNCj4gPiArCSAq
ICAgICAgICAgIHIxKnNpemUgKyByMipzaXplICsgcjEqcjIvZGl2aXNvclt1bml0c10NCj4gPiAr
CSAqDQo+ID4gKwkgKiBkcm9wIHRoZSBsYXN0IHRlcm0gYmVjYXVzZSBpdCdzIHRvbyBzbWFsbCBh
bmQgcGVyZm9ybSB0aGUNCj4gPiArCSAqIGNhbGN1bGF0aW9uIGNsZXZlcmx5IGJ5IGRlY3JlbWV0
aW5nIGkgdG8gYmUgYXV0b21hdGljYWxseSBkZWFsaW5nDQo+ID4gKwkgKiB3aXRoIGV2ZXJ5dGhp
bmcgbXVsdGlwbGllZCBieSBkaXZpc29yW3VuaXRzXSAqLw0KPiA+ICsNCj4gPiArCS0taTsNCj4g
PiArCXNpemUgPSBzaXplICogYmxrX3NpemUgKiBkaXZpc29yW3VuaXRzXSArIHIxICogc2l6ZSAr
IHIyICoNCj4gPiBibGtfc2l6ZTsNCj4gDQo+IFRoZSBsYXN0IHRlcm0gaXMgYWN0dWFsbHkgbm90
IHRoYXQgc21hbGwuIEhlcmUgaXMgYW4gZXhhbXBsZToNCg0KSXQncyBhbHdheXMgPCAxIGluIHRo
ZSBmaW5hbCBlcXVhdGlvbiBiZWNhdXNlIGl0J3MgZGl2aWRlZCBieSB0aGUgc3F1YXJlDQpvZiBk
aXZpc29yW3VuaXRzXS4gIEhvd2V2ZXIsIHRoYXQgY2FuIG1ha2UgYSBjb250cmlidXRpb24gdG8g
dGhlIHJvdW5kDQp1cCwgSSBzdXBwb3NlIGxlYWRpbmcgdG8gdGhlIHRydW5jYXRpb24geW91IHNl
ZQ0KDQo+IHNpemUgPSA4MTkyICBibGtfc2l6ZSA9IDEwMjQNCj4gDQo+ICdBcyBpcycgdGhlIGFs
Z29yaXRobSBnaXZlcyB1cyAnOC4zOCBNQicsIGFuZCBpZiB3ZSBhZGQgIisgcjEgKiByMSAvDQo+
IGRpdmlzb3JbdW5pdHNdIiB3ZSBnZXQgJzguMzkgTUInICh0aGUgY29ycmVjdCBhbnN3ZXIgaXMg
ODE5MiAqIDEwMjQgPQ0KPiA4Mzg4NjA4IHdoaWNoIGlzIDguMzkpLg0KPiANCj4gQm90aCByMSBh
bmQgcjIgYXJlIDwgZGl2aXNvclt1bml0c10gaGVyZSBzbyByMSAqIHIyIHdvbid0IG92ZXJmbG93
IHUzMiwNCj4gSSBzdWdnZXN0IHdlIGFkZCB0aGlzIHRlcm0uDQo+IA0KPiA+DQo+ID4gIAl3aGls
ZSAoc2l6ZSA+PSBkaXZpc29yW3VuaXRzXSkgew0KPiA+ICAJCXJlbWFpbmRlciA9IGRvX2Rpdihz
aXplLCBkaXZpc29yW3VuaXRzXSk7DQo+ID4gQEAgLTgxLDggKzEwMCwxNSBAQCB2b2lkIHN0cmlu
Z19nZXRfc2l6ZSh1NjQgc2l6ZSwgdTY0IGJsa19zaXplLCBjb25zdCBlbnVtIHN0cmluZ19zaXpl
X3VuaXRzIHVuaXRzLA0KPiA+ICAJfQ0KPiA+DQo+ID4gIAlzZl9jYXAgPSBzaXplOw0KPiA+IC0J
Zm9yIChqID0gMDsgc2ZfY2FwKjEwIDwgMTAwMDsgaisrKQ0KPiA+ICsJcm91bmQgPSA1MDA7DQo+
ID4gKwlmb3IgKGogPSAwOyBzZl9jYXAqMTAgPCAxMDAwOyBqKyspIHsNCj4gPiAgCQlzZl9jYXAg
Kj0gMTA7DQo+ID4gKwkJcm91bmQgLz0gMTA7DQo+ID4gKwl9DQo+ID4gKw0KPiA+ICsJLyogYWRk
IGEgNSB0byB0aGUgZGlnaXQgYmVsb3cgd2hhdCB3aWxsIGJlIHByaW50ZWQgdG8gZW5zdXJlDQo+
ID4gKwkgKiBhbiBhcml0aG1ldGljYWwgcm91bmQgdXAgKi8NCj4gPiArCXJlbWFpbmRlciArPSBy
b3VuZDsNCj4gPg0KPiA+ICAJaWYgKGopIHsNCj4gPiAgCQlyZW1haW5kZXIgKj0gMTAwMDsNCj4g
DQo+IENhbiBJIHBvc3QgdGhpcyBzb2x1dGlvbiB3aXRoIHlvdXIgU3VnZ2VzdGVkLWJ5IG9yIGRv
IHlvdSBwbGFuIHRvIGRvIGl0DQo+IHlvdXJzZWxmPw0KDQpJdCB3YXMgYSBzdWdnZXN0aW9uIHdo
ZW4gSSBleHBsYWluZWQgd2hhdCB0aGUgbWlzc2luZyBzb3VyY2VzIG9mDQpwcmVjaXNpb24gd2Vy
ZSwgSSBkb24ndCB0aGluayBpdCdzIHJlYWxseSBhIHN1Z2dlc3Rpb24gd2hlbiBpdCBjb21lcw0K
d2l0aCBhbiBleGVtcGxhcnkgcGF0Y2guICBIb3dldmVyLCB0aGUgd2hvbGUgdGhpbmcgbmVlZHMg
cG9saXNoaW5nDQpiZWNhdXNlIGFsbCB0aGUgZGl2aXNpb25zIG5lZWQgdG8gYmUgZWxpbWluYXRl
ZCwgc2luY2UgdGhleSdyZSBhIGh1Z2UNCnNvdXJjZSBvZiBwcm9ibGVtcyBmb3IgbW9zdCAzMiBi
aXQgQ1BVcywgc28gSSBjYW4gZml4IGl0IGFsbCB0aGUgd2F5IGFuZA0KcmVwb3N0Lg0KDQpKYW1l
cw0KDQo=
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261892 — Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface

On Tue, Nov 03 2015, James Bottomley <jbottomley@odin.com> wrote:

>
> It was a suggestion when I explained what the missing sources of
> precision were, I don't think it's really a suggestion when it comes
> with an exemplary patch.

ex·em·pla·ry
adjective

    1.
    serving as a desirable model; representing the best of its kind.

Said exemplary patch produces "1.10 KiB" for size=2047,
blk_size=1. (This is caused by the introduction of rounding, and is
probably fixable.)

James, I do understand the algorithm you're trying to use. What I don't
understand is why you insist on using the approach of reducing size and
blk_size all the way before multiplying them. It seems much simpler to
just reduce them till they're below U32_MAX (not keeping track of any
remainders at that point), multiply them, and then proceed as usual,
This avoids having to deal with weird cross-multiplication terms, gives
more accurate results (yes, I tested that) and avoids the extra 64/32
division you introduce by decrementing i.

Rasmus

To be precise, the body I suggest is

	while (blk_size > U32_MAX) {
		do_div(blk_size, divisor[units]);
		i++;
	}
	while (size > U32_MAX) {
		do_div(size, divisor[units]);
		i++;
	}
	size *= blk_size;
	while (size > divisor[units]) {
		remainder = do_div(size, divisor[units]);
		i++;
	}

whether the last one should be > or >= is debatable; I think 1024 KiB is
better than 1.00 MiB.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261903 — Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface

T24gVHVlLCAyMDE1LTExLTAzIGF0IDIxOjU3ICswMTAwLCBSYXNtdXMgVmlsbGVtb2VzIHdyb3Rl
Og0KPiBPbiBUdWUsIE5vdiAwMyAyMDE1LCBKYW1lcyBCb3R0b21sZXkgPGpib3R0b21sZXlAb2Rp
bi5jb20+IHdyb3RlOg0KPiANCj4gPg0KPiA+IEl0IHdhcyBhIHN1Z2dlc3Rpb24gd2hlbiBJIGV4
cGxhaW5lZCB3aGF0IHRoZSBtaXNzaW5nIHNvdXJjZXMgb2YNCj4gPiBwcmVjaXNpb24gd2VyZSwg
SSBkb24ndCB0aGluayBpdCdzIHJlYWxseSBhIHN1Z2dlc3Rpb24gd2hlbiBpdCBjb21lcw0KPiA+
IHdpdGggYW4gZXhlbXBsYXJ5IHBhdGNoLg0KPiANCj4gZXjCt2VtwrdwbGHCt3J5DQo+IGFkamVj
dGl2ZQ0KPiANCj4gICAgIDEuDQo+ICAgICBzZXJ2aW5nIGFzIGEgZGVzaXJhYmxlIG1vZGVsOyBy
ZXByZXNlbnRpbmcgdGhlIGJlc3Qgb2YgaXRzIGtpbmQuDQo+IA0KPiBTYWlkIGV4ZW1wbGFyeSBw
YXRjaCBwcm9kdWNlcyAiMS4xMCBLaUIiIGZvciBzaXplPTIwNDcsDQo+IGJsa19zaXplPTEuIChU
aGlzIGlzIGNhdXNlZCBieSB0aGUgaW50cm9kdWN0aW9uIG9mIHJvdW5kaW5nLCBhbmQgaXMNCj4g
cHJvYmFibHkgZml4YWJsZS4pDQo+IA0KPiBKYW1lcywgSSBkbyB1bmRlcnN0YW5kIHRoZSBhbGdv
cml0aG0geW91J3JlIHRyeWluZyB0byB1c2UuIFdoYXQgSSBkb24ndA0KPiB1bmRlcnN0YW5kIGlz
IHdoeSB5b3UgaW5zaXN0IG9uIHVzaW5nIHRoZSBhcHByb2FjaCBvZiByZWR1Y2luZyBzaXplIGFu
ZA0KPiBibGtfc2l6ZSBhbGwgdGhlIHdheSBiZWZvcmUgbXVsdGlwbHlpbmcgdGhlbS4gSXQgc2Vl
bXMgbXVjaCBzaW1wbGVyIHRvDQo+IGp1c3QgcmVkdWNlIHRoZW0gdGlsbCB0aGV5J3JlIGJlbG93
IFUzMl9NQVggKG5vdCBrZWVwaW5nIHRyYWNrIG9mIGFueQ0KPiByZW1haW5kZXJzIGF0IHRoYXQg
cG9pbnQpLCBtdWx0aXBseSB0aGVtLCBhbmQgdGhlbiBwcm9jZWVkIGFzIHVzdWFsLA0KPiBUaGlz
IGF2b2lkcyBoYXZpbmcgdG8gZGVhbCB3aXRoIHdlaXJkIGNyb3NzLW11bHRpcGxpY2F0aW9uIHRl
cm1zLCBnaXZlcw0KPiBtb3JlIGFjY3VyYXRlIHJlc3VsdHMgKHllcywgSSB0ZXN0ZWQgdGhhdCkg
YW5kIGF2b2lkcyB0aGUgZXh0cmEgNjQvMzINCj4gZGl2aXNpb24geW91IGludHJvZHVjZSBieSBk
ZWNyZW1lbnRpbmcgaS4NCg0KV2VsbCwgd29vZCBhbmQgdHJlZXMsIEkgdGhpbmsuICBJIGRvbid0
IGJlbGlldmUgdGhlcmUncyBhbnkgbW9yZQ0KYWNjdXJhY3kgd2l0aCB0aGUgc2Vjb25kIG9yZGVy
IHRlcm0sIGJ1dCBpdCBpcyBhIGxvdCBzaW1wbGVyIGZvciBhbnlvbmUNCnRvIHVuZGVyc3RhbmQu
ICBJJ2xsIHBvc3QgYSB2Mi4NCg0KSmFtZXMNCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258911 — [PATCH v3 2/4] lib/string_helpers.c: protect string_get_size() against blk_size=0

Division by zero happens if blk_size=0 is supplied to string_get_size().
Add WARN_ON() and set size to 0 to report '0 B'.

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

diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index f6c27dc..ff3575b 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -50,6 +50,11 @@ void string_get_size(u64 size, u32 blk_size, const enum string_size_units units,
 
 	tmp[0] = '\0';
 	i = 0;
+
+	/* Calling string_get_size() with blk_size=0 is wrong! */
+	if (WARN_ON(!blk_size))
+		size = 0;
+
 	if (!size)
 		goto out;
 
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259014 — Re: [PATCH v3 2/4] lib/string_helpers.c: protect string_get_size() against blk_size=0

On Thu, Oct 29, 2015 at 6:30 PM, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> Division by zero happens if blk_size=0 is supplied to string_get_size().
> Add WARN_ON() and set size to 0 to report '0 B'.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> ---
>  lib/string_helpers.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index f6c27dc..ff3575b 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -50,6 +50,11 @@ void string_get_size(u64 size, u32 blk_size, const enum string_size_units units,
>
>         tmp[0] = '\0';
>         i = 0;
> +
> +       /* Calling string_get_size() with blk_size=0 is wrong! */
> +       if (WARN_ON(!blk_size))
> +               size = 0;
> +
>         if (!size)
>                 goto out;
>
> --
> 2.4.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259091 — Re: [PATCH v3 2/4] lib/string_helpers.c: protect string_get_size() against blk_size=0

T24gVGh1LCAyMDE1LTEwLTI5IGF0IDE3OjMwICswMTAwLCBWaXRhbHkgS3V6bmV0c292IHdyb3Rl
Og0KPiBEaXZpc2lvbiBieSB6ZXJvIGhhcHBlbnMgaWYgYmxrX3NpemU9MCBpcyBzdXBwbGllZCB0
byBzdHJpbmdfZ2V0X3NpemUoKS4NCj4gQWRkIFdBUk5fT04oKSBhbmQgc2V0IHNpemUgdG8gMCB0
byByZXBvcnQgJzAgQicuDQo+IA0KPiBTaWduZWQtb2ZmLWJ5OiBWaXRhbHkgS3V6bmV0c292IDx2
a3V6bmV0c0ByZWRoYXQuY29tPg0KPiAtLS0NCj4gIGxpYi9zdHJpbmdfaGVscGVycy5jIHwgNSAr
KysrKw0KPiAgMSBmaWxlIGNoYW5nZWQsIDUgaW5zZXJ0aW9ucygrKQ0KPiANCj4gZGlmZiAtLWdp
dCBhL2xpYi9zdHJpbmdfaGVscGVycy5jIGIvbGliL3N0cmluZ19oZWxwZXJzLmMNCj4gaW5kZXgg
ZjZjMjdkYy4uZmYzNTc1YiAxMDA2NDQNCj4gLS0tIGEvbGliL3N0cmluZ19oZWxwZXJzLmMNCj4g
KysrIGIvbGliL3N0cmluZ19oZWxwZXJzLmMNCj4gQEAgLTUwLDYgKzUwLDExIEBAIHZvaWQgc3Ry
aW5nX2dldF9zaXplKHU2NCBzaXplLCB1MzIgYmxrX3NpemUsIGNvbnN0IGVudW0gc3RyaW5nX3Np
emVfdW5pdHMgdW5pdHMsDQo+ICANCj4gIAl0bXBbMF0gPSAnXDAnOw0KPiAgCWkgPSAwOw0KPiAr
DQo+ICsJLyogQ2FsbGluZyBzdHJpbmdfZ2V0X3NpemUoKSB3aXRoIGJsa19zaXplPTAgaXMgd3Jv
bmchICovDQo+ICsJaWYgKFdBUk5fT04oIWJsa19zaXplKSkNCg0KR2V0IHJpZCBvZiB0aGUgV0FS
Tl9PTjsgaXQncyB0aGUgc3RhbmRhcmQgdGhpbmcgdG8gZG8gZm9yIGEgcGFydGlhbGx5DQpjb25u
ZWN0ZWQgZGV2aWNlLiAgU2VlaW5nIHplcm8gaXMgc3RhbmRhcmQgaW4gYSB3aG9sZSB2YXJpZXR5
IG9mDQpzaXR1YXRpb25zLiAgU0NTSSBzaGltcyB0aGUgemVybyBidXQgbW9zdCBvdGhlciBkcml2
ZXJzIGRvbid0Lg0KDQpKYW1lcw0KDQo=
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259104 — Re: [PATCH v3 2/4] lib/string_helpers.c: protect string_get_size() against blk_size=0

On Fri, Oct 30, 2015 at 1:00 AM, James Bottomley <jbottomley@odin.com> wrote:
> On Thu, 2015-10-29 at 17:30 +0100, Vitaly Kuznetsov wrote:
>> Division by zero happens if blk_size=0 is supplied to string_get_size().
>> Add WARN_ON() and set size to 0 to report '0 B'.
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  lib/string_helpers.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
>> index f6c27dc..ff3575b 100644
>> --- a/lib/string_helpers.c
>> +++ b/lib/string_helpers.c
>> @@ -50,6 +50,11 @@ void string_get_size(u64 size, u32 blk_size, const enum string_size_units units,
>>
>>       tmp[0] = '\0';
>>       i = 0;
>> +
>> +     /* Calling string_get_size() with blk_size=0 is wrong! */
>> +     if (WARN_ON(!blk_size))
>
> Get rid of the WARN_ON; it's the standard thing to do for a partially
> connected device.  Seeing zero is standard in a whole variety of
> situations.  SCSI shims the zero but most other drivers don't.

For *block* size? It will crash the kernel. I've checked, it wasn't
changed from the beginning (b9f28d863594).

+       exp = divisor[units] / (u32)blk_size;

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web