[PATCH v3 0/4] x86/efi: use binary units when printing

[PATCH v3 0/4] x86/efi: use binary units when printing

[Andy Shevchenko]

The patch series exports the arrays of binary and decimal units as it's
described by IEC.

First user of it is EFI code which would print sizes and other values using
binary prefix.

James, is this now okay to you?

Matt, I suppose we need to update the stuff in your tree.

Since v2:
- address James comment (don't nail array size)
- fix a title and commit message for patch 3 to be in align with the change

Andy Shevchenko (3):
  lib/string_helpers: export string_units_{2,10} for others
  lib/string_helpers: fix indentation in few places
  x86/efi: Use proper units in efi_find_mirror()

Robert Elliott (1):
  x86/efi: print size in binary units in efi_print_memmap

 arch/x86/platform/efi/efi.c    | 27 ++++++++++++++++++---------
 include/linux/string_helpers.h |  3 +++
 lib/string_helpers.c           | 26 ++++++++++++++------------
 3 files changed, 35 insertions(+), 21 deletions(-)

-- 
2.7.0.rc3

[PATCH v3 1/4] lib/string_helpers: export string_units_{2,10} for others

[Andy Shevchenko]

There is one user coming which would like to use those string arrays. It might
be useful for any other user in the future.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/string_helpers.h |  3 +++
 lib/string_helpers.c           | 21 ++++++++++++---------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index dabe643..1d16240 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -10,6 +10,9 @@ enum string_size_units {
 	STRING_UNITS_2,		/* use binary powers of 2^10 */
 };
 
+extern const char *const string_units_10[];
+extern const char *const string_units_2[];
+
 void string_get_size(u64 size, u64 blk_size, enum string_size_units units,
 		     char *buf, int len);
 
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 5939f63..86124c9 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -13,6 +13,15 @@
 #include <linux/string.h>
 #include <linux/string_helpers.h>
 
+const char *const string_units_10[] = {
+	"B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB",
+};
+EXPORT_SYMBOL(string_units_10);
+const char *const string_units_2[] = {
+	"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB", "ZiB", "YiB",
+};
+EXPORT_SYMBOL(string_units_2);
+
 /**
  * string_get_size - get the size in the specified units
  * @size:	The size to be converted in blocks
@@ -29,15 +38,9 @@
 void string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
 		     char *buf, int len)
 {
-	static const char *const units_10[] = {
-		"B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB"
-	};
-	static const char *const units_2[] = {
-		"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB", "ZiB", "YiB"
-	};
 	static const char *const *const units_str[] = {
-		[STRING_UNITS_10] = units_10,
-		[STRING_UNITS_2] = units_2,
+		[STRING_UNITS_10] = string_units_10,
+		[STRING_UNITS_2] = string_units_2,
 	};
 	static const unsigned int divisor[] = {
 		[STRING_UNITS_10] = 1000,
@@ -92,7 +95,7 @@ void string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
 	}
 
  out:
-	if (i >= ARRAY_SIZE(units_2))
+	if (i >= ARRAY_SIZE(string_units_2))
 		unit = "UNK";
 	else
 		unit = units_str[units][i];
-- 
2.7.0.rc3

[PATCH v3 2/4] lib/string_helpers: fix indentation in few places

[Andy Shevchenko]

Fix the indentation of label and put snprintf() to one line.

There is no functional change.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/string_helpers.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 86124c9..2ebd724 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -94,14 +94,13 @@ void string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
 		tmp[j+1] = '\0';
 	}
 
- out:
+out:
 	if (i >= ARRAY_SIZE(string_units_2))
 		unit = "UNK";
 	else
 		unit = units_str[units][i];
 
-	snprintf(buf, len, "%u%s %s", (u32)size,
-		 tmp, unit);
+	snprintf(buf, len, "%u%s %s", (u32)size, tmp, unit);
 }
 EXPORT_SYMBOL(string_get_size);
 
-- 
2.7.0.rc3

[PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Andy Shevchenko]

From: Robert Elliott <elliott@hpe.com>

Print the size in the best-fit B, KiB, MiB, etc. units rather than
always MiB. This avoids rounding, which can be misleading.

Use proper IEC binary units (KiB, MiB, etc.) rather than misuse SI
decimal units (KB, MB, etc.).

old:
    efi: mem61: [Persistent Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff) (16384MB)

new:
    efi: mem61: [Persistent Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff] (16 GiB)

Signed-off-by: Robert Elliott <elliott@hpe.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/platform/efi/efi.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index e0846b5..3badc8a 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -35,10 +35,12 @@
 #include <linux/efi.h>
 #include <linux/efi-bgrt.h>
 #include <linux/export.h>
+#include <linux/bitops.h>
 #include <linux/bootmem.h>
 #include <linux/slab.h>
 #include <linux/memblock.h>
 #include <linux/spinlock.h>
+#include <linux/string_helpers.h>
 #include <linux/uaccess.h>
 #include <linux/time.h>
 #include <linux/io.h>
@@ -117,6 +119,14 @@ void efi_get_time(struct timespec *now)
 	now->tv_nsec = 0;
 }
 
+static char * __init efi_size_format(char *buf, size_t size, u64 bytes)
+{
+	unsigned long i = bytes ? __ffs64(bytes) / 10 : 0;
+
+	snprintf(buf, size, "%llu %s", bytes >> (i * 10), string_units_2[i]);
+	return buf;
+}
+
 void __init efi_find_mirror(void)
 {
 	void *p;
@@ -225,21 +235,20 @@ int __init efi_memblock_x86_reserve_range(void)
 void __init efi_print_memmap(void)
 {
 #ifdef EFI_DEBUG
-	efi_memory_desc_t *md;
 	void *p;
 	int i;
 
 	for (p = memmap.map, i = 0;
 	     p < memmap.map_end;
 	     p += memmap.desc_size, i++) {
-		char buf[64];
+		efi_memory_desc_t *md = p;
+		u64 size = md->num_pages << EFI_PAGE_SHIFT;
+		char buf[64], buf3[32];
 
-		md = p;
-		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%lluMB)\n",
+		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%s)\n",
 			i, efi_md_typeattr_format(buf, sizeof(buf), md),
-			md->phys_addr,
-			md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1,
-			(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
+			md->phys_addr, md->phys_addr + size - 1,
+			efi_size_format(buf3, sizeof(buf3), size));
 	}
 #endif  /*  EFI_DEBUG  */
 }
-- 
2.7.0.rc3

[PATCH v3 4/4] x86/efi: Use proper units in efi_find_mirror()

[Andy Shevchenko]

Like in efi_print_mmap() use the proper units when printing sizes.

Currently it's hardcoded to 'MiB', though it might be changed to adaptive
efi_size_format() in the future.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/platform/efi/efi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 3badc8a..95f70da 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -144,8 +144,8 @@ void __init efi_find_mirror(void)
 		}
 	}
 	if (mirror_size)
