From: James Bottomley <James.Bottomley@HansenPartnership.com> 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: Robert Elliott <elliott@hpe.com> Subject: Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap Date: Sat, 23 Jan 2016 08:44:05 -0800 [thread overview] Message-ID: <1453567445.2470.24.camel@HansenPartnership.com> (raw) In-Reply-To: <1453560913-134672-4-git-send-email-andriy.shevchenko@linux.intel.com> 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 */ > }
WARNING: multiple messages have this Message-ID (diff)
From: James Bottomley <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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: Robert Elliott <elliott-ZPxbGqLxI0U@public.gmane.org> Subject: Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap Date: Sat, 23 Jan 2016 08:44:05 -0800 [thread overview] Message-ID: <1453567445.2470.24.camel@HansenPartnership.com> (raw) In-Reply-To: <1453560913-134672-4-git-send-email-andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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 */ > }
next prev parent reply other threads:[~2016-01-23 16:44 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-01-23 14:55 [PATCH v3 0/4] x86/efi: use binary units when printing Andy Shevchenko 2016-01-23 14:55 ` [PATCH v3 1/4] lib/string_helpers: export string_units_{2,10} for others Andy Shevchenko 2016-01-23 16:14 ` James Bottomley 2016-01-23 16:14 ` James Bottomley 2016-01-23 16:58 ` Andy Shevchenko 2016-01-23 16:58 ` Andy Shevchenko 2016-01-23 14:55 ` [PATCH v3 2/4] lib/string_helpers: fix indentation in few places Andy Shevchenko 2016-01-23 14:55 ` [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap Andy Shevchenko 2016-01-23 16:44 ` James Bottomley [this message] 2016-01-23 16:44 ` James Bottomley 2016-01-23 17:18 ` Andy Shevchenko 2016-01-23 17:18 ` Andy Shevchenko 2016-01-23 18:03 ` James Bottomley 2016-01-23 18:03 ` James Bottomley 2016-01-25 8:31 ` Andy Shevchenko 2016-01-25 8:31 ` Andy Shevchenko 2016-01-25 15:25 ` James Bottomley 2016-01-25 15:25 ` James Bottomley 2016-01-23 18:12 ` James Bottomley 2016-01-23 18:12 ` James Bottomley 2016-01-23 20:29 ` One Thousand Gnomes 2016-01-23 20:43 ` H. Peter Anvin 2016-01-23 20:43 ` H. Peter Anvin 2016-01-25 18:02 ` Elliott, Robert (Persistent Memory) 2016-01-25 18:02 ` Elliott, Robert (Persistent Memory) 2016-01-25 18:56 ` James Bottomley 2016-01-25 18:56 ` James Bottomley 2016-01-25 19:28 ` Andy Shevchenko 2016-01-25 19:28 ` Andy Shevchenko 2016-01-25 19:45 ` James Bottomley 2016-01-25 19:45 ` James Bottomley 2016-01-25 20:01 ` Andy Shevchenko 2016-01-25 20:01 ` Andy Shevchenko 2016-01-25 20:18 ` James Bottomley 2016-01-25 20:18 ` James Bottomley 2016-01-25 20:37 ` Elliott, Robert (Persistent Memory) 2016-01-25 20:37 ` Elliott, Robert (Persistent Memory) 2016-01-26 11:50 ` Matt Fleming 2016-01-26 11:59 ` Andy Shevchenko 2016-01-26 11:59 ` Andy Shevchenko 2016-01-28 9:29 ` Matt Fleming 2016-01-28 9:29 ` Matt Fleming 2016-01-28 11:15 ` Andy Shevchenko 2016-01-28 11:15 ` Andy Shevchenko 2016-01-25 20:44 ` James Bottomley 2016-01-25 20:44 ` James Bottomley 2016-01-23 14:55 ` [PATCH v3 4/4] x86/efi: Use proper units in efi_find_mirror() Andy Shevchenko 2016-01-23 14:55 ` Andy Shevchenko 2016-01-23 16:34 ` [PATCH v3 0/4] x86/efi: use binary units when printing James Bottomley 2016-01-23 16:34 ` James Bottomley 2016-01-23 17:20 ` Andy Shevchenko 2016-01-23 17:20 ` Andy Shevchenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1453567445.2470.24.camel@HansenPartnership.com \ --to=james.bottomley@hansenpartnership.com \ --cc=akpm@linux-foundation.org \ --cc=andriy.shevchenko@linux.intel.com \ --cc=elliott@hpe.com \ --cc=hpa@zytor.com \ --cc=linux-efi@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@rasmusvillemoes.dk \ --cc=matt@codeblueprint.co.uk \ --cc=mingo@redhat.com \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.