[PATCH v2 0/1] Touch kernel watchdog with sched count

[PATCH v2 0/1] Touch kernel watchdog with sched count

[Xi Wang]

The main purpose of kernel watchdog is to test whether scheduler can
still schedule tasks on a cpu. In order to reduce latency / jitter
from periodically invoking watchdog reset in thread context, we can
simply test if pick_next_task can run. This is done by forcing resched
and checking rq->sched_count. Compared to actually resetting watchdog
from cpu stop / migration threads, we lose coverage on: a migration
thread actually get picked and we actually context switch to the
migration thread. These steps are unlikely to silently fail. The
change would provide nearly the same level of protection with less
overhead.

With this patch we can still switch back to the old method with the
boot option watchdog_touch_with_thread. However code for the old
method can be completely removed in the future.


v2:
 - Use sched_count instead of having sched calling into watchdog code
 - Remove the sysctl and add a boot option, which can be removed later
 - Changed the subject line
  


Xi Wang (1):
  sched: watchdog: Touch kernel watchdog with sched count

 include/linux/sched.h |  4 ++++
 kernel/sched/core.c   | 23 ++++++++++++++++++++--
 kernel/sched/sched.h  |  6 +++++-
 kernel/watchdog.c     | 44 +++++++++++++++++++++++++++++++++++++------
 4 files changed, 68 insertions(+), 9 deletions(-)

-- 
2.29.0.rc1.297.gfa9743e501-goog

[PATCH v2 1/1] sched: watchdog: Touch kernel watchdog with sched count

[Xi Wang]

The main purpose of kernel watchdog is to test whether scheduler can
still schedule tasks on a cpu. In order to reduce latency / jitter
from periodically invoking watchdog reset in thread context, we can
simply test if pick_next_task can run. This is done by forcing resched
and checking rq->sched_count. Compared to actually resetting watchdog
from cpu stop / migration threads, we lose coverage on: a migration
thread actually get picked and we actually context switch to the
migration thread. These steps are unlikely to silently fail. The
change would provide nearly the same level of protection with less
overhead.

With this patch we can still switch back to the old method with the
boot option watchdog_touch_with_thread. However code for the old
method can be completely removed in the future.

Suggested-by: Paul Turner <pjt@google.com>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Xi Wang <xii@google.com>
---
 include/linux/sched.h |  4 ++++
 kernel/sched/core.c   | 23 ++++++++++++++++++++--
 kernel/sched/sched.h  |  6 +++++-
 kernel/watchdog.c     | 44 +++++++++++++++++++++++++++++++++++++------
 4 files changed, 68 insertions(+), 9 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index d383cf09e78f..1e3bef9a9cdb 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1662,6 +1662,10 @@ extern int sched_setattr(struct task_struct *, const struct sched_attr *);
 extern int sched_setattr_nocheck(struct task_struct *, const struct sched_attr *);
 extern struct task_struct *idle_task(int cpu);
 
