| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 5 Feb 2009 19:07:49 +0100
From: Ingo Molnar <mingo@...e.hu>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: Mandeep Singh Baines <msb@...gle.com>,
Frédéric Weisbecker <fweisbec@...il.com>,
Peter Zijlstra <peterz@...radead.org>,
linux-kernel@...r.kernel.org, rientjes@...gle.com,
mbligh@...gle.com, thockin@...gle.com
Subject: Re: [PATCH 2/2 v4] softlockup: check all tasks in hung_task
* Andrew Morton <akpm@...ux-foundation.org> wrote:
> On Thu, 5 Feb 2009 15:34:53 +0100 Ingo Molnar <mingo@...e.hu> wrote:
>
> >
> > Subject: [PATCH] softlockup: check all tasks in hung_task
> >
> > Impact: extend the scope of hung-task checks
> >
>
> A nanonit:
agreed.
> > +static const int hung_task_batching = 1024;
>
> static const definitions look pretty but they're a bit misleading.
>
> > static void check_hung_uninterruptible_tasks(unsigned long timeout)
> > {
> > + int batch_count = hung_task_batching;
> > int max_count = sysctl_hung_task_check_count;
> > unsigned long now = get_timestamp();
> > struct task_struct *g, *t;
> > @@ -131,6 +159,13 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)
> > do_each_thread(g, t) {
> > if (!--max_count)
> > goto unlock;
> > + if (!--batch_count) {
> > + batch_count = hung_task_batching;
> > + rcu_lock_break(g, t);
> > + /* Exit if t or g was unhashed during refresh. */
> > + if (t->state == TASK_DEAD || g->state == TASK_DEAD)
> > + goto unlock;
> > + }
> > /* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */
> > if (t->state == TASK_UNINTERRUPTIBLE)
> > check_hung_task(t, now, timeout);
>
> The reader of this area of the code will expect that hung_task_batching
> is a variable. It _looks_ like the value of that variable can be altered
> at any time by some other thread. It _looks_ like this code will explode
> if someone has accidentally set hung_task_batching to zero, etc.
>
> But none of that is actually true, because hung_task_batching is, surprisingly,
> a compile-time constant.
>
> All this misleadingness would be fixed if it were called
> HUNG_TASK_BATCHING. But then it wouldn't be pretty.
i keep running into this paradox myself too. Explicit const C types are the
perfect replacements for defines, but they create confusion by making it
look like a variable.
I tend to agree with you that avoiding the confusion is more important than
having a type - it's not like we are about to have any type related troubles
here. So i amended the commit in the way below - does that look good to you?
Ingo
---------------->
>From 9d03ba30018a546d20d4aa8bba58978492c82520 Mon Sep 17 00:00:00 2001
From: Mandeep Singh Baines <msb@...gle.com>
Date: Wed, 4 Feb 2009 20:35:48 -0800
Subject: [PATCH] softlockup: check all tasks in hung_task
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Impact: extend the scope of hung-task checks
Changed the default value of hung_task_check_count to PID_MAX_LIMIT.
hung_task_batch_count added to put an upper bound on the critical
section. Every hung_task_batch_count checks, the rcu lock is never
held for a too long time.
Keeping the critical section small minimizes time preemption is disabled
and keeps rcu grace periods small.
To prevent following a stale pointer, get_task_struct is called on g and t.
To verify that g and t have not been unhashed while outside the critical
section, the task states are checked.
The design was proposed by Frédéric Weisbecker.
Signed-off-by: Mandeep Singh Baines <msb@...gle.com>
Suggested-by: Frédéric Weisbecker <fweisbec@...il.com>
Signed-off-by: Ingo Molnar <mingo@...e.hu>
---
kernel/hung_task.c | 39 +++++++++++++++++++++++++++++++++++++--
1 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/kernel/hung_task.c b/kernel/hung_task.c
index ba8ccd4..3c6190b 100644
--- a/kernel/hung_task.c
+++ b/kernel/hung_task.c
@@ -17,9 +17,18 @@
#include <linux/sysctl.h>
/*
- * Have a reasonable limit on the number of tasks checked:
+ * The number of tasks checked:
*/
-unsigned long __read_mostly sysctl_hung_task_check_count = 1024;
+unsigned long __read_mostly sysctl_hung_task_check_count = PID_MAX_LIMIT;
+
+/*
+ * Limit number of tasks checked in a batch.
+ *
+ * This value controls the preemptibility of khungtaskd since preemption
+ * is disabled during the critical section. It also controls the size of
+ * the RCU grace period. So it needs to be upper-bound.
+ */
+#define HUNG_TASK_BATCHING 1024;
/*
* Zero means infinite timeout - no checking done:
@@ -110,6 +119,24 @@ static void check_hung_task(struct task_struct *t, unsigned long now,
}
/*
+ * To avoid extending the RCU grace period for an unbounded amount of time,
+ * periodically exit the critical section and enter a new one.
+ *
+ * For preemptible RCU it is sufficient to call rcu_read_unlock in order
+ * exit the grace period. For classic RCU, a reschedule is required.
+ */
+static void rcu_lock_break(struct task_struct *g, struct task_struct *t)
+{
+ get_task_struct(g);
+ get_task_struct(t);
+ rcu_read_unlock();
+ cond_resched();
+ rcu_read_lock();
+ put_task_struct(t);
+ put_task_struct(g);
+}
+
+/*
* Check whether a TASK_UNINTERRUPTIBLE does not get woken up for
* a really long time (120 seconds). If that happens, print out
* a warning.
@@ -117,6 +144,7 @@ static void check_hung_task(struct task_struct *t, unsigned long now,
static void check_hung_uninterruptible_tasks(unsigned long timeout)
{
int max_count = sysctl_hung_task_check_count;
+ int batch_count = HUNG_TASK_BATCHING;
unsigned long now = get_timestamp();
struct task_struct *g, *t;
@@ -131,6 +159,13 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)
do_each_thread(g, t) {
if (!--max_count)
goto unlock;
+ if (!--batch_count) {
+ batch_count = HUNG_TASK_BATCHING;
+ rcu_lock_break(g, t);
+ /* Exit if t or g was unhashed during refresh. */
+ if (t->state == TASK_DEAD || g->state == TASK_DEAD)
+ goto unlock;
+ }
/* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */
if (t->state == TASK_UNINTERRUPTIBLE)
check_hung_task(t, now, timeout);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists