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

Groups > linux.kernel > #1320474 > unrolled thread

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

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

  Re: [PATCH v3 3/4] x86/efi: print size in binary units in  efi_print_memmap Matt Fleming <matt@codeblueprint.co.uk> - 2016-01-28 10:40 +0100
    Re: [PATCH v3 3/4] x86/efi: print size in binary units in  efi_print_memmap Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-01-28 12:20 +0100

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

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.

[toc] | [next] | [standalone]

#1320573

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web