Linux-Doc Archive on lore.kernel.org
 help / color / Atom feed
From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
To: Dmitry Safonov <dima@arista.com>, 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 22:28:53 +0900
Message-ID: <41fd7652-df1f-26f6-aba0-b87ebae07db6@i-love.sakura.ne.jp> (raw)
In-Reply-To: <9a919c32-4e0e-ca1b-887f-c329543913d3@i-love.sakura.ne.jp>

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.)
> 
>> 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 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;
> 
> .
> 


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.

  reply index

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-24 17:02 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 [this message]
2019-07-26 13:58         ` Dmitry Safonov

Reply instructions:

You may reply publically 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=41fd7652-df1f-26f6-aba0-b87ebae07db6@i-love.sakura.ne.jp \
    --to=penguin-kernel@i-love.sakura.ne.jp \
    --cc=0x7f454c46@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=corbet@lwn.net \
    --cc=dima@arista.com \
    --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=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

Linux-Doc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-doc/0 linux-doc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-doc linux-doc/ https://lore.kernel.org/linux-doc \
		linux-doc@vger.kernel.org
	public-inbox-index linux-doc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-doc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git