Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap
From: James Bottomley
Date: Mon Jan 25 2016 - 13:56:20 EST
On Mon, 2016-01-25 at 18:02 +0000, Elliott, Robert (Persistent Memory)
wrote:
>
>
> > -----Original Message-----
> > From: James Bottomley [mailto:James.Bottomley@xxxxxxxxxxxxxxxxxxxxx
> > ]
> > Sent: Saturday, January 23, 2016 10:44 AM
> > To: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; Matt
> > Fleming
> > <matt@xxxxxxxxxxxxxxxxxxx>; Thomas Gleixner <tglx@xxxxxxxxxxxxx>;
> > Ingo
> > Molnar <mingo@xxxxxxxxxx>; H . Peter Anvin <hpa@xxxxxxxxx>; linux-
> > efi@xxxxxxxxxxxxxxx; Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>;
> > Andrew
> > Morton <akpm@xxxxxxxxxxxxxxxxxxxx>; linux-kernel @ vger . kernel .
> > org
> > <linux-kernel@xxxxxxxxxxxxxxx>
> > Cc: Elliott, Robert (Persistent Memory) <elliott@xxxxxxx>
> > 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@xxxxxxx>
> > >
> > > 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)
{