Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1275872 > unrolled thread
| Started by | Rasmus Villemoes <linux@rasmusvillemoes.dk> |
|---|---|
| First post | 2015-11-23 22:40 +0100 |
| Last post | 2015-11-24 00:20 +0100 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-23 22:40 +0100
Re: [PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-24 00:10 +0100
Re: [PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-26 22:50 +0100
Re: [PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits Tejun Heo <tj@kernel.org> - 2015-11-24 00:20 +0100
| From | Rasmus Villemoes <linux@rasmusvillemoes.dk> |
|---|---|
| Date | 2015-11-23 22:40 +0100 |
| Subject | [PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits |
| Message-ID | <qy706-7p3-19@gated-at.bofh.it> |
Maurizio Lombardi reported a problem [1] 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. For consistency, change all the
members to bit fields. gcc doesn't generate much worse code with these
changes (in fact, bloat-o-meter says we save 300 bytes - which I think
is a little surprising).
I didn't find a BUILD_BUG/compiletime_assertion/... which would work
outside function context, so for now I just open-coded it.
[1] http://thread.gmane.org/gmane.linux.kernel/2034835
Reported-by: Maurizio Lombardi <mlombard@redhat.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Joe Perches <joe@perches.com>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
lib/vsprintf.c | 41 +++++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 63ca52366049..01c3aa638582 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 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 */
-};
+ unsigned int type:8; /* format_type enum */
+ signed int field_width:24; /* width of output field */
+ unsigned int flags:8; /* flags to number() */
+ unsigned int base:8; /* number base, 8, 10 or 16 only */
+ signed int precision:16; /* # of digits/chars */
+} __packed;
+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,
@@ -1641,6 +1641,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) {
@@ -1723,16 +1724,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;
}
}
@@ -1789,19 +1790,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.6.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2015-11-24 00:10 +0100 |
| Message-ID | <qy8pc-8rU-1@gated-at.bofh.it> |
| In reply to | #1275872 |
On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> Maurizio Lombardi reported a problem [1] 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. For consistency, change all the
> members to bit fields. gcc doesn't generate much worse code with these
> changes (in fact, bloat-o-meter says we save 300 bytes - which I think
> is a little surprising).
>
> I didn't find a BUILD_BUG/compiletime_assertion/... which would work
> outside function context, so for now I just open-coded it.
And any objections to put it into vsnprintf() ?
>
> [1] http://thread.gmane.org/gmane.linux.kernel/2034835
>
> Reported-by: Maurizio Lombardi <mlombard@redhat.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Joe Perches <joe@perches.com>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> lib/vsprintf.c | 41 +++++++++++++++++++++--------------------
> 1 file changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 63ca52366049..01c3aa638582 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 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 */
> -};
> + unsigned int type:8; /* format_type enum */
> + signed int field_width:24; /* width of output field */
> + unsigned int flags:8; /* flags to number() */
> + unsigned int base:8; /* number base, 8, 10 or 16 only */
> + signed int precision:16; /* # of digits/chars */
> +} __packed;
> +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,
> @@ -1641,6 +1641,7 @@ static noinline_for_stack
> int format_decode(const char *fmt, struct printf_spec *spec)
> {
> const char *start = fmt;
> + char qualifier;
u8 ?
>
> /* we finished early by reading the field width */
> if (spec->type == FORMAT_TYPE_WIDTH) {
> @@ -1723,16 +1724,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;
> }
> }
> @@ -1789,19 +1790,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.6.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Rasmus Villemoes <linux@rasmusvillemoes.dk> |
|---|---|
| Date | 2015-11-26 22:50 +0100 |
| Message-ID | <qzcAq-2gR-21@gated-at.bofh.it> |
| In reply to | #1275946 |
On Tue, Nov 24 2015, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes > <linux@rasmusvillemoes.dk> wrote: >> Maurizio Lombardi reported a problem [1] 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. For consistency, change all the >> members to bit fields. gcc doesn't generate much worse code with these >> changes (in fact, bloat-o-meter says we save 300 bytes - which I think >> is a little surprising). >> >> I didn't find a BUILD_BUG/compiletime_assertion/... which would work >> outside function context, so for now I just open-coded it. > > And any objections to put it into vsnprintf() ? I'd like to keep it close to the type definition. And I was hoping someone would come forward and say "yeah, that's been bugging me too, here's a patch I've been sitting on to fix that". Almost every compiler released this decade has _Static_assert, it's about time we start using that instead of the current mess of homegrown workarounds... Rasmus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2015-11-24 00:20 +0100 |
| Message-ID | <qy8yR-8vh-1@gated-at.bofh.it> |
| In reply to | #1275872 |
On Mon, Nov 23, 2015 at 10:29:21PM +0100, Rasmus Villemoes wrote: > Maurizio Lombardi reported a problem [1] 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. For consistency, change all the > members to bit fields. gcc doesn't generate much worse code with these > changes (in fact, bloat-o-meter says we save 300 bytes - which I think > is a little surprising). > > I didn't find a BUILD_BUG/compiletime_assertion/... which would work > outside function context, so for now I just open-coded it. > > [1] http://thread.gmane.org/gmane.linux.kernel/2034835 > > Reported-by: Maurizio Lombardi <mlombard@redhat.com> > Cc: Tejun Heo <tj@kernel.org> > Cc: Joe Perches <joe@perches.com> > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> FWIW, Acked-by: Tejun Heo <tj@kernel.org> Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web