From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756677AbcARVki (ORCPT ); Mon, 18 Jan 2016 16:40:38 -0500 Received: from mail-wm0-f54.google.com ([74.125.82.54]:34716 "EHLO mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756530AbcARVkf (ORCPT ); Mon, 18 Jan 2016 16:40:35 -0500 From: Rasmus Villemoes To: Andy Shevchenko Cc: Robert Elliott , Matt Fleming , Andrew Morton , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , linux-kernel@vger.kernel.org, Brian Norris , Hariprasad S Subject: Re: [PATCH v2 08/11] lib/vsprintf: allow range of prefix for %pl[From[To]] Organization: D03 References: <1452810221-116505-1-git-send-email-andriy.shevchenko@linux.intel.com> <1452810221-116505-9-git-send-email-andriy.shevchenko@linux.intel.com> X-Hashcash: 1:20:160118:andriy.shevchenko@linux.intel.com::LEVXn2M+oy3ijoDD:000000000000000000000000000009TK X-Hashcash: 1:20:160118:elliott@hpe.com::Iq9YFRnXwHhFjsSv:001LFl X-Hashcash: 1:20:160118:hpa@zytor.com::IatOxWcNDVmuaL41:00001Bu/ X-Hashcash: 1:20:160118:mingo@redhat.com::2up6q/UUr8Nu5FjC:02GDd X-Hashcash: 1:20:160118:akpm@linux-foundation.org::Nq/pl8GWAVuhENgj:0000000000000000000000000000000000002rtA X-Hashcash: 1:20:160118:linux-kernel@vger.kernel.org::dt3DkicBgmWhg5sZ:0000000000000000000000000000000003658 X-Hashcash: 1:20:160118:hariprasad@chelsio.com::NW+leUV8ga2Jk8am:0000000000000000000000000000000000000003qeP X-Hashcash: 1:20:160118:computersforpeace@gmail.com::aUiJoz35MEESE8i9:000000000000000000000000000000000072X6 X-Hashcash: 1:20:160118:tglx@linutronix.de::AqhptN40G6xf+ziy:000000000000000000000000000000000000000000077Fu X-Hashcash: 1:20:160118:matt@codeblueprint.co.uk::G2wvL74TH4LWajJr:00000000000000000000000000000000000008Nul Date: Mon, 18 Jan 2016 22:40:32 +0100 In-Reply-To: <1452810221-116505-9-git-send-email-andriy.shevchenko@linux.intel.com> (Andy Shevchenko's message of "Fri, 15 Jan 2016 00:23:38 +0200") Message-ID: <87io2qefcf.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 14 2016, Andy Shevchenko wrote: > The user may supply the range of desired prefixes in a format %pl[From[To]], > where 'From' is letter of minimum allowed prefix, and 'To' is a maximum > correspondently. > Sorry, but yuck. I really don't think a %p extension is a good way to achieve this pretty-printing (with or without the min/max unit thing added). Also, this makes it too easy to lose information (since %plX would round down to X), so we'd neither get something representing the passed-in value nor something human readable (as your test case { .v = 1097364144125ULL, .r = "1071644671 KiB" } shows). > > Signed-off-by: Andy Shevchenko > --- > Documentation/printk-formats.txt | 4 ++++ > lib/test_printf.c | 35 ++++++++++++++++++++++++++++++++--- > lib/vsprintf.c | 18 +++++++++++++++++- > 3 files changed, 53 insertions(+), 4 deletions(-) > > diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt > index 8e8b4c0..3b41899 100644 > --- a/Documentation/printk-formats.txt > +++ b/Documentation/printk-formats.txt > @@ -289,9 +289,13 @@ struct clk: > > unsigned long long value in human-readable form (with IEC binary prefix): > > + %pl[From[To]] > + > %pl 1023 KiB (1047552) > %pl 1048575 B (1048575) > + %plKM 1023 KiB (1048575) > %pl 1 MiB (1048576) > + %plG 2 TiB (2199023255552) > > For printing given unsigned long long value in human-readable form with > automatically choosen IEC binary prefix. > diff --git a/lib/test_printf.c b/lib/test_printf.c > index 1e078c2..ae35885 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -420,10 +420,39 @@ human_readable(void) > { .v = 1048575ULL, .r = "1048575 B" }, > { .v = 1047552ULL, .r = "1023 KiB" }, > }; > - unsigned int i; > + struct hr_test_data htdplM[] = { > + { .v = 1097364144128ULL, .r = "1022 GiB" }, > + { .v = 1097364144125ULL, .r = "1046527 MiB" }, > + { .v = 1048576ULL, .r = "1 MiB" }, > + { .v = 1048575ULL, .r = "0 MiB" }, > + { .v = 1047552ULL, .r = "0 MiB" }, > + }; > + struct hr_test_data htdplKM[] = { > + { .v = 1097364144128ULL, .r = "1046528 MiB" }, > + { .v = 1097364144125ULL, .r = "1071644671 KiB" }, > + { .v = 1048578ULL, .r = "1024 KiB" }, > + { .v = 1048576ULL, .r = "1 MiB" }, > + { .v = 1048575ULL, .r = "1023 KiB" }, > + { .v = 1047552ULL, .r = "1023 KiB" }, > + }; > + struct hr_test_array { > + const char *fmt; > + struct hr_test_data *data; > + size_t sz; > + }; > + struct hr_test_array hta[] = { > + { "%pl", htdpl, ARRAY_SIZE(htdpl) }, > + { "%plM", htdplM, ARRAY_SIZE(htdplM) }, > + { "%plKM", htdplKM, ARRAY_SIZE(htdplKM) }, > + /* No reversed %pl[To[From]] order */ > + { "%plMK", htdplM, ARRAY_SIZE(htdplM) }, > + }; > + unsigned int i, j; > > - for (i = 0; i < ARRAY_SIZE(htdpl); i++) > - test(htdpl[i].r, "%pl", &htdpl[i].v); > + for (j = 0; j < ARRAY_SIZE(hta); j++) { > + for (i = 0; i < hta[j].sz; i++) > + test(hta[j].data[i].r, hta[j].fmt, &hta[j].data[i].v); > + } > } > > static void __init > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 8288470..fbcb221 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -1310,12 +1310,28 @@ char *human_readable(char *buf, char *end, const void *addr, > struct printf_spec spec, const char *fmt) > { > unsigned long long value = *(unsigned long long *)addr; > + unsigned short from = 0, to = STRING_UNITS_2_NUM - 1; > unsigned long index; > + unsigned int i; > + Is there any reason you try to use every integer rank? from, to, index and i never take values other than [0..9], right? So unsigned int (or u8 if you somehow think that would generate better code) should be good for them all? > + /* Parse %pl[From[To]] */ > + for (i = 0; i < STRING_UNITS_2_NUM; i++) > + if (fmt[1] == string_units_2[i][0]) > + break; > + if (i < STRING_UNITS_2_NUM) { > + from = i; > + for (i = 0; i < STRING_UNITS_2_NUM; i++) > + if (fmt[2] == string_units_2[i][0]) > + break; > + if (i < STRING_UNITS_2_NUM && i >= from) > + to = i; > + } > > if (value) > index = __ffs64(value) / 10; > else > index = 0; > + index = clamp_t(unsigned long, index, from, to); And then you wouldn't need to use the _t version here. > /* Print numeric part */ > buf = number(buf, end, value >> (index * 10), default_dec_spec); > @@ -1459,7 +1475,7 @@ int kptr_restrict __read_mostly; > * Do not use this feature without some mechanism to verify the > * correctness of the format string and va_list arguments. > * - 'K' For a kernel pointer that should be hidden from unprivileged users > - * - 'l' For a value in human-readable form (with IEC binary prefix) > + * - 'l[from[to]]' For a value in human-readable form (with IEC binary prefix) > * - 'NF' For a netdev_features_t > * - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with > * a certain separator (' ' by default):