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

From: James Bottomley <James.Bottomley@HansenPartnership.com>
To: Andy Shevchenko <andy.shevchenko@gmail.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" <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
Date: Mon, 25 Jan 2016 11:45:31 -0800	[thread overview]
Message-ID: <1453751131.2363.54.camel@HansenPartnership.com> (raw)
In-Reply-To: <CAHp75VdYRzC9TcDpKxq=xddA0zgVk=5_25uXaimn1tOMZVeK4Q@mail.gmail.com>

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