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


Groups > linux.kernel > #1323561 > unrolled thread

[PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

Started byMatt Fleming <matt@codeblueprint.co.uk>
First post2016-02-01 23:10 +0100
Last post2016-02-09 14:20 +0100
Articles 9 — 5 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.


Contents

  [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-01 23:10 +0100
    Re: [PATCH 14/14] x86/efi: Print size in binary units in  efi_print_memmap Laszlo Ersek <lersek@redhat.com> - 2016-02-02 10:30 +0100
      Re: [PATCH 14/14] x86/efi: Print size in binary units in  efi_print_memmap Ingo Molnar <mingo@kernel.org> - 2016-02-03 11:50 +0100
        Re: [PATCH 14/14] x86/efi: Print size in binary units in  efi_print_memmap Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-03 12:30 +0100
          Re: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-02-03 13:40 +0100
          RE: [PATCH 14/14] x86/efi: Print size in binary units in  efi_print_memmap "Elliott, Robert (Persistent Memory)" <elliott@hpe.com> - 2016-02-03 16:30 +0100
            Re: [PATCH 14/14] x86/efi: Print size in binary units in  efi_print_memmap Ingo Molnar <mingo@kernel.org> - 2016-02-09 13:30 +0100
              Re: [PATCH 14/14] x86/efi: Print size in binary units in  efi_print_memmap Laszlo Ersek <lersek@redhat.com> - 2016-02-09 14:00 +0100
                Re: [PATCH 14/14] x86/efi: Print size in binary units in  efi_print_memmap Ingo Molnar <mingo@kernel.org> - 2016-02-09 14:20 +0100

#1323561 — [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

FromMatt Fleming <matt@codeblueprint.co.uk>
Date2016-02-01 23:10 +0100
Subject[PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap
Message-ID<qXuPy-WW-27@gated-at.bofh.it>
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>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Taku Izumi <izumi.taku@jp.fujitsu.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 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 e80826e6f3a9..2c457c5e8203 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 << PAGE_SHIFT;
+		char buf[64], buf2[64];
 
-		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(buf2, sizeof(buf2), size));
 	}
 #endif  /*  EFI_DEBUG  */
 }
-- 
2.6.2

[toc] | [next] | [standalone]


#1323906 — Re: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

FromLaszlo Ersek <lersek@redhat.com>
Date2016-02-02 10:30 +0100
SubjectRe: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap
Message-ID<qXFrA-sR-11@gated-at.bofh.it>
In reply to#1323561
I'll take being CC'd as "please offer an opinion", so I'll offer one. :)

On 02/01/16 23:07, Matt Fleming 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>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Taku Izumi <izumi.taku@jp.fujitsu.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
> ---
>  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 e80826e6f3a9..2c457c5e8203 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"
> +	};

Blech. Blech blech blech. As far as I'm concerned, "IEC binary units"
rewrite history. I propose to just say "KB" & friends.

Not sure if I should refer kernel list subscribers to GNU utility
manuals :), but "dd" gets it right:

       N  and  BYTES  may  be followed by the following multiplicative
       suffixes: c =1, w =2, b =512, kB =1000, K =1024, MB =1000*1000,
       M  =1024*1024, xM =M GB =1000*1000*1000, G =1024*1024*1024, and
       so on for T, P, E, Z, Y.

Anyway, feel free to ignore this.

> +	unsigned long i = bytes ? __ffs64(bytes) / 10 : 0;
> +
> +	snprintf(buf, size, "%llu %s", bytes >> (i * 10), units_2[i]);
> +	return buf;
> +}

The calculation seems correct, and I agree "bytes" should have type
"u64" -- this makes it clearer why we don't have to climb higher than
offset 6 (count 7) in "units".

However, since we're printing the result of the right shift with the
%llu conversion specifier, I believe I'd appreciate either an explicit
((unsigned long long)bytes) >> ... cast there, or a macro for the
conversion specifier. (In userland I'd write "%"PRIu64, but I don't know
if the kernel has anything like that.)

> +
>  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 << PAGE_SHIFT;
> +		char buf[64], buf2[64];
>  
> -		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(buf2, sizeof(buf2), size));
>  	}
>  #endif  /*  EFI_DEBUG  */
>  }
> 

Hm, apparently %llx is used to print u64 here as well. Did I write this
code? :) Is %ll used interchangeably with 64-bits?