+#ifdef CONFIG_SOFTLOCKUP_DETECTOR
+extern unsigned int sched_get_count(int cpu);
+#endif
+
 /**
  * is_idle_task - is the specified task an idle task?
  * @p: the task in question.
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 8160ab5263f8..378f0f36c402 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4293,8 +4293,6 @@ static inline void schedule_debug(struct task_struct *prev, bool preempt)
 	rcu_sleep_check();
 
 	profile_hit(SCHED_PROFILING, __builtin_return_address(0));
-
-	schedstat_inc(this_rq()->sched_count);
 }
 
 static void put_prev_task_balance(struct rq *rq, struct task_struct *prev,
@@ -4492,6 +4490,12 @@ static void __sched notrace __schedule(bool preempt)
 	clear_tsk_need_resched(prev);
 	clear_preempt_need_resched();
 
+#ifdef CONFIG_SOFTLOCKUP_DETECTOR
+	this_rq()->sched_count++; /* sched count is also used by watchdog */
+#else
+	schedstat_inc(this_rq()->sched_count);
+#endif
+
 	if (likely(prev != next)) {
 		rq->nr_switches++;
 		/*
@@ -5117,6 +5121,21 @@ struct task_struct *idle_task(int cpu)
 	return cpu_rq(cpu)->idle;
 }
 
+#ifdef CONFIG_SOFTLOCKUP_DETECTOR
+
+/**
+ * sched_get_count - get the sched count of a CPU.
+ * @cpu: the CPU in question.
+ *
+ * Return: sched count.
+ */
+unsigned int sched_get_count(int cpu)
+{
+	return cpu_rq(cpu)->sched_count;
+}
+
+#endif
+
 /**
  * find_process_by_pid - find a process with a matching PID value.
  * @pid: the pid in question.
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 28709f6b0975..f23255981d52 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -959,6 +959,11 @@ struct rq {
 	u64			clock_pelt;
 	unsigned long		lost_idle_time;
 
+#if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_SOFTLOCKUP_DETECTOR)
+	/* Also used by watchdog - no longer grouping with other sched stats */
+	unsigned int		sched_count;
+#endif
+
 	atomic_t		nr_iowait;
 
 #ifdef CONFIG_MEMBARRIER
@@ -1036,7 +1041,6 @@ struct rq {
 	unsigned int		yld_count;
 
 	/* schedule() stats */
-	unsigned int		sched_count;
 	unsigned int		sched_goidle;
 
 	/* try_to_wake_up() stats */
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 5abb5b22ad13..df7f7e585502 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -170,6 +170,7 @@ static bool softlockup_initialized __read_mostly;
 static u64 __read_mostly sample_period;
 
 static DEFINE_PER_CPU(unsigned long, watchdog_touch_ts);
+static DEFINE_PER_CPU(unsigned int, watchdog_sched_prev);
 static DEFINE_PER_CPU(struct hrtimer, watchdog_hrtimer);
 static DEFINE_PER_CPU(bool, softlockup_touch_sync);
 static DEFINE_PER_CPU(bool, soft_watchdog_warn);
@@ -177,6 +178,12 @@ static DEFINE_PER_CPU(unsigned long, hrtimer_interrupts);
 static DEFINE_PER_CPU(unsigned long, hrtimer_interrupts_saved);
 static unsigned long soft_lockup_nmi_warn;
 
+/*
+ * Touch watchdog if __schedule and pick_next_task can run - avoid actual
+ * context switch and associated latency for most cases
+ */
+int __read_mostly watchdog_touch_with_sched = 1;
+
 static int __init nowatchdog_setup(char *str)
 {
 	watchdog_user_enabled = 0;
@@ -198,6 +205,13 @@ static int __init watchdog_thresh_setup(char *str)
 }
 __setup("watchdog_thresh=", watchdog_thresh_setup);
 
+static int __init watchdog_touch_with_thread_setup(char *str)
+{
+	watchdog_touch_with_sched = 0;
+	return 1;
+}
+__setup("watchdog_touch_with_thread", watchdog_touch_with_thread_setup);
+
 static void __lockup_detector_cleanup(void);
 
 /*
@@ -239,6 +253,9 @@ static void set_sample_period(void)
 static void __touch_watchdog(void)
 {
 	__this_cpu_write(watchdog_touch_ts, get_timestamp());
+	if (watchdog_touch_with_sched)
+		__this_cpu_write(watchdog_sched_prev,
+				 sched_get_count(smp_processor_id()));
 }
 
 /**
@@ -351,12 +368,14 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
 	/* kick the hardlockup detector */
 	watchdog_interrupt_count();
 
-	/* kick the softlockup detector */
-	if (completion_done(this_cpu_ptr(&softlockup_completion))) {
-		reinit_completion(this_cpu_ptr(&softlockup_completion));
-		stop_one_cpu_nowait(smp_processor_id(),
-				softlockup_fn, NULL,
-				this_cpu_ptr(&softlockup_stop_work));
+	if (!watchdog_touch_with_sched) {
+		/* kick the softlockup detector */
+		if (completion_done(this_cpu_ptr(&softlockup_completion))) {
+			reinit_completion(this_cpu_ptr(&softlockup_completion));
+			stop_one_cpu_nowait(smp_processor_id(),
+					softlockup_fn, NULL,
+					this_cpu_ptr(&softlockup_stop_work));
+		}
 	}
 
 	/* .. and repeat */
@@ -378,6 +397,19 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
 		return HRTIMER_RESTART;
 	}
 
+	if (watchdog_touch_with_sched) {
+		/* Trigger reschedule for the next round */
+		set_tsk_need_resched(current);
+		set_preempt_need_resched();
+		/* sched_count increase in __schedule is taken as watchdog touched */
+		if (sched_get_count(smp_processor_id()) -
+		    __this_cpu_read(watchdog_sched_prev)) {
+			__touch_watchdog();
+			__this_cpu_write(soft_watchdog_warn, false);
+			return HRTIMER_RESTART;
+		}
+	}
+
 	/* check for a softlockup
 	 * This is done by making sure a high priority task is
 	 * being scheduled.  The task touches the watchdog to
-- 
2.29.0.rc1.297.gfa9743e501-goog

Re: [PATCH v2 1/1] sched: watchdog: Touch kernel watchdog with sched count

[Peter Zijlstra]

On Tue, Oct 20, 2020 at 01:57:04PM -0700, Xi Wang wrote:
> The main purpose of kernel watchdog is to test whether scheduler can
> still schedule tasks on a cpu. In order to reduce latency / jitter
> from periodically invoking watchdog reset in thread context, we can
> simply test if pick_next_task can run. This is done by forcing resched
> and checking rq->sched_count.

Whitespace exists for a reason, use more of it. Also, the above isn't
actually accurate anymore and carried over from the previous
implementation.

> Compared to actually resetting watchdog
> from cpu stop / migration threads, we lose coverage on: a migration
> thread actually get picked and we actually context switch to the
> migration thread. These steps are unlikely to silently fail. The
> change would provide nearly the same level of protection with less
> overhead.
> 
> With this patch we can still switch back to the old method with the
> boot option watchdog_touch_with_thread. However code for the old
> method can be completely removed in the future.

I'd suggest removing all the #ifdef and config muck. Either its a good
idea or it's not.

Re: [PATCH v2 1/1] sched: watchdog: Touch kernel watchdog with sched count

[Peter Zijlstra]

On Tue, Oct 20, 2020 at 01:57:04PM -0700, Xi Wang wrote:

> +	if (watchdog_touch_with_sched) {
> +		/* Trigger reschedule for the next round */
> +		set_tsk_need_resched(current);
> +		set_preempt_need_resched();

Blergh.. that's gross. This relies on this being in IRQ context and
either: PREEMPT=y *OR* this always being from userspace. Otherwise
there's no guarantee the return-from-interrupt will actually schedule.

> +		/* sched_count increase in __schedule is taken as watchdog touched */
> +		if (sched_get_count(smp_processor_id()) -
> +		    __this_cpu_read(watchdog_sched_prev)) {
> +			__touch_watchdog();
> +			__this_cpu_write(soft_watchdog_warn, false);
> +			return HRTIMER_RESTART;
> +		}
> +	}
> +
>  	/* check for a softlockup
>  	 * This is done by making sure a high priority task is
>  	 * being scheduled.  The task touches the watchdog to
> -- 
> 2.29.0.rc1.297.gfa9743e501-goog
> 

Re: [PATCH v2 1/1] sched: watchdog: Touch kernel watchdog with sched count

[Peter Zijlstra]

On Wed, Oct 21, 2020 at 12:12:57PM +0200, Peter Zijlstra wrote:
> On Tue, Oct 20, 2020 at 01:57:04PM -0700, Xi Wang wrote:
> 
> > +	if (watchdog_touch_with_sched) {
> > +		/* Trigger reschedule for the next round */
> > +		set_tsk_need_resched(current);
> > +		set_preempt_need_resched();
> 
> Blergh.. that's gross. This relies on this being in IRQ context and
> either: PREEMPT=y *OR* this always being from userspace. Otherwise
> there's no guarantee the return-from-interrupt will actually schedule.

*compeltely* untested.

---
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 996884dcc9fd..f3913a6ed5e1 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -745,8 +745,7 @@ static void pfault_interrupt(struct ext_code ext_code,
 			 * is no kernel task state to trample. Rely on the
 			 * return to userspace schedule() to block. */
 			__set_current_state(TASK_UNINTERRUPTIBLE);
-			set_tsk_need_resched(tsk);
-			set_preempt_need_resched();
+			resched_current_from_IRQ();
 		}
 	}
 out:
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 8583b15dbc83..e5447b8473d1 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1832,6 +1832,8 @@ static inline int test_tsk_need_resched(struct task_struct *tsk)
 	return unlikely(test_tsk_thread_flag(tsk,TIF_NEED_RESCHED));
 }
 