-		pr_info("Memory: %lldM/%lldM mirrored memory\n",
-			mirror_size>>20, total_size>>20);
+		pr_info("Memory: %lld MiB/%lld MiB mirrored memory\n",
+			mirror_size >> 20, total_size >> 20);
 }
 
 /*
-- 
2.7.0.rc3

[PATCH v3 4/4] x86/efi: Use proper units in efi_find_mirror()

[Andy Shevchenko]

Like in efi_print_mmap() use the proper units when printing sizes.

Currently it's hardcoded to 'MiB', though it might be changed to adaptive
efi_size_format() in the future.

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 arch/x86/platform/efi/efi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 3badc8a..95f70da 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -144,8 +144,8 @@ void __init efi_find_mirror(void)
 		}
 	}
 	if (mirror_size)
-		pr_info("Memory: %lldM/%lldM mirrored memory\n",
-			mirror_size>>20, total_size>>20);
+		pr_info("Memory: %lld MiB/%lld MiB mirrored memory\n",
+			mirror_size >> 20, total_size >> 20);
 }
 
 /*
-- 
2.7.0.rc3

Re: [PATCH v3 1/4] lib/string_helpers: export string_units_{2,10} for others

[James Bottomley]

On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:
> There is one user coming which would like to use those string arrays.
> It might
> be useful for any other user in the future.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/string_helpers.h |  3 +++
>  lib/string_helpers.c           | 21 ++++++++++++---------
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/string_helpers.h
> b/include/linux/string_helpers.h
> index dabe643..1d16240 100644
> --- a/include/linux/string_helpers.h
> +++ b/include/linux/string_helpers.h
> @@ -10,6 +10,9 @@ enum string_size_units {
>  	STRING_UNITS_2,		/* use binary powers of 2^10
> */
>  };
>  
> +extern const char *const string_units_10[];
> +extern const char *const string_units_2[];
> +
>  void string_get_size(u64 size, u64 blk_size, enum string_size_units
> units,
>  		     char *buf, int len);
>  
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index 5939f63..86124c9 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -13,6 +13,15 @@
>  #include <linux/string.h>
>  #include <linux/string_helpers.h>
>  
> +const char *const string_units_10[] = {
> +	"B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB",
> +};
> +EXPORT_SYMBOL(string_units_10);
> +const char *const string_units_2[] = {
> +	"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB", "ZiB", "YiB",
> +};
> +EXPORT_SYMBOL(string_units_2);
> +
>  /**
>   * string_get_size - get the size in the specified units
>   * @size:	The size to be converted in blocks
> @@ -29,15 +38,9 @@
>  void string_get_size(u64 size, u64 blk_size, const enum
> string_size_units units,
>  		     char *buf, int len)
>  {
> -	static const char *const units_10[] = {
> -		"B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB"
> -	};
> -	static const char *const units_2[] = {
> -		"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB",
> "ZiB", "YiB"
> -	};
>  	static const char *const *const units_str[] = {
> -		[STRING_UNITS_10] = units_10,
> -		[STRING_UNITS_2] = units_2,
> +		[STRING_UNITS_10] = string_units_10,
> +		[STRING_UNITS_2] = string_units_2,
>  	};
>  	static const unsigned int divisor[] = {
>  		[STRING_UNITS_10] = 1000,
> @@ -92,7 +95,7 @@ void string_get_size(u64 size, u64 blk_size, const
> enum string_size_units units,
>  	}
>  
>   out:
> -	if (i >= ARRAY_SIZE(units_2))
> +	if (i >= ARRAY_SIZE(string_units_2))

so now, no-one other than string_helpers.c can tell the size of the
array ... I don't think that's an improvement.  Also for a trivial
patch I'm starting to think there should be a three strikes rule: we
get a large number of bugs from allegedly trivial reworks which
wouldn't have happened if we'd retained the original working code in
the first place.

After two attempts, doesn't it perhaps strike you that a helper
function rather than a direct export would get over this difficulty? 
 It might also address the precision problem you introduced.

James

Re: [PATCH v3 1/4] lib/string_helpers: export string_units_{2,10} for others

[James Bottomley]

On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:
> There is one user coming which would like to use those string arrays.
> It might
> be useful for any other user in the future.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  include/linux/string_helpers.h |  3 +++
>  lib/string_helpers.c           | 21 ++++++++++++---------
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/string_helpers.h
> b/include/linux/string_helpers.h
> index dabe643..1d16240 100644
> --- a/include/linux/string_helpers.h
> +++ b/include/linux/string_helpers.h
> @@ -10,6 +10,9 @@ enum string_size_units {
>  	STRING_UNITS_2,		/* use binary powers of 2^10
> */
>  };
>  
> +extern const char *const string_units_10[];
> +extern const char *const string_units_2[];
> +
>  void string_get_size(u64 size, u64 blk_size, enum string_size_units
> units,
>  		     char *buf, int len);
>  
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index 5939f63..86124c9 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -13,6 +13,15 @@
>  #include <linux/string.h>
>  #include <linux/string_helpers.h>
>  
> +const char *const string_units_10[] = {
> +	"B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB",
> +};
> +EXPORT_SYMBOL(string_units_10);
> +const char *const string_units_2[] = {
> +	"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB", "ZiB", "YiB",
> +};
> +EXPORT_SYMBOL(string_units_2);
> +
>  /**
>   * string_get_size - get the size in the specified units
>   * @size:	The size to be converted in blocks
> @@ -29,15 +38,9 @@
>  void string_get_size(u64 size, u64 blk_size, const enum
> string_size_units units,
>  		     char *buf, int len)
>  {
> -	static const char *const units_10[] = {
> -		"B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB"
> -	};
> -	static const char *const units_2[] = {
> -		"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB",
> "ZiB", "YiB"
> -	};
>  	static const char *const *const units_str[] = {
> -		[STRING_UNITS_10] = units_10,
> -		[STRING_UNITS_2] = units_2,
> +		[STRING_UNITS_10] = string_units_10,
> +		[STRING_UNITS_2] = string_units_2,
>  	};
>  	static const unsigned int divisor[] = {
>  		[STRING_UNITS_10] = 1000,
> @@ -92,7 +95,7 @@ void string_get_size(u64 size, u64 blk_size, const
> enum string_size_units units,
>  	}
>  
>   out:
> -	if (i >= ARRAY_SIZE(units_2))
> +	if (i >= ARRAY_SIZE(string_units_2))

so now, no-one other than string_helpers.c can tell the size of the
array ... I don't think that's an improvement.  Also for a trivial
patch I'm starting to think there should be a three strikes rule: we
get a large number of bugs from allegedly trivial reworks which
wouldn't have happened if we'd retained the original working code in
the first place.

After two attempts, doesn't it perhaps strike you that a helper
function rather than a direct export would get over this difficulty? 
 It might also address the precision problem you introduced.

James

Re: [PATCH v3 0/4] x86/efi: use binary units when printing

[James Bottomley]

On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:
> The patch series exports the arrays of binary and decimal units as
> it's
> described by IEC.
> 
> First user of it is EFI code which would print sizes and other values
> using
> binary prefix.
> 
> James, is this now okay to you?

Patch 2 is still pointless. I happen to like the <tab><space>label:
convention.  I realise others don't but an entire patch simply to
change my convention to your convention is a bit overkill.

I'll comment on the rest in the patches.

James

> Matt, I suppose we need to update the stuff in your tree.
> 
> Since v2:
> - address James comment (don't nail array size)
> - fix a title and commit message for patch 3 to be in align with the
> change
> 
> Andy Shevchenko (3):
>   lib/string_helpers: export string_units_{2,10} for others
>   lib/string_helpers: fix indentation in few places
>   x86/efi: Use proper units in efi_find_mirror()
> 
> Robert Elliott (1):
>   x86/efi: print size in binary units in efi_print_memmap
> 
>  arch/x86/platform/efi/efi.c    | 27 ++++++++++++++++++---------
>  include/linux/string_helpers.h |  3 +++
>  lib/string_helpers.c           | 26 ++++++++++++++------------
>  3 files changed, 35 insertions(+), 21 deletions(-)
> 

Re: [PATCH v3 0/4] x86/efi: use binary units when printing

[James Bottomley]

On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:
> The patch series exports the arrays of binary and decimal units as
> it's
> described by IEC.
> 
> First user of it is EFI code which would print sizes and other values
> using
> binary prefix.
> 
> James, is this now okay to you?

Patch 2 is still pointless. I happen to like the <tab><space>label:
convention.  I realise others don't but an entire patch simply to
change my convention to your convention is a bit overkill.

I'll comment on the rest in the patches.

James

> Matt, I suppose we need to update the stuff in your tree.
> 
> Since v2:
> - address James comment (don't nail array size)
> - fix a title and commit message for patch 3 to be in align with the
> change
> 
> Andy Shevchenko (3):
>   lib/string_helpers: export string_units_{2,10} for others
>   lib/string_helpers: fix indentation in few places
>   x86/efi: Use proper units in efi_find_mirror()
> 
> Robert Elliott (1):
>   x86/efi: print size in binary units in efi_print_memmap
> 
>  arch/x86/platform/efi/efi.c    | 27 ++++++++++++++++++---------
>  include/linux/string_helpers.h |  3 +++
>  lib/string_helpers.c           | 26 ++++++++++++++------------
>  3 files changed, 35 insertions(+), 21 deletions(-)
> 

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[James Bottomley]

On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:
> From: Robert Elliott <elliott@hpe.com>
> 
> Print the size in the best-fit B, KiB, MiB, etc. units rather than
> always MiB. This avoids rounding, which can be misleading.
> 
> Use proper IEC binary units (KiB, MiB, etc.) rather than misuse SI
> decimal units (KB, MB, etc.).
> 
> old:
>     efi: mem61: [Persistent Memory  |   |  |  |  |  |  |  
>  |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff)
> (16384MB)
> 
> new:
>     efi: mem61: [Persistent Memory  |   |  |  |  |  |  |  
>  |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff] (16 GiB)
> 
> Signed-off-by: Robert Elliott <elliott@hpe.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/x86/platform/efi/efi.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/platform/efi/efi.c
> b/arch/x86/platform/efi/efi.c
> index e0846b5..3badc8a 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -35,10 +35,12 @@
>  #include <linux/efi.h>
>  #include <linux/efi-bgrt.h>
>  #include <linux/export.h>
> +#include <linux/bitops.h>
>  #include <linux/bootmem.h>
>  #include <linux/slab.h>
>  #include <linux/memblock.h>
>  #include <linux/spinlock.h>
> +#include <linux/string_helpers.h>
>  #include <linux/uaccess.h>
>  #include <linux/time.h>
>  #include <linux/io.h>
> @@ -117,6 +119,14 @@ void efi_get_time(struct timespec *now)
>  	now->tv_nsec = 0;
>  }
>  
> +static char * __init efi_size_format(char *buf, size_t size, u64
> bytes)
> +{
> +	unsigned long i = bytes ? __ffs64(bytes) / 10 : 0;

What if size is zero, which might happen on a UEFI screw up?  Also it
gives really odd results for non power of two memory sizes. 16384MB
prints as 16GiB but 16385 prints as 16385MiB.

If the goal is to have a clean interface reporting only the first four
significant figures and a size exponent, then a helper would be much
better than trying to open code this ad hoc.

Not an attack on you patch per-se, but I really hate the IEC convention
that was essentially a ploy by disk manufacturers to inflate their disk
sizes by 10% simply by relabelling them.  Everyone was happy when a GB
was 2^30, now everyone's simply confused whenever they see GB.  We had
to pander to this in block devices because people got annoyed when we
reported a size that was different from the label but are you sure we
have to extend the madness to memory?

James


> +	snprintf(buf, size, "%llu %s", bytes >> (i * 10),
> string_units_2[i]);
>
> +	return buf;
> +}
> +
>  void __init efi_find_mirror(void)
>  {
>  	void *p;
> @@ -225,21 +235,20 @@ int __init efi_memblock_x86_reserve_range(void)
>  void __init efi_print_memmap(void)
>  {
>  #ifdef EFI_DEBUG
> -	efi_memory_desc_t *md;
>  	void *p;
>  	int i;
>  
>  	for (p = memmap.map, i = 0;
>  	     p < memmap.map_end;
>  	     p += memmap.desc_size, i++) {
> -		char buf[64];
> +		efi_memory_desc_t *md = p;
> +		u64 size = md->num_pages << EFI_PAGE_SHIFT;
> +		char buf[64], buf3[32];
>  
> -		md = p;
> -		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx]
> (%lluMB)\n",
> +		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx]
> (%s)\n",
>  			i, efi_md_typeattr_format(buf, sizeof(buf),
> md),
> -			md->phys_addr,
> -			md->phys_addr + (md->num_pages <<
> EFI_PAGE_SHIFT) - 1,
> -			(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
> +			md->phys_addr, md->phys_addr + size - 1,
> +			efi_size_format(buf3, sizeof(buf3), size));
>  	}
>  #endif  /*  EFI_DEBUG  */
>  }

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[James Bottomley]

On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:
> From: Robert Elliott <elliott-ZPxbGqLxI0U@public.gmane.org>
> 
> Print the size in the best-fit B, KiB, MiB, etc. units rather than
> always MiB. This avoids rounding, which can be misleading.
> 
> Use proper IEC binary units (KiB, MiB, etc.) rather than misuse SI
> decimal units (KB, MB, etc.).
> 
> old:
>     efi: mem61: [Persistent Memory  |   |  |  |  |  |  |  
>  |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff)
> (16384MB)
> 
> new:
>     efi: mem61: [Persistent Memory  |   |  |  |  |  |  |  
>  |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff] (16 GiB)
> 
> Signed-off-by: Robert Elliott <elliott-ZPxbGqLxI0U@public.gmane.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  arch/x86/platform/efi/efi.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/platform/efi/efi.c
> b/arch/x86/platform/efi/efi.c
> index e0846b5..3badc8a 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -35,10 +35,12 @@
>  #include <linux/efi.h>
>  #include <linux/efi-bgrt.h>
>  #include <linux/export.h>
> +#include <linux/bitops.h>
>  #include <linux/bootmem.h>
>  #include <linux/slab.h>
>  #include <linux/memblock.h>
>  #include <linux/spinlock.h>
> +#include <linux/string_helpers.h>
>  #include <linux/uaccess.h>
>  #include <linux/time.h>
>  #include <linux/io.h>
> @@ -117,6 +119,14 @@ void efi_get_time(struct timespec *now)
>  	now->tv_nsec = 0;
>  }
>  
> +static char * __init efi_size_format(char *buf, size_t size, u64
> bytes)
> +{
> +	unsigned long i = bytes ? __ffs64(bytes) / 10 : 0;

What if size is zero, which might happen on a UEFI screw up?  Also it
gives really odd results for non power of two memory sizes. 16384MB
prints as 16GiB but 16385 prints as 16385MiB.

If the goal is to have a clean interface reporting only the first four
significant figures and a size exponent, then a helper would be much
better than trying to open code this ad hoc.

Not an attack on you patch per-se, but I really hate the IEC convention
that was essentially a ploy by disk manufacturers to inflate their disk
sizes by 10% simply by relabelling them.  Everyone was happy when a GB
was 2^30, now everyone's simply confused whenever they see GB.  We had
to pander to this in block devices because people got annoyed when we
reported a size that was different from the label but are you sure we
have to extend the madness to memory?

James


> +	snprintf(buf, size, "%llu %s", bytes >> (i * 10),
> string_units_2[i]);
>
> +	return buf;
> +}
> +
>  void __init efi_find_mirror(void)
>  {
>  	void *p;
> @@ -225,21 +235,20 @@ int __init efi_memblock_x86_reserve_range(void)
>  void __init efi_print_memmap(void)
>  {
>  #ifdef EFI_DEBUG
> -	efi_memory_desc_t *md;
>  	void *p;
>  	int i;
>  
>  	for (p = memmap.map, i = 0;
>  	     p < memmap.map_end;
>  	     p += memmap.desc_size, i++) {
> -		char buf[64];
> +		efi_memory_desc_t *md = p;
> +		u64 size = md->num_pages << EFI_PAGE_SHIFT;
> +		char buf[64], buf3[32];
>  
> -		md = p;
> -		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx]
> (%lluMB)\n",
> +		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx]
> (%s)\n",
>  			i, efi_md_typeattr_format(buf, sizeof(buf),
> md),
> -			md->phys_addr,
> -			md->phys_addr + (md->num_pages <<
> EFI_PAGE_SHIFT) - 1,
> -			(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
> +			md->phys_addr, md->phys_addr + size - 1,
> +			efi_size_format(buf3, sizeof(buf3), size));
>  	}
>  #endif  /*  EFI_DEBUG  */
>  }

Re: [PATCH v3 1/4] lib/string_helpers: export string_units_{2,10} for others

[Andy Shevchenko]

On Sat, Jan 23, 2016 at 6:14 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:

> After two attempts, doesn't it perhaps strike you that a helper
> function rather than a direct export would get over this difficulty?

Yes, it would.

>  It might also address the precision problem you introduced.

You mean the issue with a unit range in the other cases (non-efi)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/4] lib/string_helpers: export string_units_{2,10} for others

[Andy Shevchenko]

On Sat, Jan 23, 2016 at 6:14 PM, James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:

> After two attempts, doesn't it perhaps strike you that a helper
> function rather than a direct export would get over this difficulty?

Yes, it would.

>  It might also address the precision problem you introduced.

You mean the issue with a unit range in the other cases (non-efi)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Andy Shevchenko]

On Sat, Jan 23, 2016 at 6:44 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:

>> +static char * __init efi_size_format(char *buf, size_t size, u64
>> bytes)
>> +{
>> +     unsigned long i = bytes ? __ffs64(bytes) / 10 : 0;
>
> What if size is zero, which might happen on a UEFI screw up?

size of what? Of input buffer?

>  Also it
> gives really odd results for non power of two memory sizes. 16384MB
> prints as 16GiB but 16385 prints as 16385MiB.

Adaptive precision. I don't think the idea is to print a nearby numbers here.

> If the goal is to have a clean interface reporting only the first four
> significant figures and a size exponent, then a helper would be much
> better than trying to open code this ad hoc.

No. You get it wrong. The initial idea was (actually not mine, see
authorship) to print an exact number with units and reduce whenever
it's possible, i.e number is a multiplication of certain unit.

> Not an attack on you patch per-se, but I really hate the IEC convention
> that was essentially a ploy by disk manufacturers to inflate their disk
> sizes by 10% simply by relabelling them.

>  Everyone was happy when a GB was 2^30,

No, not everyone. It was a misspelling done by some first storage
producer. Try to look at the problem from physics point of view. Units
are essential part of a value. There is an agreement how to use
multipliers and their code names. 1000 x Unit means kiloUnit. As per
agreement.

> now everyone's simply confused whenever they see GB.  We had
> to pander to this in block devices because people got annoyed when we
> reported a size that was different from the label but are you sure we
> have to extend the madness to memory?

I actually don't know who is from us is being more conservative. I
could call a madness to mess things from ancient (classical
mathematics and physics) with something which has less than hundred
years in development.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Andy Shevchenko]

