Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Paul E. McKenney]

On Tue, Aug 08, 2017 at 07:08:00PM -0400, Prarit Bhargava wrote:
> 
> 
> On 08/08/2017 04:28 AM, Peter Zijlstra wrote:
> > On Mon, Aug 07, 2017 at 01:36:39PM -0700, Paul E. McKenney wrote:
> >> On Mon, Aug 07, 2017 at 04:06:09PM -0400, Prarit Bhargava wrote:
> > 
> >>> peterz?  Want to offer a suggestion?  The issue is that I'm changing a 
> >>> bool
> >>> config option to an int and that impacts all the arch's defconfigs.  John 
> >>> points
> >>> out that this is a lot of churn and we're both wondering if there's a 
> >>> better way
> >>> to do the configs.
> >>
> >> The usual approach is to keep the old bool Kconfig option, and add another
> >> int Kconfig option that depends on the original one.  The tests for
> >> the int value get a bit more complex, but one way to handle this is to
> >> define a cpp macro something like the following:
> >>
> >> #ifdef CONFIG_OLD_OPTION
> >> #define CPP_NEW_OPTION 0
> >> #else
> >> #define CPP_NEW_OPTION CONFIG_NEW_OPTION
> >> #endif
> >>
> >> Then use CPP_NEW_OPTION, where zero means disabled and other numbers
> >> select the available options.
> >>
> >> Adjust to suit depending on what values mean what.
> >>
> >> Another approach is to make the range of the new Kconfig option
> >> depend on the old option:
> >>
> >> config NEW_OPTION
> >>int "your description here"
> >>range 1 5 if OLD_OPTION
> >>range 0 0 if !OLD_OPTION
> >>default 0
> >>help
> >>  your help here
> >>
> >> Again, adjust to suit depending on what values mean what.
> > 
> > Right this. Except I don't see the !OLD_OPTION working as expected.
> > A 'new' config will not include the old one, so the !OLD_OPTION thing
> > will 'always' be true.
> > 
> > So your:
> > 
> >> @@ -1,8 +1,46 @@
> >>  menu "printk and dmesg options"
> >>
> >> +choice
> >> +   prompt "printk default clock"
> >> +   config PRINTK_TIME_DISABLE
> >> +   bool "Disabled"
> >> +   help
> >> +Selecting this option disables the time stamps of printk().
> >> +
> >> +   config PRINTK_TIME_LOCAL
> >> +   bool "Local Clock"
> >> +   help
> >> + Selecting this option causes the time stamps of printk() to be
> >> + stamped with the unadjusted hardware clock.
> >> +
> >> +   config PRINTK_TIME_BOOT
> >> +   bool "CLOCK_BOOTTIME"
> >> +   help
> >> + Selecting this option causes the time stamps of printk() to be
> >> + stamped with the adjusted boottime clock.
> >> +
> >> +   config PRINTK_TIME_MONO
> >> +   bool "CLOCK_MONOTONIC"
> >> +   help
> >> + Selecting this option causes the time stamps of printk() to be
> >> + stamped with the adjusted monotonic clock.
> >> +
> >> +   config PRINTK_TIME_REAL
> >> +   bool "CLOCK_REALTIME"
> >> +   help
> >> + Selecting this option causes the time stamps of printk() to be
> >> + stamped with the adjusted realtime clock.
> >> +
> >> +endchoice
> >> +
> >>  config PRINTK_TIME
> > 
> > Change that into something like:
> > 
> > config PRINTK_CLOCK
> > 
> > 
> >> -   bool "Show timing information on printks"
> >> +   int "Show time stamp information on printks"
> >> depends on PRINTK
> >> +   default 0 if PRINTK_TIME_DISABLE
> >> +   default 1 if PRINTK_TIME_LOCAL
> > 
> > And that into:
> > 
> > default 1 if PRINTK_TIME_LOCAL || PRINTK_TIME
> > 
> >> +   default 2 if PRINTK_TIME_BOOT
> >> +   default 3 if PRINTK_TIME_MONO
> >> +   default 4 if PRINTK_TIME_REAL
> >>  help
> >>Selecting this option causes time stamps of the printk()
> > 
> > Then the old PRINTK_TIME symbol will auto-convert into the new
> > equivalent.
> > 
> 
> I don't think there's an easy code way around this.  Essentially this Kconfig
> code boils down to properly evaluating
> 
> config PRINTK_CLOCK
>   default 1 if PRINTK_TIME
>   default 0
> 
> where there is no Kconfig entry for PRINTK_TIME.
> 
> If undefined CONFIG_PRINTK_TIME is used in a config, it is immediately
> scrubbed by the kconfig script so it doesn't "exist" when CONFIG_PRINTK_CLOCK
> is evaluated.  The result of that is CONFIG_PRINT_CLOCK=0.
> 
> I tried
> 
> config PRINTK_TIME
>   bool "old config option"
> 
> then I end up with both a CONFIG_PRINTK_CLOCK=1 and a CONFIG_PRINTK_TIME=y in
> the resulting config which is confusing.
> 
> I've debated using the other suggestion that Paul made but TBH (sorry
> Paul) it seems like I'm avoiding the real but noisy solution of
> 
>   s/PRINTK_TIME=y/PRINTK_TIME=1/g
> 
> I'm obviously open to other suggestions...

It is someone else's turn to provide a suggestion.  ;-)

Thanx, Paul

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Paul E. McKenney]

On Tue, Aug 08, 2017 at 07:08:00PM -0400, Prarit Bhargava wrote:
> 
> 
> On 08/08/2017 04:28 AM, Peter Zijlstra wrote:
> > On Mon, Aug 07, 2017 at 01:36:39PM -0700, Paul E. McKenney wrote:
> >> On Mon, Aug 07, 2017 at 04:06:09PM -0400, Prarit Bhargava wrote:
> > 
> >>> peterz?  Want to offer a suggestion?  The issue is that I'm changing a 
> >>> bool
> >>> config option to an int and that impacts all the arch's defconfigs.  John 
> >>> points
> >>> out that this is a lot of churn and we're both wondering if there's a 
> >>> better way
> >>> to do the configs.
> >>
> >> The usual approach is to keep the old bool Kconfig option, and add another
> >> int Kconfig option that depends on the original one.  The tests for
> >> the int value get a bit more complex, but one way to handle this is to
> >> define a cpp macro something like the following:
> >>
> >> #ifdef CONFIG_OLD_OPTION
> >> #define CPP_NEW_OPTION 0
> >> #else
> >> #define CPP_NEW_OPTION CONFIG_NEW_OPTION
> >> #endif
> >>
> >> Then use CPP_NEW_OPTION, where zero means disabled and other numbers
> >> select the available options.
> >>
> >> Adjust to suit depending on what values mean what.
> >>
> >> Another approach is to make the range of the new Kconfig option
> >> depend on the old option:
> >>
> >> config NEW_OPTION
> >>int "your description here"
> >>range 1 5 if OLD_OPTION
> >>range 0 0 if !OLD_OPTION
> >>default 0
> >>help
> >>  your help here
> >>
> >> Again, adjust to suit depending on what values mean what.
> > 
> > Right this. Except I don't see the !OLD_OPTION working as expected.
> > A 'new' config will not include the old one, so the !OLD_OPTION thing
> > will 'always' be true.
> > 
> > So your:
> > 
> >> @@ -1,8 +1,46 @@
> >>  menu "printk and dmesg options"
> >>
> >> +choice
> >> +   prompt "printk default clock"
> >> +   config PRINTK_TIME_DISABLE
> >> +   bool "Disabled"
> >> +   help
> >> +Selecting this option disables the time stamps of printk().
> >> +
> >> +   config PRINTK_TIME_LOCAL
> >> +   bool "Local Clock"
> >> +   help
> >> + Selecting this option causes the time stamps of printk() to be
> >> + stamped with the unadjusted hardware clock.
> >> +
> >> +   config PRINTK_TIME_BOOT
> >> +   bool "CLOCK_BOOTTIME"
> >> +   help
> >> + Selecting this option causes the time stamps of printk() to be
> >> + stamped with the adjusted boottime clock.
> >> +
> >> +   config PRINTK_TIME_MONO
> >> +   bool "CLOCK_MONOTONIC"
> >> +   help
> >> + Selecting this option causes the time stamps of printk() to be
> >> + stamped with the adjusted monotonic clock.
> >> +
> >> +   config PRINTK_TIME_REAL
> >> +   bool "CLOCK_REALTIME"
> >> +   help
> >> + Selecting this option causes the time stamps of printk() to be
> >> + stamped with the adjusted realtime clock.
> >> +
> >> +endchoice
> >> +
> >>  config PRINTK_TIME
> > 
> > Change that into something like:
> > 
> > config PRINTK_CLOCK
> > 
> > 
> >> -   bool "Show timing information on printks"
> >> +   int "Show time stamp information on printks"
> >> depends on PRINTK
> >> +   default 0 if PRINTK_TIME_DISABLE
> >> +   default 1 if PRINTK_TIME_LOCAL
> > 
> > And that into:
> > 
> > default 1 if PRINTK_TIME_LOCAL || PRINTK_TIME
> > 
> >> +   default 2 if PRINTK_TIME_BOOT
> >> +   default 3 if PRINTK_TIME_MONO
> >> +   default 4 if PRINTK_TIME_REAL
> >>  help
> >>Selecting this option causes time stamps of the printk()
> > 
> > Then the old PRINTK_TIME symbol will auto-convert into the new
> > equivalent.
> > 
> 
> I don't think there's an easy code way around this.  Essentially this Kconfig
> code boils down to properly evaluating
> 
> config PRINTK_CLOCK
>   default 1 if PRINTK_TIME
>   default 0
> 
> where there is no Kconfig entry for PRINTK_TIME.
> 
> If undefined CONFIG_PRINTK_TIME is used in a config, it is immediately
> scrubbed by the kconfig script so it doesn't "exist" when CONFIG_PRINTK_CLOCK
> is evaluated.  The result of that is CONFIG_PRINT_CLOCK=0.
> 
> I tried
> 
> config PRINTK_TIME
>   bool "old config option"
> 
> then I end up with both a CONFIG_PRINTK_CLOCK=1 and a CONFIG_PRINTK_TIME=y in
> the resulting config which is confusing.
> 
> I've debated using the other suggestion that Paul made but TBH (sorry
> Paul) it seems like I'm avoiding the real but noisy solution of
> 
>   s/PRINTK_TIME=y/PRINTK_TIME=1/g
> 
> I'm obviously open to other suggestions...

It is someone else's turn to provide a suggestion.  ;-)

Thanx, Paul

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Prarit Bhargava]

On 08/08/2017 04:28 AM, Peter Zijlstra wrote:
> On Mon, Aug 07, 2017 at 01:36:39PM -0700, Paul E. McKenney wrote:
>> On Mon, Aug 07, 2017 at 04:06:09PM -0400, Prarit Bhargava wrote:
> 
>>> peterz?  Want to offer a suggestion?  The issue is that I'm changing a bool
>>> config option to an int and that impacts all the arch's defconfigs.  John 
>>> points
>>> out that this is a lot of churn and we're both wondering if there's a 
>>> better way
>>> to do the configs.
>>
>> The usual approach is to keep the old bool Kconfig option, and add another
>> int Kconfig option that depends on the original one.  The tests for
>> the int value get a bit more complex, but one way to handle this is to
>> define a cpp macro something like the following:
>>
>> #ifdef CONFIG_OLD_OPTION
>> #define CPP_NEW_OPTION 0
>> #else
>> #define CPP_NEW_OPTION CONFIG_NEW_OPTION
>> #endif
>>
>> Then use CPP_NEW_OPTION, where zero means disabled and other numbers
>> select the available options.
>>
>> Adjust to suit depending on what values mean what.
>>
>> Another approach is to make the range of the new Kconfig option
>> depend on the old option:
>>
>> config NEW_OPTION
>>  int "your description here"
>>  range 1 5 if OLD_OPTION
>>  range 0 0 if !OLD_OPTION
>>  default 0
>>  help
>>your help here
>>
>> Again, adjust to suit depending on what values mean what.
> 
> Right this. Except I don't see the !OLD_OPTION working as expected.
> A 'new' config will not include the old one, so the !OLD_OPTION thing
> will 'always' be true.
> 
> So your:
> 
>> @@ -1,8 +1,46 @@
>>  menu "printk and dmesg options"
>>
>> +choice
>> +   prompt "printk default clock"
>> +   config PRINTK_TIME_DISABLE
>> +   bool "Disabled"
>> +   help
>> +Selecting this option disables the time stamps of printk().
>> +
>> +   config PRINTK_TIME_LOCAL
>> +   bool "Local Clock"
>> +   help
>> + Selecting this option causes the time stamps of printk() to be
>> + stamped with the unadjusted hardware clock.
>> +
>> +   config PRINTK_TIME_BOOT
>> +   bool "CLOCK_BOOTTIME"
>> +   help
>> + Selecting this option causes the time stamps of printk() to be
>> + stamped with the adjusted boottime clock.
>> +
>> +   config PRINTK_TIME_MONO
>> +   bool "CLOCK_MONOTONIC"
>> +   help
>> + Selecting this option causes the time stamps of printk() to be
>> + stamped with the adjusted monotonic clock.
>> +
>> +   config PRINTK_TIME_REAL
>> +   bool "CLOCK_REALTIME"
>> +   help
>> + Selecting this option causes the time stamps of printk() to be
>> + stamped with the adjusted realtime clock.
>> +
>> +endchoice
>> +
>>  config PRINTK_TIME
> 
> Change that into something like:
> 
> config PRINTK_CLOCK
> 
> 
>> -   bool "Show timing information on printks"
>> +   int "Show time stamp information on printks"
>> depends on PRINTK
>> +   default 0 if PRINTK_TIME_DISABLE
>> +   default 1 if PRINTK_TIME_LOCAL
> 
> And that into:
> 
>   default 1 if PRINTK_TIME_LOCAL || PRINTK_TIME
> 
>> +   default 2 if PRINTK_TIME_BOOT
>> +   default 3 if PRINTK_TIME_MONO
>> +   default 4 if PRINTK_TIME_REAL
>>help
>>  Selecting this option causes time stamps of the printk()
> 
> Then the old PRINTK_TIME symbol will auto-convert into the new
> equivalent.
> 

