RE: [PATCH] kernel/hung_task.c: force ignore_loglevel before panic

2018-12-12 Thread Liu, Chuansheng 



> -Original Message-
> From: Petr Mladek [mailto:pmla...@suse.com]
> Sent: Wednesday, December 12, 2018 6:18 PM
> To: Liu, Chuansheng 
> Cc: Tetsuo Handa ; Sergey Senozhatsky
> ; a...@linux-foundation.org;
> sergey.senozhat...@gmail.com; rost...@goodmis.org; dvyu...@google.com;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] kernel/hung_task.c: force ignore_loglevel before panic
> 
> On Wed 2018-12-12 01:16:11, Liu, Chuansheng wrote:
> >
> >
> > > -Original Message-
> > > From: Tetsuo Handa [mailto:penguin-ker...@i-love.sakura.ne.jp]
> > > Sent: Tuesday, December 11, 2018 6:02 PM
> > > To: Liu, Chuansheng ; Sergey Senozhatsky
> > > 
> > > Cc: a...@linux-foundation.org; pmla...@suse.com;
> > > sergey.senozhat...@gmail.com; rost...@goodmis.org;
> dvyu...@google.com;
> > > linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH] kernel/hung_task.c: force ignore_loglevel before 
> > > panic
> > >
> > > On 2018/12/11 10:16, Liu, Chuansheng wrote:
> > > > We may enhance it by:
> > > > -   if (sysctl_hung_task_warnings) {
> > > > +   if (sysctl_hung_task_panic || sysctl_hung_task_warnings) {
> > > > if (sysctl_hung_task_warnings > 0)
> > > > sysctl_hung_task_warnings--;
> > >
> > > Why ignore sysctl_hung_task_warnings? The administrator can already
> > > configure as sysctl_hung_task_warnings == -1 && sysctl_hung_task_panic
> == 1
> > > if he/she does not want to suppress neither sched_show_task() nor
> > > debug_show_all_locks()/trigger_all_cpu_backtrace(). Someone might want
> that
> > > sysctl_hung_task_warnings == 0 (which is a request to suppress only
> > > sched_show_task()) should not be ignored by sysctl_hung_task_panic == 1
> > > (which is a request to trigger panic).
> >
> >
> > My complete idea is in patch V1 which has been sent. Paste here:
> > If sysctl_hung_task_panic == 1, I will force sched_show_task(t) and set
> > hung_task_call_panic = true
> > hung_task_show_lock = true
> 
> Please, do not mix two changes into one patch.
Thanks your suggestion.

> 
> Add console_verbose() in one patch. It is simple and
> everyone has agreed with it so far.
Has sent it out, please help review.

> 
> Force sched_show_task() when hung_task_call_panic == 1 in
> another patch. It seems to be controversial and should be
> discussed/changed separately.
I found some other points also, and will send out patches later.

Re: [PATCH] kernel/hung_task.c: force ignore_loglevel before panic

2018-12-12 Thread Petr Mladek 
On Wed 2018-12-12 01:16:11, Liu, Chuansheng wrote:
> 
> 
> > -Original Message-
> > From: Tetsuo Handa [mailto:penguin-ker...@i-love.sakura.ne.jp]
> > Sent: Tuesday, December 11, 2018 6:02 PM
> > To: Liu, Chuansheng ; Sergey Senozhatsky
> > 
> > Cc: a...@linux-foundation.org; pmla...@suse.com;
> > sergey.senozhat...@gmail.com; rost...@goodmis.org; dvyu...@google.com;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] kernel/hung_task.c: force ignore_loglevel before panic
> > 
> > On 2018/12/11 10:16, Liu, Chuansheng wrote:
> > > We may enhance it by:
> > > -   if (sysctl_hung_task_warnings) {
> > > +   if (sysctl_hung_task_panic || sysctl_hung_task_warnings) {
> > > if (sysctl_hung_task_warnings > 0)
> > > sysctl_hung_task_warnings--;
> > 
> > Why ignore sysctl_hung_task_warnings? The administrator can already
> > configure as sysctl_hung_task_warnings == -1 && sysctl_hung_task_panic == 1
> > if he/she does not want to suppress neither sched_show_task() nor
> > debug_show_all_locks()/trigger_all_cpu_backtrace(). Someone might want that
> > sysctl_hung_task_warnings == 0 (which is a request to suppress only
> > sched_show_task()) should not be ignored by sysctl_hung_task_panic == 1
> > (which is a request to trigger panic).
> 
> 
> My complete idea is in patch V1 which has been sent. Paste here:
> If sysctl_hung_task_panic == 1, I will force sched_show_task(t) and set
> hung_task_call_panic = true
> hung_task_show_lock = true

Please, do not mix two changes into one patch.

Add console_verbose() in one patch. It is simple and
everyone has agreed with it so far.

Force sched_show_task() when hung_task_call_panic == 1 in
another patch. It seems to be controversial and should be
discussed/changed separately.

Best Regards,
Petr

Re: [PATCH] kernel/hung_task.c: force ignore_loglevel before panic

2018-12-11 Thread Petr Mladek 
On Tue 2018-12-11 01:16:10, Liu, Chuansheng wrote:
> > From: Tetsuo Handa [mailto:penguin-ker...@i-love.sakura.ne.jp]
> > On 2018/12/10 15:11, Sergey Senozhatsky wrote:
> > > On (12/10/18 05:58), Liu, Chuansheng wrote:
> > >>> On (12/10/18 05:40), Liu, Chuansheng wrote:
> >  @@ -130,6 +130,13 @@ static void check_hung_task(struct task_struct

> > -if (!sysctl_hung_task_warnings && !sysctl_hung_task_panic)
> > -return;
> > +if (sysctl_hung_task_panic) {
> > +console_verbose();
> > +hung_task_show_lock = true;
> > +hung_task_call_panic = true;
> > +}
> > (...snipped...)
> > -if (sysctl_hung_task_panic) {
> > -hung_task_show_lock = true;
> > -hung_task_call_panic = true;
> > -}
> Thanks Tetsuo, I prefer this option, which makes code more readable.

I like this variant as well. Also using console_verbose() looks
like a better choice.


> More thoughts in this condition of sysctl_hung_task_warnings == 0 && 
> sysctl_hung_task_panic == 1,
> in this case, debug_show_all_locks() may not output useful information if 
> LOCK DEBUG config is not enabled.
> trigger_all_cpu_backtrace() will not show the hung task for debugging either.
> 
> We may enhance it by:
> -   if (sysctl_hung_task_warnings) {
> +   if (sysctl_hung_task_panic || sysctl_hung_task_warnings) {
> if (sysctl_hung_task_warnings > 0)
> sysctl_hung_task_warnings--;

I agree with Tetsuo that we should not touch this.

The warnings will get typically printed because panic() will get
triggered on the first occasion. And we should respect when the admin
disables the warnings completely.

Best Regards,
Petr

Re: [PATCH] kernel/hung_task.c: force ignore_loglevel before panic

2018-12-11 Thread Tetsuo Handa 
On 2018/12/11 10:16, Liu, Chuansheng wrote:
> We may enhance it by:
> -   if (sysctl_hung_task_warnings) {
> +   if (sysctl_hung_task_panic || sysctl_hung_task_warnings) {
> if (sysctl_hung_task_warnings > 0)
> sysctl_hung_task_warnings--;

Why ignore sysctl_hung_task_warnings? The administrator can already
configure as sysctl_hung_task_warnings == -1 && sysctl_hung_task_panic == 1
if he/she does not want to suppress neither sched_show_task() nor
debug_show_all_locks()/trigger_all_cpu_backtrace(). Someone might want
that sysctl_hung_task_warnings == 0 (which is a request to suppress only
sched_show_task()) should not be ignored by sysctl_hung_task_panic == 1
(which is a request to trigger panic).

RE: [PATCH] kernel/hung_task.c: force ignore_loglevel before panic

2018-12-10 Thread Liu, Chuansheng 


> -Original Message-
> From: Tetsuo Handa [mailto:penguin-ker...@i-love.sakura.ne.jp]
> Sent: Monday, December 10, 2018 5:59 PM
> To: Sergey Senozhatsky ; Liu,
> Chuansheng 
> Cc: a...@linux-foundation.org; pmla...@suse.com;
> sergey.senozhat...@gmail.com; rost...@goodmis.org; dvyu...@google.com;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] kernel/hung_task.c: force ignore_loglevel before panic
> 
> On 2018/12/10 15:11, Sergey Senozhatsky wrote:
> > On (12/10/18 05:58), Liu, Chuansheng wrote:
> >>> On (12/10/18 05:40), Liu, Chuansheng wrote:
> >>>> @@ -130,6 +130,13 @@ static void check_hung_task(struct task_struct
> >>>> *t,
> >>> unsigned long timeout)
> >>>> init_utsname()->version);
> >>>> pr_err("\"echo 0 > 
> >>>> /proc/sys/kernel/hung_task_timeout_secs\""
> >>>> " disables this message.\n");
> >>>> +   /* When sysctl_hung_task_panic is set, we have to force
> >>>> +* ignore_loglevel to get really useful hung task
> >>>> +* information.
> >>>> +*/
> >>>> +   if (sysctl_hung_task_panic && !ignore_loglevel)
> >>>> +   ignore_loglevel = true;
> >>>
> >>>   console_verbose()?
> >>
> >> Thanks Sergey, it is really my need. I will prepare for a new version
> >> of patch:)
> >
> > Let's wait for people to take a look at this patch first.
> 
> Shouldn't console_verbose() be called like
> 
> -if (!sysctl_hung_task_warnings && !sysctl_hung_task_panic)
> +if (sysctl_hung_task_panic)
> +console_verbose();
> +else if (!sysctl_hung_task_warnings)
>  return;
> 
> or
> 
> -if (!sysctl_hung_task_warnings && !sysctl_hung_task_panic)
> -return;
> +if (sysctl_hung_task_panic)
> +console_verbose();
> 
> or
> 
> -if (!sysctl_hung_task_warnings && !sysctl_hung_task_panic)
> -return;
> +if (sysctl_hung_task_panic) {
> +console_verbose();
> +hung_task_show_lock = true;
> +hung_task_call_panic = true;
> +}
> (...snipped...)
> -if (sysctl_hung_task_panic) {
> -hung_task_show_lock = true;
> -hung_task_call_panic = true;
> -}
Thanks Tetsuo, I prefer this option, which makes code more readable.

> 
> so that sysctl_hung_task_warnings == 0 && sysctl_hung_task_panic == 1 will
> call debug_show_all_locks() and trigger_all_cpu_backtrace() with verbose 
> level?
More thoughts in this condition of sysctl_hung_task_warnings == 0 && 
sysctl_hung_task_panic == 1,
in this case, debug_show_all_locks() may not output useful information if LOCK 
DEBUG config is not enabled.
trigger_all_cpu_backtrace() will not show the hung task for debugging either.

We may enhance it by:
-   if (sysctl_hung_task_warnings) {
+   if (sysctl_hung_task_panic || sysctl_hung_task_warnings) {
if (sysctl_hung_task_warnings > 0)
sysctl_hung_task_warnings--;

Re: [PATCH] kernel/hung_task.c: force ignore_loglevel before panic

2018-12-10 Thread Sergey Senozhatsky 
On (12/10/18 18:58), Tetsuo Handa wrote:
>  +*/
>  +   if (sysctl_hung_task_panic && !ignore_loglevel)
>  +   ignore_loglevel = true;
> >>>
> >>>   console_verbose()?
> >>
> >> Thanks Sergey, it is really my need. I will prepare for a new version of 
> >> patch:)
> > 
> > Let's wait for people to take a look at this patch first.


> Shouldn't console_verbose() be called like
> 
> +if (sysctl_hung_task_panic)
> +console_verbose();

Sure, that was what I meant. IOW console_verbose() instead of
`ignore_loglevel = true' assignment.

-ss

Re: [PATCH] kernel/hung_task.c: force ignore_loglevel before panic

2018-12-10 Thread Tetsuo Handa 
On 2018/12/10 15:11, Sergey Senozhatsky wrote:
> On (12/10/18 05:58), Liu, Chuansheng wrote:
>>> On (12/10/18 05:40), Liu, Chuansheng wrote:
 @@ -130,6 +130,13 @@ static void check_hung_task(struct task_struct *t,
>>> unsigned long timeout)
 init_utsname()->version);
 pr_err("\"echo 0 > 
 /proc/sys/kernel/hung_task_timeout_secs\""
 " disables this message.\n");
 +   /* When sysctl_hung_task_panic is set, we have to force
 +* ignore_loglevel to get really useful hung task
 +* information.
 +*/
 +   if (sysctl_hung_task_panic && !ignore_loglevel)
 +   ignore_loglevel = true;
>>>
>>> console_verbose()?
>>
>> Thanks Sergey, it is really my need. I will prepare for a new version of 
>> patch:)
> 
> Let's wait for people to take a look at this patch first.

Shouldn't console_verbose() be called like

-if (!sysctl_hung_task_warnings && !sysctl_hung_task_panic)
+if (sysctl_hung_task_panic)
+console_verbose();
+else if (!sysctl_hung_task_warnings)
 return;

or

-if (!sysctl_hung_task_warnings && !sysctl_hung_task_panic)
-return;
+if (sysctl_hung_task_panic)
+console_verbose();

or

-if (!sysctl_hung_task_warnings && !sysctl_hung_task_panic)
-return;
+if (sysctl_hung_task_panic) {
+console_verbose();
+hung_task_show_lock = true;
+hung_task_call_panic = true;
+}
(...snipped...)
-if (sysctl_hung_task_panic) {
-hung_task_show_lock = true;
-hung_task_call_panic = true;
-}

so that sysctl_hung_task_warnings == 0 && sysctl_hung_task_panic == 1
will call debug_show_all_locks() and trigger_all_cpu_backtrace() with
verbose level?

Re: [PATCH] kernel/hung_task.c: force ignore_loglevel before panic

2018-12-09 Thread Sergey Senozhatsky 
On (12/10/18 05:58), Liu, Chuansheng wrote:
> > On (12/10/18 05:40), Liu, Chuansheng wrote:
> > > @@ -130,6 +130,13 @@ static void check_hung_task(struct task_struct *t,
> > unsigned long timeout)
> > > init_utsname()->version);
> > > pr_err("\"echo 0 > 
> > > /proc/sys/kernel/hung_task_timeout_secs\""
> > > " disables this message.\n");
> > > +   /* When sysctl_hung_task_panic is set, we have to force
> > > +* ignore_loglevel to get really useful hung task
> > > +* information.
> > > +*/
> > > +   if (sysctl_hung_task_panic && !ignore_loglevel)
> > > +   ignore_loglevel = true;
> > 
> > console_verbose()?
> 
> Thanks Sergey, it is really my need. I will prepare for a new version of 
> patch:)

Let's wait for people to take a look at this patch first.

-ss

RE: [PATCH] kernel/hung_task.c: force ignore_loglevel before panic

2018-12-09 Thread Liu, Chuansheng 



> -Original Message-
> From: Sergey Senozhatsky [mailto:sergey.senozhatsky.w...@gmail.com]
> Sent: Monday, December 10, 2018 1:46 PM
> To: Liu, Chuansheng 
> Cc: a...@linux-foundation.org; pmla...@suse.com;
> sergey.senozhat...@gmail.com; rost...@goodmis.org; dvyu...@google.com;
> penguin-ker...@i-love.sakura.ne.jp; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] kernel/hung_task.c: force ignore_loglevel before panic
> 
> On (12/10/18 05:40), Liu, Chuansheng wrote:
> > @@ -130,6 +130,13 @@ static void check_hung_task(struct task_struct *t,
> unsigned long timeout)
> > init_utsname()->version);
> > pr_err("\"echo 0 > 
> > /proc/sys/kernel/hung_task_timeout_secs\""
> > " disables this message.\n");
> > +   /* When sysctl_hung_task_panic is set, we have to force
> > +* ignore_loglevel to get really useful hung task
> > +* information.
> > +*/
> > +   if (sysctl_hung_task_panic && !ignore_loglevel)
> > +   ignore_loglevel = true;
> 
>   console_verbose()?

Thanks Sergey, it is really my need. I will prepare for a new version of patch:)

Re: [PATCH] kernel/hung_task.c: force ignore_loglevel before panic

2018-12-09 Thread Sergey Senozhatsky 
On (12/10/18 05:40), Liu, Chuansheng wrote:
> @@ -130,6 +130,13 @@ static void check_hung_task(struct task_struct *t, 
> unsigned long timeout)
> init_utsname()->version);
> pr_err("\"echo 0 > /proc/sys/kernel/hung_task_timeout_secs\""
> " disables this message.\n");
> +   /* When sysctl_hung_task_panic is set, we have to force
> +* ignore_loglevel to get really useful hung task
> +* information.
> +*/
> +   if (sysctl_hung_task_panic && !ignore_loglevel)
> +   ignore_loglevel = true;

console_verbose()?

-ss

[PATCH] kernel/hung_task.c: force ignore_loglevel before panic

2018-12-09 Thread Liu, Chuansheng 


Based on patch commit 401c636a0eeb ("kernel/hung_task.c:
show all hung tasks before panic"), we could get the
call stack of hung task.

However, if the console loglevel is not high, we still
can not get the useful information in practice, and
in most cases users don't set console loglevel to
high level.

This patch is to force ignore_loglevel before system
panic, so that the real useful information can be seen
in the console, instead of being like the following,
which doesn't have hung task information.

[  246.916600] INFO: task init:1 blocked for more than 120 seconds.
[  246.922320]   Tainted: G U  W 
4.19.0-quilt-2e5dc0ac-g51b6c21d76cc #1
[  246.926790] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this 
message.
[  246.932553] Kernel panic - not syncing: hung_task: blocked tasks
[  246.938503] CPU: 2 PID: 479 Comm: khungtaskd Tainted: G U  W 
4.19.0-quilt-2e5dc0ac-g51b6c21d76cc #1
[  246.990266] Call Trace:
[  246.991707]  dump_stack+0x4f/0x65
[  246.993710]  panic+0xde/0x231
[  246.995445]  watchdog+0x290/0x410
[  246.997390]  kthread+0x12c/0x150
[  246.999301]  ? reset_hung_task_detector+0x20/0x20
[  247.004825]  ? kthread_create_worker_on_cpu+0x70/0x70
[  247.007735]  ret_from_fork+0x35/0x40
[  247.010280] reboot: panic mode set: p,w
[  247.012619] Kernel Offset: 0x3400 from 0x8100 (relocation 
range: 0x8000-0xbfff)

Signed-off-by: Chuansheng Liu 
---
 include/linux/printk.h | 2 +-
 kernel/hung_task.c | 7 +++
 kernel/printk/printk.c | 2 +-
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index cf3eccf..24748c1 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -59,8 +59,8 @@ static inline const char *printk_skip_headers(const char 
*buffer)
  */
 #define CONSOLE_LOGLEVEL_DEFAULT CONFIG_CONSOLE_LOGLEVEL_DEFAULT
 #define CONSOLE_LOGLEVEL_QUIET  CONFIG_CONSOLE_LOGLEVEL_QUIET
-
 extern int console_printk[];
+extern bool ignore_loglevel;

 #define console_loglevel (console_printk[0])
 #define default_message_loglevel (console_printk[1])
diff --git a/kernel/hung_task.c b/kernel/hung_task.c
index cb8e3e8..7d942d1 100644
--- a/kernel/hung_task.c
+++ b/kernel/hung_task.c
@@ -130,6 +130,13 @@ static void check_hung_task(struct task_struct *t, 
unsigned long timeout)
init_utsname()->version);
pr_err("\"echo 0 > /proc/sys/kernel/hung_task_timeout_secs\""
" disables this message.\n");
+   /* When sysctl_hung_task_panic is set, we have to force
+* ignore_loglevel to get really useful hung task
+* information.
+*/
+   if (sysctl_hung_task_panic && !ignore_loglevel)
+   ignore_loglevel = true;
+
sched_show_task(t);
hung_task_show_lock = true;
}
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 1b2a029..31a7a56 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1135,7 +1135,7 @@ void __init setup_log_buf(int early)
free, (free * 100) / __LOG_BUF_LEN);
 }

-static bool __read_mostly ignore_loglevel;
+bool __read_mostly ignore_loglevel;

 static int __init ignore_loglevel_setup(char *str)
 {
--
2.7.4