introduce for_each_process_thread_{break,continue}() helpers

Hello,

IMHO this makes sense in any case, but mostly this is preparation for another change: show_state_filter() should be preemptible. But this needs more discussion, I'll write another email/patch when I fully understand the hard-lockup caused by sysrq-t.

Oleg.

 include/linux/sched.h | 10 ++++++++++
 kernel/exit.c         | 42 ++++++++++++++++++++++++++++++++++++++++++
 kernel/hung_task.c    | 41 ++++++++++++++++++++++++-----------------
 3 files changed, 76 insertions(+), 17 deletions(-)

Example:

	rcu_read_lock();
	for_each_process_thread(p, t) {
		do_something_slow(p, t);

		if (SPENT_TOO_MANY_TIME) {
			for_each_process_thread_break(p, t);
			rcu_read_unlock();
			schedule();
			rcu_read_lock();
			for_each_process_thread_continue(&p, &t);
		}
	}
	rcu_read_unlock();

This looks similar to rcu_lock_break(), but much better and the next patch changes check_hung_uninterruptible_tasks() to use these new helpers. But my real target is show_state_filter() which can trivially lead to lockup.

Compared to rcu_lock_break(), for_each_process_thread_continue() never gives up, it relies on fact that both process and thread lists are sorted by the task->start_time key. So, for example, even if both leader/thread are already dead we can find the next alive process and continue.

TODO: This is another indication we should rename pid_alive(tsk) (and probably change it), most users call it to ensure that rcu_read_lock() still protects this tsk.

NOTE: it seems that, contrary to the comment, task_struct->start_time is not really monotonic, and this should be probably fixed. Until then _continue() might skip more threads with the same ->start_time than necessary.

---
 include/linux/sched.h | 10 ++++++++++
 kernel/exit.c         | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

index 253538f..004c9d5 100644
@@ -2859,6 +2859,16 @@ extern bool current_is_single_threaded(void);
 #define for_each_process_thread(p, t)	\
 	for_each_process(p) for_each_thread(p, t)
 
+static inline void
+for_each_process_thread_break(struct task_struct *p, struct task_struct *t)
+{
+	get_task_struct(p);
+	get_task_struct(t);
+}
+
+extern void
+for_each_process_thread_continue(struct task_struct **, struct task_struct **);
+
 static inline int get_nr_threads(struct task_struct *tsk)
 {
 	return tsk->signal->nr_threads;

2.5.0

Thanks for review and sorry for delay, I am travelling till the end of this week.

Yeees, I don't think this can actually hurt "in practice", but I agree, compared to rcu_lock_break() this is only bounded by PID_MAX_DEFAULT in theory.

Will you agree if I just add the "int max_scan" argument and make it return a boolean for the start? The caller will need to abort the for_each_process_thread() loop if _continue() fails.

Probably this is not what we actually want for show_filter_state(), we can make it better later. We can "embed" the rcu_lock_break() logic into _continue(), or change break/continue to record the state (leader_start_time, thread_start_time) so that a "false" return from _continue() means that the caller needs another schedule(),

	struct state state;

        rcu_read_lock();
        for_each_process_thread(p, t) {
                do_something_slow(p, t);

                if (SPENT_TOO_MANY_TIME) {
                        for_each_process_thread_break(p, t, &state);
        another_break:
                        rcu_read_unlock();
                        schedule();
                        rcu_read_lock();
                        if (!for_each_process_thread_continue(&p, &t, LIMIT, &state))
                        	goto another_break;
               	}
       	}
	rcu_read_unlock();

Not sure. I'd like to do something simple for the start. We need to make show_state_filter() "preemptible" in any case. And even killable, I think. Not only it can trivially trigger the soft-lockups (at least), it can simply never finish.

Oleg.

Yeah, no objection raelly. Just make sure to put a comment explaining the limitation/issues with whatever you descide to go for.

Change check_hung_uninterruptible_tasks() to use the new helpers and remove the "can_cont" logic from rcu_lock_break() which we will probably export later for other users (show_state_filter).

We could add for_each_process_thread_break/continue right into rcu_lock_break() but see the next patch.

---
 kernel/hung_task.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

index d234022..517f52e 100644
@@ -134,20 +134,11 @@ static void check_hung_task(struct task_struct *t, unsigned long timeout)
  * For preemptible RCU it is sufficient to call rcu_read_unlock in order
  * to exit the grace period. For classic RCU, a reschedule is required.
  */
-static bool rcu_lock_break(struct task_struct *g, struct task_struct *t)
+static void rcu_lock_break(void)
 {
-	bool can_cont;
-
-	get_task_struct(g);
-	get_task_struct(t);
 	rcu_read_unlock();
 	cond_resched();
 	rcu_read_lock();
-	can_cont = pid_alive(g) && pid_alive(t);
-	put_task_struct(t);
-	put_task_struct(g);
-
-	return can_cont;
 }
 
 /*

@@ -170,16 +161,18 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)
 
 	rcu_read_lock();
 	for_each_process_thread(g, t) {
-		if (!max_count--)
+		/* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */
+		if (t->state == TASK_UNINTERRUPTIBLE)
+			check_hung_task(t, timeout);
+
+		if (!--max_count)
 			goto unlock;
 		if (!--batch_count) {
 			batch_count = HUNG_TASK_BATCHING;
-			if (!rcu_lock_break(g, t))
-				goto unlock;
+			for_each_process_thread_break(g, t);
+			rcu_lock_break();
+			for_each_process_thread_continue(&g, &t);
 		}
-		/* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */
-		if (t->state == TASK_UNINTERRUPTIBLE)
-			check_hung_task(t, timeout);
 	}
  unlock:
 	rcu_read_unlock();