I don't think there's an easy code way around this.  Essentially this Kconfig
code boils down to properly evaluating

config PRINTK_CLOCK
default 1 if PRINTK_TIME
default 0

where there is no Kconfig entry for PRINTK_TIME.

If undefined CONFIG_PRINTK_TIME is used in a config, it is immediately
scrubbed by the kconfig script so it doesn't "exist" when CONFIG_PRINTK_CLOCK
is evaluated.  The result of that is CONFIG_PRINT_CLOCK=0.

I tried

config PRINTK_TIME
bool "old config option"

then I end up with both a CONFIG_PRINTK_CLOCK=1 and a CONFIG_PRINTK_TIME=y in
the resulting config which is confusing.

I've debated using the other suggestion that Paul made but TBH (sorry
Paul) it seems like I'm avoiding the real but noisy solution of

s/PRINTK_TIME=y/PRINTK_TIME=1/g

I'm obviously open to other suggestions...

P.

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Prarit Bhargava]

On 08/08/2017 04:28 AM, Peter Zijlstra wrote:
> On Mon, Aug 07, 2017 at 01:36:39PM -0700, Paul E. McKenney wrote:
>> On Mon, Aug 07, 2017 at 04:06:09PM -0400, Prarit Bhargava wrote:
> 
>>> peterz?  Want to offer a suggestion?  The issue is that I'm changing a bool
>>> config option to an int and that impacts all the arch's defconfigs.  John 
>>> points
>>> out that this is a lot of churn and we're both wondering if there's a 
>>> better way
>>> to do the configs.
>>
>> The usual approach is to keep the old bool Kconfig option, and add another
>> int Kconfig option that depends on the original one.  The tests for
>> the int value get a bit more complex, but one way to handle this is to
>> define a cpp macro something like the following:
>>
>> #ifdef CONFIG_OLD_OPTION
>> #define CPP_NEW_OPTION 0
>> #else
>> #define CPP_NEW_OPTION CONFIG_NEW_OPTION
>> #endif
>>
>> Then use CPP_NEW_OPTION, where zero means disabled and other numbers
>> select the available options.
>>
>> Adjust to suit depending on what values mean what.
>>
>> Another approach is to make the range of the new Kconfig option
>> depend on the old option:
>>
>> config NEW_OPTION
>>  int "your description here"
>>  range 1 5 if OLD_OPTION
>>  range 0 0 if !OLD_OPTION
>>  default 0
>>  help
>>your help here
>>
>> Again, adjust to suit depending on what values mean what.
> 
> Right this. Except I don't see the !OLD_OPTION working as expected.
> A 'new' config will not include the old one, so the !OLD_OPTION thing
> will 'always' be true.
> 
> So your:
> 
>> @@ -1,8 +1,46 @@
>>  menu "printk and dmesg options"
>>
>> +choice
>> +   prompt "printk default clock"
>> +   config PRINTK_TIME_DISABLE
>> +   bool "Disabled"
>> +   help
>> +Selecting this option disables the time stamps of printk().
>> +
>> +   config PRINTK_TIME_LOCAL
>> +   bool "Local Clock"
>> +   help
>> + Selecting this option causes the time stamps of printk() to be
>> + stamped with the unadjusted hardware clock.
>> +
>> +   config PRINTK_TIME_BOOT
>> +   bool "CLOCK_BOOTTIME"
>> +   help
>> + Selecting this option causes the time stamps of printk() to be
>> + stamped with the adjusted boottime clock.
>> +
>> +   config PRINTK_TIME_MONO
>> +   bool "CLOCK_MONOTONIC"
>> +   help
>> + Selecting this option causes the time stamps of printk() to be
>> + stamped with the adjusted monotonic clock.
>> +
>> +   config PRINTK_TIME_REAL
>> +   bool "CLOCK_REALTIME"
>> +   help
>> + Selecting this option causes the time stamps of printk() to be
>> + stamped with the adjusted realtime clock.
>> +
>> +endchoice
>> +
>>  config PRINTK_TIME
> 
> Change that into something like:
> 
> config PRINTK_CLOCK
> 
> 
>> -   bool "Show timing information on printks"
>> +   int "Show time stamp information on printks"
>> depends on PRINTK
>> +   default 0 if PRINTK_TIME_DISABLE
>> +   default 1 if PRINTK_TIME_LOCAL
> 
> And that into:
> 
>   default 1 if PRINTK_TIME_LOCAL || PRINTK_TIME
> 
>> +   default 2 if PRINTK_TIME_BOOT
>> +   default 3 if PRINTK_TIME_MONO
>> +   default 4 if PRINTK_TIME_REAL
>>help
>>  Selecting this option causes time stamps of the printk()
> 
> Then the old PRINTK_TIME symbol will auto-convert into the new
> equivalent.
> 

I don't think there's an easy code way around this.  Essentially this Kconfig
code boils down to properly evaluating

config PRINTK_CLOCK
default 1 if PRINTK_TIME
default 0

where there is no Kconfig entry for PRINTK_TIME.

If undefined CONFIG_PRINTK_TIME is used in a config, it is immediately
scrubbed by the kconfig script so it doesn't "exist" when CONFIG_PRINTK_CLOCK
is evaluated.  The result of that is CONFIG_PRINT_CLOCK=0.

I tried

config PRINTK_TIME
bool "old config option"

then I end up with both a CONFIG_PRINTK_CLOCK=1 and a CONFIG_PRINTK_TIME=y in
the resulting config which is confusing.

I've debated using the other suggestion that Paul made but TBH (sorry
Paul) it seems like I'm avoiding the real but noisy solution of

s/PRINTK_TIME=y/PRINTK_TIME=1/g

I'm obviously open to other suggestions...

P.

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Prarit Bhargava]

On 08/07/2017 08:19 PM, Sergey Senozhatsky wrote:
> On (08/07/17 11:52), Prarit Bhargava wrote:



> [..]
>> +if (strlen(param) == 1) {
>> +/* Preserve legacy boolean settings */
>> +if (!strcmp("0", param) || !strcmp("n", param) ||
>> +!strcmp("N", param))
>> +_printk_time = PRINTK_TIME_DISABLE;
>> +if (!strcmp("1", param) || !strcmp("y", param) ||
>> +!strcmp("Y", param))
>> +_printk_time = PRINTK_TIME_LOCAL;
>> +}
>> +if (_printk_time == -1) {
>> +for (stamp = 0; stamp <= 4; stamp++) {
>> +if (!strncmp(printk_time_str[stamp], param,
>> + strlen(param))) {
>> +_printk_time = stamp;
>> +break;
>> +}
>> +}
>> +}
> 
>   you can use match_string() here.

match_string doesn't match correctly.  Others have made requests for, for
example, printk.time=r printk.time=real, etc.  Instead of butchering the code
for a bunch of strings I chose to allow partial matching and simplify the code.

P.

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Prarit Bhargava]

On 08/07/2017 08:19 PM, Sergey Senozhatsky wrote:
> On (08/07/17 11:52), Prarit Bhargava wrote:



> [..]
>> +if (strlen(param) == 1) {
>> +/* Preserve legacy boolean settings */
>> +if (!strcmp("0", param) || !strcmp("n", param) ||
>> +!strcmp("N", param))
>> +_printk_time = PRINTK_TIME_DISABLE;
>> +if (!strcmp("1", param) || !strcmp("y", param) ||
>> +!strcmp("Y", param))
>> +_printk_time = PRINTK_TIME_LOCAL;
>> +}
>> +if (_printk_time == -1) {
>> +for (stamp = 0; stamp <= 4; stamp++) {
>> +if (!strncmp(printk_time_str[stamp], param,
>> + strlen(param))) {
>> +_printk_time = stamp;
>> +break;
>> +}
>> +}
>> +}
> 
>   you can use match_string() here.

match_string doesn't match correctly.  Others have made requests for, for
example, printk.time=r printk.time=real, etc.  Instead of butchering the code
for a bunch of strings I chose to allow partial matching and simplify the code.

P.

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Prarit Bhargava]

On 08/07/2017 08:19 PM, Sergey Senozhatsky wrote:
> On (08/07/17 11:52), Prarit Bhargava wrote:
> [..]
>> +/**
>> + * enum printk_time_type - Timestamp types for printk() messages.
>> + * @PRINTK_TIME_DISABLE: No time stamp.
>> + * @PRINTK_TIME_LOCAL: Local hardware clock timestamp.
>> + * @PRINTK_TIME_BOOT: Boottime clock timestamp.
>> + * @PRINTK_TIME_MONO: Monotonic clock timestamp.
>> + * @PRINTK_TIME_REAL: Realtime clock timestamp.  On 32-bit
>> + * systems selecting the real clock printk timestamp may lead to unlikely
>> + * situations where a timestamp is wrong because the real time offset is 
>> read
>> + * without the protection of a sequence lock in the call to 
>> ktime_get_log_ts()
>> + * in printk_get_ts() below.
>> + */
>> +enum printk_time_type {
>> +PRINTK_TIME_DISABLE = 0,
>> +PRINTK_TIME_LOCAL = 1,
>> +PRINTK_TIME_BOOT = 2,
>> +PRINTK_TIME_MONO = 3,
>> +PRINTK_TIME_REAL = 4,
>> +};
> 
> may be call the entire thing 'timestamp surces' or something?

Done.