On Sat, Jan 23, 2016 at 6:44 PM, James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:

>> +static char * __init efi_size_format(char *buf, size_t size, u64
>> bytes)
>> +{
>> +     unsigned long i = bytes ? __ffs64(bytes) / 10 : 0;
>
> What if size is zero, which might happen on a UEFI screw up?

size of what? Of input buffer?

>  Also it
> gives really odd results for non power of two memory sizes. 16384MB
> prints as 16GiB but 16385 prints as 16385MiB.

Adaptive precision. I don't think the idea is to print a nearby numbers here.

> If the goal is to have a clean interface reporting only the first four
> significant figures and a size exponent, then a helper would be much
> better than trying to open code this ad hoc.

No. You get it wrong. The initial idea was (actually not mine, see
authorship) to print an exact number with units and reduce whenever
it's possible, i.e number is a multiplication of certain unit.

> Not an attack on you patch per-se, but I really hate the IEC convention
> that was essentially a ploy by disk manufacturers to inflate their disk
> sizes by 10% simply by relabelling them.

>  Everyone was happy when a GB was 2^30,

No, not everyone. It was a misspelling done by some first storage
producer. Try to look at the problem from physics point of view. Units
are essential part of a value. There is an agreement how to use
multipliers and their code names. 1000 x Unit means kiloUnit. As per
agreement.

> now everyone's simply confused whenever they see GB.  We had
> to pander to this in block devices because people got annoyed when we
> reported a size that was different from the label but are you sure we
> have to extend the madness to memory?

I actually don't know who is from us is being more conservative. I
could call a madness to mess things from ancient (classical
mathematics and physics) with something which has less than hundred
years in development.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/4] x86/efi: use binary units when printing

[Andy Shevchenko]

On Sat, Jan 23, 2016 at 6:34 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:
>> The patch series exports the arrays of binary and decimal units as
>> it's
>> described by IEC.
>>
>> First user of it is EFI code which would print sizes and other values
>> using
>> binary prefix.
>>
>> James, is this now okay to you?
>
> Patch 2 is still pointless. I happen to like the <tab><space>label:
> convention.

Even there it's just <space>label:

>  I realise others don't but an entire patch simply to
> change my convention to your convention is a bit overkill.

Main point of that patch is to bring snprintf() to one line. Is it
also your style?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/4] x86/efi: use binary units when printing

[Andy Shevchenko]

On Sat, Jan 23, 2016 at 6:34 PM, James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:
>> The patch series exports the arrays of binary and decimal units as
>> it's
>> described by IEC.
>>
>> First user of it is EFI code which would print sizes and other values
>> using
>> binary prefix.
>>
>> James, is this now okay to you?
>
> Patch 2 is still pointless. I happen to like the <tab><space>label:
> convention.

Even there it's just <space>label:

>  I realise others don't but an entire patch simply to
> change my convention to your convention is a bit overkill.

Main point of that patch is to bring snprintf() to one line. Is it
also your style?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[James Bottomley]

On Sat, 2016-01-23 at 19:18 +0200, Andy Shevchenko wrote:
> On Sat, Jan 23, 2016 at 6:44 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:
> 
> > > +static char * __init efi_size_format(char *buf, size_t size, u64
> > > bytes)
> > > +{
> > > +     unsigned long i = bytes ? __ffs64(bytes) / 10 : 0;
> > 
> > What if size is zero, which might happen on a UEFI screw up?
> 
> size of what? Of input buffer?

I mean when bytes == 0 ffs is undefined.

> >  Also it gives really odd results for non power of two memory 
> > sizes. 16384MB prints as 16GiB but 16385 prints as 16385MiB.
> 
> Adaptive precision. I don't think the idea is to print a nearby
> numbers here.

Well either there's a point to reducing to the nearest exponent or we
simply print everything in MB as the original did.  Doing it
inconsistently is asking for trouble ... and lots of user queries.  I
mean, supposing there's a range off by one ... now we print a huge
number in B.

I really advise against hacking around like this.  In any event if efi
must have this, please don't involve the parts of the kernel that try
to do this correctly, like lib/string_helpers.h

> > If the goal is to have a clean interface reporting only the first 
> > four significant figures and a size exponent, then a helper would 
> > be much better than trying to open code this ad hoc.
> 
> No. You get it wrong. The initial idea was (actually not mine, see
> authorship) to print an exact number with units and reduce whenever
> it's possible, i.e number is a multiplication of certain unit.

so you must implement the original idea no matter how inconsistent it
leads us to be?  Is it wrong to try to do better?

James

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[James Bottomley]

On Sat, 2016-01-23 at 19:18 +0200, Andy Shevchenko wrote:
> On Sat, Jan 23, 2016 at 6:44 PM, James Bottomley
> <James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> > On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:
> 
> > > +static char * __init efi_size_format(char *buf, size_t size, u64
> > > bytes)
> > > +{
> > > +     unsigned long i = bytes ? __ffs64(bytes) / 10 : 0;
> > 
> > What if size is zero, which might happen on a UEFI screw up?
> 
> size of what? Of input buffer?

I mean when bytes == 0 ffs is undefined.

> >  Also it gives really odd results for non power of two memory 
> > sizes. 16384MB prints as 16GiB but 16385 prints as 16385MiB.
> 
> Adaptive precision. I don't think the idea is to print a nearby
> numbers here.

Well either there's a point to reducing to the nearest exponent or we
simply print everything in MB as the original did.  Doing it
inconsistently is asking for trouble ... and lots of user queries.  I
mean, supposing there's a range off by one ... now we print a huge
number in B.

I really advise against hacking around like this.  In any event if efi
must have this, please don't involve the parts of the kernel that try
to do this correctly, like lib/string_helpers.h

> > If the goal is to have a clean interface reporting only the first 
> > four significant figures and a size exponent, then a helper would 
> > be much better than trying to open code this ad hoc.
> 
> No. You get it wrong. The initial idea was (actually not mine, see
> authorship) to print an exact number with units and reduce whenever
> it's possible, i.e number is a multiplication of certain unit.

so you must implement the original idea no matter how inconsistent it
leads us to be?  Is it wrong to try to do better?

James

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[James Bottomley]

On Sat, 2016-01-23 at 19:18 +0200, Andy Shevchenko wrote:
> On Sat, Jan 23, 2016 at 6:44 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > Not an attack on you patch per-se, but I really hate the IEC
> > convention
> > that was essentially a ploy by disk manufacturers to inflate their
> > disk
> > sizes by 10% simply by relabelling them.
> 
> >  Everyone was happy when a GB was 2^30,
> 
> No, not everyone. It was a misspelling done by some first storage
> producer.

No, it was the adopted convention in computer science to use units in
2^10 since all sizes were usually binary.  It actually began with
memory sizes.  So a GB never meant 10^9 bytes before 2007 because the
classical unit users had no idea what a byte was.  The first bytes were
counted in powers of 2.

>  Try to look at the problem from physics point of view. Units
> are essential part of a value. There is an agreement how to use
> multipliers and their code names. 1000 x Unit means kiloUnit. As per
> agreement.

It's not about physics (and certainly astronomers, who often take pi to
be 1 would be happy with 2^10 = 10^3) it's about politics: disk
manufacturers wanted a way to report bigger sizes, so they made a huge
fuss about the "inconsistency" of computer science using units in
increments of 2^10 and forced through this IEC standard.

> > now everyone's simply confused whenever they see GB.  We had
> > to pander to this in block devices because people got annoyed when 
> > we reported a size that was different from the label but are you 
> > sure we have to extend the madness to memory?
> 
> I actually don't know who is from us is being more conservative. I
> could call a madness to mess things from ancient (classical
> mathematics and physics) with something which has less than hundred
> years in development.

In engineering terms, counting in powers of 2 makes a lot of sense for
quantites using binary address busses and the IEC even recognised that
by inventing a new unit for it.  Having GB mean 2^30 up to 2007 and
10^9 after it is confusing for everyone born before about 1990.

James

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[James Bottomley]

On Sat, 2016-01-23 at 19:18 +0200, Andy Shevchenko wrote:
> On Sat, Jan 23, 2016 at 6:44 PM, James Bottomley
> <James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> > Not an attack on you patch per-se, but I really hate the IEC
> > convention
> > that was essentially a ploy by disk manufacturers to inflate their
> > disk
> > sizes by 10% simply by relabelling them.
> 
> >  Everyone was happy when a GB was 2^30,
> 
> No, not everyone. It was a misspelling done by some first storage
> producer.

No, it was the adopted convention in computer science to use units in
2^10 since all sizes were usually binary.  It actually began with
memory sizes.  So a GB never meant 10^9 bytes before 2007 because the
classical unit users had no idea what a byte was.  The first bytes were
counted in powers of 2.

>  Try to look at the problem from physics point of view. Units
> are essential part of a value. There is an agreement how to use
> multipliers and their code names. 1000 x Unit means kiloUnit. As per
> agreement.

It's not about physics (and certainly astronomers, who often take pi to
be 1 would be happy with 2^10 = 10^3) it's about politics: disk
manufacturers wanted a way to report bigger sizes, so they made a huge
fuss about the "inconsistency" of computer science using units in
increments of 2^10 and forced through this IEC standard.

> > now everyone's simply confused whenever they see GB.  We had
> > to pander to this in block devices because people got annoyed when 
> > we reported a size that was different from the label but are you 
> > sure we have to extend the madness to memory?
> 
> I actually don't know who is from us is being more conservative. I
> could call a madness to mess things from ancient (classical
> mathematics and physics) with something which has less than hundred
> years in development.

In engineering terms, counting in powers of 2 makes a lot of sense for
quantites using binary address busses and the IEC even recognised that
by inventing a new unit for it.  Having GB mean 2^30 up to 2007 and
10^9 after it is confusing for everyone born before about 1990.

James

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[One Thousand Gnomes]

All a bit revisionist. Everyone else on the planet was upset about it
because it broke things like calculating bit density because the prefixes
for the bit capacity are not in metric form. BIPM (keeper of the SI
units) never approved powers of two as an interpretation. IEC came into
line in 1999, ISO followed.

Disk sizes have been decimal since at least the 1970s. The original IBM
10MB hard disc for example was 10MB not 10MiB.

Powers of two are only validly referred to as KiB, MiB, GiB as of all
current standard body positions. Powers of 10 based units are kB, MB, GB)

(The best one is CD and DVD: DVD uses the proper definition, CD uses MiB,
although given the multiple sector sizes and encodings on CD it's all
manure anyway)

Alan

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[H. Peter Anvin]

On January 23, 2016 12:29:26 PM PST, One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
>All a bit revisionist. Everyone else on the planet was upset about it
>because it broke things like calculating bit density because the
>prefixes
>for the bit capacity are not in metric form. BIPM (keeper of the SI
>units) never approved powers of two as an interpretation. IEC came into
>line in 1999, ISO followed.
>
>Disk sizes have been decimal since at least the 1970s. The original IBM
>10MB hard disc for example was 10MB not 10MiB.
>
>Powers of two are only validly referred to as KiB, MiB, GiB as of all
>current standard body positions. Powers of 10 based units are kB, MB,
>GB)
>
>(The best one is CD and DVD: DVD uses the proper definition, CD uses
>MiB,
>although given the multiple sector sizes and encodings on CD it's all
>manure anyway)
>
>Alan

Then there are oddball definitions like 1 MB = 1,024,000, which IBM used for disk for a long time.  At least IEC tried to come up with a unambiguous way to denote these prefixes.  It was less of an issue for kilo- since the binary prefix was always capitalized.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[H. Peter Anvin]

On January 23, 2016 12:29:26 PM PST, One Thousand Gnomes <gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org> wrote:
>All a bit revisionist. Everyone else on the planet was upset about it
>because it broke things like calculating bit density because the
>prefixes
>for the bit capacity are not in metric form. BIPM (keeper of the SI
>units) never approved powers of two as an interpretation. IEC came into
>line in 1999, ISO followed.
>
>Disk sizes have been decimal since at least the 1970s. The original IBM
>10MB hard disc for example was 10MB not 10MiB.
>
>Powers of two are only validly referred to as KiB, MiB, GiB as of all
>current standard body positions. Powers of 10 based units are kB, MB,
>GB)
>
>(The best one is CD and DVD: DVD uses the proper definition, CD uses
>MiB,
>although given the multiple sector sizes and encodings on CD it's all
>manure anyway)
>
>Alan

Then there are oddball definitions like 1 MB = 1,024,000, which IBM used for disk for a long time.  At least IEC tried to come up with a unambiguous way to denote these prefixes.  It was less of an issue for kilo- since the binary prefix was always capitalized.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Andy Shevchenko]

On Sat, 2016-01-23 at 10:03 -0800, James Bottomley wrote:
> On Sat, 2016-01-23 at 19:18 +0200, Andy Shevchenko wrote:
> > On Sat, Jan 23, 2016 at 6:44 PM, James Bottomley
> > <James.Bottomley@hansenpartnership.com> wrote:
> > > On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:
> > 
> > > > +static char * __init efi_size_format(char *buf, size_t size,
> > > > u64
> > > > bytes)
> > > > +{
> > > > +     unsigned long i = bytes ? __ffs64(bytes) / 10 : 0;
> > > 
> > > What if size is zero, which might happen on a UEFI screw up?
> > 
> > size of what? Of input buffer?
> 
> I mean when bytes == 0 ffs is undefined.

Well, someone misread the above code ;-)

