[PATCH] kernel/hung_task.c: use timeout diff when timeout is updated

[PATCH] kernel/hung_task.c: use timeout diff when timeout is updated

[Tetsuo Handa]

>From 529ff00b556e110c6e801c39e94b06f559307136 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Thu, 17 Dec 2015 16:27:08 +0900
Subject: [PATCH] kernel/hung_task.c: use timeout diff when timeout is updated

When new timeout is written to /proc/sys/kernel/hung_task_timeout_secs,
khungtaskd is interrupted and again sleeps for full timeout duration.

This means that hang task will not be checked if new timeout is written
periodically within old timeout duration and/or checking of hang task
will be delayed for up to previous timeout duration.
Fix this by remembering last time khungtaskd checked hang task.

This change will allow other watchdog tasks (if any) to share khungtaskd
by sleeping for minimal timeout diff of all watchdog tasks. Doing more
watchdog tasks from khungtaskd will reduce the possibility of printk()
collisions by multiple watchdog threads.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 kernel/hung_task.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/kernel/hung_task.c b/kernel/hung_task.c
index e0f90c2..d234022 100644
--- a/kernel/hung_task.c
+++ b/kernel/hung_task.c
@@ -185,10 +185,12 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)
 	rcu_read_unlock();
 }
 
-static unsigned long timeout_jiffies(unsigned long timeout)
+static long hung_timeout_jiffies(unsigned long last_checked,
+				 unsigned long timeout)
 {
 	/* timeout of 0 will disable the watchdog */
-	return timeout ? timeout * HZ : MAX_SCHEDULE_TIMEOUT;
+	return timeout ? last_checked - jiffies + timeout * HZ :
+		MAX_SCHEDULE_TIMEOUT;
 }
 
 /*
@@ -224,18 +226,21 @@ EXPORT_SYMBOL_GPL(reset_hung_task_detector);
  */
 static int watchdog(void *dummy)
 {
+	unsigned long hung_last_checked = jiffies;
+
 	set_user_nice(current, 0);
 
 	for ( ; ; ) {
 		unsigned long timeout = sysctl_hung_task_timeout_secs;
+		long t = hung_timeout_jiffies(hung_last_checked, timeout);
 
-		while (schedule_timeout_interruptible(timeout_jiffies(timeout)))
-			timeout = sysctl_hung_task_timeout_secs;
-
-		if (atomic_xchg(&reset_hung_task, 0))
+		if (t <= 0) {
+			if (!atomic_xchg(&reset_hung_task, 0))
+				check_hung_uninterruptible_tasks(timeout);
+			hung_last_checked = jiffies;
 			continue;
-
-		check_hung_uninterruptible_tasks(timeout);
+		}
+		schedule_timeout_interruptible(t);
 	}
 
 	return 0;
-- 
1.8.3.1

Re: [PATCH] kernel/hung_task.c: use timeout diff when timeout is updated

[Andrew Morton]

On Thu, 17 Dec 2015 21:23:03 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> >From 529ff00b556e110c6e801c39e94b06f559307136 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Thu, 17 Dec 2015 16:27:08 +0900
> Subject: [PATCH] kernel/hung_task.c: use timeout diff when timeout is updated
> 
> When new timeout is written to /proc/sys/kernel/hung_task_timeout_secs,
> khungtaskd is interrupted and again sleeps for full timeout duration.
> 
> This means that hang task will not be checked if new timeout is written
> periodically within old timeout duration and/or checking of hang task
> will be delayed for up to previous timeout duration.
> Fix this by remembering last time khungtaskd checked hang task.
> 
> This change will allow other watchdog tasks (if any) to share khungtaskd
> by sleeping for minimal timeout diff of all watchdog tasks. Doing more
> watchdog tasks from khungtaskd will reduce the possibility of printk()
> collisions by multiple watchdog threads.

This seems like reasonable behaviour, but it is a
non-backward-compatible change.  I don't know how important that is -
probably "not very".

Please let's fully describe this behaviour in the documentation. 
Documentation/sysctl/kernel.txt.

It appears that if userspace writes a new timeout which has already
expired, a check is triggered immediately, correct?  Let's ensure that
this is documented as well.   And tested!

And it would be helpful to add a comment to hung_timeout_jiffies()
which describes the behaviour and explains the reasons for it.

Re: [PATCH] kernel/hung_task.c: use timeout diff when timeout is updated

[Tetsuo Handa]

Andrew Morton wrote:
> On Thu, 17 Dec 2015 21:23:03 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
> 
> > >From 529ff00b556e110c6e801c39e94b06f559307136 Mon Sep 17 00:00:00 2001
> > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Date: Thu, 17 Dec 2015 16:27:08 +0900
> > Subject: [PATCH] kernel/hung_task.c: use timeout diff when timeout is updated
> > 
> > When new timeout is written to /proc/sys/kernel/hung_task_timeout_secs,
> > khungtaskd is interrupted and again sleeps for full timeout duration.
> > 
> > This means that hang task will not be checked if new timeout is written
> > periodically within old timeout duration and/or checking of hang task
> > will be delayed for up to previous timeout duration.
> > Fix this by remembering last time khungtaskd checked hang task.
> > 
> > This change will allow other watchdog tasks (if any) to share khungtaskd
> > by sleeping for minimal timeout diff of all watchdog tasks. Doing more
> > watchdog tasks from khungtaskd will reduce the possibility of printk()
> > collisions by multiple watchdog threads.
> 
> This seems like reasonable behaviour, but it is a
> non-backward-compatible change.  I don't know how important that is -
> probably "not very".
> 
> Please let's fully describe this behaviour in the documentation. 
> Documentation/sysctl/kernel.txt.
> 
> It appears that if userspace writes a new timeout which has already
> expired, a check is triggered immediately, correct?  Let's ensure that
> this is documented as well.   And tested!

Yes.

> 
> And it would be helpful to add a comment to hung_timeout_jiffies()
> which describes the behaviour and explains the reasons for it.

But before doing it, I'd like to confirm hung task maintainer's will.

The reason I proposed this patch is that I want to add a watchdog task
which emits warning messages when memory allocations are stalling.
http://lkml.kernel.org/r/201512130033.ABH90650.FtFOMOFLVOJHQS@I-love.SAKURA.ne.jp

But concurrently emitting multiple backtraces is problematic. Concurrent
emitting by hung task watchdog and memory allocation stall watchdog is very
likely to occur, for it is likely that other task is also stuck in
uninterruptible sleep when one task got stuck at memory allocation.

Therefore, I started trying to use same thread for both watchdogs.
A draft patch is at
http://lkml.kernel.org/r/201512170011.IAC73451.FLtFMSJHOQFVOO@I-love.SAKURA.ne.jp .

If you prefer current hang task behavior, I'll try to preseve current
behavior. Instead, I might use two threads and try to mutex both watchdogs
using console_lock() or something like that.

So, may I ask what your preference is?

Re: [PATCH] kernel/hung_task.c: use timeout diff when timeout is updated

[Andrew Morton]

On Mon, 21 Dec 2015 20:45:23 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
> > 
> > And it would be helpful to add a comment to hung_timeout_jiffies()
> > which describes the behaviour and explains the reasons for it.
> 
> But before doing it, I'd like to confirm hung task maintainer's will.
> 
> The reason I proposed this patch is that I want to add a watchdog task
> which emits warning messages when memory allocations are stalling.
> http://lkml.kernel.org/r/201512130033.ABH90650.FtFOMOFLVOJHQS@I-love.SAKURA.ne.jp
> 
> But concurrently emitting multiple backtraces is problematic. Concurrent
> emitting by hung task watchdog and memory allocation stall watchdog is very
> likely to occur, for it is likely that other task is also stuck in
> uninterruptible sleep when one task got stuck at memory allocation.
> 
> Therefore, I started trying to use same thread for both watchdogs.
> A draft patch is at
> http://lkml.kernel.org/r/201512170011.IAC73451.FLtFMSJHOQFVOO@I-love.SAKURA.ne.jp .
> 
> If you prefer current hang task behavior, I'll try to preseve current
> behavior. Instead, I might use two threads and try to mutex both watchdogs
> using console_lock() or something like that.
> 
> So, may I ask what your preference is?

I've added linux-mm to Cc.  Please never forget that.

The general topic here is "add more diagnostics around an out-of-memory
event".  Clearly we need this, but Michal is working on the same thing
as part of his "OOM detection rework v4" work, so can we please do the
appropriate coordination and review there?

Preventing watchdog-triggered backtraces from messing each other up is
of course a good idea.  Your malloc watchdog patch adds a surprising
amount of code and adding yet another kernel thread is painful but
perhaps it's all worth it.  It's a matter of people reviewing, testing
and using the code in realistic situations and that process has hardly
begun, alas.

Sorry, that was waffly but I don't feel able to be more definite at
this time.

Re: [PATCH] kernel/hung_task.c: use timeout diff when timeout is updated

[Andrew Morton]

On Mon, 21 Dec 2015 13:45:45 -0800 Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 21 Dec 2015 20:45:23 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
> > > 
> > > And it would be helpful to add a comment to hung_timeout_jiffies()
> > > which describes the behaviour and explains the reasons for it.
> > 
> > But before doing it, I'd like to confirm hung task maintainer's will.
> > 
> > The reason I proposed this patch is that I want to add a watchdog task
> > which emits warning messages when memory allocations are stalling.
> > http://lkml.kernel.org/r/201512130033.ABH90650.FtFOMOFLVOJHQS@I-love.SAKURA.ne.jp
> > 
> > But concurrently emitting multiple backtraces is problematic. Concurrent
> > emitting by hung task watchdog and memory allocation stall watchdog is very
> > likely to occur, for it is likely that other task is also stuck in
> > uninterruptible sleep when one task got stuck at memory allocation.
> > 
> > Therefore, I started trying to use same thread for both watchdogs.
> > A draft patch is at
> > http://lkml.kernel.org/r/201512170011.IAC73451.FLtFMSJHOQFVOO@I-love.SAKURA.ne.jp .
> > 
> > If you prefer current hang task behavior, I'll try to preseve current
> > behavior. Instead, I might use two threads and try to mutex both watchdogs
> > using console_lock() or something like that.
> > 
> > So, may I ask what your preference is?
> 
> I've added linux-mm to Cc.  Please never forget that.
> 
> The general topic here is "add more diagnostics around an out-of-memory
> event".  Clearly we need this, but Michal is working on the same thing
> as part of his "OOM detection rework v4" work, so can we please do the
> appropriate coordination and review there?
> 
> Preventing watchdog-triggered backtraces from messing each other up is
> of course a good idea.  Your malloc watchdog patch adds a surprising
> amount of code and adding yet another kernel thread is painful but
> perhaps it's all worth it.  It's a matter of people reviewing, testing
> and using the code in realistic situations and that process has hardly
> begun, alas.
> 
> Sorry, that was waffly but I don't feel able to be more definite at
> this time.

So this patch is rather stuck in place. 

Can we please work out how to proceed?  I don't like hanging onto
limbopatches for ages.



From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Subject: kernel/hung_task.c: use timeout diff when timeout is updated

When new timeout is written to /proc/sys/kernel/hung_task_timeout_secs,
khungtaskd is interrupted and again sleeps for full timeout duration.

This means that hang task will not be checked if new timeout is written
periodically within old timeout duration and/or checking of hang task will
be delayed for up to previous timeout duration.  Fix this by remembering
last time khungtaskd checked hang task.

This change will allow other watchdog tasks (if any) to share khungtaskd
by sleeping for minimal timeout diff of all watchdog tasks.  Doing more
watchdog tasks from khungtaskd will reduce the possibility of printk()
collisions by multiple watchdog threads.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Aaron Tomlin <atomlin@redhat.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/hung_task.c |   21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff -puN kernel/hung_task.c~kernel-hung_taskc-use-timeout-diff-when-timeout-is-updated kernel/hung_task.c
--- a/kernel/hung_task.c~kernel-hung_taskc-use-timeout-diff-when-timeout-is-updated
+++ a/kernel/hung_task.c
@@ -185,10 +185,12 @@ static void check_hung_uninterruptible_t
 	rcu_read_unlock();
 }
 
-static unsigned long timeout_jiffies(unsigned long timeout)
+static long hung_timeout_jiffies(unsigned long last_checked,
+				 unsigned long timeout)
 {
 	/* timeout of 0 will disable the watchdog */
-	return timeout ? timeout * HZ : MAX_SCHEDULE_TIMEOUT;
+	return timeout ? last_checked - jiffies + timeout * HZ :
+		MAX_SCHEDULE_TIMEOUT;
 }
 
 /*
@@ -224,18 +226,21 @@ EXPORT_SYMBOL_GPL(reset_hung_task_detect
  */
 static int watchdog(void *dummy)
 {
+	unsigned long hung_last_checked = jiffies;
+
 	set_user_nice(current, 0);
 
 	for ( ; ; ) {
 		unsigned long timeout = sysctl_hung_task_timeout_secs;
+		long t = hung_timeout_jiffies(hung_last_checked, timeout);
 
-		while (schedule_timeout_interruptible(timeout_jiffies(timeout)))
-			timeout = sysctl_hung_task_timeout_secs;
-
-		if (atomic_xchg(&reset_hung_task, 0))
+		if (t <= 0) {
+			if (!atomic_xchg(&reset_hung_task, 0))
+				check_hung_uninterruptible_tasks(timeout);
+			hung_last_checked = jiffies;
 			continue;
-
-		check_hung_uninterruptible_tasks(timeout);
+		}
+		schedule_timeout_interruptible(t);
 	}
 
 	return 0;
_