> 
> [..]
>> +if (strlen(param) == 1) {
>> +/* Preserve legacy boolean settings */
>> +if (!strcmp("0", param) || !strcmp("n", param) ||
>> +!strcmp("N", param))
>> +_printk_time = PRINTK_TIME_DISABLE;
>> +if (!strcmp("1", param) || !strcmp("y", param) ||
>> +!strcmp("Y", param))
>> +_printk_time = PRINTK_TIME_LOCAL;
>> +}
>> +if (_printk_time == -1) {
>> +for (stamp = 0; stamp <= 4; stamp++) {
>> +if (!strncmp(printk_time_str[stamp], param,
>> + strlen(param))) {
>> +_printk_time = stamp;
>> +break;
>> +}
>> +}
>> +}
> 
>   you can use match_string() here.
> 
>> +if (_printk_time == -1) {
>> +pr_warn("printk: invalid timestamp value %s\n", param);
>> +return -EINVAL;
>> +}
> 
> `invalid timestamp value' is confusing.

I can't think of anything better than 'invalid timestamp option'.  If you find
that confusing, please explain why.

> 
> 
>> +} else if ((printk_time_setting != _printk_time) &&
>> +   (_printk_time != 0)) {
>> +pr_warn("printk: timestamp can only be set to 0(disabled) or 
>> %s\n",
>> +printk_time_str[printk_time_setting]);
> 
> ditto.

Changed to 'timestamp can only e set to 0, disabled, or '.

> 
> 
>> +return -EINVAL;
>> +}
>> +
>> +printk_time = _printk_time;
>> +pr_info("printk: timestamp set to %s\n", printk_time_str[printk_time]);
> 
> ditto.

I don't find this confusing _at all_.  Please offer an explanation of why you
think that message is confusing.

> 
> 
> [..]
> 
>> +static u64 printk_get_ts(void)
>> +{
>> +u64 mono, offset_real;
>> +
>> +if (printk_time <= PRINTK_TIME_LOCAL)
>> +return local_clock();
>> +
>> +if (printk_time == PRINTK_TIME_BOOT)
>> +return ktime_get_boot_log_ts();
>> +
>> +mono = ktime_get_real_log_ts(_real);
>> +
>> +if (printk_time == PRINTK_TIME_MONO)
>> +return mono;
>> +
>> +return mono + offset_real;
>> +}
> 
> this looks hard...

See previous suggestion from peterz to switch to a fn ptr.

> 
>> +static int printk_time;
>> +static int printk_time_setting;
> 
> how about s/printk_time_setting/printk_time_source/? or something similar?

Sure.

P.

> 
>   -ss
> 

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Prarit Bhargava]

On 08/07/2017 08:19 PM, Sergey Senozhatsky wrote:
> On (08/07/17 11:52), Prarit Bhargava wrote:
> [..]
>> +/**
>> + * enum printk_time_type - Timestamp types for printk() messages.
>> + * @PRINTK_TIME_DISABLE: No time stamp.
>> + * @PRINTK_TIME_LOCAL: Local hardware clock timestamp.
>> + * @PRINTK_TIME_BOOT: Boottime clock timestamp.
>> + * @PRINTK_TIME_MONO: Monotonic clock timestamp.
>> + * @PRINTK_TIME_REAL: Realtime clock timestamp.  On 32-bit
>> + * systems selecting the real clock printk timestamp may lead to unlikely
>> + * situations where a timestamp is wrong because the real time offset is 
>> read
>> + * without the protection of a sequence lock in the call to 
>> ktime_get_log_ts()
>> + * in printk_get_ts() below.
>> + */
>> +enum printk_time_type {
>> +PRINTK_TIME_DISABLE = 0,
>> +PRINTK_TIME_LOCAL = 1,
>> +PRINTK_TIME_BOOT = 2,
>> +PRINTK_TIME_MONO = 3,
>> +PRINTK_TIME_REAL = 4,
>> +};
> 
> may be call the entire thing 'timestamp surces' or something?

Done.

> 
> [..]
>> +if (strlen(param) == 1) {
>> +/* Preserve legacy boolean settings */
>> +if (!strcmp("0", param) || !strcmp("n", param) ||
>> +!strcmp("N", param))
>> +_printk_time = PRINTK_TIME_DISABLE;
>> +if (!strcmp("1", param) || !strcmp("y", param) ||
>> +!strcmp("Y", param))
>> +_printk_time = PRINTK_TIME_LOCAL;
>> +}
>> +if (_printk_time == -1) {
>> +for (stamp = 0; stamp <= 4; stamp++) {
>> +if (!strncmp(printk_time_str[stamp], param,
>> + strlen(param))) {
>> +_printk_time = stamp;
>> +break;
>> +}
>> +}
>> +}
> 
>   you can use match_string() here.
> 
>> +if (_printk_time == -1) {
>> +pr_warn("printk: invalid timestamp value %s\n", param);
>> +return -EINVAL;
>> +}
> 
> `invalid timestamp value' is confusing.

I can't think of anything better than 'invalid timestamp option'.  If you find
that confusing, please explain why.

> 
> 
>> +} else if ((printk_time_setting != _printk_time) &&
>> +   (_printk_time != 0)) {
>> +pr_warn("printk: timestamp can only be set to 0(disabled) or 
>> %s\n",
>> +printk_time_str[printk_time_setting]);
> 
> ditto.

Changed to 'timestamp can only e set to 0, disabled, or '.

> 
> 
>> +return -EINVAL;
>> +}
>> +
>> +printk_time = _printk_time;
>> +pr_info("printk: timestamp set to %s\n", printk_time_str[printk_time]);
> 
> ditto.

I don't find this confusing _at all_.  Please offer an explanation of why you
think that message is confusing.

> 
> 
> [..]
> 
>> +static u64 printk_get_ts(void)
>> +{
>> +u64 mono, offset_real;
>> +
>> +if (printk_time <= PRINTK_TIME_LOCAL)
>> +return local_clock();
>> +
>> +if (printk_time == PRINTK_TIME_BOOT)
>> +return ktime_get_boot_log_ts();
>> +
>> +mono = ktime_get_real_log_ts(_real);
>> +
>> +if (printk_time == PRINTK_TIME_MONO)
>> +return mono;
>> +
>> +return mono + offset_real;
>> +}
> 
> this looks hard...

See previous suggestion from peterz to switch to a fn ptr.

> 
>> +static int printk_time;
>> +static int printk_time_setting;
> 
> how about s/printk_time_setting/printk_time_source/? or something similar?

Sure.

P.

> 
>   -ss
> 

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Peter Zijlstra]

On Mon, Aug 07, 2017 at 01:36:39PM -0700, Paul E. McKenney wrote:
> On Mon, Aug 07, 2017 at 04:06:09PM -0400, Prarit Bhargava wrote:

> > peterz?  Want to offer a suggestion?  The issue is that I'm changing a bool
> > config option to an int and that impacts all the arch's defconfigs.  John 
> > points
> > out that this is a lot of churn and we're both wondering if there's a 
> > better way
> > to do the configs.
> 
> The usual approach is to keep the old bool Kconfig option, and add another
> int Kconfig option that depends on the original one.  The tests for
> the int value get a bit more complex, but one way to handle this is to
> define a cpp macro something like the following:
> 
> #ifdef CONFIG_OLD_OPTION
> #define CPP_NEW_OPTION 0
> #else
> #define CPP_NEW_OPTION CONFIG_NEW_OPTION
> #endif
> 
> Then use CPP_NEW_OPTION, where zero means disabled and other numbers
> select the available options.
> 
> Adjust to suit depending on what values mean what.
> 
> Another approach is to make the range of the new Kconfig option
> depend on the old option:
> 
> config NEW_OPTION
>   int "your description here"
>   range 1 5 if OLD_OPTION
>   range 0 0 if !OLD_OPTION
>   default 0
>   help
> your help here
> 
> Again, adjust to suit depending on what values mean what.

Right this. Except I don't see the !OLD_OPTION working as expected.
A 'new' config will not include the old one, so the !OLD_OPTION thing
will 'always' be true.

So your:

> @@ -1,8 +1,46 @@
>  menu "printk and dmesg options"
>
> +choice
> +   prompt "printk default clock"
> +   config PRINTK_TIME_DISABLE
> +   bool "Disabled"
> +   help
> +Selecting this option disables the time stamps of printk().
> +
> +   config PRINTK_TIME_LOCAL
> +   bool "Local Clock"
> +   help
> + Selecting this option causes the time stamps of printk() to be
> + stamped with the unadjusted hardware clock.
> +
> +   config PRINTK_TIME_BOOT
> +   bool "CLOCK_BOOTTIME"
> +   help
> + Selecting this option causes the time stamps of printk() to be
> + stamped with the adjusted boottime clock.
> +
> +   config PRINTK_TIME_MONO
> +   bool "CLOCK_MONOTONIC"
> +   help
> + Selecting this option causes the time stamps of printk() to be
> + stamped with the adjusted monotonic clock.
> +
> +   config PRINTK_TIME_REAL
> +   bool "CLOCK_REALTIME"
> +   help
> + Selecting this option causes the time stamps of printk() to be
> + stamped with the adjusted realtime clock.
> +
> +endchoice
> +
>  config PRINTK_TIME

Change that into something like:

config PRINTK_CLOCK


> -   bool "Show timing information on printks"
> +   int "Show time stamp information on printks"
> depends on PRINTK
> +   default 0 if PRINTK_TIME_DISABLE
> +   default 1 if PRINTK_TIME_LOCAL

And that into:

default 1 if PRINTK_TIME_LOCAL || PRINTK_TIME

> +   default 2 if PRINTK_TIME_BOOT
> +   default 3 if PRINTK_TIME_MONO
> +   default 4 if PRINTK_TIME_REAL
> help
>   Selecting this option causes time stamps of the printk()

Then the old PRINTK_TIME symbol will auto-convert into the new
equivalent.

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Peter Zijlstra]

On Mon, Aug 07, 2017 at 01:36:39PM -0700, Paul E. McKenney wrote:
> On Mon, Aug 07, 2017 at 04:06:09PM -0400, Prarit Bhargava wrote:

> > peterz?  Want to offer a suggestion?  The issue is that I'm changing a bool
> > config option to an int and that impacts all the arch's defconfigs.  John 
> > points
> > out that this is a lot of churn and we're both wondering if there's a 
> > better way
> > to do the configs.
> 
> The usual approach is to keep the old bool Kconfig option, and add another
> int Kconfig option that depends on the original one.  The tests for
> the int value get a bit more complex, but one way to handle this is to
> define a cpp macro something like the following:
> 
> #ifdef CONFIG_OLD_OPTION
> #define CPP_NEW_OPTION 0
> #else
> #define CPP_NEW_OPTION CONFIG_NEW_OPTION
> #endif
> 
> Then use CPP_NEW_OPTION, where zero means disabled and other numbers
> select the available options.
> 
> Adjust to suit depending on what values mean what.
> 
> Another approach is to make the range of the new Kconfig option
> depend on the old option:
> 
> config NEW_OPTION
>   int "your description here"
>   range 1 5 if OLD_OPTION
>   range 0 0 if !OLD_OPTION
>   default 0
>   help
> your help here
> 
> Again, adjust to suit depending on what values mean what.

Right this. Except I don't see the !OLD_OPTION working as expected.
A 'new' config will not include the old one, so the !OLD_OPTION thing
will 'always' be true.

So your:

> @@ -1,8 +1,46 @@
>  menu "printk and dmesg options"
>
> +choice
> +   prompt "printk default clock"
> +   config PRINTK_TIME_DISABLE
> +   bool "Disabled"
> +   help
> +Selecting this option disables the time stamps of printk().
> +
> +   config PRINTK_TIME_LOCAL
> +   bool "Local Clock"
> +   help
> + Selecting this option causes the time stamps of printk() to be
> + stamped with the unadjusted hardware clock.
> +
> +   config PRINTK_TIME_BOOT
> +   bool "CLOCK_BOOTTIME"
> +   help
> + Selecting this option causes the time stamps of printk() to be
> + stamped with the adjusted boottime clock.
> +
> +   config PRINTK_TIME_MONO
> +   bool "CLOCK_MONOTONIC"
> +   help
> + Selecting this option causes the time stamps of printk() to be
> + stamped with the adjusted monotonic clock.
> +
> +   config PRINTK_TIME_REAL
> +   bool "CLOCK_REALTIME"
> +   help
> + Selecting this option causes the time stamps of printk() to be
> + stamped with the adjusted realtime clock.
> +
> +endchoice
> +
>  config PRINTK_TIME

Change that into something like:

config PRINTK_CLOCK


> -   bool "Show timing information on printks"
> +   int "Show time stamp information on printks"
> depends on PRINTK
> +   default 0 if PRINTK_TIME_DISABLE
> +   default 1 if PRINTK_TIME_LOCAL

And that into:

default 1 if PRINTK_TIME_LOCAL || PRINTK_TIME

> +   default 2 if PRINTK_TIME_BOOT
> +   default 3 if PRINTK_TIME_MONO
> +   default 4 if PRINTK_TIME_REAL
> help
>   Selecting this option causes time stamps of the printk()

Then the old PRINTK_TIME symbol will auto-convert into the new
equivalent.

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Sergey Senozhatsky]

On (08/07/17 11:52), Prarit Bhargava wrote:
[..]
> +/**
> + * enum printk_time_type - Timestamp types for printk() messages.
> + * @PRINTK_TIME_DISABLE: No time stamp.
> + * @PRINTK_TIME_LOCAL: Local hardware clock timestamp.
> + * @PRINTK_TIME_BOOT: Boottime clock timestamp.
> + * @PRINTK_TIME_MONO: Monotonic clock timestamp.
> + * @PRINTK_TIME_REAL: Realtime clock timestamp.  On 32-bit
> + * systems selecting the real clock printk timestamp may lead to unlikely
> + * situations where a timestamp is wrong because the real time offset is read
> + * without the protection of a sequence lock in the call to 
> ktime_get_log_ts()
> + * in printk_get_ts() below.
> + */
> +enum printk_time_type {
> + PRINTK_TIME_DISABLE = 0,
> + PRINTK_TIME_LOCAL = 1,
> + PRINTK_TIME_BOOT = 2,
> + PRINTK_TIME_MONO = 3,
> + PRINTK_TIME_REAL = 4,
> +};

