Re: [PATCH v2 01/21] lib/vsprintf: Print time and date in human readable format via %pt
On Wed, Feb 21, 2018 at 04:05:53PM +0200, Andy Shevchenko wrote: > > TBH, I don't see much consensus among developers on this topic. > > I wouldn't like to send a new version until it would be a consensus. > > I would love to see the comments followed by an agreement from majority > of people who are in Cc list here. Not sure why I am on the CC list, but if we add it at all I do not think this should be guarded by a config option. Otherwise let's add options for %d and %s as well. Thanks. -- Dmitry
Re: [PATCH v2 01/21] lib/vsprintf: Print time and date in human readable format via %pt
On Wed, 2018-02-21 at 15:19 +0100, Geert Uytterhoeven wrote: > On Wed, Feb 21, 2018 at 2:23 PM, Andy Shevchenko > wrote: > > On Wed, 2018-02-21 at 08:38 +0100, Rasmus Villemoes wrote: > > > On 2018-02-21 00:55, Joe Perches wrote: > > > Well, on the one hand, I like to reduce the size of the kernel > > > when > > > possible and ideally make all new functionality guarded by config > > > options, but OTOH, how much does compiling out the datetime > > > formatters > > > really save? > > > > https://lists.01.org/pipermail/kbuild-all/2017-June/034950.html > > > > I understand that half a year time allows us to increase kernel text > > size by 750+ bytes unconditionally. > > > > I would really like to not use any option. > > Agreed. > > FTR, growth of my atari_defconfig kernel between v4.7 and v4.15: > > add/remove: 351/155 grow/shrink: 691/429 up/down: 63095/-38665 (24430) > add/remove: 394/156 grow/shrink: 595/709 up/down: 61173/-31092 (30081) > add/remove: 1315/711 grow/shrink: 1269/442 up/down: 172871/-92075 > (80796) > add/remove: 525/266 grow/shrink: 914/510 up/down: 116115/-46240 > (69875) > add/remove: 443/222 grow/shrink: 906/456 up/down: 77807/-40657 (37150) > add/remove: 536/296 grow/shrink: 1043/652 up/down: 97366/-65459 > (31907) > add/remove: 413/176 grow/shrink: 711/479 up/down: 75678/-41356 (34322) > add/remove: 311/145 grow/shrink: 898/438 up/down: 51655/-26851 (24804) The concern was emitted from one who cares about tiny config, i.e. i386_tiny_defconfig. -- Andy Shevchenko Intel Finland Oy
Re: [PATCH v2 01/21] lib/vsprintf: Print time and date in human readable format via %pt
On Wed, Feb 21, 2018 at 2:23 PM, Andy Shevchenko
wrote:
> On Wed, 2018-02-21 at 08:38 +0100, Rasmus Villemoes wrote:
>> On 2018-02-21 00:55, Joe Perches wrote:
>> > On Tue, 2018-02-20 at 23:43 +0200, Andy Shevchenko wrote:
>> > > There are users which print time and date represented by content
>> > > of
>> > > struct rtc_time in human readable format.
>> > >
>> > > Instead of open coding that each time introduce %ptR[dt][rv]
>> > > specifier.
>> > >
>> > > Note, users have to select PRINTK_PEXT_TIMEDATE option in a
>> > > Kconfig.
>> >
>> > Not sure this is a great option.
>> > Not just the name, the need to select it.
>>
>> Bikeshedding first: If you do keep the config option, please use
>> PRINTF,
>> not PRINTK - vsprintf can be and is used by lots of code other than
>> printk.
>
> OK.
>
>> Well, on the one hand, I like to reduce the size of the kernel when
>> possible and ideally make all new functionality guarded by config
>> options, but OTOH, how much does compiling out the datetime formatters
>> really save?
>
> https://lists.01.org/pipermail/kbuild-all/2017-June/034950.html
>
> I understand that half a year time allows us to increase kernel text
> size by 750+ bytes unconditionally.
>
> I would really like to not use any option.
Agreed.
FTR, growth of my atari_defconfig kernel between v4.7 and v4.15:
add/remove: 351/155 grow/shrink: 691/429 up/down: 63095/-38665 (24430)
add/remove: 394/156 grow/shrink: 595/709 up/down: 61173/-31092 (30081)
add/remove: 1315/711 grow/shrink: 1269/442 up/down: 172871/-92075 (80796)
add/remove: 525/266 grow/shrink: 914/510 up/down: 116115/-46240 (69875)
add/remove: 443/222 grow/shrink: 906/456 up/down: 77807/-40657 (37150)
add/remove: 536/296 grow/shrink: 1043/652 up/down: 97366/-65459 (31907)
add/remove: 413/176 grow/shrink: 711/479 up/down: 75678/-41356 (34322)
add/remove: 311/145 grow/shrink: 898/438 up/down: 51655/-26851 (24804)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v2 01/21] lib/vsprintf: Print time and date in human readable format via %pt
> TBH, I don't see much consensus among developers on this topic. > I wouldn't like to send a new version until it would be a consensus. I would love to see the comments followed by an agreement from majority of people who are in Cc list here. -- Andy Shevchenko Intel Finland Oy
Re: [PATCH v2 01/21] lib/vsprintf: Print time and date in human readable format via %pt
On Wed, 2018-02-21 at 10:33 +0100, Geert Uytterhoeven wrote:
> Hi Andy,
>
> On Tue, Feb 20, 2018 at 10:43 PM, Andy Shevchenko
> wrote:
> > There are users which print time and date represented by content of
> > struct rtc_time in human readable format.
> >
> > Instead of open coding that each time introduce %ptR[dt][rv]
> > specifier.
>
> Thanks for your patch!
>
> > Note, users have to select PRINTK_PEXT_TIMEDATE option in a Kconfig.
>
> Is it worthwhile making this an option?
People were complaining before
https://lists.01.org/pipermail/kbuild-all/2017-June/034950.html
> > --- a/Documentation/core-api/printk-formats.rst
> > +++ b/Documentation/core-api/printk-formats.rst
> > @@ -412,6 +412,37 @@ Examples::
> >
> > Passed by reference.
> >
> > +Time and date
> > +-
> > +
> > +::
> > +
> > + %pt[R] -mm-dd HH:MM:SS
> > + %pt[R]d -mm-dd
> > + %pt[R]t HH:MM:SS
>
> [R] suggests the "R" is optional?
> But if it's missing, it prints the hex pointer value?
Yes.
> > + %pt[R][dt]
>
> What's the purpose of this?
A place holder to extend.
> > +
> > + R for struct rtc_time
> > +
> > +Note, users have to select PRINTK_PEXT_TIMEDATE option in a
> > Kconfig.
> > +
> > +struct rtc_time
> > +~~~
> > +
> > +::
> > +
> > + %ptR[dt][rv]
>
> What's the purpose of this paragraph, compared to the previous one?
This is first batch to make it working for struct rtc_time. We have
several users (and I have some local patches WIP) to print time64_t /
timespec64 which would use different letters and paragraphs to explain.
I could remove it and return like it was in v1 (with the exception for
new R letter added).
TBH, I don't see much consensus among developers on this topic.
I wouldn't like to send a new version until it would be a consensus.
> > +static noinline_for_stack
> > +char *date_str(char *buf, char *end, const struct rtc_time *tm,
> > bool v, bool r)
> > +{
> > + int year = tm->tm_year + (r ? 0 : 1900);
> > + int mon = tm->tm_mon + (r ? 0 : 1);
> > +
> > + if (unlikely(v && (unsigned int)tm->tm_year > 200))
> > + buf = string(buf, end, "", default_str_spec);
> > + else
> > + buf = number(buf, end, year, default_dec04_spec);
> > +
> > + if (buf < end)
> > + *buf = '-';
>
> Instead of all these checks to avoid overflowing the passed buffer, it
> may be simpler to format everything in a fixed-size buffer on the
> stack,
> and copy whatever will fit in the target buffer at the end.
I dropped that idea since the most heavier call is number().
We still need to do several of them one way or the other.
So, I really don't see much benefit of doing your way.
> > +static noinline_for_stack
> > +char *rtc_str(char *buf, char *end, const struct rtc_time *tm,
> > const char *fmt)
> > +{
> > + bool have_t = true, have_d = true;
> > + bool validate = false;
> > + bool raw = false;
> > + int count = 1;
> > + bool found;
> > +
> > + switch (fmt[++count]) {
> > + case 'd':
> > + have_t = false;
> > + break;
> > + case 't':
> > + have_d = false;
> > + break;
> > + }
> > +
> > + /* No %pt[dt] supplied */
> > + if (have_d && have_t)
> > + --count;
>
> First increment count, then rollback.
> What about:
>
> switch (fmt[count]) {
> case 'd':
> have_t = false;
> count++;
> break;
> case 't':
> have_d = false;
> count++;
> break;
> }
Or simple:
default:
--count;
break;
?
I really need to come up with the next pile for time64_t which I suppose
will require rethinking of format parsing and printing functions here.
--
Andy Shevchenko
Intel Finland Oy
Re: [PATCH v2 01/21] lib/vsprintf: Print time and date in human readable format via %pt
On Wed, 2018-02-21 at 08:38 +0100, Rasmus Villemoes wrote: > On 2018-02-21 00:55, Joe Perches wrote: > > On Tue, 2018-02-20 at 23:43 +0200, Andy Shevchenko wrote: > > > There are users which print time and date represented by content > > > of > > > struct rtc_time in human readable format. > > > > > > Instead of open coding that each time introduce %ptR[dt][rv] > > > specifier. > > > > > > Note, users have to select PRINTK_PEXT_TIMEDATE option in a > > > Kconfig. > > > > Not sure this is a great option. > > Not just the name, the need to select it. > > Bikeshedding first: If you do keep the config option, please use > PRINTF, > not PRINTK - vsprintf can be and is used by lots of code other than > printk. OK. > Well, on the one hand, I like to reduce the size of the kernel when > possible and ideally make all new functionality guarded by config > options, but OTOH, how much does compiling out the datetime formatters > really save? https://lists.01.org/pipermail/kbuild-all/2017-June/034950.html I understand that half a year time allows us to increase kernel text size by 750+ bytes unconditionally. I would really like to not use any option. > Also, I agree with Joe's concern about the need to select > it. So, what exactly you are proposing? > Maybe if we had a gcc plugin that did %pFOO validation it could also > warn about %pBAR being used without a corresponding config option > being > set. But we don't have that currently... We have not, so, it's out of scope. If it's a big impediment, then I'm not the guy who will do the job. -- Andy Shevchenko Intel Finland Oy
Re: [PATCH v2 01/21] lib/vsprintf: Print time and date in human readable format via %pt
On Tue, 2018-02-20 at 15:55 -0800, Joe Perches wrote: > On Tue, 2018-02-20 at 23:43 +0200, Andy Shevchenko wrote: > > There are users which print time and date represented by content of > > struct rtc_time in human readable format. > > > > Instead of open coding that each time introduce %ptR[dt][rv] > > specifier. > > > > Note, users have to select PRINTK_PEXT_TIMEDATE option in a Kconfig. > > Not sure this is a great option. > Not just the name, the need to select it. kbuildbot and some people complained about + text size. https://lists.01.org/pipermail/kbuild-all/2017-June/034950.html I would really like to compile it always. > > + int year = tm->tm_year + (r ? 0 : 1900); > > + int mon = tm->tm_mon + (r ? 0 : 1); > > What happens with negative values? Same as before. > Perhaps these temporaries should be unsigned int. No, the type of them is int, so, I'll keep it int. > > + if (unlikely(v && (unsigned int)tm->tm_min > 59)) > > leap seconds are allowed in the struct Alexandre answered already, but I would add that this is part of existing ABI, so, I wouldn't go to change this. -- Andy Shevchenko Intel Finland Oy
Re: [PATCH v2 01/21] lib/vsprintf: Print time and date in human readable format via %pt
Hi Andy,
On Tue, Feb 20, 2018 at 10:43 PM, Andy Shevchenko
wrote:
> There are users which print time and date represented by content of
> struct rtc_time in human readable format.
>
> Instead of open coding that each time introduce %ptR[dt][rv] specifier.
Thanks for your patch!
> Note, users have to select PRINTK_PEXT_TIMEDATE option in a Kconfig.
Is it worthwhile making this an option?
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -412,6 +412,37 @@ Examples::
>
> Passed by reference.
>
> +Time and date
> +-
> +
> +::
> +
> + %pt[R] -mm-dd HH:MM:SS
> + %pt[R]d -mm-dd
> + %pt[R]t HH:MM:SS
[R] suggests the "R" is optional?
But if it's missing, it prints the hex pointer value?
> + %pt[R][dt]
What's the purpose of this?
> +
> + R for struct rtc_time
> +
> +Note, users have to select PRINTK_PEXT_TIMEDATE option in a Kconfig.
> +
> +struct rtc_time
> +~~~
> +
> +::
> +
> + %ptR[dt][rv]
What's the purpose of this paragraph, compared to the previous one?
> +
> +For printing date and time as represented by struct rtc_time structure in
> +human readable format.
> @@ -1443,6 +1458,132 @@ char *address_val(char *buf, char *end, const void
> *addr, const char *fmt)
> return special_hex_number(buf, end, num, size);
> }
>
> +static noinline_for_stack
> +char *date_str(char *buf, char *end, const struct rtc_time *tm, bool v, bool
> r)
> +{
> + int year = tm->tm_year + (r ? 0 : 1900);
> + int mon = tm->tm_mon + (r ? 0 : 1);
> +
> + if (unlikely(v && (unsigned int)tm->tm_year > 200))
> + buf = string(buf, end, "", default_str_spec);
> + else
> + buf = number(buf, end, year, default_dec04_spec);
> +
> + if (buf < end)
> + *buf = '-';
Instead of all these checks to avoid overflowing the passed buffer, it
may be simpler to format everything in a fixed-size buffer on the stack,
and copy whatever will fit in the target buffer at the end.
> + buf++;
> +
> + if (unlikely(v && (unsigned int)tm->tm_mon > 11))
> + buf = string(buf, end, "**", default_str_spec);
> + else
> + buf = number(buf, end, mon, default_dec02_spec);
> +
> + if (buf < end)
> + *buf = '-';
> + buf++;
> +
> + if (unlikely(v && (unsigned int)tm->tm_mday > 31))
> + buf = string(buf, end, "**", default_str_spec);
> + else
> + buf = number(buf, end, tm->tm_mday, default_dec02_spec);
> +
> + return buf;
> +}
> +
> +static noinline_for_stack
> +char *time_str(char *buf, char *end, const struct rtc_time *tm, bool v, bool
> r)
> +{
> + if (unlikely(v && (unsigned int)tm->tm_hour > 24))
> + buf = string(buf, end, "**", default_str_spec);
> + else
> + buf = number(buf, end, tm->tm_hour, default_dec02_spec);
> +
> + if (buf < end)
> + *buf = ':';
Likewise.
> + buf++;
> +
> + if (unlikely(v && (unsigned int)tm->tm_min > 59))
> + buf = string(buf, end, "**", default_str_spec);
> + else
> + buf = number(buf, end, tm->tm_min, default_dec02_spec);
> +
> + if (buf < end)
> + *buf = ':';
> + buf++;
> +
> + if (unlikely(v && (unsigned int)tm->tm_sec > 59))
> + buf = string(buf, end, "**", default_str_spec);
> + else
> + buf = number(buf, end, tm->tm_sec, default_dec02_spec);
> +
> + return buf;
> +}
> +
> +static noinline_for_stack
> +char *rtc_str(char *buf, char *end, const struct rtc_time *tm, const char
> *fmt)
> +{
> + bool have_t = true, have_d = true;
> + bool validate = false;
> + bool raw = false;
> + int count = 1;
> + bool found;
> +
> + switch (fmt[++count]) {
> + case 'd':
> + have_t = false;
> + break;
> + case 't':
> + have_d = false;
> + break;
> + }
> +
> + /* No %pt[dt] supplied */
> + if (have_d && have_t)
> + --count;
First increment count, then rollback.
What about:
switch (fmt[count]) {
case 'd':
have_t = false;
count++;
break;
case 't':
have_d = false;
count++;
break;
}
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v2 01/21] lib/vsprintf: Print time and date in human readable format via %pt
On 20/02/2018 at 15:55:07 -0800, Joe Perches wrote:
> On Tue, 2018-02-20 at 23:43 +0200, Andy Shevchenko wrote:
> > +static noinline_for_stack
> > +char *time_str(char *buf, char *end, const struct rtc_time *tm, bool v,
> > bool r)
> > +{
>
> Maybe use unsigned int temporaries here too for hour, min, sec
>
> > + if (unlikely(v && (unsigned int)tm->tm_hour > 24))
> > + buf = string(buf, end, "**", default_str_spec);
> > + else
> > + buf = number(buf, end, tm->tm_hour, default_dec02_spec);
> > +
> > + if (buf < end)
> > + *buf = ':';
> > + buf++;
> > +
> > + if (unlikely(v && (unsigned int)tm->tm_min > 59))
>
> leap seconds are allowed in the struct
>
No, they are not:
https://elixir.bootlin.com/linux/v4.15.4/source/drivers/rtc/rtc-lib.c#L108
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [PATCH v2 01/21] lib/vsprintf: Print time and date in human readable format via %pt
On 2018-02-21 00:55, Joe Perches wrote: > On Tue, 2018-02-20 at 23:43 +0200, Andy Shevchenko wrote: >> There are users which print time and date represented by content of >> struct rtc_time in human readable format. >> >> Instead of open coding that each time introduce %ptR[dt][rv] specifier. >> >> Note, users have to select PRINTK_PEXT_TIMEDATE option in a Kconfig. > > Not sure this is a great option. > Not just the name, the need to select it. Bikeshedding first: If you do keep the config option, please use PRINTF, not PRINTK - vsprintf can be and is used by lots of code other than printk. Well, on the one hand, I like to reduce the size of the kernel when possible and ideally make all new functionality guarded by config options, but OTOH, how much does compiling out the datetime formatters really save? Also, I agree with Joe's concern about the need to select it. Maybe if we had a gcc plugin that did %pFOO validation it could also warn about %pBAR being used without a corresponding config option being set. But we don't have that currently... Rasmus
Re: [PATCH v2 01/21] lib/vsprintf: Print time and date in human readable format via %pt
On Tue, 2018-02-20 at 23:43 +0200, Andy Shevchenko wrote:
> There are users which print time and date represented by content of
> struct rtc_time in human readable format.
>
> Instead of open coding that each time introduce %ptR[dt][rv] specifier.
>
> Note, users have to select PRINTK_PEXT_TIMEDATE option in a Kconfig.
Not sure this is a great option.
Not just the name, the need to select it.
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> +static noinline_for_stack
> +char *date_str(char *buf, char *end, const struct rtc_time *tm, bool v, bool
> r)
> +{
> + int year = tm->tm_year + (r ? 0 : 1900);
> + int mon = tm->tm_mon + (r ? 0 : 1);
What happens with negative values?
Perhaps these temporaries should be unsigned int.
> +
> + if (unlikely(v && (unsigned int)tm->tm_year > 200))
> + buf = string(buf, end, "", default_str_spec);
> + else
> + buf = number(buf, end, year, default_dec04_spec);
> +
> + if (buf < end)
> + *buf = '-';
> + buf++;
> +
> + if (unlikely(v && (unsigned int)tm->tm_mon > 11))
> + buf = string(buf, end, "**", default_str_spec);
> + else
> + buf = number(buf, end, mon, default_dec02_spec);
> +
> + if (buf < end)
> + *buf = '-';
> + buf++;
> +
> + if (unlikely(v && (unsigned int)tm->tm_mday > 31))
> + buf = string(buf, end, "**", default_str_spec);
> + else
> + buf = number(buf, end, tm->tm_mday, default_dec02_spec);
> +
> + return buf;
> +}
> +
> +static noinline_for_stack
> +char *time_str(char *buf, char *end, const struct rtc_time *tm, bool v, bool
> r)
> +{
Maybe use unsigned int temporaries here too for hour, min, sec
> + if (unlikely(v && (unsigned int)tm->tm_hour > 24))
> + buf = string(buf, end, "**", default_str_spec);
> + else
> + buf = number(buf, end, tm->tm_hour, default_dec02_spec);
> +
> + if (buf < end)
> + *buf = ':';
> + buf++;
> +
> + if (unlikely(v && (unsigned int)tm->tm_min > 59))
leap seconds are allowed in the struct
> + buf = string(buf, end, "**", default_str_spec);
> + else
> + buf = number(buf, end, tm->tm_min, default_dec02_spec);
> +
> + if (buf < end)
> + *buf = ':';
> + buf++;
> +
> + if (unlikely(v && (unsigned int)tm->tm_sec > 59))
> + buf = string(buf, end, "**", default_str_spec);
> + else
> + buf = number(buf, end, tm->tm_sec, default_dec02_spec);
> +
> + return buf;
> +}
>