2.5.0

If sysctl_hung_task_check_count is low and the first "max_count" threads do not exit, check_hung_uninterruptible_tasks() will never check other tasks, this is just ugly.

Now that we have for_each_process_thread_continue(), we can save g/t before return and restart on the next call.

---
 kernel/hung_task.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

index 517f52e..e6e516d 100644
@@ -148,6 +148,9 @@ static void rcu_lock_break(void)
  */
 static void check_hung_uninterruptible_tasks(unsigned long timeout)
 {
+	/* we have only one watchdog() thread */
+	static struct task_struct *g_saved, *t_saved;
+
 	int max_count = sysctl_hung_task_check_count;
 	int batch_count = HUNG_TASK_BATCHING;
 	struct task_struct *g, *t;

@@ -160,18 +163,29 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)
 		return;
 
 	rcu_read_lock();
+	if (g_saved) {
+		g = g_saved;
+		t = t_saved;
+		g_saved = NULL;
+		goto resume;
+	}
+
 	for_each_process_thread(g, t) {
 		/* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */
 		if (t->state == TASK_UNINTERRUPTIBLE)
 			check_hung_task(t, timeout);
 
-		if (!--max_count)
-			goto unlock;
-		if (!--batch_count) {
-			batch_count = HUNG_TASK_BATCHING;
+		if (!--max_count || !--batch_count) {
 			for_each_process_thread_break(g, t);
+			if (!max_count) {
+				g_saved = g;
+				t_saved = t;
+				goto unlock;
+			}
 			rcu_lock_break();
+ resume:
 			for_each_process_thread_continue(&g, &t);
+			batch_count = HUNG_TASK_BATCHING;
 		}
 	}
  unlock:

2.5.0

Yes, we need to do something with show_state_filter() anyway, I think.

OTOH, I believe this simple change in multi_cpu_stop() makes sense too regardless.

Oleg.

Suppose that stop_machine(fn) hangs because fn() hangs. In this case NMI hard-lockup can be triggered on another CPU which does nothing wrong and the trace from nmi_panic() won't help to investigate the problem.

And this change "fixes" the problem we (seem to) hit in practice.

- stop_two_cpus(0, 1) races with show_state_filter() running on CPU_0.

- CPU_1 already spins in MULTI_STOP_PREPARE state, it detects the soft lockup and tries to report the problem.

- show_state_filter() enables preemption, CPU_0 calls multi_cpu_stop() which goes to MULTI_STOP_DISABLE_IRQ state and disables interrupts.

- CPU_1 spends more than 10 seconds trying to flush the log buffer to the slow serial console.

- NMI interrupt on CPU_0 (which now waits for CPU_1) calls nmi_panic().

---
 kernel/stop_machine.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index a467e6c..4a1ca5f 100644
@@ -21,6 +21,7 @@
 #include <linux/smpboot.h>
 #include <linux/atomic.h>
 #include <linux/lglock.h>
+#include <linux/nmi.h>
 
 /*
  * Structure to determine completion condition and record errors.  May

@@ -209,6 +210,13 @@ static int multi_cpu_stop(void *data)
 				break;
 			}
 			ack_state(msdata);
+		} else if (curstate > MULTI_STOP_PREPARE) {
+			/*
+			 * At this stage all other CPUs we depend on must spin
+			 * in the same loop. Any reason for hard-lockup should
+			 * be detected and reported on their side.
+			 */
+			touch_nmi_watchdog();
 		}
 	} while (curstate != MULTI_STOP_EXIT);
 

2.5.0