From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752900AbbIPUfs (ORCPT ); Wed, 16 Sep 2015 16:35:48 -0400 Received: from mail-wi0-f178.google.com ([209.85.212.178]:38661 "EHLO mail-wi0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752791AbbIPUfq (ORCPT ); Wed, 16 Sep 2015 16:35:46 -0400 From: Rasmus Villemoes To: Tejun Heo Cc: Maurizio Lombardi , joe@perches.com, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 0/3] fix *pbl format support Organization: D03 References: <1442394523-19176-1-git-send-email-mlombard@redhat.com> <87d1xi4kic.fsf@rasmusvillemoes.dk> <20150916174536.GE3243@mtj.duckdns.org> X-Hashcash: 1:20:150916:mlombard@redhat.com::9uTN3g+gHipsSOy/:0000000000000000000000000000000000000000000omd X-Hashcash: 1:20:150916:tj@kernel.org::RlkKkiij68ZIOPAt:00001HSJ X-Hashcash: 1:20:150916:linux-kernel@vger.kernel.org::NMPAmPY2zOPWhdp6:0000000000000000000000000000000001C5d X-Hashcash: 1:20:150916:joe@perches.com::CsBrG48R0v9t6sbv:001DGr Date: Wed, 16 Sep 2015 22:35:43 +0200 In-Reply-To: <20150916174536.GE3243@mtj.duckdns.org> (Tejun Heo's message of "Wed, 16 Sep 2015 13:45:36 -0400") Message-ID: <877fnq2jc0.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 Wed, Sep 16 2015, Tejun Heo wrote: > > I suppose (2) could work too but we really should strive to provide > something convenient to print[fk] users. The balance here is pretty > one-sided. > I just remembered: I noticed a while ago that the qualifier member is only used inside format_decode (in the end, the information is folded into the type member), so one might as well use a local variable for that. This gives option (3): Make field_width a 24 bit bitfield. I think that should be sufficient for any realistic bitmap that one would print. The patch is also rather small. Surprisingly, bloat-o-meter even says that it's also a net win in code size: add/remove: 18/16 grow/shrink: 3/2 up/down: 5551/-5775 (-224) (the huge absolute numbers are due to stuff like pointer - 1148 +1148 pointer.isra 1116 - -1116 so the functions haven't actually changed that much). I'll have to check how this can be (smaller might still be worse), but at least it doesn't seem to be a catastrophe in terms of .text bloat. [Grr, why don't we have a way to do a compile-time assert outside function context?] Only compile-tested. From: Rasmus Villemoes Date: Wed, 16 Sep 2015 22:06:14 +0200 Subject: [RFC] lib/vsprintf.c: expand field_width to 24 bits Maurizio Lombardi reported a problem with the %pb extension: It doesn't work for sufficiently large bitmaps, since the size is stashed in the field_width field of the struct printf_spec, which is currently an s16. Concretely, this manifested itself in /sys/bus/pseudo/drivers/scsi_debug/map being empty, since the bitmap printer got a size of 0, which is the 16 bit truncation of the actual bitmap size. We do want to keep struct printf_spec at 8 bytes so that it can cheaply be passed by value. The qualifier field is only used for internal bookkeeping in format_decode, so we might as well use a local variable for that. This gives us an additional 8 bits, which we can then use for the field width. To stay in 8 bytes, we need to do a little rearranging and make the type member a bitfield as well. Signed-off-by: Rasmus Villemoes --- lib/vsprintf.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 95cd63b43b99..cce2a780a82e 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -380,13 +380,13 @@ enum format_type { }; struct printf_spec { - u8 type; /* format_type enum */ + u8 type:8; /* format_type enum */ + s32 field_width:24; /* width of output field */ u8 flags; /* flags to number() */ u8 base; /* number base, 8, 10 or 16 only */ - u8 qualifier; /* number qualifier, one of 'hHlLtzZ' */ - s16 field_width; /* width of output field */ s16 precision; /* # of digits/chars */ }; +extern char __check_printf_spec[1-2*(sizeof(struct printf_spec) != 8)]; static noinline_for_stack char *number(char *buf, char *end, unsigned long long num, @@ -1633,6 +1633,7 @@ static noinline_for_stack int format_decode(const char *fmt, struct printf_spec *spec) { const char *start = fmt; + char qualifier; /* we finished early by reading the field width */ if (spec->type == FORMAT_TYPE_WIDTH) { @@ -1715,16 +1716,16 @@ precision: qualifier: /* get the conversion qualifier */ - spec->qualifier = -1; + qualifier = 0; if (*fmt == 'h' || _tolower(*fmt) == 'l' || _tolower(*fmt) == 'z' || *fmt == 't') { - spec->qualifier = *fmt++; - if (unlikely(spec->qualifier == *fmt)) { - if (spec->qualifier == 'l') { - spec->qualifier = 'L'; + qualifier = *fmt++; + if (unlikely(qualifier == *fmt)) { + if (qualifier == 'l') { + qualifier = 'L'; ++fmt; - } else if (spec->qualifier == 'h') { - spec->qualifier = 'H'; + } else if (qualifier == 'h') { + qualifier = 'H'; ++fmt; } } @@ -1781,19 +1782,19 @@ qualifier: return fmt - start; } - if (spec->qualifier == 'L') + if (qualifier == 'L') spec->type = FORMAT_TYPE_LONG_LONG; - else if (spec->qualifier == 'l') { + else if (qualifier == 'l') { BUILD_BUG_ON(FORMAT_TYPE_ULONG + SIGN != FORMAT_TYPE_LONG); spec->type = FORMAT_TYPE_ULONG + (spec->flags & SIGN); - } else if (_tolower(spec->qualifier) == 'z') { + } else if (_tolower(qualifier) == 'z') { spec->type = FORMAT_TYPE_SIZE_T; - } else if (spec->qualifier == 't') { + } else if (qualifier == 't') { spec->type = FORMAT_TYPE_PTRDIFF; - } else if (spec->qualifier == 'H') { + } else if (qualifier == 'H') { BUILD_BUG_ON(FORMAT_TYPE_UBYTE + SIGN != FORMAT_TYPE_BYTE); spec->type = FORMAT_TYPE_UBYTE + (spec->flags & SIGN); - } else if (spec->qualifier == 'h') { + } else if (qualifier == 'h') { BUILD_BUG_ON(FORMAT_TYPE_USHORT + SIGN != FORMAT_TYPE_SHORT); spec->type = FORMAT_TYPE_USHORT + (spec->flags & SIGN); } else { -- 2.1.3