may be call the entire thing 'timestamp surces' or something?

[..]
> + if (strlen(param) == 1) {
> + /* Preserve legacy boolean settings */
> + if (!strcmp("0", param) || !strcmp("n", param) ||
> + !strcmp("N", param))
> + _printk_time = PRINTK_TIME_DISABLE;
> + if (!strcmp("1", param) || !strcmp("y", param) ||
> + !strcmp("Y", param))
> + _printk_time = PRINTK_TIME_LOCAL;
> + }
> + if (_printk_time == -1) {
> + for (stamp = 0; stamp <= 4; stamp++) {
> + if (!strncmp(printk_time_str[stamp], param,
> +  strlen(param))) {
> + _printk_time = stamp;
> + break;
> + }
> + }
> + }

you can use match_string() here.

> + if (_printk_time == -1) {
> + pr_warn("printk: invalid timestamp value %s\n", param);
> + return -EINVAL;
> + }

`invalid timestamp value' is confusing.


> + } else if ((printk_time_setting != _printk_time) &&
> +(_printk_time != 0)) {
> + pr_warn("printk: timestamp can only be set to 0(disabled) or 
> %s\n",
> + printk_time_str[printk_time_setting]);

ditto.


> + return -EINVAL;
> + }
> +
> + printk_time = _printk_time;
> + pr_info("printk: timestamp set to %s\n", printk_time_str[printk_time]);

ditto.


[..]

> +static u64 printk_get_ts(void)
> +{
> + u64 mono, offset_real;
> +
> + if (printk_time <= PRINTK_TIME_LOCAL)
> + return local_clock();
> +
> + if (printk_time == PRINTK_TIME_BOOT)
> + return ktime_get_boot_log_ts();
> +
> + mono = ktime_get_real_log_ts(_real);
> +
> + if (printk_time == PRINTK_TIME_MONO)
> + return mono;
> +
> + return mono + offset_real;
> +}

this looks hard...

> +static int printk_time;
> +static int printk_time_setting;

how about s/printk_time_setting/printk_time_source/? or something similar?

-ss

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Sergey Senozhatsky]

On (08/07/17 11:52), Prarit Bhargava wrote:
[..]
> +/**
> + * enum printk_time_type - Timestamp types for printk() messages.
> + * @PRINTK_TIME_DISABLE: No time stamp.
> + * @PRINTK_TIME_LOCAL: Local hardware clock timestamp.
> + * @PRINTK_TIME_BOOT: Boottime clock timestamp.
> + * @PRINTK_TIME_MONO: Monotonic clock timestamp.
> + * @PRINTK_TIME_REAL: Realtime clock timestamp.  On 32-bit
> + * systems selecting the real clock printk timestamp may lead to unlikely
> + * situations where a timestamp is wrong because the real time offset is read
> + * without the protection of a sequence lock in the call to 
> ktime_get_log_ts()
> + * in printk_get_ts() below.
> + */
> +enum printk_time_type {
> + PRINTK_TIME_DISABLE = 0,
> + PRINTK_TIME_LOCAL = 1,
> + PRINTK_TIME_BOOT = 2,
> + PRINTK_TIME_MONO = 3,
> + PRINTK_TIME_REAL = 4,
> +};

may be call the entire thing 'timestamp surces' or something?

[..]
> + if (strlen(param) == 1) {
> + /* Preserve legacy boolean settings */
> + if (!strcmp("0", param) || !strcmp("n", param) ||
> + !strcmp("N", param))
> + _printk_time = PRINTK_TIME_DISABLE;
> + if (!strcmp("1", param) || !strcmp("y", param) ||
> + !strcmp("Y", param))
> + _printk_time = PRINTK_TIME_LOCAL;
> + }
> + if (_printk_time == -1) {
> + for (stamp = 0; stamp <= 4; stamp++) {
> + if (!strncmp(printk_time_str[stamp], param,
> +  strlen(param))) {
> + _printk_time = stamp;
> + break;
> + }
> + }
> + }

you can use match_string() here.

> + if (_printk_time == -1) {
> + pr_warn("printk: invalid timestamp value %s\n", param);
> + return -EINVAL;
> + }

`invalid timestamp value' is confusing.


> + } else if ((printk_time_setting != _printk_time) &&
> +(_printk_time != 0)) {
> + pr_warn("printk: timestamp can only be set to 0(disabled) or 
> %s\n",
> + printk_time_str[printk_time_setting]);

ditto.


> + return -EINVAL;
> + }
> +
> + printk_time = _printk_time;
> + pr_info("printk: timestamp set to %s\n", printk_time_str[printk_time]);

ditto.


[..]

> +static u64 printk_get_ts(void)
> +{
> + u64 mono, offset_real;
> +
> + if (printk_time <= PRINTK_TIME_LOCAL)
> + return local_clock();
> +
> + if (printk_time == PRINTK_TIME_BOOT)
> + return ktime_get_boot_log_ts();
> +
> + mono = ktime_get_real_log_ts(_real);
> +
> + if (printk_time == PRINTK_TIME_MONO)
> + return mono;
> +
> + return mono + offset_real;
> +}

this looks hard...

> +static int printk_time;
> +static int printk_time_setting;

how about s/printk_time_setting/printk_time_source/? or something similar?

-ss

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Paul E. McKenney]

On Mon, Aug 07, 2017 at 04:06:09PM -0400, Prarit Bhargava wrote:
> 
> 
> On 08/07/2017 02:47 PM, John Stultz wrote:
> > On Mon, Aug 7, 2017 at 11:04 AM, Prarit Bhargava  wrote:
> >> On 08/07/2017 12:52 PM, John Stultz wrote:
> >>> Still not quite following why you're updating all the defconfigs. I'd
> >>> make sure the Kconfig default settings are right, and leave updating
> >>> the defconfig to arch/device maintainers. It adds a lot of noise to
> >>> the patch.
> >>
> >> Hmm ... I thought it was up to the patch submitter to make sure that
> >> 'make defconfig' still worked?  Are you sure I can leave that broken?
> >>
> >> /me *really* doesn't want to get yelled at by every arch maintainer.
> > 
> > No. Don't break systems, but at the same time, can't you use the
> > default value in Kconfig to set it properly so the old defconfig
> > settings don't really matter?
> > 
> > Apologies if I've not followed the issue properly, but it is odd, as
> > I'm not sure I can think of a patch I've seen before that had so much
> > defconfig noise in it.  Again, I've not looked into it closely, so it
> > may just be my own ignorance, but it makes me suspect there is a
> > better way.
> > 
> 
> peterz?  Want to offer a suggestion?  The issue is that I'm changing a bool
> config option to an int and that impacts all the arch's defconfigs.  John 
> points
> out that this is a lot of churn and we're both wondering if there's a better 
> way
> to do the configs.

The usual approach is to keep the old bool Kconfig option, and add another
int Kconfig option that depends on the original one.  The tests for
the int value get a bit more complex, but one way to handle this is to
define a cpp macro something like the following:

#ifdef CONFIG_OLD_OPTION
#define CPP_NEW_OPTION 0
#else
#define CPP_NEW_OPTION CONFIG_NEW_OPTION
#endif

Then use CPP_NEW_OPTION, where zero means disabled and other numbers
select the available options.

Adjust to suit depending on what values mean what.

Another approach is to make the range of the new Kconfig option
depend on the old option:

config NEW_OPTION
int "your description here"
range 1 5 if OLD_OPTION
range 0 0 if !OLD_OPTION
default 0
help
  your help here

Again, adjust to suit depending on what values mean what.

Thanx, Paul

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Paul E. McKenney]

On Mon, Aug 07, 2017 at 04:06:09PM -0400, Prarit Bhargava wrote:
> 
> 
> On 08/07/2017 02:47 PM, John Stultz wrote:
> > On Mon, Aug 7, 2017 at 11:04 AM, Prarit Bhargava  wrote:
> >> On 08/07/2017 12:52 PM, John Stultz wrote:
> >>> Still not quite following why you're updating all the defconfigs. I'd
> >>> make sure the Kconfig default settings are right, and leave updating
> >>> the defconfig to arch/device maintainers. It adds a lot of noise to
> >>> the patch.
> >>
> >> Hmm ... I thought it was up to the patch submitter to make sure that
> >> 'make defconfig' still worked?  Are you sure I can leave that broken?
> >>
> >> /me *really* doesn't want to get yelled at by every arch maintainer.
> > 
> > No. Don't break systems, but at the same time, can't you use the
> > default value in Kconfig to set it properly so the old defconfig
> > settings don't really matter?
> > 
> > Apologies if I've not followed the issue properly, but it is odd, as
> > I'm not sure I can think of a patch I've seen before that had so much
> > defconfig noise in it.  Again, I've not looked into it closely, so it
> > may just be my own ignorance, but it makes me suspect there is a
> > better way.
> > 
> 
> peterz?  Want to offer a suggestion?  The issue is that I'm changing a bool
> config option to an int and that impacts all the arch's defconfigs.  John 
> points
> out that this is a lot of churn and we're both wondering if there's a better 
> way
> to do the configs.

The usual approach is to keep the old bool Kconfig option, and add another
int Kconfig option that depends on the original one.  The tests for
the int value get a bit more complex, but one way to handle this is to
define a cpp macro something like the following:

#ifdef CONFIG_OLD_OPTION
#define CPP_NEW_OPTION 0
#else
#define CPP_NEW_OPTION CONFIG_NEW_OPTION
#endif

Then use CPP_NEW_OPTION, where zero means disabled and other numbers
select the available options.

Adjust to suit depending on what values mean what.

Another approach is to make the range of the new Kconfig option
depend on the old option:

config NEW_OPTION
int "your description here"
range 1 5 if OLD_OPTION
range 0 0 if !OLD_OPTION
default 0
help
  your help here

Again, adjust to suit depending on what values mean what.

Thanx, Paul

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Prarit Bhargava]

On 08/07/2017 02:47 PM, John Stultz wrote:
> On Mon, Aug 7, 2017 at 11:04 AM, Prarit Bhargava  wrote:
>> On 08/07/2017 12:52 PM, John Stultz wrote:
>>> Still not quite following why you're updating all the defconfigs. I'd
>>> make sure the Kconfig default settings are right, and leave updating
>>> the defconfig to arch/device maintainers. It adds a lot of noise to
>>> the patch.
>>
>> Hmm ... I thought it was up to the patch submitter to make sure that
>> 'make defconfig' still worked?  Are you sure I can leave that broken?
>>
>> /me *really* doesn't want to get yelled at by every arch maintainer.
> 
> No. Don't break systems, but at the same time, can't you use the
> default value in Kconfig to set it properly so the old defconfig
> settings don't really matter?
> 
> Apologies if I've not followed the issue properly, but it is odd, as
> I'm not sure I can think of a patch I've seen before that had so much
> defconfig noise in it.  Again, I've not looked into it closely, so it
> may just be my own ignorance, but it makes me suspect there is a
> better way.
> 

peterz?  Want to offer a suggestion?  The issue is that I'm changing a bool
config option to an int and that impacts all the arch's defconfigs.  John points
out that this is a lot of churn and we're both wondering if there's a better way
to do the configs.

P.

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Prarit Bhargava]

On 08/07/2017 02:47 PM, John Stultz wrote:
> On Mon, Aug 7, 2017 at 11:04 AM, Prarit Bhargava  wrote:
>> On 08/07/2017 12:52 PM, John Stultz wrote:
>>> Still not quite following why you're updating all the defconfigs. I'd
>>> make sure the Kconfig default settings are right, and leave updating
>>> the defconfig to arch/device maintainers. It adds a lot of noise to
>>> the patch.
>>
>> Hmm ... I thought it was up to the patch submitter to make sure that
>> 'make defconfig' still worked?  Are you sure I can leave that broken?
>>
>> /me *really* doesn't want to get yelled at by every arch maintainer.
> 
> No. Don't break systems, but at the same time, can't you use the
> default value in Kconfig to set it properly so the old defconfig
> settings don't really matter?
> 
> Apologies if I've not followed the issue properly, but it is odd, as
> I'm not sure I can think of a patch I've seen before that had so much
> defconfig noise in it.  Again, I've not looked into it closely, so it
> may just be my own ignorance, but it makes me suspect there is a
> better way.
> 