Also, I notice there's room for unification with ia64 code. In
"arch/ia64/kernel/efi.c", the efi_init() function open-codes a similar
suffix conversion -- notice it says KB and friends! :)

So maybe the efi_size_format() helper could go into
"drivers/firmware/efi/efi.c", near its friend efi_md_typeattr_format().

Furthermore, the ia64 source has a macro efi_md_size(). Perhaps it can
be moved to a common header and used here as well. Not too important.

... Ultimately, my only semi-important point here is that
efi_size_format() should go into common EFI driver code, and be
(optionally) used on ia64 as well.

Thanks
Laszlo

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


#1325187 — Re: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

FromIngo Molnar <mingo@kernel.org>
Date2016-02-03 11:50 +0100
SubjectRe: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap
Message-ID<qY3ay-Nu-23@gated-at.bofh.it>
In reply to#1323906
* Laszlo Ersek <lersek@redhat.com> wrote:

> I'll take being CC'd as "please offer an opinion", so I'll offer one. :)
> 
> On 02/01/16 23:07, Matt Fleming 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>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Taku Izumi <izumi.taku@jp.fujitsu.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
> > ---
> >  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 e80826e6f3a9..2c457c5e8203 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"
> > +	};
> 
> Blech. Blech blech blech. As far as I'm concerned, "IEC binary units"
> rewrite history. I propose to just say "KB" & friends.

So I kind of agree. Memory is almost never measured in marketing bytes, we should 
simply output KB/MB/GB/TB/PB/EB like the rest of the memory management code does 
and ignore all the 'i' silliness that infests storage sizes ...

Thanks,

	Ingo

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


#1325226 — Re: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

FromMatt Fleming <matt@codeblueprint.co.uk>
Date2016-02-03 12:30 +0100
SubjectRe: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap
Message-ID<qY3Nf-1gx-9@gated-at.bofh.it>
In reply to#1325187
On Wed, 03 Feb, at 11:40:45AM, Ingo Molnar wrote:
> 
> So I kind of agree. Memory is almost never measured in marketing bytes, we should 
> simply output KB/MB/GB/TB/PB/EB like the rest of the memory management code does 
> and ignore all the 'i' silliness that infests storage sizes ...

Thanks guys.

OK, this patch has caused enough headaches. Let's drop it from this
series.

Robert, Andy, feel free to resubmit it after you've addressed
everyone's concerns and we can discuss it in isolation.

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


#1325334

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2016-02-03 13:40 +0100
Message-ID<qY4SZ-1Xc-5@gated-at.bofh.it>
In reply to#1325226
On Wed, Feb 3, 2016 at 1:28 PM, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Wed, 03 Feb, at 11:40:45AM, Ingo Molnar wrote:
>>
>> So I kind of agree. Memory is almost never measured in marketing bytes, we should
>> simply output KB/MB/GB/TB/PB/EB like the rest of the memory management code does
>> and ignore all the 'i' silliness that infests storage sizes ...
>
> Thanks guys.
>
> OK, this patch has caused enough headaches. Let's drop it from this
> series.
>
> Robert, Andy, feel free to resubmit it after you've addressed
> everyone's concerns and we can discuss it in isolation.

Personally I gave up to promote standards. I have started with KB, MB,
etc, but I like that eventually we have a distinct standard for binary
vs. decimal units.

-- 
With Best Regards,
Andy Shevchenko

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


#1325534 — RE: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

From"Elliott, Robert (Persistent Memory)" <elliott@hpe.com>
Date2016-02-03 16:30 +0100
SubjectRE: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap
Message-ID<qY7xw-3G4-3@gated-at.bofh.it>
In reply to#1325226
> -----Original Message-----
> From: Matt Fleming [mailto:matt@codeblueprint.co.uk]
> Sent: Wednesday, February 3, 2016 5:28 AM
> To: Ingo Molnar <mingo@kernel.org>
> Cc: Laszlo Ersek <lersek@redhat.com>; H . Peter Anvin <hpa@zytor.com>;
> Thomas Gleixner <tglx@linutronix.de>; linux-efi@vger.kernel.org; linux-
> kernel@vger.kernel.org; Elliott, Robert (Persistent Memory)
> <elliott@hpe.com>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>;
> Ard Biesheuvel <ard.biesheuvel@linaro.org>; Taku Izumi
> <izumi.taku@jp.fujitsu.com>; Linus Torvalds <torvalds@linux-
> foundation.org>; Andrew Morton <akpm@linux-foundation.org>
> Subject: Re: [PATCH 14/14] x86/efi: Print size in binary units in
> efi_print_memmap
...
> OK, this patch has caused enough headaches. Let's drop it from this
> series.
> 
> Robert, Andy, feel free to resubmit it after you've addressed
> everyone's concerns and we can discuss it in isolation.

