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

Groups > linux.kernel > #1705714 > unrolled thread

Re: [PATCH v4] printk: Add monotonic, boottime, and realtime 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 v4] printk: Add monotonic, boottime, and realtime  timestamps Mark Salyzyn <salyzyn@android.com> - 2017-08-07 19:00 +0200
    Re: [PATCH v4] printk: Add monotonic, boottime, and realtime  timestamps Prarit Bhargava <prarit@redhat.com> - 2017-08-07 20:10 +0200

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

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

[toc] | [next] | [standalone]

#1705766

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web