peterz?  Want to offer a suggestion?  The issue is that I'm changing a bool
config option to an int and that impacts all the arch's defconfigs.  John points
out that this is a lot of churn and we're both wondering if there's a better way
to do the configs.

P.

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[John Stultz]

On Mon, Aug 7, 2017 at 11:04 AM, Prarit Bhargava  wrote:
> On 08/07/2017 12:52 PM, John Stultz wrote:
>> Still not quite following why you're updating all the defconfigs. I'd
>> make sure the Kconfig default settings are right, and leave updating
>> the defconfig to arch/device maintainers. It adds a lot of noise to
>> the patch.
>
> Hmm ... I thought it was up to the patch submitter to make sure that
> 'make defconfig' still worked?  Are you sure I can leave that broken?
>
> /me *really* doesn't want to get yelled at by every arch maintainer.

No. Don't break systems, but at the same time, can't you use the
default value in Kconfig to set it properly so the old defconfig
settings don't really matter?

Apologies if I've not followed the issue properly, but it is odd, as
I'm not sure I can think of a patch I've seen before that had so much
defconfig noise in it.  Again, I've not looked into it closely, so it
may just be my own ignorance, but it makes me suspect there is a
better way.


>>> +u64 ktime_get_real_log_ts(u64 *offset_real)
>>> +{
>>> +   *offset_real = ktime_to_ns(tk_core.timekeeper.offs_real);
>>> +
>>> +   if (timekeeping_active)
>>> +   return ktime_get_mono_fast_ns();
>>> +   else
>>> +   return local_clock();
>>> +}
>>> +
>>> +u64 ktime_get_boot_log_ts(void)
>>> +{
>>> +   if (timekeeping_active)
>>> +   return ktime_get_boot_fast_ns();
>>> +   else
>>> +   return local_clock();
>>> +}
>>
>> This feels a little tacked on and duplicative.  I'd suggest having one
>> function that returns the offset_real and offset_boot or have a
>> separate get_mono_log_ts() so its at least consistent.
>
> I have a better suggestion that I was toying with -- exporting
> timekeeping_active and using the existing ktime_get_mono|boot|real|_fast_ns()
> functions with a function pointer would simplify this code.

Yea. That sounds cleaner.


>> Additionally,
>> in the commit message, you call out the lack of locking between
>> grabing the offs_real and calling get_mono_fast_ns(), but I worry it
>> may be particularly problematic on 32bit systems, and you don't have
>> any notes in the actual code about it (it looks like an oversight).
>>
>
> I was told to move that comment to the kdoc description by Luis R. Rodriguez.

You can have it both ways. :)  Its just without any mention near the
function, it just looks buggy (well, because it technically is), so
either we should fix it properly or at least document that its
intentionally buggy (which again, doesn't feel great here - we already
have lots of caveats around the _fast_ns() accessors, so this is
layering subtle breakage on top of other subtle behavior).


>> Also, when timekeeping_active flips over, and we change from local
>> clock to the specified clock, do we see a discontinuity in the log? I
>> know folks use to gripe about that back in the day.
>>
>
> I have tested this across many systems and haven't seen a discontinunity
> yet.  I've done both large and small cpu footprint systems and haven't seen
> anything like that.

Ok.

thanks
-john

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[John Stultz]

On Mon, Aug 7, 2017 at 11:04 AM, Prarit Bhargava  wrote:
> On 08/07/2017 12:52 PM, John Stultz wrote:
>> Still not quite following why you're updating all the defconfigs. I'd
>> make sure the Kconfig default settings are right, and leave updating
>> the defconfig to arch/device maintainers. It adds a lot of noise to
>> the patch.
>
> Hmm ... I thought it was up to the patch submitter to make sure that
> 'make defconfig' still worked?  Are you sure I can leave that broken?
>
> /me *really* doesn't want to get yelled at by every arch maintainer.

No. Don't break systems, but at the same time, can't you use the
default value in Kconfig to set it properly so the old defconfig
settings don't really matter?

Apologies if I've not followed the issue properly, but it is odd, as
I'm not sure I can think of a patch I've seen before that had so much
defconfig noise in it.  Again, I've not looked into it closely, so it
may just be my own ignorance, but it makes me suspect there is a
better way.


>>> +u64 ktime_get_real_log_ts(u64 *offset_real)
>>> +{
>>> +   *offset_real = ktime_to_ns(tk_core.timekeeper.offs_real);
>>> +
>>> +   if (timekeeping_active)
>>> +   return ktime_get_mono_fast_ns();
>>> +   else
>>> +   return local_clock();
>>> +}
>>> +
>>> +u64 ktime_get_boot_log_ts(void)
>>> +{
>>> +   if (timekeeping_active)
>>> +   return ktime_get_boot_fast_ns();
>>> +   else
>>> +   return local_clock();
>>> +}
>>
>> This feels a little tacked on and duplicative.  I'd suggest having one
>> function that returns the offset_real and offset_boot or have a
>> separate get_mono_log_ts() so its at least consistent.
>
> I have a better suggestion that I was toying with -- exporting
> timekeeping_active and using the existing ktime_get_mono|boot|real|_fast_ns()
> functions with a function pointer would simplify this code.

Yea. That sounds cleaner.


>> Additionally,
>> in the commit message, you call out the lack of locking between
>> grabing the offs_real and calling get_mono_fast_ns(), but I worry it
>> may be particularly problematic on 32bit systems, and you don't have
>> any notes in the actual code about it (it looks like an oversight).
>>
>
> I was told to move that comment to the kdoc description by Luis R. Rodriguez.

You can have it both ways. :)  Its just without any mention near the
function, it just looks buggy (well, because it technically is), so
either we should fix it properly or at least document that its
intentionally buggy (which again, doesn't feel great here - we already
have lots of caveats around the _fast_ns() accessors, so this is
layering subtle breakage on top of other subtle behavior).


>> Also, when timekeeping_active flips over, and we change from local
>> clock to the specified clock, do we see a discontinuity in the log? I
>> know folks use to gripe about that back in the day.
>>
>
> I have tested this across many systems and haven't seen a discontinunity
> yet.  I've done both large and small cpu footprint systems and haven't seen
> anything like that.

Ok.

thanks
-john

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Prarit Bhargava]

On 08/07/2017 12:58 PM, Mark Salyzyn wrote:
> On 08/07/2017 08:52 AM, Prarit Bhargava wrote:
>> diff --git a/arch/arm/configs/aspeed_g4_defconfig
>> b/arch/arm/configs/aspeed_g4_defconfig
>> index cfc2465e8b77..5f3c50914e92 100644
>> --- a/arch/arm/configs/aspeed_g4_defconfig
>> +++ b/arch/arm/configs/aspeed_g4_defconfig
>> @@ -162,7 +162,7 @@ CONFIG_JFFS2_FS_XATTR=y
>>   CONFIG_UBIFS_FS=y
>>   CONFIG_SQUASHFS=y
>>   CONFIG_SQUASHFS_XZ=y
>> -CONFIG_PRINTK_TIME=y
>> +CONFIG_PRINTK_TIME_LOCAL=y
>>   CONFIG_DYNAMIC_DEBUG=y
>>   CONFIG_STRIP_ASM_SYMS=y
>>   CONFIG_DEBUG_FS=y
> Many have had misgivings, let me try another pass at this.
> 
> We (royal we) should really look into adjusting configuration parsing to allow
> an easy transition from boolean to selection ... I am sure this is not the 
> first
> time bistate/tristate was moved to a number.
> 
> An idea? Maybe look into a way to deal with this to use something _other_ than
> CONFIG_PRINTK_TIME to hold the selection, and keep a (hidden/legacy?)
> CONFIG_PRINTK_TIME that when selected sets CONFIG_PRINTK_TIME_LOCAL, and 
> switch
> to _not_ CONFIG_PRINTK_TIME_DISABLE as the internal mechanical replacement for
> it. I do not know how disruptive this will be, but is worth it if the codebase
> supports it, and legacy config retained?

I looked for one but couldn't find one.  The kernel is a big place, though, and
perhaps it already exists :/.