We could just delete the size print altogether - better to print
nothing than a silently rounded number.  The end address already
communicates the size - it's just not as readable.  

The e820 table prints don't bother with a size print. 

That would also shorten these extremely wide prints to 116
characters (131 if printk time is enabled).

[    0.000000] BIOS-e820: [mem 0x0000001880000000-0x000000207fffffff] reserved
vs.
[    0.000000] efi: mem62: [Reserved           |   |  |NV|  |  |  |  |   |WB|WT|WC|UC] range=[0x0000001880000000-0x000000207fffffff] (32 GiB)

---
Robert Elliott, HPE Persistent Memory

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


#1330191 — Re: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

FromIngo Molnar <mingo@kernel.org>
Date2016-02-09 13:30 +0100
SubjectRe: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap
Message-ID<r0fAE-3Wc-55@gated-at.bofh.it>
In reply to#1325534
* Elliott, Robert (Persistent Memory) <elliott@hpe.com> wrote:

> 
> > -----Original Message-----
> > From: Matt Fleming [mailto:matt@codeblueprint.co.uk]
> > Sent: Wednesday, February 3, 2016 5:28 AM
> > To: Ingo Molnar <mingo@kernel.org>
> > Cc: Laszlo Ersek <lersek@redhat.com>; H . Peter Anvin <hpa@zytor.com>;
> > Thomas Gleixner <tglx@linutronix.de>; linux-efi@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Elliott, Robert (Persistent Memory)
> > <elliott@hpe.com>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>;
> > Ard Biesheuvel <ard.biesheuvel@linaro.org>; Taku Izumi
> > <izumi.taku@jp.fujitsu.com>; Linus Torvalds <torvalds@linux-
> > foundation.org>; Andrew Morton <akpm@linux-foundation.org>
> > Subject: Re: [PATCH 14/14] x86/efi: Print size in binary units in
> > efi_print_memmap
> ...
> > OK, this patch has caused enough headaches. Let's drop it from this
> > series.
> > 
> > Robert, Andy, feel free to resubmit it after you've addressed
> > everyone's concerns and we can discuss it in isolation.
> 
> We could just delete the size print altogether - better to print
> nothing than a silently rounded number.  The end address already
> communicates the size - it's just not as readable.  
> 
> The e820 table prints don't bother with a size print. 
> 
> That would also shorten these extremely wide prints to 116
> characters (131 if printk time is enabled).
> 
> [    0.000000] BIOS-e820: [mem 0x0000001880000000-0x000000207fffffff] reserved
> vs.
> [    0.000000] efi: mem62: [Reserved           |   |  |NV|  |  |  |  |   |WB|WT|WC|UC] range=[0x0000001880000000-0x000000207fffffff] (32 GiB)

So I find the latter a lot more readable - my terminals are wide enough ;-)

Humans are also rather bad at parsing 64-bit hexa address ranges at a glance, so 
the size display is very useful.

But the flags portion should be shortened via appropriately chosen 
single-character abbreviations for the flags. Anyone deeply intimate with the code 
will recognize the flags - others won't care one way or another.

plus there's no need to write out 'range='.
  
... and please keep the size and just use GB/TB for chrissake.

I.e. something like this would work for me:

> [    0.000000] efi: mem62: 0x0000001880000000-0x000000207fffffff (  32 GB) .N....BTCU "Reserved"

Thanks,

	Ingo

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


#1330215 — Re: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