+extern void resched_current_from_IRQ(void);
+
 /*
  * cond_resched() and cond_resched_lock(): latency reduction via
  * explicit rescheduling in places that are safe. The return
diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c
index aa897c3f2e92..a21c16089452 100644
--- a/kernel/rcu/tiny.c
+++ b/kernel/rcu/tiny.c
@@ -70,8 +70,7 @@ void rcu_sched_clock_irq(int user)
 	if (user) {
 		rcu_qs();
 	} else if (rcu_ctrlblk.donetail != rcu_ctrlblk.curtail) {
-		set_tsk_need_resched(current);
-		set_preempt_need_resched();
+		resched_current_from_IRQ();
 	}
 }
 
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index edeabc232c21..39aae3663bea 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2558,8 +2558,7 @@ void rcu_sched_clock_irq(int user)
 	if (smp_load_acquire(this_cpu_ptr(&rcu_data.rcu_urgent_qs))) {
 		/* Idle and userspace execution already are quiescent states. */
 		if (!rcu_is_cpu_rrupt_from_idle() && !user) {
-			set_tsk_need_resched(current);
-			set_preempt_need_resched();
+			resched_current_from_IRQ();
 		}
 		__this_cpu_write(rcu_data.rcu_urgent_qs, false);
 	}
@@ -2687,8 +2686,7 @@ static __latent_entropy void rcu_core(void)
 	if (!(preempt_count() & PREEMPT_MASK)) {
 		rcu_preempt_deferred_qs(current);
 	} else if (rcu_preempt_need_deferred_qs(current)) {
-		set_tsk_need_resched(current);
-		set_preempt_need_resched();
+		resched_current_from_IRQ();
 	}
 
 	/* Update RCU state based on any recent quiescent states. */
diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index 8760b6ead770..fbafc3aba268 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -656,8 +656,7 @@ static void rcu_exp_handler(void *unused)
 			rcu_report_exp_rdp(rdp);
 		} else {
 			rdp->exp_deferred_qs = true;
-			set_tsk_need_resched(t);
-			set_preempt_need_resched();
+			resched_current_from_IRQ();
 		}
 		return;
 	}
@@ -725,8 +724,7 @@ static void rcu_exp_need_qs(void)
 	__this_cpu_write(rcu_data.cpu_no_qs.b.exp, true);
 	/* Store .exp before .rcu_urgent_qs. */
 	smp_store_release(this_cpu_ptr(&rcu_data.rcu_urgent_qs), true);
-	set_tsk_need_resched(current);
-	set_preempt_need_resched();
+	resched_current_from_IRQ();
 }
 
 /* Invoked on each online non-idle CPU for expedited quiescent state. */
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index fd8a52e9a887..b98294585c56 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -625,8 +625,7 @@ static void rcu_read_unlock_special(struct task_struct *t)
 			// Enabling BH or preempt does reschedule, so...
 			// Also if no expediting, slow is OK.
 			// Plus nohz_full CPUs eventually get tick enabled.
-			set_tsk_need_resched(current);
-			set_preempt_need_resched();
+			resched_current_from_IRQ();
 			if (IS_ENABLED(CONFIG_IRQ_WORK) && irqs_were_disabled &&
 			    !rdp->defer_qs_iw_pending && exp) {
 				// Get scheduler to re-evaluate and call hooks.
@@ -689,8 +688,7 @@ static void rcu_flavor_sched_clock_irq(int user)
 	    (preempt_count() & (PREEMPT_MASK | SOFTIRQ_MASK))) {
 		/* No QS, force context switch if deferred. */
 		if (rcu_preempt_need_deferred_qs(t)) {
-			set_tsk_need_resched(t);
-			set_preempt_need_resched();
+			resched_current_from_IRQ();
 		}
 	} else if (rcu_preempt_need_deferred_qs(t)) {
 		rcu_preempt_deferred_qs(t); /* Report deferred QS. */
diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h
index 0fde39b8daab..74973ab3c14d 100644
--- a/kernel/rcu/tree_stall.h
+++ b/kernel/rcu/tree_stall.h
@@ -566,8 +566,7 @@ static void print_cpu_stall(unsigned long gps)
 	 * progress and it could be we're stuck in kernel space without context
 	 * switches for an entirely unreasonable amount of time.
 	 */
-	set_tsk_need_resched(current);
-	set_preempt_need_resched();
+	resched_current_from_IRQ();
 }
 
 static void check_cpu_stall(struct rcu_data *rdp)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 4a51461a2708..79c4ee72c9b6 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -641,6 +641,17 @@ void resched_cpu(int cpu)
 	raw_spin_unlock_irqrestore(&rq->lock, flags);
 }
 