>> +
>> +static int printk_time_set(const char *val, const struct kernel_param *kp)
>> +{
>> +char *param = strstrip((char *)val);
>> +int _printk_time = -1;
>> +int stamp;
>> +
>> +if (strlen(param) == 1) {
>> +/* Preserve legacy boolean settings */
>> +if (!strcmp("0", param) || !strcmp("n", param) ||
> if strlen(param) == 1, then param[0] == '0' etc works fine and is KISS.
>> +/*
>> + * Only allow enabling and disabling of the current printk_time
>> + * setting.  Changing it from one setting to another confuses
>> + * userspace.
>> + */
>> +if (printk_time_setting == PRINTK_TIME_DISABLE) {
>> +printk_time_setting = _printk_time;
>> +} else if ((printk_time_setting != _printk_time) &&
>> +   (_printk_time != 0)) {
>> +pr_warn("printk: timestamp can only be set to 0(disabled) or %s\n",
>> +printk_time_str[printk_time_setting]);
>> +return -EINVAL;
>> +}
> I agree with the restriction in the general case.  However (as hinted at
> before() #ifdef CONFIG_PRINTK_TIME_RESTRICT (default y, or #ifndef
> CONFIG_PRINTK_TIME_DEBUG default n) around this will allow us users to choose 
> if
> we are confused or not. I can see being able to change it on the fly as an
> option. Especially since we have /sys/module/printk/parameters/time.

Yeah, but I think that should be a later enhancement.

P.

> 
> -- Mark

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Prarit Bhargava]

On 08/07/2017 12:58 PM, Mark Salyzyn wrote:
> On 08/07/2017 08:52 AM, Prarit Bhargava wrote:
>> diff --git a/arch/arm/configs/aspeed_g4_defconfig
>> b/arch/arm/configs/aspeed_g4_defconfig
>> index cfc2465e8b77..5f3c50914e92 100644
>> --- a/arch/arm/configs/aspeed_g4_defconfig
>> +++ b/arch/arm/configs/aspeed_g4_defconfig
>> @@ -162,7 +162,7 @@ CONFIG_JFFS2_FS_XATTR=y
>>   CONFIG_UBIFS_FS=y
>>   CONFIG_SQUASHFS=y
>>   CONFIG_SQUASHFS_XZ=y
>> -CONFIG_PRINTK_TIME=y
>> +CONFIG_PRINTK_TIME_LOCAL=y
>>   CONFIG_DYNAMIC_DEBUG=y
>>   CONFIG_STRIP_ASM_SYMS=y
>>   CONFIG_DEBUG_FS=y
> Many have had misgivings, let me try another pass at this.
> 
> We (royal we) should really look into adjusting configuration parsing to allow
> an easy transition from boolean to selection ... I am sure this is not the 
> first
> time bistate/tristate was moved to a number.
> 
> An idea? Maybe look into a way to deal with this to use something _other_ than
> CONFIG_PRINTK_TIME to hold the selection, and keep a (hidden/legacy?)
> CONFIG_PRINTK_TIME that when selected sets CONFIG_PRINTK_TIME_LOCAL, and 
> switch
> to _not_ CONFIG_PRINTK_TIME_DISABLE as the internal mechanical replacement for
> it. I do not know how disruptive this will be, but is worth it if the codebase
> supports it, and legacy config retained?

I looked for one but couldn't find one.  The kernel is a big place, though, and
perhaps it already exists :/.

>> +
>> +static int printk_time_set(const char *val, const struct kernel_param *kp)
>> +{
>> +char *param = strstrip((char *)val);
>> +int _printk_time = -1;
>> +int stamp;
>> +
>> +if (strlen(param) == 1) {
>> +/* Preserve legacy boolean settings */
>> +if (!strcmp("0", param) || !strcmp("n", param) ||
> if strlen(param) == 1, then param[0] == '0' etc works fine and is KISS.
>> +/*
>> + * Only allow enabling and disabling of the current printk_time
>> + * setting.  Changing it from one setting to another confuses
>> + * userspace.
>> + */
>> +if (printk_time_setting == PRINTK_TIME_DISABLE) {
>> +printk_time_setting = _printk_time;
>> +} else if ((printk_time_setting != _printk_time) &&
>> +   (_printk_time != 0)) {
>> +pr_warn("printk: timestamp can only be set to 0(disabled) or %s\n",
>> +printk_time_str[printk_time_setting]);
>> +return -EINVAL;
>> +}
> I agree with the restriction in the general case.  However (as hinted at
> before() #ifdef CONFIG_PRINTK_TIME_RESTRICT (default y, or #ifndef
> CONFIG_PRINTK_TIME_DEBUG default n) around this will allow us users to choose 
> if
> we are confused or not. I can see being able to change it on the fly as an
> option. Especially since we have /sys/module/printk/parameters/time.

Yeah, but I think that should be a later enhancement.

P.

> 
> -- Mark

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Prarit Bhargava]

On 08/07/2017 12:52 PM, John Stultz wrote:
> On Mon, Aug 7, 2017 at 8:52 AM, Prarit Bhargava  wrote:
>> printk.time=1/CONFIG_PRINTK_TIME=1 adds a unmodified local hardware clock
>> timestamp to printk messages.  The local hardware clock loses time each
>> day making it difficult to determine exactly when an issue has occurred in
>> the kernel log, and making it difficult to determine how kernel and
>> hardware issues relate to each other in real time.
>>
>> Make printk output different timestampes by adding options for no
>> timestamp, the local hardware clock, the monotonic clock, the boottime
>> clock, and the real clock.  Allow a user to pick one of the clocks by
>> using the printk.time kernel parameter.  Output the type of clock in
>> /sys/module/printk/parameters/time so userspace programs can interpret the
>> timestamp.
>>
>> Real clock & 32-bit systems:  Selecting the real clock printk timestamp may
>> lead to unlikely situations where a timestamp is wrong because the real time
>> offset is read without the protection of a sequence lock in the call to
>> ktime_get_log_ts() in printk_get_ts().
>>
>> v2: Use peterz's suggested Kconfig options.  Merge patchset together.
>> Fix i386 !CONFIG_PRINTK builds.
>>
>> v3: Fixed x86_64_defconfig. Added printk_time_type enum and
>> printk_time_str for better output. Added BOOTTIME clock functionality.
>>
>> v4: Fix messages, add additional printk.time options, and fix configs.
>>
>> Signed-off-by: Prarit Bhargava 
>> Cc: Mark Salyzyn 
>> Cc: Jonathan Corbet 
>> Cc: Petr Mladek 
>> Cc: Sergey Senozhatsky 
>> Cc: Steven Rostedt 
>> Cc: John Stultz 
>> Cc: Thomas Gleixner 
>> Cc: Stephen Boyd 
>> Cc: Andrew Morton 
>> Cc: Greg Kroah-Hartman 
>> Cc: "Paul E. McKenney" 
>> Cc: Christoffer Dall 
>> Cc: Deepa Dinamani 
>> Cc: Ingo Molnar 
>> Cc: Joel Fernandes 
>> Cc: Prarit Bhargava 
>> Cc: Kees Cook 
>> Cc: Peter Zijlstra 
>> Cc: Geert Uytterhoeven 
>> Cc: "Luis R. Rodriguez" 
>> Cc: Nicholas Piggin 
>> Cc: "Jason A. Donenfeld" 
>> Cc: Olof Johansson 
>> Cc: Josh Poimboeuf 
>> Cc: linux-...@vger.kernel.org
>> ---
>>  Documentation/admin-guide/kernel-parameters.txt|   6 +-
>>  arch/arm/configs/aspeed_g4_defconfig   |   2 +-
>>  arch/arm/configs/aspeed_g5_defconfig   |   2 +-
>>  arch/arm/configs/axm55xx_defconfig |   2 +-
>>  arch/arm/configs/bcm2835_defconfig |   2 +-
>>  arch/arm/configs/colibri_pxa270_defconfig  |   2 +-
>>  arch/arm/configs/colibri_pxa300_defconfig  |   2 +-
>>  arch/arm/configs/dove_defconfig|   2 +-
>>  arch/arm/configs/efm32_defconfig   |   2 +-
>>  arch/arm/configs/exynos_defconfig  |   2 +-
>>  arch/arm/configs/ezx_defconfig |   2 +-
>>  arch/arm/configs/h5000_defconfig   |   2 +-
>>  arch/arm/configs/hisi_defconfig|   2 +-
>>  arch/arm/configs/imote2_defconfig  |   2 +-
>>  arch/arm/configs/imx_v6_v7_defconfig   |   2 +-
>>  arch/arm/configs/keystone_defconfig|   2 +-
>>  arch/arm/configs/lpc18xx_defconfig |   2 +-
>>  arch/arm/configs/magician_defconfig|   2 +-
>>  arch/arm/configs/mmp2_defconfig|   2 +-
>>  arch/arm/configs/moxart_defconfig  |   2 +-
>>  arch/arm/configs/mps2_defconfig|   2 +-
>>  arch/arm/configs/multi_v7_defconfig|   2 +-
>>  arch/arm/configs/mvebu_v7_defconfig|   2 +-
>>  arch/arm/configs/mxs_defconfig |   2 +-
>>  arch/arm/configs/omap2plus_defconfig   |   2 +-
>>  arch/arm/configs/pxa168_defconfig  |   2 +-
>>  arch/arm/configs/pxa3xx_defconfig  |   2 +-
>>  arch/arm/configs/pxa910_defconfig  |   2 +-
>>  arch/arm/configs/pxa_defconfig |   2 +-
>>  arch/arm/configs/qcom_defconfig|   2 +-
>>  arch/arm/configs/raumfeld_defconfig|   2 +-
>>  arch/arm/configs/shmobile_defconfig|   2 +-
>>  arch/arm/configs/socfpga_defconfig |   2 +-
>>  arch/arm/configs/stm32_defconfig   |   2 +-
>>  arch/arm/configs/sunxi_defconfig   |   2 +-
>>  arch/arm/configs/tango4_defconfig  |   2 +-
>>  arch/arm/configs/tegra_defconfig  

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Prarit Bhargava]

On 08/07/2017 12:52 PM, John Stultz wrote:
> On Mon, Aug 7, 2017 at 8:52 AM, Prarit Bhargava  wrote:
>> printk.time=1/CONFIG_PRINTK_TIME=1 adds a unmodified local hardware clock
>> timestamp to printk messages.  The local hardware clock loses time each
>> day making it difficult to determine exactly when an issue has occurred in
>> the kernel log, and making it difficult to determine how kernel and
>> hardware issues relate to each other in real time.
>>
>> Make printk output different timestampes by adding options for no
>> timestamp, the local hardware clock, the monotonic clock, the boottime
>> clock, and the real clock.  Allow a user to pick one of the clocks by
>> using the printk.time kernel parameter.  Output the type of clock in
>> /sys/module/printk/parameters/time so userspace programs can interpret the
>> timestamp.
>>
>> Real clock & 32-bit systems:  Selecting the real clock printk timestamp may
>> lead to unlikely situations where a timestamp is wrong because the real time
>> offset is read without the protection of a sequence lock in the call to
>> ktime_get_log_ts() in printk_get_ts().
>>
>> v2: Use peterz's suggested Kconfig options.  Merge patchset together.
>> Fix i386 !CONFIG_PRINTK builds.
>>
>> v3: Fixed x86_64_defconfig. Added printk_time_type enum and
>> printk_time_str for better output. Added BOOTTIME clock functionality.
>>
>> v4: Fix messages, add additional printk.time options, and fix configs.
>>
>> Signed-off-by: Prarit Bhargava 
>> Cc: Mark Salyzyn 
>> Cc: Jonathan Corbet 
>> Cc: Petr Mladek 
>> Cc: Sergey Senozhatsky 
>> Cc: Steven Rostedt 
>> Cc: John Stultz 
>> Cc: Thomas Gleixner 
>> Cc: Stephen Boyd 
>> Cc: Andrew Morton 
>> Cc: Greg Kroah-Hartman 
>> Cc: "Paul E. McKenney" 
>> Cc: Christoffer Dall 
>> Cc: Deepa Dinamani 
>> Cc: Ingo Molnar 
>> Cc: Joel Fernandes 
>> Cc: Prarit Bhargava 
>> Cc: Kees Cook 
>> Cc: Peter Zijlstra 
>> Cc: Geert Uytterhoeven 
>> Cc: "Luis R. Rodriguez" 
>> Cc: Nicholas Piggin 
>> Cc: "Jason A. Donenfeld" 
>> Cc: Olof Johansson 
>> Cc: Josh Poimboeuf 
>> Cc: linux-...@vger.kernel.org
>> ---
>>  Documentation/admin-guide/kernel-parameters.txt|   6 +-
>>  arch/arm/configs/aspeed_g4_defconfig   |   2 +-
>>  arch/arm/configs/aspeed_g5_defconfig   |   2 +-
>>  arch/arm/configs/axm55xx_defconfig |   2 +-
>>  arch/arm/configs/bcm2835_defconfig |   2 +-
>>  arch/arm/configs/colibri_pxa270_defconfig  |   2 +-
>>  arch/arm/configs/colibri_pxa300_defconfig  |   2 +-
>>  arch/arm/configs/dove_defconfig|   2 +-
>>  arch/arm/configs/efm32_defconfig   |   2 +-
>>  arch/arm/configs/exynos_defconfig  |   2 +-
>>  arch/arm/configs/ezx_defconfig |   2 +-
>>  arch/arm/configs/h5000_defconfig   |   2 +-
>>  arch/arm/configs/hisi_defconfig|   2 +-
>>  arch/arm/configs/imote2_defconfig  |   2 +-
>>  arch/arm/configs/imx_v6_v7_defconfig   |   2 +-
>>  arch/arm/configs/keystone_defconfig|   2 +-
>>  arch/arm/configs/lpc18xx_defconfig |   2 +-
>>  arch/arm/configs/magician_defconfig|   2 +-
>>  arch/arm/configs/mmp2_defconfig|   2 +-
>>  arch/arm/configs/moxart_defconfig  |   2 +-
>>  arch/arm/configs/mps2_defconfig|   2 +-
>>  arch/arm/configs/multi_v7_defconfig|   2 +-
>>  arch/arm/configs/mvebu_v7_defconfig|   2 +-
>>  arch/arm/configs/mxs_defconfig |   2 +-
>>  arch/arm/configs/omap2plus_defconfig   |   2 +-
>>  arch/arm/configs/pxa168_defconfig  |   2 +-
>>  arch/arm/configs/pxa3xx_defconfig  |   2 +-
>>  arch/arm/configs/pxa910_defconfig  |   2 +-
>>  arch/arm/configs/pxa_defconfig |   2 +-
>>  arch/arm/configs/qcom_defconfig|   2 +-
>>  arch/arm/configs/raumfeld_defconfig|   2 +-
>>  arch/arm/configs/shmobile_defconfig|   2 +-
>>  arch/arm/configs/socfpga_defconfig |   2 +-
>>  arch/arm/configs/stm32_defconfig   |   2 +-
>>  arch/arm/configs/sunxi_defconfig   |   2 +-
>>  arch/arm/configs/tango4_defconfig  |   2 +-
>>  arch/arm/configs/tegra_defconfig   |   2 +-
>>  arch/arm/configs/u300_defconfig|   2 +-
>>  arch/arm/configs/u8500_defconfig   |   2 +-
>>  arch/arm/configs/vt8500_v6_v7_defconfig|   2 +-
>>  arch/arm/configs/xcep_defconfig|   2 +-
>>  arch/arm/configs/zx_defconfig  |   2 +-
>>  arch/arm64/configs/defconfig   |   2 +-
>>  arch/m68k/configs/amcore_defconfig |   2 +-
>>  arch/mips/configs/ath25_defconfig  |   2 +-
>>  

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Peter Zijlstra]

On Mon, Aug 07, 2017 at 11:52:42AM -0400, Prarit Bhargava wrote:
> +static u64 printk_get_ts(void)
> +{
> + u64 mono, offset_real;
> +
> + if (printk_time <= PRINTK_TIME_LOCAL)
> + return local_clock();
> +
> + if (printk_time == PRINTK_TIME_BOOT)
> + return ktime_get_boot_log_ts();
> +
> + mono = ktime_get_real_log_ts(_real);
> +
> + if (printk_time == PRINTK_TIME_MONO)
> + return mono;
> +
> + return mono + offset_real;
> +}
>  
>  static size_t print_time(u64 ts, char *buf)
>  {
> @@ -1643,7 +1750,7 @@ static bool cont_add(int facility, int level, enum 
> log_flags flags, const char *
>   cont.facility = facility;
>   cont.level = level;
>   cont.owner = current;
> - cont.ts_nsec = local_clock();
> + cont.ts_nsec = printk_get_ts();
>   cont.flags = flags;
>   }
>  

So you really want to do all those branches every single time you
printk() ? I know printk() is somewhat of a slow path, but shees that's
ugly.

Why not have that setup function of yours set a function pointer?

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Peter Zijlstra]

On Mon, Aug 07, 2017 at 11:52:42AM -0400, Prarit Bhargava wrote:
> +static u64 printk_get_ts(void)
> +{
> + u64 mono, offset_real;
> +
> + if (printk_time <= PRINTK_TIME_LOCAL)
> + return local_clock();
> +
> + if (printk_time == PRINTK_TIME_BOOT)
> + return ktime_get_boot_log_ts();
> +
> + mono = ktime_get_real_log_ts(_real);
> +
> + if (printk_time == PRINTK_TIME_MONO)
> + return mono;
> +
> + return mono + offset_real;
> +}
>  
>  static size_t print_time(u64 ts, char *buf)
>  {
> @@ -1643,7 +1750,7 @@ static bool cont_add(int facility, int level, enum 
> log_flags flags, const char *
>   cont.facility = facility;
>   cont.level = level;
>   cont.owner = current;
> - cont.ts_nsec = local_clock();
> + cont.ts_nsec = printk_get_ts();
>   cont.flags = flags;
>   }
>  

So you really want to do all those branches every single time you
printk() ? I know printk() is somewhat of a slow path, but shees that's
ugly.

Why not have that setup function of yours set a function pointer?

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Peter Zijlstra]

On Mon, Aug 07, 2017 at 09:52:10AM -0700, John Stultz wrote:
> On Mon, Aug 7, 2017 at 8:52 AM, Prarit Bhargava  wrote:
> > +u64 ktime_get_real_log_ts(u64 *offset_real)
> > +{
> > +   *offset_real = ktime_to_ns(tk_core.timekeeper.offs_real);
> > +
> > +   if (timekeeping_active)
> > +   return ktime_get_mono_fast_ns();
> > +   else
> > +   return local_clock();
> > +}
> > +
> > +u64 ktime_get_boot_log_ts(void)
> > +{
> > +   if (timekeeping_active)
> > +   return ktime_get_boot_fast_ns();
> > +   else
> > +   return local_clock();
> > +}
> 
> This feels a little tacked on and duplicative.  I'd suggest having one
> function that returns the offset_real and offset_boot or have a
> separate get_mono_log_ts() so its at least consistent.   Additionally,
> in the commit message, you call out the lack of locking between
> grabing the offs_real and calling get_mono_fast_ns(), but I worry it
> may be particularly problematic on 32bit systems, and you don't have
> any notes in the actual code about it (it looks like an oversight).
> 
> Also, when timekeeping_active flips over, and we change from local
> clock to the specified clock, do we see a discontinuity in the log? I
> know folks use to gripe about that back in the day.

Yeah, yuck, this smells. Please don't mix clocks like this.

I expect all you want is to avoid the explosion you get from calling the
fast things too early, right? Please use the below, which should result
in it returning 0.

---
 kernel/time/timekeeping.c | 47 +--
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index cedafa008de5..d111039e0245 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -60,8 +60,39 @@ struct tk_fast {
struct tk_read_base base[2];
 };
 
-static struct tk_fast tk_fast_mono cacheline_aligned;
-static struct tk_fast tk_fast_raw  cacheline_aligned;
+/* Suspend-time cycles value for halted fast timekeeper. */
+static u64 cycles_at_suspend;
+
+static u64 dummy_clock_read(struct clocksource *cs)
+{
+   return cycles_at_suspend;
+}
+
+static struct clocksource dummy_clock = {
+   .read = dummy_clock_read,
+};
+
+static struct tk_fast tk_fast_mono cacheline_aligned = {
+   .base = {
+   (struct tk_read_base){
+   .clock = _clock,
+   },
+   (struct tk_read_base){
+   .clock = _clock,
+   },
+   },
+};
+
+static struct tk_fast tk_fast_raw  cacheline_aligned = {
+   .base = {
+   (struct tk_read_base){
+   .clock = _clock,
+   },
+   (struct tk_read_base){
+   .clock = _clock,
+   },
+   },
+};
 
 /* flag for if timekeeping is suspended */
 int __read_mostly timekeeping_suspended;
@@ -477,18 +508,6 @@ u64 notrace ktime_get_boot_fast_ns(void)
 }
 EXPORT_SYMBOL_GPL(ktime_get_boot_fast_ns);
 
-/* Suspend-time cycles value for halted fast timekeeper. */
-static u64 cycles_at_suspend;
-
-static u64 dummy_clock_read(struct clocksource *cs)
-{
-   return cycles_at_suspend;
-}
-
-static struct clocksource dummy_clock = {
-   .read = dummy_clock_read,
-};
-
 /**
  * halt_fast_timekeeper - Prevent fast timekeeper from accessing clocksource.
  * @tk: Timekeeper to snapshot.

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Peter Zijlstra]

On Mon, Aug 07, 2017 at 09:52:10AM -0700, John Stultz wrote:
> On Mon, Aug 7, 2017 at 8:52 AM, Prarit Bhargava  wrote:
> > +u64 ktime_get_real_log_ts(u64 *offset_real)
> > +{
> > +   *offset_real = ktime_to_ns(tk_core.timekeeper.offs_real);
> > +
> > +   if (timekeeping_active)
> > +   return ktime_get_mono_fast_ns();
> > +   else
> > +   return local_clock();
> > +}
> > +
> > +u64 ktime_get_boot_log_ts(void)
> > +{
> > +   if (timekeeping_active)
> > +   return ktime_get_boot_fast_ns();
> > +   else
> > +   return local_clock();
> > +}
> 
> This feels a little tacked on and duplicative.  I'd suggest having one
> function that returns the offset_real and offset_boot or have a
> separate get_mono_log_ts() so its at least consistent.   Additionally,
> in the commit message, you call out the lack of locking between
> grabing the offs_real and calling get_mono_fast_ns(), but I worry it
> may be particularly problematic on 32bit systems, and you don't have
> any notes in the actual code about it (it looks like an oversight).
> 
> Also, when timekeeping_active flips over, and we change from local
> clock to the specified clock, do we see a discontinuity in the log? I
> know folks use to gripe about that back in the day.

Yeah, yuck, this smells. Please don't mix clocks like this.

I expect all you want is to avoid the explosion you get from calling the
fast things too early, right? Please use the below, which should result
in it returning 0.

---
 kernel/time/timekeeping.c | 47 +--
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index cedafa008de5..d111039e0245 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -60,8 +60,39 @@ struct tk_fast {
struct tk_read_base base[2];
 };
 
-static struct tk_fast tk_fast_mono cacheline_aligned;
-static struct tk_fast tk_fast_raw  cacheline_aligned;
+/* Suspend-time cycles value for halted fast timekeeper. */
+static u64 cycles_at_suspend;
+
+static u64 dummy_clock_read(struct clocksource *cs)
+{
+   return cycles_at_suspend;
+}
+
+static struct clocksource dummy_clock = {
+   .read = dummy_clock_read,
+};
+
+static struct tk_fast tk_fast_mono cacheline_aligned = {
+   .base = {
+   (struct tk_read_base){
+   .clock = _clock,
+   },
+   (struct tk_read_base){
+   .clock = _clock,
+   },
+   },
+};
+
+static struct tk_fast tk_fast_raw  cacheline_aligned = {
+   .base = {
+   (struct tk_read_base){
+   .clock = _clock,
+   },
+   (struct tk_read_base){
+   .clock = _clock,
+   },
+   },
+};
 
 /* flag for if timekeeping is suspended */
 int __read_mostly timekeeping_suspended;
@@ -477,18 +508,6 @@ u64 notrace ktime_get_boot_fast_ns(void)
 }
 EXPORT_SYMBOL_GPL(ktime_get_boot_fast_ns);
 