There is ternary operator exactly to serve this purpose.

> 
> > >  Also it gives really odd results for non power of two memory 
> > > sizes. 16384MB prints as 16GiB but 16385 prints as 16385MiB.
> > 
> > Adaptive precision. I don't think the idea is to print a nearby
> > numbers here.
> 
> Well either there's a point to reducing to the nearest exponent or we
> simply print everything in MB as the original did.  Doing it
> inconsistently is asking for trouble ... and lots of user queries.  I
> mean, supposing there's a range off by one ... now we print a huge
> number in B.

> I really advise against hacking around like this.  In any event if
> efi
> must have this, please don't involve the parts of the kernel that try
> to do this correctly, like lib/string_helpers.h

> 
> > > If the goal is to have a clean interface reporting only the
> > > first 
> > > four significant figures and a size exponent, then a helper
> > > would 
> > > be much better than trying to open code this ad hoc.
> > 
> > No. You get it wrong. The initial idea was (actually not mine, see
> > authorship) to print an exact number with units and reduce whenever
> > it's possible, i.e number is a multiplication of certain unit.
> 
> so you must implement the original idea no matter how inconsistent it
> leads us to be?  Is it wrong to try to do better?

For both comments I prefer to hear Matt's opinion as he is maintainer
of EFI stuff.

My role in this all is to reduce the code base by avoiding 'not
invented here' syndrome.

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

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Andy Shevchenko]

On Sat, 2016-01-23 at 10:03 -0800, James Bottomley wrote:
> On Sat, 2016-01-23 at 19:18 +0200, Andy Shevchenko wrote:
> > On Sat, Jan 23, 2016 at 6:44 PM, James Bottomley
> > <James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> > > On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:
> > 
> > > > +static char * __init efi_size_format(char *buf, size_t size,
> > > > u64
> > > > bytes)
> > > > +{
> > > > +     unsigned long i = bytes ? __ffs64(bytes) / 10 : 0;
> > > 
> > > What if size is zero, which might happen on a UEFI screw up?
> > 
> > size of what? Of input buffer?
> 
> I mean when bytes == 0 ffs is undefined.

Well, someone misread the above code ;-)

There is ternary operator exactly to serve this purpose.

> 
> > >  Also it gives really odd results for non power of two memory 
> > > sizes. 16384MB prints as 16GiB but 16385 prints as 16385MiB.
> > 
> > Adaptive precision. I don't think the idea is to print a nearby
> > numbers here.
> 
> Well either there's a point to reducing to the nearest exponent or we
> simply print everything in MB as the original did.  Doing it
> inconsistently is asking for trouble ... and lots of user queries.  I
> mean, supposing there's a range off by one ... now we print a huge
> number in B.

> I really advise against hacking around like this.  In any event if
> efi
> must have this, please don't involve the parts of the kernel that try
> to do this correctly, like lib/string_helpers.h

> 
> > > If the goal is to have a clean interface reporting only the
> > > first 
> > > four significant figures and a size exponent, then a helper
> > > would 
> > > be much better than trying to open code this ad hoc.
> > 
> > No. You get it wrong. The initial idea was (actually not mine, see
> > authorship) to print an exact number with units and reduce whenever
> > it's possible, i.e number is a multiplication of certain unit.
> 
> so you must implement the original idea no matter how inconsistent it
> leads us to be?  Is it wrong to try to do better?

For both comments I prefer to hear Matt's opinion as he is maintainer
of EFI stuff.

My role in this all is to reduce the code base by avoiding 'not
invented here' syndrome.

-- 
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[James Bottomley]

On Mon, 2016-01-25 at 10:31 +0200, Andy Shevchenko wrote:
> On Sat, 2016-01-23 at 10:03 -0800, James Bottomley wrote:
> > On Sat, 2016-01-23 at 19:18 +0200, Andy Shevchenko wrote:
> > > > If the goal is to have a clean interface reporting only the
> > > > first 
> > > > four significant figures and a size exponent, then a helper
> > > > would 
> > > > be much better than trying to open code this ad hoc.
> > > 
> > > No. You get it wrong. The initial idea was (actually not mine,
> > > see
> > > authorship) to print an exact number with units and reduce
> > > whenever
> > > it's possible, i.e number is a multiplication of certain unit.
> > 
> > so you must implement the original idea no matter how inconsistent
> > it
> > leads us to be?  Is it wrong to try to do better?
> 
> For both comments I prefer to hear Matt's opinion as he is maintainer
> of EFI stuff.
> 
> My role in this all is to reduce the code base by avoiding 'not
> invented here' syndrome.

To get up to the starting blocks on that one, you need a usable
interface.  Neither of the series has that for lib/string_helper.c, so
this is a NAK to both of those parts.

I already told you how to do it in a way that creates a maintainable
interface.  It looks obvious to me and I was politely waiting for you
to produce it, but if you're saying your not able to, I suppose I can
produce the patch.

James

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[James Bottomley]

On Mon, 2016-01-25 at 10:31 +0200, Andy Shevchenko wrote:
> On Sat, 2016-01-23 at 10:03 -0800, James Bottomley wrote:
> > On Sat, 2016-01-23 at 19:18 +0200, Andy Shevchenko wrote:
> > > > If the goal is to have a clean interface reporting only the
> > > > first 
> > > > four significant figures and a size exponent, then a helper
> > > > would 
> > > > be much better than trying to open code this ad hoc.
> > > 
> > > No. You get it wrong. The initial idea was (actually not mine,
> > > see
> > > authorship) to print an exact number with units and reduce
> > > whenever
> > > it's possible, i.e number is a multiplication of certain unit.
> > 
> > so you must implement the original idea no matter how inconsistent
> > it
> > leads us to be?  Is it wrong to try to do better?
> 
> For both comments I prefer to hear Matt's opinion as he is maintainer
> of EFI stuff.
> 
> My role in this all is to reduce the code base by avoiding 'not
> invented here' syndrome.

To get up to the starting blocks on that one, you need a usable
interface.  Neither of the series has that for lib/string_helper.c, so
this is a NAK to both of those parts.

I already told you how to do it in a way that creates a maintainable
interface.  It looks obvious to me and I was politely waiting for you
to produce it, but if you're saying your not able to, I suppose I can
produce the patch.

James

RE: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Saturday, January 23, 2016 10:44 AM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Matt Fleming
> <matt@codeblueprint.co.uk>; Thomas Gleixner <tglx@linutronix.de>; Ingo
> Molnar <mingo@redhat.com>; H . Peter Anvin <hpa@zytor.com>; linux-
> efi@vger.kernel.org; Rasmus Villemoes <linux@rasmusvillemoes.dk>; Andrew
> Morton <akpm@linux-foundation.org>; linux-kernel @ vger . kernel . org
> <linux-kernel@vger.kernel.org>
> Cc: Elliott, Robert (Persistent Memory) <elliott@hpe.com>
> Subject: Re: [PATCH v3 3/4] x86/efi: print size in binary units in
> efi_print_memmap
> 
> On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:
> > From: Robert Elliott <elliott@hpe.com>
> >
> > Print the size in the best-fit B, KiB, MiB, etc. units rather than
> > always MiB. This avoids rounding, which can be misleading.
> >

...
> 
> What if size is zero, which might happen on a UEFI screw up?  

> Also it gives really odd results for non power of two memory sizes. 
> 16384MB prints as 16GiB but 16385 prints as 16385MiB.
> If the goal is to have a clean interface reporting only the first four
> significant figures and a size exponent, then a helper would be much
> better than trying to open code this ad hoc.

An impetus for the patch was to stop rounding the sub-MiB values,
which is misleading and can hide bugs.  For my systems, the
minimum size of a range happens to be 4 KiB, so I wanted at least
that resolution. However, I don't want to print everything as KiB,
because that makes big sizes less clear.

Example - old output:
efi: mem00: [Conventional Memory...] range=[0x0000000000000000-0x0000000000001000) (0MB)
efi: mem01: [Loader Data        ...] range=[0x0000000000001000-0x0000000000002000) (0MB)
efi: mem02: [Conventional Memory...] range=[0x0000000000002000-0x0000000000093000) (0MB)
efi: mem03: [Reserved           ...] range=[0x0000000000093000-0x0000000000094000) (0MB)

Proposed output:
efi: mem00: [Conventional Memory...] range=[0x0000000000000000-0x0000000000092fff] (588 KiB @ 0 B)
efi: mem01: [Reserved           ...] range=[0x0000000000093000-0x0000000000093fff] (4 KiB @ 588 KiB)
efi: mem02: [Conventional Memory...] range=[0x0000000000094000-0x000000000009ffff] (48 KiB @ 592 KiB)
efi: mem03: [Loader Data        ...] range=[0x0000000000100000-0x00000000013e8fff] (19364 KiB @ 1 MiB)
(notes:
 - from a different system
 - including both base and size
 - Matt didn't like printing the base so that's been removed)

