From: Dmitry Safonov <dima@arista.com>
To: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
linux-kernel@vger.kernel.org
Cc: Dmitry Safonov <0x7f454c46@gmail.com>,
Andrew Morton <akpm@linux-foundation.org>,
Dmitry Vyukov <dvyukov@google.com>,
Ingo Molnar <mingo@kernel.org>, Jonathan Corbet <corbet@lwn.net>,
Thomas Gleixner <tglx@linutronix.de>,
"Peter Zijlstra (Intel)" <peterz@infradead.org>,
Vasiliy Khoruzhick <vasilykh@arista.com>,
linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] hung_task: Allow printing warnings every check interval
Date: Fri, 26 Jul 2019 14:58:31 +0100 [thread overview]
Message-ID: <eb3d9058-a623-3c10-68e2-f11498a7c174@arista.com> (raw)
In-Reply-To: <41fd7652-df1f-26f6-aba0-b87ebae07db6@i-love.sakura.ne.jp>
On 7/26/19 2:28 PM, Tetsuo Handa wrote:
> On 2019/07/26 20:29, Tetsuo Handa wrote:
>> On 2019/07/25 23:25, Dmitry Safonov wrote:
>>> Yes, also current distributions already using the counter to print
>>> warnings number of times and then silently ignore. I.e., on my Arch
>>> Linux setup:
>>> hung_task_warnings:10
>>
>> You can propose changing the default value of hung_task_warnings to -1.
>>
>> Current patch might be inconvenient because printk() from hung_task_warning(t, false)
>> fails to go to consoles when that "t" was blocked for more than "timeout" seconds, for
>>
>> if (sysctl_hung_task_panic) {
>> console_verbose();
>> hung_task_show_lock = true;
>> hung_task_call_panic = true;
>> }
>>
>> path which is intended to force printk() to go to consoles is ignored by
>>
>> /* Don't print warings twice */
>> if (!sysctl_hung_task_interval_warnings)
>> hung_task_warning(t, true);
>>
>> when panic() should be called. (The vmcore would contain the printk() output which
>> was not sent to consoles if kdump is configured. But vmcore is not always available.)
Fair enough.
>>> Yes, that's why it's disabled by default (=0).
>>> I tend to agree that printing with KERN_DEBUG may be better, but in my
>>> point of view the patch isn't enough justification for patching
>>> sched_show_task() and show_stack().
>>
>> You can propose sched_show_task_log_lvl() and show_stack_log_lvl() like show_trace_log_lvl().
I'll try, not sure how well it will go..
>>
>> I think that sysctl_hung_task_interval_warnings should not be decremented automatically.
>> I guess that that variable should become a boolean which controls whether to report threads
>> (with KERN_DEBUG level) which was blocked for more than sysctl_hung_task_check_interval_secs
>> seconds (or a tristate which also controls whether the report should be sent to consoles
>> (because KERN_DEBUG level likely prevents sending to consoles)), and
>> hung_task_warning(t, false) should be called like
>>
>> if (time_is_after_jiffies(t->last_switch_time + timeout * HZ)) {
>> if (sysctl_hung_task_interval_warnings)
>> hung_task_warning(t, false);
>> return;
>> }
>>
>> rather than
>>
>> if (sysctl_hung_task_interval_warnings)
>> hung_task_warning(t, false);
>> if (time_is_after_jiffies(t->last_switch_time + timeout * HZ))
>> return;
Good point, will do.
> Well, another direction is to disassociate sysctl_hung_task_panic from
> sysctl_hung_task_timeout_secs. Since nobody would want to call panic() when
> a thread was blocked for only one second, allow sysctl_hung_task_panic to
> specify larger than 1, and interpret it as sysctl_hung_task_timeout_secs for
> calling panic(). Roughly speaking:
>
> - if (sysctl_hung_task_panic) {
> + unsigned long panic_timeout = READ_ONCE(sysctl_hung_task_panic)
> + if (panic_timeout == 1 || (panic_timeout > 1 &&
> + (jiffies - t->last_switch_time) / HZ >= panic_timeout)) {
> console_verbose();
> hung_task_show_lock = true;
> hung_task_call_panic = true;
> }
>
> If use of different loglevel is not a requirement for you, this would be the simplest.
No, we consider such messages as notifications/warnings, rather than
complete failures. So, it would be better to hide them from console.
They're also not rate-limited which is a bummer with slow serial
consoles that we've on some devices (9600).
Thanks for the review,
Dmitry
prev parent reply other threads:[~2019-07-26 13:58 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-24 17:02 [PATCH] hung_task: Allow printing warnings every check interval Dmitry Safonov
2019-07-25 10:38 ` Tetsuo Handa
2019-07-25 14:25 ` Dmitry Safonov
2019-07-26 11:29 ` Tetsuo Handa
2019-07-26 13:28 ` Tetsuo Handa
2019-07-26 13:58 ` Dmitry Safonov [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=eb3d9058-a623-3c10-68e2-f11498a7c174@arista.com \
--to=dima@arista.com \
--cc=0x7f454c46@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=corbet@lwn.net \
--cc=dvyukov@google.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=penguin-kernel@i-love.sakura.ne.jp \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=vasilykh@arista.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).