-/* Suspend-time cycles value for halted fast timekeeper. */
-static u64 cycles_at_suspend;
-
-static u64 dummy_clock_read(struct clocksource *cs)
-{
-   return cycles_at_suspend;
-}
-
-static struct clocksource dummy_clock = {
-   .read = dummy_clock_read,
-};
-
 /**
  * halt_fast_timekeeper - Prevent fast timekeeper from accessing clocksource.
  * @tk: Timekeeper to snapshot.

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Mark Salyzyn]

On 08/07/2017 08:52 AM, Prarit Bhargava wrote:

diff --git a/arch/arm/configs/aspeed_g4_defconfig 
b/arch/arm/configs/aspeed_g4_defconfig
index cfc2465e8b77..5f3c50914e92 100644
--- a/arch/arm/configs/aspeed_g4_defconfig
+++ b/arch/arm/configs/aspeed_g4_defconfig
@@ -162,7 +162,7 @@ CONFIG_JFFS2_FS_XATTR=y
  CONFIG_UBIFS_FS=y
  CONFIG_SQUASHFS=y
  CONFIG_SQUASHFS_XZ=y
-CONFIG_PRINTK_TIME=y
+CONFIG_PRINTK_TIME_LOCAL=y
  CONFIG_DYNAMIC_DEBUG=y
  CONFIG_STRIP_ASM_SYMS=y
  CONFIG_DEBUG_FS=y

Many have had misgivings, let me try another pass at this.

We (royal we) should really look into adjusting configuration parsing to 
allow an easy transition from boolean to selection ... I am sure this is 
not the first time bistate/tristate was moved to a number.


An idea? Maybe look into a way to deal with this to use something 
_other_ than CONFIG_PRINTK_TIME to hold the selection, and keep a 
(hidden/legacy?) CONFIG_PRINTK_TIME that when selected sets 
CONFIG_PRINTK_TIME_LOCAL, and switch to _not_ CONFIG_PRINTK_TIME_DISABLE 
as the internal mechanical replacement for it. I do not know how 
disruptive this will be, but is worth it if the codebase supports it, 
and legacy config retained?

+
+static int printk_time_set(const char *val, const struct kernel_param *kp)
+{
+   char *param = strstrip((char *)val);
+   int _printk_time = -1;
+   int stamp;
+
+   if (strlen(param) == 1) {
+   /* Preserve legacy boolean settings */
+   if (!strcmp("0", param) || !strcmp("n", param) ||

if strlen(param) == 1, then param[0] == '0' etc works fine and is KISS.

+   /*
+* Only allow enabling and disabling of the current printk_time
+* setting.  Changing it from one setting to another confuses
+* userspace.
+*/
+   if (printk_time_setting == PRINTK_TIME_DISABLE) {
+   printk_time_setting = _printk_time;
+   } else if ((printk_time_setting != _printk_time) &&
+  (_printk_time != 0)) {
+   pr_warn("printk: timestamp can only be set to 0(disabled) or 
%s\n",
+   printk_time_str[printk_time_setting]);
+   return -EINVAL;
+   }
I agree with the restriction in the general case.  However (as hinted at 
before() #ifdef CONFIG_PRINTK_TIME_RESTRICT (default y, or #ifndef 
CONFIG_PRINTK_TIME_DEBUG default n) around this will allow us users to 
choose if we are confused or not. I can see being able to change it on 
the fly as an option. Especially since we have 
/sys/module/printk/parameters/time.


-- Mark

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Mark Salyzyn]

On 08/07/2017 08:52 AM, Prarit Bhargava wrote:

diff --git a/arch/arm/configs/aspeed_g4_defconfig 
b/arch/arm/configs/aspeed_g4_defconfig
index cfc2465e8b77..5f3c50914e92 100644
--- a/arch/arm/configs/aspeed_g4_defconfig
+++ b/arch/arm/configs/aspeed_g4_defconfig
@@ -162,7 +162,7 @@ CONFIG_JFFS2_FS_XATTR=y
  CONFIG_UBIFS_FS=y
  CONFIG_SQUASHFS=y
  CONFIG_SQUASHFS_XZ=y
-CONFIG_PRINTK_TIME=y
+CONFIG_PRINTK_TIME_LOCAL=y
  CONFIG_DYNAMIC_DEBUG=y
  CONFIG_STRIP_ASM_SYMS=y
  CONFIG_DEBUG_FS=y

Many have had misgivings, let me try another pass at this.

We (royal we) should really look into adjusting configuration parsing to 
allow an easy transition from boolean to selection ... I am sure this is 
not the first time bistate/tristate was moved to a number.


