Re: [PATCH 2/2] allow printk delay after multi lines

[Dave Young]

On Tue, Feb 9, 2010 at 10:31 AM, Dave Young <hidave.darkstar@xxxxxxxxx> wrote:
> On Tue, Feb 9, 2010 at 5:56 AM, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>> On Sat, 6 Feb 2010 21:40:56 +0800
>> Dave Young <hidave.darkstar@xxxxxxxxx> wrote:
>>
>>> printk delay help us to capture printk messages on some unconvenient senarios,
>>> but it is still not easy to read.
>>>
>>> Add another sysctl variable printk_delay_per_lines to make it more readable.
>>> We can set the lines according to screen height, then take pictures by camera.
>>>
>>> kmesg will delay printk_delay_per_lines * printk_delay_msecs milliseconds
>>> after every printk_delay_per_lines lines when printk_delay is enabled.
>>>
>>> Setting the lines by proc/sysctl interface:
>>> /proc/sys/kernel/printk_delay_per_lines
>>>
>>> Andrew, sorry, I have not find time to cleanup the kernel.h sysctl variables.
>>> If I'm free I will try to do it.
>>>
>>> The value range from 1 - 100, default value is 1
>>>
>>> ...
>>>
>>> --- linux-2.6.orig/include/linux/kernel.h   2010-02-02 13:38:09.537495564 +0800
>>> +++ linux-2.6/include/linux/kernel.h Â2010-02-02 13:40:47.657480122 +0800
>>> @@ -246,6 +246,7 @@ extern bool printk_timed_ratelimit(unsig
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âunsigned int interval_msec);
>>>
>>> Âextern int printk_delay_msec;
>>> +extern int printk_delay_per_lines;
>>>
>>> Â/*
>>> Â * Print a one-time message (analogous to WARN_ONCE() et al):
>>> --- linux-2.6.orig/kernel/printk.c  Â2010-02-02 13:39:19.446657319 +0800
>>> +++ linux-2.6/kernel/printk.c 2010-02-02 13:40:47.660813615 +0800
>>> @@ -656,16 +656,26 @@ static int new_text_line = 1;
>>> Âstatic char printk_buf[1024];
>>>
>>> Âint printk_delay_msec __read_mostly;
>>> +int printk_delay_per_lines __read_mostly;
>>>
>>> Âstatic inline void printk_delay(void)
>>> Â{
>>> Â Â Â if (unlikely(printk_delay_msec)) {
>>> - Â Â Â Â Â Â int m = printk_delay_msec;
>>> + Â Â Â Â Â Â static int m, l;
>>>
>>> + Â Â Â Â Â Â if (!l)
>>> + Â Â Â Â Â Â Â Â Â Â l = printk_delay_per_lines;
>>> +
>>> + Â Â Â Â Â Â if (--l) {
>>> + Â Â Â Â Â Â Â Â Â Â m += printk_delay_msec;
>>> + Â Â Â Â Â Â Â Â Â Â return;
>>> + Â Â Â Â Â Â }
>>> + Â Â Â Â Â Â m += printk_delay_msec;
>>> Â Â Â Â Â Â Â while (m--) {
>>> Â Â Â Â Â Â Â Â Â Â Â mdelay(1);
>>> Â Â Â Â Â Â Â Â Â Â Â touch_nmi_watchdog();
>>> Â Â Â Â Â Â Â }
>>> + Â Â Â Â Â Â m = 0;
>>> Â Â Â }
>>> Â}
>>
>> - The default value is zero, not 1. ÂAnd zero will be treated as 4G.
>> ÂThat's a bug.
>
> Will fix.
>
>>
>> - This feature would be a lot more useful if the user could specify
>> Âprintk_delay_per_lines on the boot command line. ÂDitto
>> Âprintk_delay_msec. ÂSo you can stop the important mesages from
>> Âscrolling off. Â(I think there's already a way to do that, but I'm
>> Âtoo lazy to go remember what it was).
>
> I think it's the similar "boot_delay". It's a little different as
> calibration is not done on the early booting phase, so busy loop
> is used instead to delay. It's hard to make boot delay accurate.
>
> Maybe delay_per_lines can be done while booting as well,
> but it should be another patch, Âisn't it?

Or merge the two in one function, so we can set the boot param,
ie. call boot_delay in boot phase, mdelay otherwise.

>
>>
>> - The permitted range of 1-100 for printk_delay_per_lines seems
>> Âarbitrary and unneeded. ÂWhy shouldn't I be able to set it to 10,000?
>> ÂI see no harm in permitting that.
>>
>> - If the user sets printk_delay_per_lines=N, the kernel will pause
>> Âfor N*printk_delay_msec every N lines. ÂThis is odd, and unintuitive.
>> ÂAnd it'll really hurt if I set printk_delay_per_lines=10000!
>>
>> ÂI'd expect the kernel to pause for printk_delay_msec every N lines,
>> Âand I think that would be better.
>
> Fine to me as well. Thanks for your comments.
>

BTW, for the sysctl variables. I found in kernel.h there's:
printk/console_loglevel/panic related extern variables for sysctl.

console_loglevel and panic related variables are used here and there,
not just for sysctl, so I think it should stay in kernel.h.

The printk related variables for sysctl and all the extern variables in
sysctl.c can be move to sysctl.h. Seems no need new head file.

Andrew, what's your opinion?

> --
> Regards
> dave
>



-- 
Regards
dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/