With persistent memory (NVDIMMs) bringing storage device capacities
into the memory subsystem, MiB is too small.  Seeing a 1 TiB NVDIMM
as 1 TiB is a lot clearer than having to recognize 1048576 MiB as
the same value (especially since these power-of-two quantities
don't just chop off zeros on the right).

Examples:
efi: mem50: [Runtime Data       ...] range=[0x00000000784ff000-0x00000000788fefff] (4 MiB @ 1971196 KiB)
efi: mem56: [Conventional Memory...] range=[0x0000000100000000-0x000000087fffffff] (30 GiB @ 4 GiB)
efi: mem58: [Memory Mapped I/O  ...] range=[0x0000000080000000-0x000000008fffffff] (256 MiB @ 2 GiB)
efi: mem60: [Persistent Memory  ...] range=[0x0000001480000000-0x0000001a7fffffff] (24 GiB @ 82 GiB)


---
Robert Elliott, HPE Persistent Memory

RE: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Saturday, January 23, 2016 10:44 AM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Matt Fleming
> <matt@codeblueprint.co.uk>; Thomas Gleixner <tglx@linutronix.de>; Ingo
> Molnar <mingo@redhat.com>; H . Peter Anvin <hpa@zytor.com>; linux-
> efi@vger.kernel.org; Rasmus Villemoes <linux@rasmusvillemoes.dk>; Andrew
> Morton <akpm@linux-foundation.org>; linux-kernel @ vger . kernel . org
> <linux-kernel@vger.kernel.org>
> Cc: Elliott, Robert (Persistent Memory) <elliott@hpe.com>
> Subject: Re: [PATCH v3 3/4] x86/efi: print size in binary units in
> efi_print_memmap
> 
> On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:
> > From: Robert Elliott <elliott@hpe.com>
> >
> > Print the size in the best-fit B, KiB, MiB, etc. units rather than
> > always MiB. This avoids rounding, which can be misleading.
> >

...
> 
> What if size is zero, which might happen on a UEFI screw up?  

> Also it gives really odd results for non power of two memory sizes. 
> 16384MB prints as 16GiB but 16385 prints as 16385MiB.
> If the goal is to have a clean interface reporting only the first four
> significant figures and a size exponent, then a helper would be much
> better than trying to open code this ad hoc.

An impetus for the patch was to stop rounding the sub-MiB values,
which is misleading and can hide bugs.  For my systems, the
minimum size of a range happens to be 4 KiB, so I wanted at least
that resolution. However, I don't want to print everything as KiB,
because that makes big sizes less clear.

Example - old output:
efi: mem00: [Conventional Memory...] range=[0x0000000000000000-0x0000000000001000) (0MB)
efi: mem01: [Loader Data        ...] range=[0x0000000000001000-0x0000000000002000) (0MB)
efi: mem02: [Conventional Memory...] range=[0x0000000000002000-0x0000000000093000) (0MB)
efi: mem03: [Reserved           ...] range=[0x0000000000093000-0x0000000000094000) (0MB)

Proposed output:
efi: mem00: [Conventional Memory...] range=[0x0000000000000000-0x0000000000092fff] (588 KiB @ 0 B)
efi: mem01: [Reserved           ...] range=[0x0000000000093000-0x0000000000093fff] (4 KiB @ 588 KiB)
efi: mem02: [Conventional Memory...] range=[0x0000000000094000-0x000000000009ffff] (48 KiB @ 592 KiB)
efi: mem03: [Loader Data        ...] range=[0x0000000000100000-0x00000000013e8fff] (19364 KiB @ 1 MiB)
(notes:
 - from a different system
 - including both base and size
 - Matt didn't like printing the base so that's been removed)

With persistent memory (NVDIMMs) bringing storage device capacities
into the memory subsystem, MiB is too small.  Seeing a 1 TiB NVDIMM
as 1 TiB is a lot clearer than having to recognize 1048576 MiB as
the same value (especially since these power-of-two quantities
don't just chop off zeros on the right).

Examples:
efi: mem50: [Runtime Data       ...] range=[0x00000000784ff000-0x00000000788fefff] (4 MiB @ 1971196 KiB)
efi: mem56: [Conventional Memory...] range=[0x0000000100000000-0x000000087fffffff] (30 GiB @ 4 GiB)
efi: mem58: [Memory Mapped I/O  ...] range=[0x0000000080000000-0x000000008fffffff] (256 MiB @ 2 GiB)
efi: mem60: [Persistent Memory  ...] range=[0x0000001480000000-0x0000001a7fffffff] (24 GiB @ 82 GiB)


---
Robert Elliott, HPE Persistent Memory

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[James Bottomley]

On Mon, 2016-01-25 at 18:02 +0000, Elliott, Robert (Persistent Memory)
wrote:
> 
> 
> > -----Original Message-----
> > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com
> > ]
> > Sent: Saturday, January 23, 2016 10:44 AM
> > To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Matt
> > Fleming
> > <matt@codeblueprint.co.uk>; Thomas Gleixner <tglx@linutronix.de>;
> > Ingo
> > Molnar <mingo@redhat.com>; H . Peter Anvin <hpa@zytor.com>; linux-
> > efi@vger.kernel.org; Rasmus Villemoes <linux@rasmusvillemoes.dk>;
> > Andrew
> > Morton <akpm@linux-foundation.org>; linux-kernel @ vger . kernel .
> > org
> > <linux-kernel@vger.kernel.org>
> > Cc: Elliott, Robert (Persistent Memory) <elliott@hpe.com>
> > Subject: Re: [PATCH v3 3/4] x86/efi: print size in binary units in
> > efi_print_memmap
> > 
> > On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:
> > > From: Robert Elliott <elliott@hpe.com>
> > > 
> > > Print the size in the best-fit B, KiB, MiB, etc. units rather
> > > than
> > > always MiB. This avoids rounding, which can be misleading.
> > > 
> 
> ...
> > 
> > What if size is zero, which might happen on a UEFI screw up?  
> 
> > Also it gives really odd results for non power of two memory sizes.
> > 16384MB prints as 16GiB but 16385 prints as 16385MiB.
> > If the goal is to have a clean interface reporting only the first
> > four
> > significant figures and a size exponent, then a helper would be
> > much
> > better than trying to open code this ad hoc.
> 
> An impetus for the patch was to stop rounding the sub-MiB values,
> which is misleading and can hide bugs.  For my systems, the
> minimum size of a range happens to be 4 KiB, so I wanted at least
> that resolution. However, I don't want to print everything as KiB,
> because that makes big sizes less clear.
> 
> Example - old output:
> efi: mem00: [Conventional Memory...] range=[0x0000000000000000
> -0x0000000000001000) (0MB)
> efi: mem01: [Loader Data        ...] range=[0x0000000000001000
> -0x0000000000002000) (0MB)
> efi: mem02: [Conventional Memory...] range=[0x0000000000002000
> -0x0000000000093000) (0MB)
> efi: mem03: [Reserved           ...] range=[0x0000000000093000
> -0x0000000000094000) (0MB)
> 
> Proposed output:
> efi: mem00: [Conventional Memory...] range=[0x0000000000000000
> -0x0000000000092fff] (588 KiB @ 0 B)
> efi: mem01: [Reserved           ...] range=[0x0000000000093000
> -0x0000000000093fff] (4 KiB @ 588 KiB)
> efi: mem02: [Conventional Memory...] range=[0x0000000000094000
> -0x000000000009ffff] (48 KiB @ 592 KiB)
> efi: mem03: [Loader Data        ...] range=[0x0000000000100000
> -0x00000000013e8fff] (19364 KiB @ 1 MiB)
> (notes:
>  - from a different system
>  - including both base and size
>  - Matt didn't like printing the base so that's been removed)
> 
> With persistent memory (NVDIMMs) bringing storage device capacities
> into the memory subsystem, MiB is too small.  Seeing a 1 TiB NVDIMM
> as 1 TiB is a lot clearer than having to recognize 1048576 MiB as
> the same value (especially since these power-of-two quantities
> don't just chop off zeros on the right).
> 
> Examples:
> efi: mem50: [Runtime Data       ...] range=[0x00000000784ff000
> -0x00000000788fefff] (4 MiB @ 1971196 KiB)
> efi: mem56: [Conventional Memory...] range=[0x0000000100000000
> -0x000000087fffffff] (30 GiB @ 4 GiB)
> efi: mem58: [Memory Mapped I/O  ...] range=[0x0000000080000000
> -0x000000008fffffff] (256 MiB @ 2 GiB)
> efi: mem60: [Persistent Memory  ...] range=[0x0000001480000000
> -0x0000001a7fffffff] (24 GiB @ 82 GiB)

OK, this is getting a bit out of hand: I didn't say your aim was bad
... I think it's a reasonable desire; I said the proposed
implementation was bad.  Using ffs leads to precision runaway and
exporting an array from string_helpers.c is simply the wrong way to do
it.

Since we've now spent more time arguing about this than it would take
to do a correct patch, this is what I was thinking.  It extracts the
precision reduction core from string_helpers.c and exposes it to all
users who want to convert to units.  I added a nozeros option becuase I
think you want it to print 1 GiB rather than 1.00 GiB for exact powers
of two.  (OK, and I fixed a bug where it will report small amounts as
1.00 B instead of whole number of bytes).  Absent the nozero option,
you could simply have used string_get_size(), with a block size of 1.

James

---

diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index dabe643..78935fae 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -10,6 +10,8 @@ enum string_size_units {
 	STRING_UNITS_2,		/* use binary powers of 2^10 */
 };
 
+void string_get_units(u64 size, const enum string_size_units units,
+		      char *buf, int len, bool nozeros);
 void string_get_size(u64 size, u64 blk_size, enum string_size_units units,
 		     char *buf, int len);
 
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 5c88204..ab6b332 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -13,21 +13,13 @@
 #include <linux/string.h>
 #include <linux/string_helpers.h>
 
-/**
- * string_get_size - get the size in the specified units
- * @size:	The size to be converted in blocks
- * @blk_size:	Size of the block (use 1 for size in bytes)
- * @units:	units to use (powers of 1000 or 1024)
- * @buf:	buffer to format to
- * @len:	length of buffer
- *
- * This function returns a string formatted to 3 significant figures
- * giving the size in the required units.  @buf should have room for
- * 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,
-		     char *buf, int len)
+static const unsigned int divisor[] = {
+	[STRING_UNITS_10] = 1000,
+	[STRING_UNITS_2] = 1024,
+};
+
+static void string_reduce(u64 size, int log, const enum string_size_units units,
+			  char *buf, int len, bool nozeros)
 {
 	static const char *const units_10[] = {
 		"B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB"
@@ -39,52 +31,23 @@ void string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
 		[STRING_UNITS_10] = units_10,
 		[STRING_UNITS_2] = units_2,
 	};
-	static const unsigned int divisor[] = {
-		[STRING_UNITS_10] = 1000,
-		[STRING_UNITS_2] = 1024,
-	};
 	static const unsigned int rounding[] = { 500, 50, 5 };
-	int i = 0, j;
-	u32 remainder = 0, sf_cap;
+	char zeros[] = ".00";
+
+	int j;
+	u32 sf_cap, remainder = 0;
 	char tmp[8];
 	const char *unit;
 
 	tmp[0] = '\0';
 
-	if (blk_size == 0)
-		size = 0;
 	if (size == 0)
 		goto out;
 
-	/* This is Napier's algorithm.  Reduce the original block size to
-	 *
-	 * coefficient * divisor[units]^i
-	 *
-	 * we do the reduction so both coefficients are just under 32 bits so
-	 * that multiplying them together won't overflow 64 bits and we keep
-	 * as much precision as possible in the numbers.
-	 *
-	 * Note: it's safe to throw away the remainders here because all the
-	 * precision is in the coefficients.
-	 */
-	while (blk_size >> 32) {
-		do_div(blk_size, divisor[units]);
-		i++;
-	}
-
-	while (size >> 32) {
-		do_div(size, divisor[units]);
-		i++;
-	}
-
-	/* now perform the actual multiplication keeping i as the sum of the
-	 * two logarithms */
-	size *= blk_size;
-
-	/* and logarithmically reduce it until it's just under the divisor */
+	/* Logarithmically reduce it until it's just under the divisor */
 	while (size >= divisor[units]) {
 		remainder = do_div(size, divisor[units]);
-		i++;
+		log++;
 	}
 
 	/* work out in j how many digits of precision we need from the
@@ -109,21 +72,93 @@ void string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
 		size += 1;
 	}
 
-	if (j) {
+	if (j && log) {
 		snprintf(tmp, sizeof(tmp), ".%03u", remainder);
 		tmp[j+1] = '\0';
+		zeros[j+1] = '\0';
+		if (nozeros && strcmp(tmp, zeros) == 0)
+			tmp[0]='\0';
 	}
 
  out:
-	if (i >= ARRAY_SIZE(units_2))
+	if (log >= ARRAY_SIZE(units_2))
 		unit = "UNK";
 	else
-		unit = units_str[units][i];
+		unit = units_str[units][log];
 
 	snprintf(buf, len, "%u%s %s", (u32)size,
 		 tmp, unit);
 }
-EXPORT_SYMBOL(string_get_size);
+
+/**
+ * string_get_units - convert size to specified units
+ * @size:	The quantity to be converted
+ * @units:	units to use (powers of 1000 or 1024)
+ * @buf:	buffer to format to
+ * @len:	length of buffer
+ * @nozereos:	eliminate zeros after the decimal point if true
+ *
+ * This function returns a string formatted to 3 significant figures
+ * giving the size in the required units.  @buf should have room for
+ * at least 9 bytes and will always be zero terminated.
+ */
+void string_get_units(u64 size, const enum string_size_units units,
+		      char *buf, int len, bool nozeros)
+{
+	string_reduce(size, 0, units, buf, len, nozeros);
+}
+
+/**
+ * string_get_size - get the size in the specified units
+ * @size:	The size to be converted in blocks
+ * @blk_size:	Size of the block (use 1 for size in bytes)
+ * @units:	units to use (powers of 1000 or 1024)
+ * @buf:	buffer to format to
+ * @len:	length of buffer
+ *
+ * This function returns a string formatted to 3 significant figures
+ * giving the size in the required units.  @buf should have room for
+ * 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,
+		     char *buf, int len)
+{
+	int i = 0;
+
+	if (blk_size == 0)
+		size = 0;
+	if (size == 0)
+		goto out;
+
+	/* This is Napier's algorithm.  Reduce the original block size to
+	 *
+	 * coefficient * divisor[units]^i
+	 *
+	 * we do the reduction so both coefficients are just under 32 bits so
+	 * that multiplying them together won't overflow 64 bits and we keep
+	 * as much precision as possible in the numbers.
+	 *
+	 * Note: it's safe to throw away the remainders here because all the
+	 * precision is in the coefficients.
+	 */
+	while (blk_size >> 32) {
+		do_div(blk_size, divisor[units]);
+		i++;
+	}
+
+	while (size >> 32) {
+		do_div(size, divisor[units]);
+		i++;
+	}
+
+	/* now perform the actual multiplication keeping i as the sum of the
+	 * two logarithms */
+	size *= blk_size;
+
+ out:
+	string_reduce(size, i, units, buf, len, false);
+}
 
 static bool unescape_space(char **src, char **dst)
 {

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[James Bottomley]

On Mon, 2016-01-25 at 18:02 +0000, Elliott, Robert (Persistent Memory)
wrote:
> 
> 
> > -----Original Message-----
> > From: James Bottomley [mailto:James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org
> > ]
> > Sent: Saturday, January 23, 2016 10:44 AM
> > To: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>; Matt
> > Fleming
> > <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>; Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>;
> > Ingo
> > Molnar <mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>; H . Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>; linux-
> > efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Rasmus Villemoes <linux-qQsb+v5E8BnlAoU/VqSP6n9LOBIZ5rWg@public.gmane.org>;
> > Andrew
> > Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>; linux-kernel @ vger . kernel .
> > org
> > <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> > Cc: Elliott, Robert (Persistent Memory) <elliott-ZPxbGqLxI0U@public.gmane.org>
> > Subject: Re: [PATCH v3 3/4] x86/efi: print size in binary units in
> > efi_print_memmap
> > 
> > On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote:
> > > From: Robert Elliott <elliott-ZPxbGqLxI0U@public.gmane.org>
> > > 
> > > Print the size in the best-fit B, KiB, MiB, etc. units rather
> > > than
> > > always MiB. This avoids rounding, which can be misleading.
> > > 
> 
> ...
> > 
> > What if size is zero, which might happen on a UEFI screw up?  
> 
> > Also it gives really odd results for non power of two memory sizes.
> > 16384MB prints as 16GiB but 16385 prints as 16385MiB.
> > If the goal is to have a clean interface reporting only the first
> > four
> > significant figures and a size exponent, then a helper would be
> > much
> > better than trying to open code this ad hoc.
> 
> An impetus for the patch was to stop rounding the sub-MiB values,
> which is misleading and can hide bugs.  For my systems, the
> minimum size of a range happens to be 4 KiB, so I wanted at least
> that resolution. However, I don't want to print everything as KiB,
> because that makes big sizes less clear.
> 
> Example - old output:
> efi: mem00: [Conventional Memory...] range=[0x0000000000000000
> -0x0000000000001000) (0MB)
> efi: mem01: [Loader Data        ...] range=[0x0000000000001000
> -0x0000000000002000) (0MB)
> efi: mem02: [Conventional Memory...] range=[0x0000000000002000
> -0x0000000000093000) (0MB)
> efi: mem03: [Reserved           ...] range=[0x0000000000093000
> -0x0000000000094000) (0MB)
> 
> Proposed output:
> efi: mem00: [Conventional Memory...] range=[0x0000000000000000
> -0x0000000000092fff] (588 KiB @ 0 B)
> efi: mem01: [Reserved           ...] range=[0x0000000000093000
> -0x0000000000093fff] (4 KiB @ 588 KiB)
> efi: mem02: [Conventional Memory...] range=[0x0000000000094000
> -0x000000000009ffff] (48 KiB @ 592 KiB)
> efi: mem03: [Loader Data        ...] range=[0x0000000000100000
> -0x00000000013e8fff] (19364 KiB @ 1 MiB)
> (notes:
>  - from a different system
>  - including both base and size
>  - Matt didn't like printing the base so that's been removed)
> 
> With persistent memory (NVDIMMs) bringing storage device capacities
> into the memory subsystem, MiB is too small.  Seeing a 1 TiB NVDIMM
> as 1 TiB is a lot clearer than having to recognize 1048576 MiB as
> the same value (especially since these power-of-two quantities
> don't just chop off zeros on the right).
> 
> Examples:
> efi: mem50: [Runtime Data       ...] range=[0x00000000784ff000
> -0x00000000788fefff] (4 MiB @ 1971196 KiB)
> efi: mem56: [Conventional Memory...] range=[0x0000000100000000
> -0x000000087fffffff] (30 GiB @ 4 GiB)
> efi: mem58: [Memory Mapped I/O  ...] range=[0x0000000080000000
> -0x000000008fffffff] (256 MiB @ 2 GiB)
> efi: mem60: [Persistent Memory  ...] range=[0x0000001480000000
> -0x0000001a7fffffff] (24 GiB @ 82 GiB)

OK, this is getting a bit out of hand: I didn't say your aim was bad
... I think it's a reasonable desire; I said the proposed
implementation was bad.  Using ffs leads to precision runaway and
exporting an array from string_helpers.c is simply the wrong way to do
it.

Since we've now spent more time arguing about this than it would take
to do a correct patch, this is what I was thinking.  It extracts the
precision reduction core from string_helpers.c and exposes it to all
users who want to convert to units.  I added a nozeros option becuase I
think you want it to print 1 GiB rather than 1.00 GiB for exact powers
of two.  (OK, and I fixed a bug where it will report small amounts as
1.00 B instead of whole number of bytes).  Absent the nozero option,
you could simply have used string_get_size(), with a block size of 1.

James

---

diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index dabe643..78935fae 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -10,6 +10,8 @@ enum string_size_units {
 	STRING_UNITS_2,		/* use binary powers of 2^10 */
 };
 
+void string_get_units(u64 size, const enum string_size_units units,
+		      char *buf, int len, bool nozeros);
 void string_get_size(u64 size, u64 blk_size, enum string_size_units units,
 		     char *buf, int len);
 
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 5c88204..ab6b332 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -13,21 +13,13 @@
 #include <linux/string.h>
 #include <linux/string_helpers.h>
 
-/**
- * string_get_size - get the size in the specified units
- * @size:	The size to be converted in blocks
- * @blk_size:	Size of the block (use 1 for size in bytes)
- * @units:	units to use (powers of 1000 or 1024)
- * @buf:	buffer to format to
- * @len:	length of buffer
- *
- * This function returns a string formatted to 3 significant figures
- * giving the size in the required units.  @buf should have room for
- * 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,
-		     char *buf, int len)
+static const unsigned int divisor[] = {
+	[STRING_UNITS_10] = 1000,
+	[STRING_UNITS_2] = 1024,
+};
+
+static void string_reduce(u64 size, int log, const enum string_size_units units,
+			  char *buf, int len, bool nozeros)
 {
 	static const char *const units_10[] = {
 		"B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB"
@@ -39,52 +31,23 @@ void string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
 		[STRING_UNITS_10] = units_10,
 		[STRING_UNITS_2] = units_2,
 	};
-	static const unsigned int divisor[] = {
-		[STRING_UNITS_10] = 1000,
-		[STRING_UNITS_2] = 1024,
-	};
 	static const unsigned int rounding[] = { 500, 50, 5 };
-	int i = 0, j;
-	u32 remainder = 0, sf_cap;
+	char zeros[] = ".00";
+
+	int j;
+	u32 sf_cap, remainder = 0;
 	char tmp[8];
 	const char *unit;
 
 	tmp[0] = '\0';
 
-	if (blk_size == 0)
-		size = 0;
 	if (size == 0)
 		goto out;
 
-	/* This is Napier's algorithm.  Reduce the original block size to
-	 *
-	 * coefficient * divisor[units]^i
-	 *
-	 * we do the reduction so both coefficients are just under 32 bits so
-	 * that multiplying them together won't overflow 64 bits and we keep
-	 * as much precision as possible in the numbers.
-	 *
-	 * Note: it's safe to throw away the remainders here because all the
-	 * precision is in the coefficients.
-	 */
-	while (blk_size >> 32) {
-		do_div(blk_size, divisor[units]);
-		i++;
-	}
-
-	while (size >> 32) {
-		do_div(size, divisor[units]);
-		i++;
-	}
-
-	/* now perform the actual multiplication keeping i as the sum of the
-	 * two logarithms */
-	size *= blk_size;
-
-	/* and logarithmically reduce it until it's just under the divisor */
+	/* Logarithmically reduce it until it's just under the divisor */
 	while (size >= divisor[units]) {
 		remainder = do_div(size, divisor[units]);
-		i++;
+		log++;
 	}
 
 	/* work out in j how many digits of precision we need from the
@@ -109,21 +72,93 @@ void string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
 		size += 1;
 	}
 
-	if (j) {
+	if (j && log) {
 		snprintf(tmp, sizeof(tmp), ".%03u", remainder);
 		tmp[j+1] = '\0';
+		zeros[j+1] = '\0';
+		if (nozeros && strcmp(tmp, zeros) == 0)
+			tmp[0]='\0';
 	}
 
  out:
-	if (i >= ARRAY_SIZE(units_2))
+	if (log >= ARRAY_SIZE(units_2))
 		unit = "UNK";
 	else
-		unit = units_str[units][i];
+		unit = units_str[units][log];
 
 	snprintf(buf, len, "%u%s %s", (u32)size,
 		 tmp, unit);
 }
-EXPORT_SYMBOL(string_get_size);
+
+/**
+ * string_get_units - convert size to specified units
+ * @size:	The quantity to be converted
+ * @units:	units to use (powers of 1000 or 1024)
+ * @buf:	buffer to format to
+ * @len:	length of buffer
+ * @nozereos:	eliminate zeros after the decimal point if true
+ *
+ * This function returns a string formatted to 3 significant figures
+ * giving the size in the required units.  @buf should have room for
+ * at least 9 bytes and will always be zero terminated.
+ */
+void string_get_units(u64 size, const enum string_size_units units,
+		      char *buf, int len, bool nozeros)
+{
+	string_reduce(size, 0, units, buf, len, nozeros);
+}
+
+/**
+ * string_get_size - get the size in the specified units
+ * @size:	The size to be converted in blocks
+ * @blk_size:	Size of the block (use 1 for size in bytes)
+ * @units:	units to use (powers of 1000 or 1024)
+ * @buf:	buffer to format to
+ * @len:	length of buffer
+ *
+ * This function returns a string formatted to 3 significant figures
+ * giving the size in the required units.  @buf should have room for
+ * 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,
+		     char *buf, int len)
+{
+	int i = 0;
+
+	if (blk_size == 0)
+		size = 0;
+	if (size == 0)
+		goto out;
+
+	/* This is Napier's algorithm.  Reduce the original block size to
+	 *
+	 * coefficient * divisor[units]^i
+	 *
+	 * we do the reduction so both coefficients are just under 32 bits so
+	 * that multiplying them together won't overflow 64 bits and we keep
+	 * as much precision as possible in the numbers.
+	 *
+	 * Note: it's safe to throw away the remainders here because all the
+	 * precision is in the coefficients.
+	 */
+	while (blk_size >> 32) {
+		do_div(blk_size, divisor[units]);
+		i++;
+	}
+
+	while (size >> 32) {
+		do_div(size, divisor[units]);
+		i++;
+	}
+
+	/* now perform the actual multiplication keeping i as the sum of the
+	 * two logarithms */
+	size *= blk_size;
+
+ out:
+	string_reduce(size, i, units, buf, len, false);
+}
 
 static bool unescape_space(char **src, char **dst)
 {

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Andy Shevchenko]

On Mon, Jan 25, 2016 at 8:56 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Mon, 2016-01-25 at 18:02 +0000, Elliott, Robert (Persistent Memory)
> wrote:

>  Using ffs leads to precision runaway

How exactly?!

> and
> exporting an array from string_helpers.c is simply the wrong way to do
> it.

This part I didn't object.

> Since we've now spent more time arguing about this than it would take
> to do a correct patch, this is what I was thinking.  It extracts the
> precision reduction core from string_helpers.c and exposes it to all
> users who want to convert to units.  I added a nozeros option becuase I
> think you want it to print 1 GiB rather than 1.00 GiB for exact powers
> of two.  (OK, and I fixed a bug where it will report small amounts as
> 1.00 B instead of whole number of bytes).  Absent the nozero option,
> you could simply have used string_get_size(), with a block size of 1.

It's good you are doing this better, but I still vote for __ffs64(),
since it would be faster on binary units.

Also, in one version I tried to convert couple of other users which
are using only KM (in general whatever range it would be) units. Any
ideas how to modify to support them?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Andy Shevchenko]

On Mon, Jan 25, 2016 at 8:56 PM, James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> On Mon, 2016-01-25 at 18:02 +0000, Elliott, Robert (Persistent Memory)
> wrote:

>  Using ffs leads to precision runaway

How exactly?!

> and
> exporting an array from string_helpers.c is simply the wrong way to do
> it.

This part I didn't object.

> Since we've now spent more time arguing about this than it would take
> to do a correct patch, this is what I was thinking.  It extracts the
> precision reduction core from string_helpers.c and exposes it to all
> users who want to convert to units.  I added a nozeros option becuase I
> think you want it to print 1 GiB rather than 1.00 GiB for exact powers
> of two.  (OK, and I fixed a bug where it will report small amounts as
> 1.00 B instead of whole number of bytes).  Absent the nozero option,
> you could simply have used string_get_size(), with a block size of 1.

It's good you are doing this better, but I still vote for __ffs64(),
since it would be faster on binary units.

Also, in one version I tried to convert couple of other users which
are using only KM (in general whatever range it would be) units. Any
ideas how to modify to support them?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[James Bottomley]

On Mon, 2016-01-25 at 21:28 +0200, Andy Shevchenko wrote:
> On Mon, Jan 25, 2016 at 8:56 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Mon, 2016-01-25 at 18:02 +0000, Elliott, Robert (Persistent
> > Memory)
> > wrote:
> 
> >  Using ffs leads to precision runaway
> 
> How exactly?!

Off by one.  A size of 0xffffffffffffffff prints 18446744073709551615 B
rather than 20 GiB.

> > and
> > exporting an array from string_helpers.c is simply the wrong way to
> > do
> > it.
> 
> This part I didn't object.
> 
> > Since we've now spent more time arguing about this than it would
> > take
> > to do a correct patch, this is what I was thinking.  It extracts
> > the
> > precision reduction core from string_helpers.c and exposes it to
> > all
> > users who want to convert to units.  I added a nozeros option
> > becuase I
> > think you want it to print 1 GiB rather than 1.00 GiB for exact
> > powers
> > of two.  (OK, and I fixed a bug where it will report small amounts
> > as
> > 1.00 B instead of whole number of bytes).  Absent the nozero
> > option,
> > you could simply have used string_get_size(), with a block size of
> > 1.
> 
> It's good you are doing this better, but I still vote for __ffs64(),
> since it would be faster on binary units.

Is speed of a start of day print a particular concern?

> Also, in one version I tried to convert couple of other users which
> are using only KM (in general whatever range it would be) units. Any
> ideas how to modify to support them?

You mean units in odd increments of 6 digits (so K, M, T ...)? no.  The
logarithmic reduction is done to the base of the unit increment (1000
or 1024) so it doesn't really fit this case and it would be hard to
adjust because we don't have enough precision in the remainder. 
 However, unless there's a huge need to keep it, I'd just fit to the
closest 3 digit increment and then everything would work.

James

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[James Bottomley]

On Mon, 2016-01-25 at 21:28 +0200, Andy Shevchenko wrote:
> On Mon, Jan 25, 2016 at 8:56 PM, James Bottomley
> <James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> > On Mon, 2016-01-25 at 18:02 +0000, Elliott, Robert (Persistent
> > Memory)
> > wrote:
> 
> >  Using ffs leads to precision runaway
> 
> How exactly?!

Off by one.  A size of 0xffffffffffffffff prints 18446744073709551615 B
rather than 20 GiB.

> > and
> > exporting an array from string_helpers.c is simply the wrong way to
> > do
> > it.
> 
> This part I didn't object.
> 
> > Since we've now spent more time arguing about this than it would
> > take
> > to do a correct patch, this is what I was thinking.  It extracts
> > the
> > precision reduction core from string_helpers.c and exposes it to
> > all
> > users who want to convert to units.  I added a nozeros option
> > becuase I
> > think you want it to print 1 GiB rather than 1.00 GiB for exact
> > powers
> > of two.  (OK, and I fixed a bug where it will report small amounts
> > as
> > 1.00 B instead of whole number of bytes).  Absent the nozero
> > option,
> > you could simply have used string_get_size(), with a block size of
> > 1.
> 
> It's good you are doing this better, but I still vote for __ffs64(),
> since it would be faster on binary units.

Is speed of a start of day print a particular concern?

> Also, in one version I tried to convert couple of other users which
> are using only KM (in general whatever range it would be) units. Any
> ideas how to modify to support them?

You mean units in odd increments of 6 digits (so K, M, T ...)? no.  The
logarithmic reduction is done to the base of the unit increment (1000
or 1024) so it doesn't really fit this case and it would be hard to
adjust because we don't have enough precision in the remainder. 
 However, unless there's a huge need to keep it, I'd just fit to the
closest 3 digit increment and then everything would work.

James

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Andy Shevchenko]

