Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1704082 > unrolled thread

Re: [PATCH v3] printk: Add boottime and real timestamps

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3] printk: Add boottime and real timestamps Mark Salyzyn <salyzyn@android.com> - 2017-08-04 17:40 +0200
    Re: [PATCH v3] printk: Add boottime and real timestamps Prarit Bhargava <prarit@redhat.com> - 2017-08-07 14:50 +0200
      Re: [PATCH v3] printk: Add boottime and real timestamps Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-08-07 15:40 +0200

#1704082 — Re: [PATCH v3] printk: Add boottime and real timestamps

On 08/03/2017 06:18 PM, Prarit Bhargava wrote:

> diff --git a/arch/arm/configs/aspeed_g4_defconfig b/arch/arm/configs/aspeed_g4_defconfig
> index cfc2465e8b77..6c73c305ad17 100644
> --- a/arch/arm/configs/aspeed_g4_defconfig
> +++ b/arch/arm/configs/aspeed_g4_defconfig
> @@ -162,7 +162,9 @@ CONFIG_JFFS2_FS_XATTR=y
>  CONFIG_UBIFS_FS=y
>  CONFIG_SQUASHFS=y
>  CONFIG_SQUASHFS_XZ=y
> -CONFIG_PRINTK_TIME=y
> +# CONFIG_PRINTK_TIME_DISABLE is not set
> +CONFIG_PRINTK_TIME_LOCAL=Y
> +CONFIG_PRINTK_TIME=1
>  CONFIG_DYNAMIC_DEBUG=y
>  CONFIG_STRIP_ASM_SYMS=y
>  CONFIG_DEBUG_FS=y

savedefconfig tells me otherwise:

-CONFIG_PRINTK_TIME=y
+CONFIG_PRINTK_TIME_LOCAL=y

is all that is needed, the rest is fluff and will cause a savedefconfig 
mismatch error.
> +static int printk_time_set(const char *val, const struct kernel_param *kp)
> +{
> +	char *param = strstrip((char *)val);
> +	int _printk_time;
> +
> +	if (strlen(param) != 1)
> +		return -EINVAL;
(see below) strlen is so restrictive, wouldn't it be nice(tm) to allow 
"boot", "monotonic" or "realtime" strings? Certainly KISS can handle the 
first letters for next to zero cost. (switch statement optimization)
> +
> +	switch (param[0]) {
> +	case '0':
> +	case 'n':
> +	case 'N':
I would wish for a few more 'convenience' cases:

case 'd': case 'D': /* Disable */
> +		_printk_time = PRINTK_TIME_DISABLE;
> +		break;
> +	case '1':
> +	case 'y':
> +	case 'Y':
> +		_printk_time = PRINTK_TIME_LOCAL;
> +		break;
> +	case '2':
case 'b': case 'B': /* Boottime */
> +		_printk_time = PRINTK_TIME_BOOT;
> +		break;
> +	case '3':
case 'm': case 'M': /* Monotonic */
> +		_printk_time = PRINTK_TIME_MONO;
> +		break;
> +	case '4':
case 'r': case 'R': /* Realtime */
> +		_printk_time = PRINTK_TIME_REAL;
> +		break;
> +	default:
> +		pr_warn("printk: invalid timestamp value\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * 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 %d(%s) ",
> +			printk_time_setting,
> +			printk_time_str[printk_time_setting]);
> +		return -EINVAL;
> +	}
> +
> +	printk_time = _printk_time;
> +	pr_info("printk: timestamp set to %d(%s).",
> +		printk_time, printk_time_str[printk_time]);
> +	return 0;
> +}
> . . .
>   /* time in seconds when suspend began for persistent clock */
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 98fe715522e8..a71ba5689814 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -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
This will ensure mutual exclusivity, no need to add # 
CONFIG_PRINTK_TIME_DISABLE is not set to configs.
> +
>   config PRINTK_TIME
> -	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
> +	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()
>   	  messages to be added to the output of the syslog() system
This "dummy" config will be automated by the select, so no need to add 
CONFIG_PRINTK_TIME=<x> to the defconfigs.

-- Mark

[toc] | [next] | [standalone]

#1705465