+void resched_current_from_IRQ(void)
+{
+	struct pt_regs *regs = get_irq_regs();
+
+	WARN_ON_ONCE(!in_irq());
+	WARN_ON_ONCE(user_mode(regs) || IS_ENABLED(CONFIG_PREEMPTION));
+
+	set_tsk_need_resched(tsk);
+	set_preempt_need_resched();
+}
+
 #ifdef CONFIG_SMP
 #ifdef CONFIG_NO_HZ_COMMON
 /*

Re: [PATCH v2 1/1] sched: watchdog: Touch kernel watchdog with sched count

[Peter Zijlstra]

On Wed, Oct 21, 2020 at 12:24:20PM +0200, Peter Zijlstra wrote:
> +void resched_current_from_IRQ(void)
> +{
> +	struct pt_regs *regs = get_irq_regs();
> +
> +	WARN_ON_ONCE(!in_irq());
> +	WARN_ON_ONCE(user_mode(regs) || IS_ENABLED(CONFIG_PREEMPTION));

! that, obviously :/

> +
> +	set_tsk_need_resched(tsk);
> +	set_preempt_need_resched();
> +}

Re: [PATCH v2 1/1] sched: watchdog: Touch kernel watchdog with sched count

[Xi Wang]

On Wed, Oct 21, 2020 at 3:13 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Oct 20, 2020 at 01:57:04PM -0700, Xi Wang wrote:
>
> > +     if (watchdog_touch_with_sched) {
> > +             /* Trigger reschedule for the next round */
> > +             set_tsk_need_resched(current);
> > +             set_preempt_need_resched();
>
> Blergh.. that's gross. This relies on this being in IRQ context and
> either: PREEMPT=y *OR* this always being from userspace. Otherwise
> there's no guarantee the return-from-interrupt will actually schedule.
>

Maybe I missed something but I think immediate rescheduling is not
required? E.g. software watchdog should fire if there is a kernel busy
loop and kernel preemption is not enabled. The current method ends up
with a thread wakeup so there is no guaranteed reschedule either?

-Xi