On Mon, Jan 25, 2016 at 9:45 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Mon, 2016-01-25 at 21:28 +0200, Andy Shevchenko wrote:
>> On Mon, Jan 25, 2016 at 8:56 PM, James Bottomley
>> <James.Bottomley@hansenpartnership.com> wrote:
>> > On Mon, 2016-01-25 at 18:02 +0000, Elliott, Robert (Persistent
>> > Memory)
>> > wrote:
>>
>> >  Using ffs leads to precision runaway
>>
>> How exactly?!
>
> Off by one.  A size of 0xffffffffffffffff prints 18446744073709551615 B
> rather than 20 GiB.

Because it's not a 20 GiB. It's exactly 20 GiB - 1 B.

AFAIU, the intention was to show _exact_ size.

>> It's good you are doing this better, but I still vote for __ffs64(),
>> since it would be faster on binary units.
>
> Is speed of a start of day print a particular concern?

If it's cheap to do, why not to do?

>> Also, in one version I tried to convert couple of other users which
>> are using only KM (in general whatever range it would be) units. Any
>> ideas how to modify to support them?
>
> You mean units in odd increments of 6 digits (so K, M, T ...)? no.  The
> logarithmic reduction is done to the base of the unit increment (1000
> or 1024) so it doesn't really fit this case and it would be hard to
> adjust because we don't have enough precision in the remainder.
>  However, unless there's a huge need to keep it, I'd just fit to the
> closest 3 digit increment and then everything would work.