On 08/04/2017 11:36 AM, Mark Salyzyn wrote:
> On 08/03/2017 06:18 PM, Prarit Bhargava wrote:
> 
>> diff --git a/arch/arm/configs/aspeed_g4_defconfig
>> b/arch/arm/configs/aspeed_g4_defconfig
>> index cfc2465e8b77..6c73c305ad17 100644
>> --- a/arch/arm/configs/aspeed_g4_defconfig
>> +++ b/arch/arm/configs/aspeed_g4_defconfig
>> @@ -162,7 +162,9 @@ CONFIG_JFFS2_FS_XATTR=y
>>  CONFIG_UBIFS_FS=y
>>  CONFIG_SQUASHFS=y
>>  CONFIG_SQUASHFS_XZ=y
>> -CONFIG_PRINTK_TIME=y
>> +# CONFIG_PRINTK_TIME_DISABLE is not set
>> +CONFIG_PRINTK_TIME_LOCAL=Y
>> +CONFIG_PRINTK_TIME=1
>>  CONFIG_DYNAMIC_DEBUG=y
>>  CONFIG_STRIP_ASM_SYMS=y
>>  CONFIG_DEBUG_FS=y
> 
> savedefconfig tells me otherwise:
> 
> -CONFIG_PRINTK_TIME=y
> +CONFIG_PRINTK_TIME_LOCAL=y
> 
> is all that is needed, the rest is fluff and will cause a savedefconfig mismatch
> error.
>> +static int printk_time_set(const char *val, const struct kernel_param *kp)
>> +{
>> +    char *param = strstrip((char *)val);
>> +    int _printk_time;
>> +
>> +    if (strlen(param) != 1)
>> +        return -EINVAL;
> (see below) strlen is so restrictive, wouldn't it be nice(tm) to allow "boot",
> "monotonic" or "realtime" strings? Certainly KISS can handle the first letters
> for next to zero cost. (switch statement optimization)

Thanks Mark,

I had asked this question before and didn't get an answer: Do I need to
worry about the ABI of /sys/modules/printk/parameters/time.  Since it hasn't
been explicitly added to the stable ABI files, I'm going to assume it wasn't
important enough to add it.  Looking at google, however, leads to a lot
of hits for 'printk.time' kernel parameters settings so I think I better
maintain the legacy boolean settings.

I'm going to do: 0/n/N/1/y/Y to maintain the existing ABI, and add (as you
suggest above 'disable|local|boottime|monotonic|realtime' as valid
parameters.

>> +
>> +    switch (param[0]) {
>> +    case '0':
>> +    case 'n':
>> +    case 'N':
> I would wish for a few more 'convenience' cases:
> 
> case 'd': case 'D': /* Disable */

It would be trivial then to add only the lower case options (I dislike upper
case settings ...) by adding

	!strcmp(printk_time_str[0], param, 1)

P.

[toc] | [prev] | [next] | [standalone]

#1705528

On (08/07/17 08:41), Prarit Bhargava wrote:
[..]
> >> +static int printk_time_set(const char *val, const struct kernel_param *kp)
> >> +{
> >> +    char *param = strstrip((char *)val);
> >> +    int _printk_time;
> >> +
> >> +    if (strlen(param) != 1)
> >> +        return -EINVAL;
> > (see below) strlen is so restrictive, wouldn't it be nice(tm) to allow "boot",
> > "monotonic" or "realtime" strings? Certainly KISS can handle the first letters
> > for next to zero cost. (switch statement optimization)
> 
> Thanks Mark,
> 
> I had asked this question before and didn't get an answer: Do I need to
> worry about the ABI of /sys/modules/printk/parameters/time.  Since it hasn't
> been explicitly added to the stable ABI files, I'm going to assume it wasn't
> important enough to add it.  Looking at google, however, leads to a lot
> of hits for 'printk.time' kernel parameters settings so I think I better
> maintain the legacy boolean settings.

yes, please. we have to preserve it.

> I'm going to do: 0/n/N/1/y/Y to maintain the existing ABI, and add (as you
> suggest above 'disable|local|boottime|monotonic|realtime' as valid
> parameters.
> 
> >> +
> >> +    switch (param[0]) {
> >> +    case '0':
> >> +    case 'n':
> >> +    case 'N':
> > I would wish for a few more 'convenience' cases:
> > 
> > case 'd': case 'D': /* Disable */
> 
> It would be trivial then to add only the lower case options (I dislike upper
> case settings ...) by adding

yes, please. lower case only.

	-ss

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web