FromLaszlo Ersek <lersek@redhat.com>
Date2016-02-09 14:00 +0100
SubjectRe: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap
Message-ID<r0g3F-48r-15@gated-at.bofh.it>
In reply to#1330191
On 02/09/16 13:20, Ingo Molnar wrote:
> 
> * Elliott, Robert (Persistent Memory) <elliott@hpe.com> wrote:
> 
>>
>>> -----Original Message-----
>>> From: Matt Fleming [mailto:matt@codeblueprint.co.uk]
>>> Sent: Wednesday, February 3, 2016 5:28 AM
>>> To: Ingo Molnar <mingo@kernel.org>
>>> Cc: Laszlo Ersek <lersek@redhat.com>; H . Peter Anvin <hpa@zytor.com>;
>>> Thomas Gleixner <tglx@linutronix.de>; linux-efi@vger.kernel.org; linux-
>>> kernel@vger.kernel.org; Elliott, Robert (Persistent Memory)
>>> <elliott@hpe.com>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>;
>>> Ard Biesheuvel <ard.biesheuvel@linaro.org>; Taku Izumi
>>> <izumi.taku@jp.fujitsu.com>; Linus Torvalds <torvalds@linux-
>>> foundation.org>; Andrew Morton <akpm@linux-foundation.org>
>>> Subject: Re: [PATCH 14/14] x86/efi: Print size in binary units in
>>> efi_print_memmap
>> ...
>>> OK, this patch has caused enough headaches. Let's drop it from this
>>> series.
>>>
>>> Robert, Andy, feel free to resubmit it after you've addressed
>>> everyone's concerns and we can discuss it in isolation.
>>
>> We could just delete the size print altogether - better to print
>> nothing than a silently rounded number.  The end address already
>> communicates the size - it's just not as readable.  
>>
>> The e820 table prints don't bother with a size print. 
>>
>> That would also shorten these extremely wide prints to 116
>> characters (131 if printk time is enabled).
>>
>> [    0.000000] BIOS-e820: [mem 0x0000001880000000-0x000000207fffffff] reserved
>> vs.
>> [    0.000000] efi: mem62: [Reserved           |   |  |NV|  |  |  |  |   |WB|WT|WC|UC] range=[0x0000001880000000-0x000000207fffffff] (32 GiB)
> 
> So I find the latter a lot more readable - my terminals are wide enough ;-)
> 
> Humans are also rather bad at parsing 64-bit hexa address ranges at a glance, so 
> the size display is very useful.
> 
> But the flags portion should be shortened via appropriately chosen 
> single-character abbreviations for the flags. Anyone deeply intimate with the code 
> will recognize the flags - others won't care one way or another.
> 
> plus there's no need to write out 'range='.
>   
> ... and please keep the size and just use GB/TB for chrissake.
> 
> I.e. something like this would work for me:
> 
>> [    0.000000] efi: mem62: 0x0000001880000000-0x000000207fffffff (  32 GB) .N....BTCU "Reserved"

Sorry to disagree :), but I count myself somewhat intimate with UEFI
(albeit more from the edk2 side), and while I can make sense of

  |NV|  |  |  |  |   |WB|WT|WC|UC]

I find

  .N....BTCU

mostly undecipherable. :)

My original goal with this printout was to (a) provide a good impression
of the entire UEFI memmap, at a glance, (b) provide sufficient detail
per-entry, if necessary.

(I don't exactly recall why I was staring at the UEFI memmap dump at
that time, maybe I was working on S3 in OVMF which took a lot of memmap
massaging, or debugging some bug; either way my eyes were bleeding
trying to decode the numeric attributes.)

My xterm, maximized, has 239 columns, which I think counts as pretty low
for today's resolutions. It is nonetheless plenty wide for the current
output. Given that we print this stuff only when debugging information
is requested, I feel that the value of the current columnar output, in
which I can follow a single attribute with my eye across all entries,
should not be diminished, by compressing the columns.

I'm not a wide screen maniac; for example I insist on source code being
wrapped at 79 characters, commit messages at 74, emails at 72 (except
diagrams and log excerpts), and so on. But debug output is different.

My 2 cents, of course...

Thanks
Laszlo

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


#1330225 — Re: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

FromIngo Molnar <mingo@kernel.org>
Date2016-02-09 14:20 +0100
SubjectRe: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap
Message-ID<r0gn1-4x9-25@gated-at.bofh.it>
In reply to#1330215
* Laszlo Ersek <lersek@redhat.com> wrote:

> >> [    0.000000] efi: mem62: 0x0000001880000000-0x000000207fffffff (  32 GB) .N....BTCU "Reserved"
> 
> Sorry to disagree :), but I count myself somewhat intimate with UEFI
> (albeit more from the edk2 side), and while I can make sense of
> 
>   |NV|  |  |  |  |   |WB|WT|WC|UC]
> 
> I find
> 
>   .N....BTCU
> 
> mostly undecipherable. :)

Ok - the longer variant is fine to me as well.

Thanks,

	Ingo

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web