An idea? Maybe look into a way to deal with this to use something 
_other_ than CONFIG_PRINTK_TIME to hold the selection, and keep a 
(hidden/legacy?) CONFIG_PRINTK_TIME that when selected sets 
CONFIG_PRINTK_TIME_LOCAL, and switch to _not_ CONFIG_PRINTK_TIME_DISABLE 
as the internal mechanical replacement for it. I do not know how 
disruptive this will be, but is worth it if the codebase supports it, 
and legacy config retained?

+
+static int printk_time_set(const char *val, const struct kernel_param *kp)
+{
+   char *param = strstrip((char *)val);
+   int _printk_time = -1;
+   int stamp;
+
+   if (strlen(param) == 1) {
+   /* Preserve legacy boolean settings */
+   if (!strcmp("0", param) || !strcmp("n", param) ||

if strlen(param) == 1, then param[0] == '0' etc works fine and is KISS.

+   /*
+* Only allow enabling and disabling of the current printk_time
+* setting.  Changing it from one setting to another confuses
+* userspace.
+*/
+   if (printk_time_setting == PRINTK_TIME_DISABLE) {
+   printk_time_setting = _printk_time;
+   } else if ((printk_time_setting != _printk_time) &&
+  (_printk_time != 0)) {
+   pr_warn("printk: timestamp can only be set to 0(disabled) or 
%s\n",
+   printk_time_str[printk_time_setting]);
+   return -EINVAL;
+   }
I agree with the restriction in the general case.  However (as hinted at 
before() #ifdef CONFIG_PRINTK_TIME_RESTRICT (default y, or #ifndef 
CONFIG_PRINTK_TIME_DEBUG default n) around this will allow us users to 
choose if we are confused or not. I can see being able to change it on 
the fly as an option. Especially since we have 
/sys/module/printk/parameters/time.


-- Mark

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[John Stultz]

On Mon, Aug 7, 2017 at 8:52 AM, Prarit Bhargava  wrote:
> printk.time=1/CONFIG_PRINTK_TIME=1 adds a unmodified local hardware clock
> timestamp to printk messages.  The local hardware clock loses time each
> day making it difficult to determine exactly when an issue has occurred in
> the kernel log, and making it difficult to determine how kernel and
> hardware issues relate to each other in real time.
>
> Make printk output different timestampes by adding options for no
> timestamp, the local hardware clock, the monotonic clock, the boottime
> clock, and the real clock.  Allow a user to pick one of the clocks by
> using the printk.time kernel parameter.  Output the type of clock in
> /sys/module/printk/parameters/time so userspace programs can interpret the
> timestamp.
>
> Real clock & 32-bit systems:  Selecting the real clock printk timestamp may
> lead to unlikely situations where a timestamp is wrong because the real time
> offset is read without the protection of a sequence lock in the call to
> ktime_get_log_ts() in printk_get_ts().
>
> v2: Use peterz's suggested Kconfig options.  Merge patchset together.
> Fix i386 !CONFIG_PRINTK builds.
>
> v3: Fixed x86_64_defconfig. Added printk_time_type enum and
> printk_time_str for better output. Added BOOTTIME clock functionality.
>
> v4: Fix messages, add additional printk.time options, and fix configs.
>
> Signed-off-by: Prarit Bhargava 
> Cc: Mark Salyzyn 
> Cc: Jonathan Corbet 
> Cc: Petr Mladek 
> Cc: Sergey Senozhatsky 
> Cc: Steven Rostedt 
> Cc: John Stultz 
> Cc: Thomas Gleixner 
> Cc: Stephen Boyd 
> Cc: Andrew Morton 
> Cc: Greg Kroah-Hartman 
> Cc: "Paul E. McKenney" 
> Cc: Christoffer Dall 
> Cc: Deepa Dinamani 
> Cc: Ingo Molnar 
> Cc: Joel Fernandes 
> Cc: Prarit Bhargava 
> Cc: Kees Cook 
> Cc: Peter Zijlstra 
> Cc: Geert Uytterhoeven 
> Cc: "Luis R. Rodriguez" 
> Cc: Nicholas Piggin 
> Cc: "Jason A. Donenfeld" 
> Cc: Olof Johansson 
> Cc: Josh Poimboeuf 
> Cc: linux-...@vger.kernel.org
> ---
>  Documentation/admin-guide/kernel-parameters.txt|   6 +-
>  arch/arm/configs/aspeed_g4_defconfig   |   2 +-
>  arch/arm/configs/aspeed_g5_defconfig   |   2 +-
>  arch/arm/configs/axm55xx_defconfig |   2 +-
>  arch/arm/configs/bcm2835_defconfig |   2 +-
>  arch/arm/configs/colibri_pxa270_defconfig  |   2 +-
>  arch/arm/configs/colibri_pxa300_defconfig  |   2 +-
>  arch/arm/configs/dove_defconfig|   2 +-
>  arch/arm/configs/efm32_defconfig   |   2 +-
>  arch/arm/configs/exynos_defconfig  |   2 +-
>  arch/arm/configs/ezx_defconfig |   2 +-
>  arch/arm/configs/h5000_defconfig   |   2 +-
>  arch/arm/configs/hisi_defconfig|   2 +-
>  arch/arm/configs/imote2_defconfig  |   2 +-
>  arch/arm/configs/imx_v6_v7_defconfig   |   2 +-
>  arch/arm/configs/keystone_defconfig|   2 +-
>  arch/arm/configs/lpc18xx_defconfig |   2 +-
>  arch/arm/configs/magician_defconfig|   2 +-
>  arch/arm/configs/mmp2_defconfig|   2 +-
>  arch/arm/configs/moxart_defconfig  |   2 +-
>  arch/arm/configs/mps2_defconfig|   2 +-
>  arch/arm/configs/multi_v7_defconfig|   2 +-
>  arch/arm/configs/mvebu_v7_defconfig|   2 +-
>  arch/arm/configs/mxs_defconfig |   2 +-
>  arch/arm/configs/omap2plus_defconfig   |   2 +-
>  arch/arm/configs/pxa168_defconfig  |   2 +-
>  arch/arm/configs/pxa3xx_defconfig  |   2 +-
>  arch/arm/configs/pxa910_defconfig  |   2 +-
>  arch/arm/configs/pxa_defconfig |   2 +-
>  arch/arm/configs/qcom_defconfig|   2 +-
>  arch/arm/configs/raumfeld_defconfig|   2 +-
>  arch/arm/configs/shmobile_defconfig|   2 +-
>  arch/arm/configs/socfpga_defconfig |   2 +-
>  arch/arm/configs/stm32_defconfig   |   2 +-
>  arch/arm/configs/sunxi_defconfig   |   2 +-
>  arch/arm/configs/tango4_defconfig  |   2 +-
>  arch/arm/configs/tegra_defconfig   |   2 +-
>  arch/arm/configs/u300_defconfig|   2 +-
>  arch/arm/configs/u8500_defconfig   |   2 

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[John Stultz]

On Mon, Aug 7, 2017 at 8:52 AM, Prarit Bhargava  wrote:
> printk.time=1/CONFIG_PRINTK_TIME=1 adds a unmodified local hardware clock
> timestamp to printk messages.  The local hardware clock loses time each
> day making it difficult to determine exactly when an issue has occurred in
> the kernel log, and making it difficult to determine how kernel and
> hardware issues relate to each other in real time.
>
> Make printk output different timestampes by adding options for no
> timestamp, the local hardware clock, the monotonic clock, the boottime
> clock, and the real clock.  Allow a user to pick one of the clocks by
> using the printk.time kernel parameter.  Output the type of clock in
> /sys/module/printk/parameters/time so userspace programs can interpret the
> timestamp.
>
> Real clock & 32-bit systems:  Selecting the real clock printk timestamp may
> lead to unlikely situations where a timestamp is wrong because the real time
> offset is read without the protection of a sequence lock in the call to
> ktime_get_log_ts() in printk_get_ts().
>
> v2: Use peterz's suggested Kconfig options.  Merge patchset together.
> Fix i386 !CONFIG_PRINTK builds.
>
> v3: Fixed x86_64_defconfig. Added printk_time_type enum and
> printk_time_str for better output. Added BOOTTIME clock functionality.
>
> v4: Fix messages, add additional printk.time options, and fix configs.
>
> Signed-off-by: Prarit Bhargava 
> Cc: Mark Salyzyn 
> Cc: Jonathan Corbet 
> Cc: Petr Mladek 
> Cc: Sergey Senozhatsky 
> Cc: Steven Rostedt 
> Cc: John Stultz 
> Cc: Thomas Gleixner 
> Cc: Stephen Boyd 
> Cc: Andrew Morton 
> Cc: Greg Kroah-Hartman 
> Cc: "Paul E. McKenney" 
> Cc: Christoffer Dall 
> Cc: Deepa Dinamani 
> Cc: Ingo Molnar 
> Cc: Joel Fernandes 
> Cc: Prarit Bhargava 
> Cc: Kees Cook 
> Cc: Peter Zijlstra 
> Cc: Geert Uytterhoeven 
> Cc: "Luis R. Rodriguez" 
> Cc: Nicholas Piggin 
> Cc: "Jason A. Donenfeld" 
> Cc: Olof Johansson 
> Cc: Josh Poimboeuf 
> Cc: linux-...@vger.kernel.org
> ---
>  Documentation/admin-guide/kernel-parameters.txt|   6 +-
>  arch/arm/configs/aspeed_g4_defconfig   |   2 +-
>  arch/arm/configs/aspeed_g5_defconfig   |   2 +-
>  arch/arm/configs/axm55xx_defconfig |   2 +-
>  arch/arm/configs/bcm2835_defconfig |   2 +-
>  arch/arm/configs/colibri_pxa270_defconfig  |   2 +-
>  arch/arm/configs/colibri_pxa300_defconfig  |   2 +-
>  arch/arm/configs/dove_defconfig|   2 +-
>  arch/arm/configs/efm32_defconfig   |   2 +-
>  arch/arm/configs/exynos_defconfig  |   2 +-
>  arch/arm/configs/ezx_defconfig |   2 +-
>  arch/arm/configs/h5000_defconfig   |   2 +-
>  arch/arm/configs/hisi_defconfig|   2 +-
>  arch/arm/configs/imote2_defconfig  |   2 +-
>  arch/arm/configs/imx_v6_v7_defconfig   |   2 +-
>  arch/arm/configs/keystone_defconfig|   2 +-
>  arch/arm/configs/lpc18xx_defconfig |   2 +-
>  arch/arm/configs/magician_defconfig|   2 +-
>  arch/arm/configs/mmp2_defconfig|   2 +-
>  arch/arm/configs/moxart_defconfig  |   2 +-
>  arch/arm/configs/mps2_defconfig|   2 +-
>  arch/arm/configs/multi_v7_defconfig|   2 +-
>  arch/arm/configs/mvebu_v7_defconfig|   2 +-
>  arch/arm/configs/mxs_defconfig |   2 +-
>  arch/arm/configs/omap2plus_defconfig   |   2 +-
>  arch/arm/configs/pxa168_defconfig  |   2 +-
>  arch/arm/configs/pxa3xx_defconfig  |   2 +-
>  arch/arm/configs/pxa910_defconfig  |   2 +-
>  arch/arm/configs/pxa_defconfig |   2 +-
>  arch/arm/configs/qcom_defconfig|   2 +-
>  arch/arm/configs/raumfeld_defconfig|   2 +-
>  arch/arm/configs/shmobile_defconfig|   2 +-
>  arch/arm/configs/socfpga_defconfig |   2 +-
>  arch/arm/configs/stm32_defconfig   |   2 +-
>  arch/arm/configs/sunxi_defconfig   |   2 +-
>  arch/arm/configs/tango4_defconfig  |   2 +-
>  arch/arm/configs/tegra_defconfig   |   2 +-
>  arch/arm/configs/u300_defconfig|   2 +-
>  arch/arm/configs/u8500_defconfig   |   2 +-
>  arch/arm/configs/vt8500_v6_v7_defconfig|   2 +-
>  arch/arm/configs/xcep_defconfig|   2 +-
>  arch/arm/configs/zx_defconfig  |   2 +-
>  arch/arm64/configs/defconfig   |   2 +-
>  arch/m68k/configs/amcore_defconfig |   2 +-
>  arch/mips/configs/ath25_defconfig  |   2 +-
>  arch/mips/configs/bcm47xx_defconfig|   2 +-
>  arch/mips/configs/bmips_be_defconfig   |   2 +-
>  arch/mips/configs/bmips_stb_defconfig