KM case:
K) if 1 MiB > value >= 0 — prints in KiB
M) if ∞ > value >= 1 MiB — prints in MiB.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Andy Shevchenko]

On Mon, Jan 25, 2016 at 9:45 PM, James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> On Mon, 2016-01-25 at 21:28 +0200, Andy Shevchenko wrote:
>> On Mon, Jan 25, 2016 at 8:56 PM, James Bottomley
>> <James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
>> > On Mon, 2016-01-25 at 18:02 +0000, Elliott, Robert (Persistent
>> > Memory)
>> > wrote:
>>
>> >  Using ffs leads to precision runaway
>>
>> How exactly?!
>
> Off by one.  A size of 0xffffffffffffffff prints 18446744073709551615 B
> rather than 20 GiB.

Because it's not a 20 GiB. It's exactly 20 GiB - 1 B.

AFAIU, the intention was to show _exact_ size.

>> It's good you are doing this better, but I still vote for __ffs64(),
>> since it would be faster on binary units.
>
> Is speed of a start of day print a particular concern?

If it's cheap to do, why not to do?

>> Also, in one version I tried to convert couple of other users which
>> are using only KM (in general whatever range it would be) units. Any
>> ideas how to modify to support them?
>
> You mean units in odd increments of 6 digits (so K, M, T ...)? no.  The
> logarithmic reduction is done to the base of the unit increment (1000
> or 1024) so it doesn't really fit this case and it would be hard to
> adjust because we don't have enough precision in the remainder.
>  However, unless there's a huge need to keep it, I'd just fit to the
> closest 3 digit increment and then everything would work.

KM case:
K) if 1 MiB > value >= 0 — prints in KiB
M) if ∞ > value >= 1 MiB — prints in MiB.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[James Bottomley]

On Mon, 2016-01-25 at 22:01 +0200, Andy Shevchenko wrote:
> On Mon, Jan 25, 2016 at 9:45 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Mon, 2016-01-25 at 21:28 +0200, Andy Shevchenko wrote:
> > > On Mon, Jan 25, 2016 at 8:56 PM, James Bottomley
> > > <James.Bottomley@hansenpartnership.com> wrote:
> > > > On Mon, 2016-01-25 at 18:02 +0000, Elliott, Robert (Persistent
> > > > Memory)
> > > > wrote:
> > > 
> > > >  Using ffs leads to precision runaway
> > > 
> > > How exactly?!
> > 
> > Off by one.  A size of 0xffffffffffffffff prints
> > 18446744073709551615 B
> > rather than 20 GiB.
> 
> Because it's not a 20 GiB. It's exactly 20 GiB - 1 B.
> 
> AFAIU, the intention was to show _exact_ size.

I think that's a bad idea:  The range shows you the exact stuff in hex,
so all the information is present if you want precision.  What's
printed in brackets is for humans to read.  I'd have to reach for a
calculator to work out that  18446744073709551615 is actually around 20
GiB.

James

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[James Bottomley]

On Mon, 2016-01-25 at 22:01 +0200, Andy Shevchenko wrote:
> On Mon, Jan 25, 2016 at 9:45 PM, James Bottomley
> <James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> > On Mon, 2016-01-25 at 21:28 +0200, Andy Shevchenko wrote:
> > > On Mon, Jan 25, 2016 at 8:56 PM, James Bottomley
> > > <James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> > > > On Mon, 2016-01-25 at 18:02 +0000, Elliott, Robert (Persistent
> > > > Memory)
> > > > wrote:
> > > 
> > > >  Using ffs leads to precision runaway
> > > 
> > > How exactly?!
> > 
> > Off by one.  A size of 0xffffffffffffffff prints
> > 18446744073709551615 B
> > rather than 20 GiB.
> 
> Because it's not a 20 GiB. It's exactly 20 GiB - 1 B.
> 
> AFAIU, the intention was to show _exact_ size.

I think that's a bad idea:  The range shows you the exact stuff in hex,
so all the information is present if you want precision.  What's
printed in brackets is for humans to read.  I'd have to reach for a
calculator to work out that  18446744073709551615 is actually around 20
GiB.

James

RE: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Elliott, Robert (Persistent Memory)]

---
Robert Elliott, HPE Persistent Memory


> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Monday, January 25, 2016 2:01 PM
> To: James Bottomley <James.Bottomley@hansenpartnership.com>
> Cc: Elliott, Robert (Persistent Memory) <elliott@hpe.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Matt Fleming
> <matt@codeblueprint.co.uk>; Thomas Gleixner <tglx@linutronix.de>; Ingo
> Molnar <mingo@redhat.com>; H . Peter Anvin <hpa@zytor.com>; linux-
> efi@vger.kernel.org; Rasmus Villemoes <linux@rasmusvillemoes.dk>; Andrew
> Morton <akpm@linux-foundation.org>; linux-kernel @ vger . kernel . org
> <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH v3 3/4] x86/efi: print size in binary units in
> efi_print_memmap
> 
> On Mon, Jan 25, 2016 at 9:45 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Mon, 2016-01-25 at 21:28 +0200, Andy Shevchenko wrote:
> >> On Mon, Jan 25, 2016 at 8:56 PM, James Bottomley
> >> <James.Bottomley@hansenpartnership.com> wrote:
> >> > On Mon, 2016-01-25 at 18:02 +0000, Elliott, Robert (Persistent
> >> > Memory)
> >> > wrote:
> >>
> >> >  Using ffs leads to precision runaway
> >>
> >> How exactly?!
> >
> > Off by one.  A size of 0xffffffffffffffff prints 18446744073709551615 B
> > rather than 20 GiB.
> 
> Because it's not a 20 GiB. It's exactly 20 GiB - 1 B.
> 
> AFAIU, the intention was to show _exact_ size.

For the UEFI memory map, that was indeed my intention.  I
don't want it silently round to "20 GiB".  Even rounding
to "19.999 GiB" is imprecise.

Another option could be to use a "~" prefix for imperfect
values, like "~20 GiB".  That would serve as a warning
that something's not quite right.

RE: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Elliott, Robert (Persistent Memory)]

---
Robert Elliott, HPE Persistent Memory


> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Monday, January 25, 2016 2:01 PM
> To: James Bottomley <James.Bottomley@hansenpartnership.com>
> Cc: Elliott, Robert (Persistent Memory) <elliott@hpe.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Matt Fleming
> <matt@codeblueprint.co.uk>; Thomas Gleixner <tglx@linutronix.de>; Ingo
> Molnar <mingo@redhat.com>; H . Peter Anvin <hpa@zytor.com>; linux-
> efi@vger.kernel.org; Rasmus Villemoes <linux@rasmusvillemoes.dk>; Andrew
> Morton <akpm@linux-foundation.org>; linux-kernel @ vger . kernel . org
> <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH v3 3/4] x86/efi: print size in binary units in
> efi_print_memmap
> 
> On Mon, Jan 25, 2016 at 9:45 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Mon, 2016-01-25 at 21:28 +0200, Andy Shevchenko wrote:
> >> On Mon, Jan 25, 2016 at 8:56 PM, James Bottomley
> >> <James.Bottomley@hansenpartnership.com> wrote:
> >> > On Mon, 2016-01-25 at 18:02 +0000, Elliott, Robert (Persistent
> >> > Memory)
> >> > wrote:
> >>
> >> >  Using ffs leads to precision runaway
> >>
> >> How exactly?!
> >
> > Off by one.  A size of 0xffffffffffffffff prints 18446744073709551615 B
> > rather than 20 GiB.
> 
> Because it's not a 20 GiB. It's exactly 20 GiB - 1 B.
> 
> AFAIU, the intention was to show _exact_ size.

For the UEFI memory map, that was indeed my intention.  I
don't want it silently round to "20 GiB".  Even rounding
to "19.999 GiB" is imprecise.

Another option could be to use a "~" prefix for imperfect
values, like "~20 GiB".  That would serve as a warning
that something's not quite right.

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[James Bottomley]

On Mon, 2016-01-25 at 22:01 +0200, Andy Shevchenko wrote:
> On Mon, Jan 25, 2016 at 9:45 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Mon, 2016-01-25 at 21:28 +0200, Andy Shevchenko wrote:
> > > Also, in one version I tried to convert couple of other users
> > > which
> > > are using only KM (in general whatever range it would be) units.
> > > Any
> > > ideas how to modify to support them?
> > 
> > You mean units in odd increments of 6 digits (so K, M, T ...)? no. 
> >  The logarithmic reduction is done to the base of the unit 
> > increment (1000 or 1024) so it doesn't really fit this case and it 
> > would be hard to adjust because we don't have enough precision in 
> > the remainder.  However, unless there's a huge need to keep it, I'd 
> > just fit to the closest 3 digit increment and then everything would
> > work.
> 
> KM case:
> K) if 1 MiB > value >= 0 — prints in KiB
> M) if ∞ > value >= 1 MiB — prints in MiB.

Actually there is a way to do this: add a fixed precision argument that
would stop the logarithmic reduction when the desired precision were
reached.  You'd still have to do the precision discrimination in the
call, so something like

num > 1Mib ? call for precision of 6 : call for precision of 3

James

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[James Bottomley]

On Mon, 2016-01-25 at 22:01 +0200, Andy Shevchenko wrote:
> On Mon, Jan 25, 2016 at 9:45 PM, James Bottomley
> <James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> > On Mon, 2016-01-25 at 21:28 +0200, Andy Shevchenko wrote:
> > > Also, in one version I tried to convert couple of other users
> > > which
> > > are using only KM (in general whatever range it would be) units.
> > > Any
> > > ideas how to modify to support them?
> > 
> > You mean units in odd increments of 6 digits (so K, M, T ...)? no. 
> >  The logarithmic reduction is done to the base of the unit 
> > increment (1000 or 1024) so it doesn't really fit this case and it 
> > would be hard to adjust because we don't have enough precision in 
> > the remainder.  However, unless there's a huge need to keep it, I'd 
> > just fit to the closest 3 digit increment and then everything would
> > work.
> 
> KM case:
> K) if 1 MiB > value >= 0 — prints in KiB
> M) if ∞ > value >= 1 MiB — prints in MiB.

Actually there is a way to do this: add a fixed precision argument that
would stop the logarithmic reduction when the desired precision were
reached.  You'd still have to do the precision discrimination in the
call, so something like

num > 1Mib ? call for precision of 6 : call for precision of 3

James

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Matt Fleming]

On Mon, 25 Jan, at 08:37:58PM, Elliott, Robert (Persistent Memory) wrote:
> 
> For the UEFI memory map, that was indeed my intention.  I
> don't want it silently round to "20 GiB".  Even rounding
> to "19.999 GiB" is imprecise.

OK, let's just go with your original patch Robert (minus the @ addr
bit) since it's pretty small and does what we want for this specific
case.

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Andy Shevchenko]

On Tue, 2016-01-26 at 11:50 +0000, Matt Fleming wrote:
> On Mon, 25 Jan, at 08:37:58PM, Elliott, Robert (Persistent Memory)
> wrote:
> > 
> > For the UEFI memory map, that was indeed my intention.  I
> > don't want it silently round to "20 GiB".  Even rounding
> > to "19.999 GiB" is imprecise.
> 
> OK, let's just go with your original patch Robert (minus the @ addr
> bit) since it's pretty small and does what we want for this specific
> case.

However I am against this, but seems reviewers do not leave a chance to
us, I would propose to copy-and-paste table of binary prefixes and use
__ffs64().

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

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Andy Shevchenko]

On Tue, 2016-01-26 at 11:50 +0000, Matt Fleming wrote:
> On Mon, 25 Jan, at 08:37:58PM, Elliott, Robert (Persistent Memory)
> wrote:
> > 
> > For the UEFI memory map, that was indeed my intention.  I
> > don't want it silently round to "20 GiB".  Even rounding
> > to "19.999 GiB" is imprecise.
> 
> OK, let's just go with your original patch Robert (minus the @ addr
> bit) since it's pretty small and does what we want for this specific
> case.

However I am against this, but seems reviewers do not leave a chance to
us, I would propose to copy-and-paste table of binary prefixes and use
__ffs64().

-- 
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Matt Fleming]

On Tue, 26 Jan, at 01:59:26PM, Andy Shevchenko wrote:
> On Tue, 2016-01-26 at 11:50 +0000, Matt Fleming wrote:
> > On Mon, 25 Jan, at 08:37:58PM, Elliott, Robert (Persistent Memory)
> > wrote:
> > > 
> > > For the UEFI memory map, that was indeed my intention.  I
> > > don't want it silently round to "20 GiB".  Even rounding
> > > to "19.999 GiB" is imprecise.
> > 
> > OK, let's just go with your original patch Robert (minus the @ addr
> > bit) since it's pretty small and does what we want for this specific
> > case.
> 
> However I am against this, but seems reviewers do not leave a chance to
> us, I would propose to copy-and-paste table of binary prefixes and use
> __ffs64().

Is there a benefit to this approach other than __ffs64() being faster?
It is a neglibible performance gain anyway because this is hidden
behind efi=debug, and so by definition you're not looking for ultra
performance.

Which makes picking between __ffs64() and non-__ffs64() a wash if
we're not going to be reusing existing code.

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Matt Fleming]

On Tue, 26 Jan, at 01:59:26PM, Andy Shevchenko wrote:
> On Tue, 2016-01-26 at 11:50 +0000, Matt Fleming wrote:
> > On Mon, 25 Jan, at 08:37:58PM, Elliott, Robert (Persistent Memory)
> > wrote:
> > > 
> > > For the UEFI memory map, that was indeed my intention.  I
> > > don't want it silently round to "20 GiB".  Even rounding
> > > to "19.999 GiB" is imprecise.
> > 
> > OK, let's just go with your original patch Robert (minus the @ addr
> > bit) since it's pretty small and does what we want for this specific
> > case.
> 
> However I am against this, but seems reviewers do not leave a chance to
> us, I would propose to copy-and-paste table of binary prefixes and use
> __ffs64().

Is there a benefit to this approach other than __ffs64() being faster?
It is a neglibible performance gain anyway because this is hidden
behind efi=debug, and so by definition you're not looking for ultra
performance.

Which makes picking between __ffs64() and non-__ffs64() a wash if
we're not going to be reusing existing code.

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Andy Shevchenko]

On Thu, 2016-01-28 at 09:29 +0000, Matt Fleming wrote:
> On Tue, 26 Jan, at 01:59:26PM, Andy Shevchenko wrote:
> > On Tue, 2016-01-26 at 11:50 +0000, Matt Fleming wrote:
> > > On Mon, 25 Jan, at 08:37:58PM, Elliott, Robert (Persistent
> > > Memory)
> > > wrote:
> > > > 
> > > > For the UEFI memory map, that was indeed my intention.  I
> > > > don't want it silently round to "20 GiB".  Even rounding
> > > > to "19.999 GiB" is imprecise.
> > > 
> > > OK, let's just go with your original patch Robert (minus the @
> > > addr
> > > bit) since it's pretty small and does what we want for this
> > > specific
> > > case.
> > 
> > However I am against this, but seems reviewers do not leave a
> > chance to
> > us, I would propose to copy-and-paste table of binary prefixes and
> > use
> > __ffs64().
> 
> Is there a benefit to this approach other than __ffs64() being
> faster?
> It is a neglibible performance gain anyway because this is hidden
> behind efi=debug, and so by definition you're not looking for ultra
> performance.
> 
> Which makes picking between __ffs64() and non-__ffs64() a wash if
> we're not going to be reusing existing code.

For me it looks cleaner, smaller, thus better to maintenance, and
faster. So, my variant below, though it's up to you what to choose.

>From e16660cf999aec754a0f7a95604b8f9e86c40d55 Mon Sep 17 00:00:00 2001
From: Robert Elliott <elliott@hpe.com>
Date: Tue, 12 Jan 2016 15:13:06 +0200
Subject: [PATCH 1/1] x86/efi: print size in binary units in
efi_print_memmap

Print the size in the best-fit B, KiB, MiB, etc. units rather than
always MiB. This avoids rounding, which can be misleading.

Use proper IEC binary units (KiB, MiB, etc.) rather than misuse SI
decimal units (KB, MB, etc.).

old:
    efi: mem61: [Persistent
Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-
0x0000000c7fffffff) (16384MB)

new:
    efi: mem61: [Persistent
Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-
0x0000000c7fffffff] (16 GiB)

Signed-off-by: Robert Elliott <elliott@hpe.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/platform/efi/efi.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index e0846b5..63fb5cf 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -35,6 +35,7 @@
 #include <linux/efi.h>
 #include <linux/efi-bgrt.h>
 #include <linux/export.h>
+#include <linux/bitops.h>
 #include <linux/bootmem.h>
 #include <linux/slab.h>
 #include <linux/memblock.h>
@@ -117,6 +118,17 @@ void efi_get_time(struct timespec *now)
 	now->tv_nsec = 0;
 }
 
+static char * __init efi_size_format(char *buf, size_t size, u64
bytes)
+{
+	static const char *const units_2[] = {
+		"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB"
+	};
+	unsigned long i = bytes ? __ffs64(bytes) / 10 : 0;
+
+	snprintf(buf, size, "%llu %s", bytes >> (i * 10), units_2[i]);
+	return buf;
+}
+
 void __init efi_find_mirror(void)
 {
 	void *p;
@@ -225,21 +237,20 @@ int __init efi_memblock_x86_reserve_range(void)
 void __init efi_print_memmap(void)
 {
 #ifdef EFI_DEBUG
-	efi_memory_desc_t *md;
 	void *p;
 	int i;
 
 	for (p = memmap.map, i = 0;
 	     p < memmap.map_end;
 	     p += memmap.desc_size, i++) {
-		char buf[64];
+		efi_memory_desc_t *md = p;
+		u64 size = md->num_pages << EFI_PAGE_SHIFT;
+		char buf[64], buf3[32];
 
-		md = p;
-		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx]
(%lluMB)\n",
+		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx]
(%s)\n",
 			i, efi_md_typeattr_format(buf, sizeof(buf),
md),
-			md->phys_addr,
-			md->phys_addr + (md->num_pages <<
EFI_PAGE_SHIFT) - 1,
-			(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
+			md->phys_addr, md->phys_addr + size - 1,
+			efi_size_format(buf3, sizeof(buf3), size));
 	}
 #endif  /*  EFI_DEBUG  */
 }
-- 
2.7.0.rc3


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

Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap

[Andy Shevchenko]

On Thu, 2016-01-28 at 09:29 +0000, Matt Fleming wrote:
> On Tue, 26 Jan, at 01:59:26PM, Andy Shevchenko wrote:
> > On Tue, 2016-01-26 at 11:50 +0000, Matt Fleming wrote:
> > > On Mon, 25 Jan, at 08:37:58PM, Elliott, Robert (Persistent
> > > Memory)
> > > wrote:
> > > > 
> > > > For the UEFI memory map, that was indeed my intention.  I
> > > > don't want it silently round to "20 GiB".  Even rounding
> > > > to "19.999 GiB" is imprecise.
> > > 
> > > OK, let's just go with your original patch Robert (minus the @
> > > addr
> > > bit) since it's pretty small and does what we want for this
> > > specific
> > > case.
> > 
> > However I am against this, but seems reviewers do not leave a
> > chance to
> > us, I would propose to copy-and-paste table of binary prefixes and
> > use
> > __ffs64().
> 
> Is there a benefit to this approach other than __ffs64() being
> faster?
> It is a neglibible performance gain anyway because this is hidden
> behind efi=debug, and so by definition you're not looking for ultra
> performance.
> 
> Which makes picking between __ffs64() and non-__ffs64() a wash if
> we're not going to be reusing existing code.

For me it looks cleaner, smaller, thus better to maintenance, and
faster. So, my variant below, though it's up to you what to choose.

From e16660cf999aec754a0f7a95604b8f9e86c40d55 Mon Sep 17 00:00:00 2001
From: Robert Elliott <elliott@hpe.com>
Date: Tue, 12 Jan 2016 15:13:06 +0200
Subject: [PATCH 1/1] x86/efi: print size in binary units in
efi_print_memmap

Print the size in the best-fit B, KiB, MiB, etc. units rather than
always MiB. This avoids rounding, which can be misleading.

Use proper IEC binary units (KiB, MiB, etc.) rather than misuse SI
decimal units (KB, MB, etc.).

old:
    efi: mem61: [Persistent
Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-
0x0000000c7fffffff) (16384MB)

new:
    efi: mem61: [Persistent
Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-
0x0000000c7fffffff] (16 GiB)

Signed-off-by: Robert Elliott <elliott@hpe.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/platform/efi/efi.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index e0846b5..63fb5cf 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -35,6 +35,7 @@
 #include <linux/efi.h>
 #include <linux/efi-bgrt.h>
 #include <linux/export.h>
+#include <linux/bitops.h>
 #include <linux/bootmem.h>
 #include <linux/slab.h>
 #include <linux/memblock.h>
@@ -117,6 +118,17 @@ void efi_get_time(struct timespec *now)
 	now->tv_nsec = 0;
 }
 
+static char * __init efi_size_format(char *buf, size_t size, u64
bytes)
+{
+	static const char *const units_2[] = {
+		"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB"
+	};
+	unsigned long i = bytes ? __ffs64(bytes) / 10 : 0;
+
+	snprintf(buf, size, "%llu %s", bytes >> (i * 10), units_2[i]);
+	return buf;
+}
+
 void __init efi_find_mirror(void)
 {
 	void *p;
@@ -225,21 +237,20 @@ int __init efi_memblock_x86_reserve_range(void)
 void __init efi_print_memmap(void)
 {
 #ifdef EFI_DEBUG
-	efi_memory_desc_t *md;
 	void *p;
 	int i;
 
 	for (p = memmap.map, i = 0;
 	     p < memmap.map_end;
 	     p += memmap.desc_size, i++) {
-		char buf[64];
+		efi_memory_desc_t *md = p;
+		u64 size = md->num_pages << EFI_PAGE_SHIFT;
+		char buf[64], buf3[32];
 
-		md = p;
-		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx]
(%lluMB)\n",
+		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx]
(%s)\n",
 			i, efi_md_typeattr_format(buf, sizeof(buf),
md),
-			md->phys_addr,
-			md->phys_addr + (md->num_pages <<
EFI_PAGE_SHIFT) - 1,
-			(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
+			md->phys_addr, md->phys_addr + size - 1,
+			efi_size_format(buf3, sizeof(buf3), size));
 	}
 #endif  /*  EFI_DEBUG  */
 }
-- 
2.7.0.rc3


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