Miscellaneous fixes for 3.17

From: "Paul E. McKenney" <[email protected]>

The explicit local_irq_save() in __lock_task_sighand() is needed to avoid a potential deadlock condition, as noted in a841796f11c90d53 (signal:
align __lock_task_sighand() irq disabling and RCU). However, someone reading the code might be forgiven for concluding that this separate local_irq_save() was completely unnecessary. This commit therefore adds a comment referencing the shiny new block comment on rcu_read_unlock().

Reported-by Oleg Nesterov <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

Acked-by Oleg Nesterov <[email protected]>

---
 kernel/signal.c | 4 ++++
 1 file changed, 4 insertions(+)

kernel/signal.c +0 -0

index a4077e90f19f..46161e744760 100644

kernel/signal.c +0 -0

 		local_irq_save(*flags);
 		rcu_read_lock();
 		sighand = rcu_dereference(tsk->sighand);

Quoted 21 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

The explicit local_irq_save() in __lock_task_sighand() is needed to avoid a potential deadlock condition, as noted in a841796f11c90d53 (signal:
align __lock_task_sighand() irq disabling and RCU). However, someone reading the code might be forgiven for concluding that this separate local_irq_save() was completely unnecessary. This commit therefore adds a comment referencing the shiny new block comment on rcu_read_unlock().

Reported-by Oleg Nesterov <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

Acked-by Oleg Nesterov <[email protected]>

---
 kernel/signal.c | 4 ++++
 1 file changed, 4 insertions(+)

kernel/signal.c +0 -0

index a4077e90f19f..46161e744760 100644

kernel/signal.c struct sighand_struct *__lock_task_sighand(struct task_struct *tsk, · line 1269

Quoted 7 lines

kernel/signal.c +4 -0

@@ -1263,6 +1263,10 @@ struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
 	struct sighand_struct *sighand;
 
 	for (;;) {
+		/*
+		 * Disable interrupts early to avoid deadlocks.
+		 * See rcu_read_unlock comment header for details.
+		 */

A pair of brackets are missing here: rcu_read_unlock() after that, please add:

Reviewed-by Lai Jiangshan <[email protected]>

It reminds me that I should keep my effort to solve the deadlock problem where rcu_read_unlock() is overlapped with schedule locks.

Quoted 3 lines

kernel/signal.c +0 -0

 		local_irq_save(*flags);
 		rcu_read_lock();
 		sighand = rcu_dereference(tsk->sighand);

From: "Paul E. McKenney" <[email protected]>

In kernels built with CONFIG_NO_HZ_FULL, tick_do_timer_cpu is constant once boot completes. Thus, there is no need to wrap it in ACCESS_ONCE() in code that is built only when CONFIG_NO_HZ_FULL. This commit therefore removes the redundant ACCESS_ONCE().

Signed-off-by Paul E. McKenney <[email protected]>

---
 kernel/rcu/tree_plugin.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/rcu/tree_plugin.h +1 -1

index 02ac0fb186b8..5da9f9b3abc9 100644
@@ -2844,7 +2844,7 @@ static bool rcu_nohz_full_cpu(struct rcu_state *rsp)
 static void rcu_bind_gp_kthread(void)
 {
 #ifdef CONFIG_NO_HZ_FULL
-	int cpu = ACCESS_ONCE(tick_do_timer_cpu);
+	int cpu = tick_do_timer_cpu;
 
 	if (cpu < 0 || cpu >= nr_cpu_ids)
 		return;

Signature · 1 line

1.8.1.5

From: "Paul E. McKenney" <[email protected]>

RCU contains code of the following forms:

        ACCESS_ONCE(x)++;
        ACCESS_ONCE(x) += y;
        ACCESS_ONCE(x) -= y;

Now these constructs do operate correctly, but they really result in a pair of volatile accesses, one to do the load and another to do the store. This can be confusing, as the casual reader might well assume that (for example) gcc might generate a memory-to-memory add instruction for each of these three cases. In fact, gcc will do no such thing. Also, there is a good chance that the kernel will move to separate load and store variants of ACCESS_ONCE(), and constructs like the above could easily confuse both people and scripts attempting to make that sort of change. Finally, most of RCU's read-modify-write uses of ACCESS_ONCE() really only need the store to be volatile, so that the read-modify-write form might be misleading.

This commit therefore changes the above forms in RCU so that each instance of ACCESS_ONCE() either does a load or a store, but not both. In a few cases, ACCESS_ONCE() was not critical, for example, for maintaining statisitics. In these cases, ACCESS_ONCE() has been dispensed with entirely.

Hi Paul,

Quoted 20 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

RCU contains code of the following forms:

        ACCESS_ONCE(x)++;
        ACCESS_ONCE(x) += y;
        ACCESS_ONCE(x) -= y;

Now these constructs do operate correctly, but they really result in a pair of volatile accesses, one to do the load and another to do the store. This can be confusing, as the casual reader might well assume that (for example) gcc might generate a memory-to-memory add instruction for each of these three cases. In fact, gcc will do no such thing. Also, there is a good chance that the kernel will move to separate load and store variants of ACCESS_ONCE(), and constructs like the above could easily confuse both people and scripts attempting to make that sort of change. Finally, most of RCU's read-modify-write uses of ACCESS_ONCE() really only need the store to be volatile, so that the read-modify-write form might be misleading.

Quoted 6 lines

This commit therefore changes the above forms in RCU so that each instance of ACCESS_ONCE() either does a load or a store, but not both. In a few cases, ACCESS_ONCE() was not critical, for example, for maintaining statisitics. In these cases, ACCESS_ONCE() has been dispensed with entirely.

Is there any reason why |=, &= cannot be replaced similarly? Also there are a few more in tree_plugin.h. Please find patch below:

kernel/rcu/tree.c +0 -0

        raw_spin_unlock_irqrestore(&rnp_old->lock, flags);
        wake_up(&rsp->gp_wq);  /* Memory barrier implied by wake_up() path. */
 }

kernel/rcu/tree_plugin.h +0 -0

index 1a4ab26..752d382 100644

kernel/rcu/tree_plugin.h +0 -0

                rdp->n_nocbs_invoked += c;
        }
        return 0;

Signature · 2 lines

-- 
Pranith

Good catch, I clearly didn't include enough patterns in my search.

But please see below. And please rebase onto branch rcu/dev in
git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git,
as this patch set does not apply.

Thanx, Paul

kernel/rcu/tree.c static int rcu_gp_fqs(struct rcu_state *rsp, int · line 1704

Quoted 13 lines

Signed-off-by Pranith Kumar <[email protected]>

---

kernel/rcu/tree.c +1 -1

index dac6d20..f500395 100644
@@ -1700,7 +1700,7 @@ static int rcu_gp_fqs(struct rcu_state *rsp, int
 fqs_state_in)
        if (ACCESS_ONCE(rsp->gp_flags) & RCU_GP_FLAG_FQS) {
                raw_spin_lock_irq(&rnp->lock);
                smp_mb__after_unlock_lock();
-               ACCESS_ONCE(rsp->gp_flags) &= ~RCU_GP_FLAG_FQS;
+               ACCESS_ONCE(rsp->gp_flags) = rsp->gp_flags & ~RCU_GP_FLAG_FQS;

Here we need ACCESS_ONCE() around both instances of rsp->gp_flags.

Quoted 6 lines · Paul E. McKenney

Good catch, I clearly didn't include enough patterns in my search.

But please see below. And please rebase onto branch rcu/dev in
git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git,
as this patch set does not apply.

OK, I will resend the patch. One question below:

Quoted 18 lines

Thanx, Paul

kernel/rcu/tree.c static int rcu_gp_fqs(struct rcu_state *rsp, int · line 1704

Quoted 13 lines

Signed-off-by Pranith Kumar <[email protected]>

---

kernel/rcu/tree.c +1 -1

index dac6d20..f500395 100644
@@ -1700,7 +1700,7 @@ static int rcu_gp_fqs(struct rcu_state *rsp, int
 fqs_state_in)
        if (ACCESS_ONCE(rsp->gp_flags) & RCU_GP_FLAG_FQS) {
                raw_spin_lock_irq(&rnp->lock);
                smp_mb__after_unlock_lock();
-               ACCESS_ONCE(rsp->gp_flags) &= ~RCU_GP_FLAG_FQS;
+               ACCESS_ONCE(rsp->gp_flags) = rsp->gp_flags & ~RCU_GP_FLAG_FQS;

Here we need ACCESS_ONCE() around both instances of rsp->gp_flags.

I see that all accesses of gp_flags are wrapped with ACCESS_ONCE(). Is there any reason why we can't declare it as 'volatile' and not use ACCESS_ONCE everywhere?

From: "Paul E. McKenney" <[email protected]>

The current approach to RCU priority boosting uses an rt_mutex strictly for its priority-boosting side effects. The rt_mutex_init_proxy_locked() function is used by the booster to initialize the lock as held by the boostee. The booster then uses rt_mutex_lock() to acquire this rt_mutex, which priority-boosts the boostee. When the boostee reaches the end of its outermost RCU read-side critical section, it checks a field in its task structure to see whether it has been boosted, and, if so, uses rt_mutex_unlock() to release the rt_mutex. The booster can then go on to boost the next task that is blocking the current RCU grace period.

But reasonable implementations of rt_mutex_unlock() might result in the boostee referencing the rt_mutex's data after releasing it.

Quoted 12 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

The current approach to RCU priority boosting uses an rt_mutex strictly for its priority-boosting side effects. The rt_mutex_init_proxy_locked() function is used by the booster to initialize the lock as held by the boostee. The booster then uses rt_mutex_lock() to acquire this rt_mutex, which priority-boosts the boostee. When the boostee reaches the end of its outermost RCU read-side critical section, it checks a field in its task structure to see whether it has been boosted, and, if so, uses rt_mutex_unlock() to release the rt_mutex. The booster can then go on to boost the next task that is blocking the current RCU grace period.

Quoted 2 lines

But reasonable implementations of rt_mutex_unlock() might result in the boostee referencing the rt_mutex's data after releasing it.

XXXX_unlock(lock_ptr) should not reference to the lock_ptr after it has unlocked the lock. (*) So I think this patch is unneeded. Although its adding overhead is at slow-patch, but it adds REVIEW-burden.

And although the original rt_mutex_unlock() violates the rule(*) when the fast-cmpxchg-path, but it is fixed now.

It is the lock-subsystem's responsible to do this. I prefer to add the wait_for_complete() stuff until the future when the boostee needs to re-access the booster after rt_mutex_unlock() instead of now.

From: "Paul E. McKenney" <[email protected]>

Binding the grace-period kthreads to the timekeeping CPU resulted in significant performance decreases for some workloads. For more detail, see:

https://lkml.org/lkml/2014/6/3/395 for benchmark numbers

https://lkml.org/lkml/2014/6/4/218 for CPU statistics

It turns out that it is necessary to bind the grace-period kthreads to the timekeeping CPU only when all but CPU 0 is a nohz_full CPU on the one hand or if CONFIG_NO_HZ_FULL_SYSIDLE=y on the other. In other cases, it suffices to bind the grace-period kthreads to the set of non-nohz_full CPUs.

This commit therefore creates a tick_nohz_not_full_mask that is the complement of tick_nohz_full_mask, and then binds the grace-period kthread to the set of CPUs indicated by this new mask, which covers the CONFIG_NO_HZ_FULL_SYSIDLE=n case. The CONFIG_NO_HZ_FULL_SYSIDLE=y case still binds the grace-period kthreads to the timekeeping CPU. This commit also includes the tick_nohz_full_enabled() check suggested by Frederic Weisbecker.

Reported-by Jet Chen <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

[ paulmck: Created housekeeping_affine() per fweisbec feedback. ]

---
 include/linux/tick.h     | 19 +++++++++++++++++++
 kernel/rcu/tree_plugin.h | 14 +++++++++-----
 kernel/time/tick-sched.c | 10 ++++++++++
 3 files changed, 38 insertions(+), 5 deletions(-)

include/linux/tick.h +1 -0

index b84773cb9f4c..c39af3261351 100644
@@ -12,6 +12,7 @@
 #include <linux/hrtimer.h>
 #include <linux/context_tracking_state.h>
 #include <linux/cpumask.h>
+#include <linux/sched.h>
 
 #ifdef CONFIG_GENERIC_CLOCKEVENTS
 

include/linux/tick.h +17 -0

 static inline bool tick_nohz_full_enabled(void)
 {

@@ -194,6 +196,23 @@ static inline void tick_nohz_full_kick_all(void) { }
 static inline void __tick_nohz_task_switch(struct task_struct *tsk) { }
 #endif
 
+static inline bool is_housekeeping_cpu(int cpu)
+{
+#ifdef CONFIG_NO_HZ_FULL
+	if (tick_nohz_full_enabled())
+		return cpumask_test_cpu(cpu, tick_nohz_not_full_mask);
+#endif
+	return true;
+}
+
+static inline void housekeeping_affine(struct task_struct *t)
+{
+#ifdef CONFIG_NO_HZ_FULL
+	if (tick_nohz_full_enabled())
+		set_cpus_allowed_ptr(t, tick_nohz_not_full_mask);
+#endif
+}
+
 static inline void tick_nohz_full_check(void)
 {
 	if (tick_nohz_full_enabled())

kernel/rcu/tree_plugin.h +9 -5

index 719587af7b10..b39ba7239bd6 100644
@@ -2846,12 +2846,16 @@ static bool rcu_nohz_full_cpu(struct rcu_state *rsp)
  */
 static void rcu_bind_gp_kthread(void)
 {
-#ifdef CONFIG_NO_HZ_FULL
-	int cpu = tick_do_timer_cpu;
+	int __maybe_unused cpu;
 
-	if (cpu < 0 || cpu >= nr_cpu_ids)
+	if (!tick_nohz_full_enabled())
 		return;
-	if (raw_smp_processor_id() != cpu)
+#ifdef CONFIG_NO_HZ_FULL_SYSIDLE
+	cpu = tick_do_timer_cpu;
+	if (cpu >= 0 && cpu < nr_cpu_ids && raw_smp_processor_id() != cpu)
 		set_cpus_allowed_ptr(current, cpumask_of(cpu));
-#endif /* #ifdef CONFIG_NO_HZ_FULL */
+#else /* #ifdef CONFIG_NO_HZ_FULL_SYSIDLE */
+	if (!is_housekeeping_cpu(raw_smp_processor_id()))
+		housekeeping_affine(current);
+#endif /* #else #ifdef CONFIG_NO_HZ_FULL_SYSIDLE */
 }

kernel/time/tick-sched.c +10 -0

index 6558b7ac112d..e023134d63a1 100644
@@ -154,6 +154,7 @@ static void tick_sched_handle(struct tick_sched *ts, struct pt_regs *regs)
 
 #ifdef CONFIG_NO_HZ_FULL
 cpumask_var_t tick_nohz_full_mask;
+cpumask_var_t tick_nohz_not_full_mask;
 bool tick_nohz_full_running;
 
 static bool can_stop_full_tick(void)

@@ -281,6 +282,7 @@ static int __init tick_nohz_full_setup(char *str)
 	int cpu;
 
 	alloc_bootmem_cpumask_var(&tick_nohz_full_mask);
+	alloc_bootmem_cpumask_var(&tick_nohz_not_full_mask);
 	if (cpulist_parse(str, tick_nohz_full_mask) < 0) {
 		pr_warning("NOHZ: Incorrect nohz_full cpumask\n");
 		return 1;

@@ -291,6 +293,8 @@ static int __init tick_nohz_full_setup(char *str)
 		pr_warning("NO_HZ: Clearing %d from nohz_full range for timekeeping\n", cpu);
 		cpumask_clear_cpu(cpu, tick_nohz_full_mask);
 	}
+	cpumask_andnot(tick_nohz_not_full_mask,
+		       cpu_possible_mask, tick_nohz_full_mask);
 	tick_nohz_full_running = true;
 
 	return 1;

@@ -332,9 +336,15 @@ static int tick_nohz_init_all(void)
 		pr_err("NO_HZ: Can't allocate full dynticks cpumask\n");
 		return err;
 	}
+	if (!alloc_cpumask_var(&tick_nohz_not_full_mask, GFP_KERNEL)) {
+		pr_err("NO_HZ: Can't allocate not-full dynticks cpumask\n");
+		return err;
+	}
 	err = 0;
 	cpumask_setall(tick_nohz_full_mask);
 	cpumask_clear_cpu(smp_processor_id(), tick_nohz_full_mask);
+	cpumask_clear(tick_nohz_not_full_mask);
+	cpumask_set_cpu(smp_processor_id(), tick_nohz_not_full_mask);
 	tick_nohz_full_running = true;
 #endif
 	return err;

Signature · 1 line

1.8.1.5

Quoted 46 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

Binding the grace-period kthreads to the timekeeping CPU resulted in significant performance decreases for some workloads. For more detail, see:

https://lkml.org/lkml/2014/6/3/395 for benchmark numbers

https://lkml.org/lkml/2014/6/4/218 for CPU statistics

It turns out that it is necessary to bind the grace-period kthreads to the timekeeping CPU only when all but CPU 0 is a nohz_full CPU on the one hand or if CONFIG_NO_HZ_FULL_SYSIDLE=y on the other. In other cases, it suffices to bind the grace-period kthreads to the set of non-nohz_full CPUs.

This commit therefore creates a tick_nohz_not_full_mask that is the complement of tick_nohz_full_mask, and then binds the grace-period kthread to the set of CPUs indicated by this new mask, which covers the CONFIG_NO_HZ_FULL_SYSIDLE=n case. The CONFIG_NO_HZ_FULL_SYSIDLE=y case still binds the grace-period kthreads to the timekeeping CPU. This commit also includes the tick_nohz_full_enabled() check suggested by Frederic Weisbecker.

Reported-by Jet Chen <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

[ paulmck: Created housekeeping_affine() per fweisbec feedback. ]

---
 include/linux/tick.h     | 19 +++++++++++++++++++
 kernel/rcu/tree_plugin.h | 14 +++++++++-----
 kernel/time/tick-sched.c | 10 ++++++++++
 3 files changed, 38 insertions(+), 5 deletions(-)

include/linux/tick.h +1 -0

index b84773cb9f4c..c39af3261351 100644
@@ -12,6 +12,7 @@
 #include <linux/hrtimer.h>
 #include <linux/context_tracking_state.h>
 #include <linux/cpumask.h>
+#include <linux/sched.h>
 
 #ifdef CONFIG_GENERIC_CLOCKEVENTS
 

include/linux/tick.h static inline u64 get_cpu_iowait_time_us(int cpu, u64 *unused) { return -1; } · line 166

Quoted 4 lines

include/linux/tick.h +1 -0

@@ -162,6 +163,7 @@ static inline u64 get_cpu_iowait_time_us(int cpu, u64 *unused) { return -1; }
 #ifdef CONFIG_NO_HZ_FULL
 extern bool tick_nohz_full_running;
 extern cpumask_var_t tick_nohz_full_mask;
+extern cpumask_var_t tick_nohz_not_full_mask;

So I'm still puzzled by this mask.

How about creating a temporary cpumask on top of tick_nohz_full_mask from housekeeping_affine().

If you wonder about performance, this can be called once for good from rcu_spawn_gp_kthread() (that would be much better than checking that all the time from the kthread itself anyway).

In case you're out of time, I can handle that. Just checking your opinion.

Thanks.

Quoted 101 lines

include/linux/tick.h +17 -0

 static inline bool tick_nohz_full_enabled(void)
 {

@@ -194,6 +196,23 @@ static inline void tick_nohz_full_kick_all(void) { }
 static inline void __tick_nohz_task_switch(struct task_struct *tsk) { }
 #endif
 
+static inline bool is_housekeeping_cpu(int cpu)
+{
+#ifdef CONFIG_NO_HZ_FULL
+	if (tick_nohz_full_enabled())
+		return cpumask_test_cpu(cpu, tick_nohz_not_full_mask);
+#endif
+	return true;
+}
+
+static inline void housekeeping_affine(struct task_struct *t)
+{
+#ifdef CONFIG_NO_HZ_FULL
+	if (tick_nohz_full_enabled())
+		set_cpus_allowed_ptr(t, tick_nohz_not_full_mask);
+#endif
+}
+
 static inline void tick_nohz_full_check(void)
 {
 	if (tick_nohz_full_enabled())

kernel/rcu/tree_plugin.h +9 -5

index 719587af7b10..b39ba7239bd6 100644
@@ -2846,12 +2846,16 @@ static bool rcu_nohz_full_cpu(struct rcu_state *rsp)
  */
 static void rcu_bind_gp_kthread(void)
 {
-#ifdef CONFIG_NO_HZ_FULL
-	int cpu = tick_do_timer_cpu;
+	int __maybe_unused cpu;
 
-	if (cpu < 0 || cpu >= nr_cpu_ids)
+	if (!tick_nohz_full_enabled())
 		return;
-	if (raw_smp_processor_id() != cpu)
+#ifdef CONFIG_NO_HZ_FULL_SYSIDLE
+	cpu = tick_do_timer_cpu;
+	if (cpu >= 0 && cpu < nr_cpu_ids && raw_smp_processor_id() != cpu)
 		set_cpus_allowed_ptr(current, cpumask_of(cpu));
-#endif /* #ifdef CONFIG_NO_HZ_FULL */
+#else /* #ifdef CONFIG_NO_HZ_FULL_SYSIDLE */
+	if (!is_housekeeping_cpu(raw_smp_processor_id()))
+		housekeeping_affine(current);
+#endif /* #else #ifdef CONFIG_NO_HZ_FULL_SYSIDLE */
 }

kernel/time/tick-sched.c +10 -0

index 6558b7ac112d..e023134d63a1 100644
@@ -154,6 +154,7 @@ static void tick_sched_handle(struct tick_sched *ts, struct pt_regs *regs)
 
 #ifdef CONFIG_NO_HZ_FULL
 cpumask_var_t tick_nohz_full_mask;
+cpumask_var_t tick_nohz_not_full_mask;
 bool tick_nohz_full_running;
 
 static bool can_stop_full_tick(void)

@@ -281,6 +282,7 @@ static int __init tick_nohz_full_setup(char *str)
 	int cpu;
 
 	alloc_bootmem_cpumask_var(&tick_nohz_full_mask);
+	alloc_bootmem_cpumask_var(&tick_nohz_not_full_mask);
 	if (cpulist_parse(str, tick_nohz_full_mask) < 0) {
 		pr_warning("NOHZ: Incorrect nohz_full cpumask\n");
 		return 1;

@@ -291,6 +293,8 @@ static int __init tick_nohz_full_setup(char *str)
 		pr_warning("NO_HZ: Clearing %d from nohz_full range for timekeeping\n", cpu);
 		cpumask_clear_cpu(cpu, tick_nohz_full_mask);
 	}
+	cpumask_andnot(tick_nohz_not_full_mask,
+		       cpu_possible_mask, tick_nohz_full_mask);
 	tick_nohz_full_running = true;
 
 	return 1;

@@ -332,9 +336,15 @@ static int tick_nohz_init_all(void)
 		pr_err("NO_HZ: Can't allocate full dynticks cpumask\n");
 		return err;
 	}
+	if (!alloc_cpumask_var(&tick_nohz_not_full_mask, GFP_KERNEL)) {
+		pr_err("NO_HZ: Can't allocate not-full dynticks cpumask\n");
+		return err;
+	}
 	err = 0;
 	cpumask_setall(tick_nohz_full_mask);
 	cpumask_clear_cpu(smp_processor_id(), tick_nohz_full_mask);
+	cpumask_clear(tick_nohz_not_full_mask);
+	cpumask_set_cpu(smp_processor_id(), tick_nohz_not_full_mask);
 	tick_nohz_full_running = true;
 #endif
 	return err;

Signature · 1 line

1.8.1.5

Quoted 57 lines · Frederic Weisbecker

Quoted 46 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

Binding the grace-period kthreads to the timekeeping CPU resulted in significant performance decreases for some workloads. For more detail, see:

https://lkml.org/lkml/2014/6/3/395 for benchmark numbers

https://lkml.org/lkml/2014/6/4/218 for CPU statistics

It turns out that it is necessary to bind the grace-period kthreads to the timekeeping CPU only when all but CPU 0 is a nohz_full CPU on the one hand or if CONFIG_NO_HZ_FULL_SYSIDLE=y on the other. In other cases, it suffices to bind the grace-period kthreads to the set of non-nohz_full CPUs.

This commit therefore creates a tick_nohz_not_full_mask that is the complement of tick_nohz_full_mask, and then binds the grace-period kthread to the set of CPUs indicated by this new mask, which covers the CONFIG_NO_HZ_FULL_SYSIDLE=n case. The CONFIG_NO_HZ_FULL_SYSIDLE=y case still binds the grace-period kthreads to the timekeeping CPU. This commit also includes the tick_nohz_full_enabled() check suggested by Frederic Weisbecker.

Reported-by Jet Chen <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

[ paulmck: Created housekeeping_affine() per fweisbec feedback. ]

---
 include/linux/tick.h     | 19 +++++++++++++++++++
 kernel/rcu/tree_plugin.h | 14 +++++++++-----
 kernel/time/tick-sched.c | 10 ++++++++++
 3 files changed, 38 insertions(+), 5 deletions(-)

include/linux/tick.h +1 -0

index b84773cb9f4c..c39af3261351 100644
@@ -12,6 +12,7 @@
 #include <linux/hrtimer.h>
 #include <linux/context_tracking_state.h>
 #include <linux/cpumask.h>
+#include <linux/sched.h>
 
 #ifdef CONFIG_GENERIC_CLOCKEVENTS
 

include/linux/tick.h static inline u64 get_cpu_iowait_time_us(int cpu, u64 *unused) { return -1; } · line 166

Quoted 4 lines

include/linux/tick.h +1 -0

@@ -162,6 +163,7 @@ static inline u64 get_cpu_iowait_time_us(int cpu, u64 *unused) { return -1; }
 #ifdef CONFIG_NO_HZ_FULL
 extern bool tick_nohz_full_running;
 extern cpumask_var_t tick_nohz_full_mask;
+extern cpumask_var_t tick_nohz_not_full_mask;

So I'm still puzzled by this mask.

How about creating a temporary cpumask on top of tick_nohz_full_mask from housekeeping_affine().

Quoted 3 lines

If you wonder about performance, this can be called once for good from rcu_spawn_gp_kthread() (that would be much better than checking that all the time from the kthread itself anyway).

I was figuring that a fair number of the kthreads might eventually be using this, not just for the grace-period kthreads.

In addition,
my concern about once-for-good affinity is for the NO_HZ_FULL_SYSIDLE=y case, where moving the grace-period kthreads prevents ever entering full-system idle state.

Or am I missing some use case?

Quoted 58 lines · Paul E. McKenney

Quoted 57 lines · Frederic Weisbecker

Quoted 46 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

Binding the grace-period kthreads to the timekeeping CPU resulted in significant performance decreases for some workloads. For more detail, see:

https://lkml.org/lkml/2014/6/3/395 for benchmark numbers

https://lkml.org/lkml/2014/6/4/218 for CPU statistics

It turns out that it is necessary to bind the grace-period kthreads to the timekeeping CPU only when all but CPU 0 is a nohz_full CPU on the one hand or if CONFIG_NO_HZ_FULL_SYSIDLE=y on the other. In other cases, it suffices to bind the grace-period kthreads to the set of non-nohz_full CPUs.

This commit therefore creates a tick_nohz_not_full_mask that is the complement of tick_nohz_full_mask, and then binds the grace-period kthread to the set of CPUs indicated by this new mask, which covers the CONFIG_NO_HZ_FULL_SYSIDLE=n case. The CONFIG_NO_HZ_FULL_SYSIDLE=y case still binds the grace-period kthreads to the timekeeping CPU. This commit also includes the tick_nohz_full_enabled() check suggested by Frederic Weisbecker.

Reported-by Jet Chen <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

[ paulmck: Created housekeeping_affine() per fweisbec feedback. ]

---
 include/linux/tick.h     | 19 +++++++++++++++++++
 kernel/rcu/tree_plugin.h | 14 +++++++++-----
 kernel/time/tick-sched.c | 10 ++++++++++
 3 files changed, 38 insertions(+), 5 deletions(-)

include/linux/tick.h +1 -0

index b84773cb9f4c..c39af3261351 100644
@@ -12,6 +12,7 @@
 #include <linux/hrtimer.h>
 #include <linux/context_tracking_state.h>
 #include <linux/cpumask.h>
+#include <linux/sched.h>
 
 #ifdef CONFIG_GENERIC_CLOCKEVENTS
 

include/linux/tick.h static inline u64 get_cpu_iowait_time_us(int cpu, u64 *unused) { return -1; } · line 166

Quoted 4 lines

include/linux/tick.h +1 -0

@@ -162,6 +163,7 @@ static inline u64 get_cpu_iowait_time_us(int cpu, u64 *unused) { return -1; }
 #ifdef CONFIG_NO_HZ_FULL
 extern bool tick_nohz_full_running;
 extern cpumask_var_t tick_nohz_full_mask;
+extern cpumask_var_t tick_nohz_not_full_mask;

So I'm still puzzled by this mask.

How about creating a temporary cpumask on top of tick_nohz_full_mask from housekeeping_affine().

Quoted 6 lines

Quoted 3 lines

If you wonder about performance, this can be called once for good from rcu_spawn_gp_kthread() (that would be much better than checking that all the time from the kthread itself anyway).

I was figuring that a fair number of the kthreads might eventually be using this, not just for the grace-period kthreads.

Ok makes sense. But can we just rename the cpumask to housekeeping_mask?

Quoted 6 lines

In addition,
my concern about once-for-good affinity is for the NO_HZ_FULL_SYSIDLE=y case, where moving the grace-period kthreads prevents ever entering full-system idle state.

Or am I missing some use case?

No that's what I had in mind. But rcu_spawn_gp_kthread() still looks like a better place for that. Or am I missing something else?

Quoted 59 lines · Frederic Weisbecker

Quoted 58 lines · Paul E. McKenney

Quoted 57 lines · Frederic Weisbecker

Quoted 46 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

Binding the grace-period kthreads to the timekeeping CPU resulted in significant performance decreases for some workloads. For more detail, see:

https://lkml.org/lkml/2014/6/3/395 for benchmark numbers

https://lkml.org/lkml/2014/6/4/218 for CPU statistics

It turns out that it is necessary to bind the grace-period kthreads to the timekeeping CPU only when all but CPU 0 is a nohz_full CPU on the one hand or if CONFIG_NO_HZ_FULL_SYSIDLE=y on the other. In other cases, it suffices to bind the grace-period kthreads to the set of non-nohz_full CPUs.

This commit therefore creates a tick_nohz_not_full_mask that is the complement of tick_nohz_full_mask, and then binds the grace-period kthread to the set of CPUs indicated by this new mask, which covers the CONFIG_NO_HZ_FULL_SYSIDLE=n case. The CONFIG_NO_HZ_FULL_SYSIDLE=y case still binds the grace-period kthreads to the timekeeping CPU. This commit also includes the tick_nohz_full_enabled() check suggested by Frederic Weisbecker.

Reported-by Jet Chen <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

[ paulmck: Created housekeeping_affine() per fweisbec feedback. ]

---
 include/linux/tick.h     | 19 +++++++++++++++++++
 kernel/rcu/tree_plugin.h | 14 +++++++++-----
 kernel/time/tick-sched.c | 10 ++++++++++
 3 files changed, 38 insertions(+), 5 deletions(-)

include/linux/tick.h +1 -0

index b84773cb9f4c..c39af3261351 100644
@@ -12,6 +12,7 @@
 #include <linux/hrtimer.h>
 #include <linux/context_tracking_state.h>
 #include <linux/cpumask.h>
+#include <linux/sched.h>
 
 #ifdef CONFIG_GENERIC_CLOCKEVENTS
 

include/linux/tick.h static inline u64 get_cpu_iowait_time_us(int cpu, u64 *unused) { return -1; } · line 166

Quoted 4 lines

include/linux/tick.h +1 -0

@@ -162,6 +163,7 @@ static inline u64 get_cpu_iowait_time_us(int cpu, u64 *unused) { return -1; }
 #ifdef CONFIG_NO_HZ_FULL
 extern bool tick_nohz_full_running;
 extern cpumask_var_t tick_nohz_full_mask;
+extern cpumask_var_t tick_nohz_not_full_mask;

So I'm still puzzled by this mask.

How about creating a temporary cpumask on top of tick_nohz_full_mask from housekeeping_affine().

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

If you wonder about performance, this can be called once for good from rcu_spawn_gp_kthread() (that would be much better than checking that all the time from the kthread itself anyway).

I was figuring that a fair number of the kthreads might eventually be using this, not just for the grace-period kthreads.

Ok makes sense. But can we just rename the cpumask to housekeeping_mask?

Good point! After all, it someday might be something other than the complement of tick_nohz_full_mask.

Quoted 5 lines

Quoted 5 lines

In addition,
my concern about once-for-good affinity is for the NO_HZ_FULL_SYSIDLE=y case, where moving the grace-period kthreads prevents ever entering full-system idle state.

Quoted 4 lines

Quoted 1 line

Or am I missing some use case?

No that's what I had in mind. But rcu_spawn_gp_kthread() still looks like a better place for that. Or am I missing something else?

My fear was that sysadmins would move it in the NO_HZ_FULL_SYSIDLE=y case, in which case it needs to move back.

Quoted 79 lines · Paul E. McKenney

Quoted 59 lines · Frederic Weisbecker

Quoted 58 lines · Paul E. McKenney

Quoted 57 lines · Frederic Weisbecker

Quoted 46 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

Binding the grace-period kthreads to the timekeeping CPU resulted in significant performance decreases for some workloads. For more detail, see:

https://lkml.org/lkml/2014/6/3/395 for benchmark numbers

https://lkml.org/lkml/2014/6/4/218 for CPU statistics

It turns out that it is necessary to bind the grace-period kthreads to the timekeeping CPU only when all but CPU 0 is a nohz_full CPU on the one hand or if CONFIG_NO_HZ_FULL_SYSIDLE=y on the other. In other cases, it suffices to bind the grace-period kthreads to the set of non-nohz_full CPUs.

This commit therefore creates a tick_nohz_not_full_mask that is the complement of tick_nohz_full_mask, and then binds the grace-period kthread to the set of CPUs indicated by this new mask, which covers the CONFIG_NO_HZ_FULL_SYSIDLE=n case. The CONFIG_NO_HZ_FULL_SYSIDLE=y case still binds the grace-period kthreads to the timekeeping CPU. This commit also includes the tick_nohz_full_enabled() check suggested by Frederic Weisbecker.

Reported-by Jet Chen <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

[ paulmck: Created housekeeping_affine() per fweisbec feedback. ]

---
 include/linux/tick.h     | 19 +++++++++++++++++++
 kernel/rcu/tree_plugin.h | 14 +++++++++-----
 kernel/time/tick-sched.c | 10 ++++++++++
 3 files changed, 38 insertions(+), 5 deletions(-)

include/linux/tick.h +1 -0

index b84773cb9f4c..c39af3261351 100644
@@ -12,6 +12,7 @@
 #include <linux/hrtimer.h>
 #include <linux/context_tracking_state.h>
 #include <linux/cpumask.h>
+#include <linux/sched.h>
 
 #ifdef CONFIG_GENERIC_CLOCKEVENTS
 

include/linux/tick.h static inline u64 get_cpu_iowait_time_us(int cpu, u64 *unused) { return -1; } · line 166

Quoted 4 lines

include/linux/tick.h +1 -0

@@ -162,6 +163,7 @@ static inline u64 get_cpu_iowait_time_us(int cpu, u64 *unused) { return -1; }
 #ifdef CONFIG_NO_HZ_FULL
 extern bool tick_nohz_full_running;
 extern cpumask_var_t tick_nohz_full_mask;
+extern cpumask_var_t tick_nohz_not_full_mask;

So I'm still puzzled by this mask.

How about creating a temporary cpumask on top of tick_nohz_full_mask from housekeeping_affine().

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

If you wonder about performance, this can be called once for good from rcu_spawn_gp_kthread() (that would be much better than checking that all the time from the kthread itself anyway).

I was figuring that a fair number of the kthreads might eventually be using this, not just for the grace-period kthreads.

Ok makes sense. But can we just rename the cpumask to housekeeping_mask?

Good point! After all, it someday might be something other than the complement of tick_nohz_full_mask.

Quoted 7 lines

Quoted 6 lines

In addition,
my concern about once-for-good affinity is for the NO_HZ_FULL_SYSIDLE=y case, where moving the grace-period kthreads prevents ever entering full-system idle state.

Or am I missing some use case?

Quoted 5 lines

Quoted 2 lines

No that's what I had in mind. But rcu_spawn_gp_kthread() still looks like a better place for that. Or am I missing something else?

My fear was that sysadmins would move it in the NO_HZ_FULL_SYSIDLE=y case, in which case it needs to move back.

If you use kthread_bind(), this can't be overriden by the user. See PF_NO_SETAFFINITY.

Fair point. This would be a kthread_bind_housekeeping(), then.

But I need to create an kthread_bind_mask() or some such that acts like kthread_bind(), but which takes a cpumask rather than a cpu. Easy enough to do, but sounds like 3.18 material rather than 3.17 material. So this is on my 3.18 list.

Thanx, Paul

Quoted 2 lines

I was figuring that a fair number of the kthreads might eventually be using this, not just for the grace-period kthreads.

Ok makes sense. But can we just rename the cpumask to housekeeping_mask?

Quoted 4 lines · Frederic Weisbecker

Quoted 2 lines

I was figuring that a fair number of the kthreads might eventually be using this, not just for the grace-period kthreads.

Ok makes sense. But can we just rename the cpumask to housekeeping_mask?

That would imply that all no-nohz processors are housekeeping? So all processors with a tick are housekeeping?

Could we make that set configurable? Ideally I'd like to have the ability restrict the housekeeping to one processor.

Quoted 5 lines · Christoph Lameter

Quoted 3 lines · Frederic Weisbecker

Quoted 2 lines

I was figuring that a fair number of the kthreads might eventually be using this, not just for the grace-period kthreads.

Quoted 4 lines

Quoted 1 line

Ok makes sense. But can we just rename the cpumask to housekeeping_mask?

That would imply that all no-nohz processors are housekeeping? So all processors with a tick are housekeeping?

Well, now that I think about it again, I would really like to keep housekeeping to CPU 0 when nohz_full= is passed.

Quoted 2 lines

Could we make that set configurable? Ideally I'd like to have the ability restrict the housekeeping to one processor.

Ah, I'm curious about your usecase. But I think we can do that. And we should.

In fact I think that Paul could keep affining grace period kthread to CPU 0 for the sole case when we have nohz_full= parameter passed.

I think the performance issues reported to him refer to CONFIG_NO_HZ_FULL=y config without nohz_full= parameter passed. That's the most important to address.

Optimizing the "nohz_full= passed" case is probably not very useful and worse it complicate things a lot.

What do you think Paul? Can we simplify things that way? I'm pretty sure that nobody cares about optimizing the nohz_full= case. That would really simplify things to stick to CPU 0.

Quoted 11 lines · Frederic Weisbecker

Quoted 9 lines · Christoph Lameter

Quoted 4 lines · Frederic Weisbecker

Quoted 2 lines

I was figuring that a fair number of the kthreads might eventually be using this, not just for the grace-period kthreads.

Ok makes sense. But can we just rename the cpumask to housekeeping_mask?

That would imply that all no-nohz processors are housekeeping? So all processors with a tick are housekeeping?

Quoted 2 lines

Well, now that I think about it again, I would really like to keep housekeeping to CPU 0 when nohz_full= is passed.

When CONFIG_NO_HZ_FULL_SYSIDLE=y, then housekeeping kthreads are bound to CPU 0. However, doing this causes significant slowdowns according to Fengguang's testing, so when CONFIG_NO_HZ_FULL_SYSIDLE=n, I bind the housekeeping kthreads to the set of non-nohz_full CPUs.

Quoted 15 lines

Quoted 3 lines

Could we make that set configurable? Ideally I'd like to have the ability restrict the housekeeping to one processor.

Ah, I'm curious about your usecase. But I think we can do that. And we should.

In fact I think that Paul could keep affining grace period kthread to CPU 0 for the sole case when we have nohz_full= parameter passed.

I think the performance issues reported to him refer to CONFIG_NO_HZ_FULL=y config without nohz_full= parameter passed. That's the most important to address.

Optimizing the "nohz_full= passed" case is probably not very useful and worse it complicate things a lot.

Quoted 3 lines

What do you think Paul? Can we simplify things that way? I'm pretty sure that nobody cares about optimizing the nohz_full= case. That would really simplify things to stick to CPU 0.

When we have CONFIG_NO_HZ_FULL_SYSIDLE=y, agreed. In that case, having housekeeping CPUs on CPUs other than CPU 0 means that you never reach full-system-idle state.

But in other cases, we appear to need more than one housekeeping CPU. This is especially the case when people run general workloads on systems that have NO_HZ_FULL=y, which appears to be a significant fraction of the systems these days.

Quoted 12 lines · Paul E. McKenney

Quoted 11 lines · Frederic Weisbecker

Quoted 9 lines · Christoph Lameter

Quoted 4 lines · Frederic Weisbecker

Quoted 2 lines

I was figuring that a fair number of the kthreads might eventually be using this, not just for the grace-period kthreads.

Ok makes sense. But can we just rename the cpumask to housekeeping_mask?

That would imply that all no-nohz processors are housekeeping? So all processors with a tick are housekeeping?

Quoted 7 lines

Quoted 2 lines

Well, now that I think about it again, I would really like to keep housekeeping to CPU 0 when nohz_full= is passed.

When CONFIG_NO_HZ_FULL_SYSIDLE=y, then housekeeping kthreads are bound to CPU 0. However, doing this causes significant slowdowns according to Fengguang's testing, so when CONFIG_NO_HZ_FULL_SYSIDLE=n, I bind the housekeeping kthreads to the set of non-nohz_full CPUs.

But did he see these slowdowns with nohz_full= parameter passed? I doubt he tested that. And I'm not sure that people who need full dynticks will run the usecases that trigger slowdowns with grace period kthreads.

I also doubt that people will often omit other CPUs than CPU 0 nohz_full= range.

Quoted 18 lines

Quoted 17 lines

Quoted 2 lines

Could we make that set configurable? Ideally I'd like to have the ability restrict the housekeeping to one processor.

Ah, I'm curious about your usecase. But I think we can do that. And we should.

In fact I think that Paul could keep affining grace period kthread to CPU 0 for the sole case when we have nohz_full= parameter passed.

I think the performance issues reported to him refer to CONFIG_NO_HZ_FULL=y config without nohz_full= parameter passed. That's the most important to address.

Optimizing the "nohz_full= passed" case is probably not very useful and worse it complicate things a lot.

What do you think Paul? Can we simplify things that way? I'm pretty sure that nobody cares about optimizing the nohz_full= case. That would really simplify things to stick to CPU 0.

Quoted 3 lines

When we have CONFIG_NO_HZ_FULL_SYSIDLE=y, agreed. In that case, having housekeeping CPUs on CPUs other than CPU 0 means that you never reach full-system-idle state.

That said I expect CONFIG_NO_HZ_FULL_SYSIDLE=y to be always enable for those who run NO_HZ_FULL in the long run.

Quoted 4 lines

But in other cases, we appear to need more than one housekeeping CPU. This is especially the case when people run general workloads on systems that have NO_HZ_FULL=y, which appears to be a significant fraction of the systems these days.

Yeah NO_HZ_FULL=y is likely to be enabled in many distros. But you know the amount of nohz_full= users.

Quoted 25 lines · Frederic Weisbecker

Quoted 12 lines · Paul E. McKenney

Quoted 11 lines · Frederic Weisbecker

Quoted 9 lines · Christoph Lameter

Quoted 4 lines · Frederic Weisbecker

Quoted 2 lines

I was figuring that a fair number of the kthreads might eventually be using this, not just for the grace-period kthreads.

Ok makes sense. But can we just rename the cpumask to housekeeping_mask?

That would imply that all no-nohz processors are housekeeping? So all processors with a tick are housekeeping?

Quoted 7 lines

Quoted 2 lines

Well, now that I think about it again, I would really like to keep housekeeping to CPU 0 when nohz_full= is passed.

When CONFIG_NO_HZ_FULL_SYSIDLE=y, then housekeeping kthreads are bound to CPU 0. However, doing this causes significant slowdowns according to Fengguang's testing, so when CONFIG_NO_HZ_FULL_SYSIDLE=n, I bind the housekeeping kthreads to the set of non-nohz_full CPUs.

But did he see these slowdowns with nohz_full= parameter passed? I doubt he tested that. And I'm not sure that people who need full dynticks will run the usecases that trigger slowdowns with grace period kthreads.

Quoted 2 lines

I also doubt that people will often omit other CPUs than CPU 0 nohz_full= range.

Agreed, this is only a problem when people run workloads for which NO_HZ_FULL is not well-suited. Which is why I settled on designating the non-nohz_full= CPUs as the housekeeping CPUs -- people wanting to run general workloads not suited to NO_HZ_FULL probably won't specify nohz_full=. If they don't, then any CPU can be a housekeeping CPU.

Quoted 19 lines

Quoted 19 lines

Quoted 18 lines

Quoted 3 lines

Could we make that set configurable? Ideally I'd like to have the ability restrict the housekeeping to one processor.

Ah, I'm curious about your usecase. But I think we can do that. And we should.

In fact I think that Paul could keep affining grace period kthread to CPU 0 for the sole case when we have nohz_full= parameter passed.

I think the performance issues reported to him refer to CONFIG_NO_HZ_FULL=y config without nohz_full= parameter passed. That's the most important to address.

Optimizing the "nohz_full= passed" case is probably not very useful and worse it complicate things a lot.

What do you think Paul? Can we simplify things that way? I'm pretty sure that nobody cares about optimizing the nohz_full= case. That would really simplify things to stick to CPU 0.

Quoted 6 lines

Quoted 3 lines

When we have CONFIG_NO_HZ_FULL_SYSIDLE=y, agreed. In that case, having housekeeping CPUs on CPUs other than CPU 0 means that you never reach full-system-idle state.

That said I expect CONFIG_NO_HZ_FULL_SYSIDLE=y to be always enable for those who run NO_HZ_FULL in the long run.

Hmmm... That probably means that we need boot-time parameters to make sysidle detection really happen. Otherwise, many users will get a nasty surprise once CONFIG_NO_HZ_FULL_SYSIDLE=y is enabled on systems that really aren't running HPC or RT workloads.

I suppose that I could confine SYSIDLE's attention to the nohz_full= CPUs -- that might actually make things work nicely in all cases with no configuration of any sort required. I will need to give this some thought.

Quoted 8 lines

Quoted 5 lines

But in other cases, we appear to need more than one housekeeping CPU. This is especially the case when people run general workloads on systems that have NO_HZ_FULL=y, which appears to be a significant fraction of the systems these days.

Yeah NO_HZ_FULL=y is likely to be enabled in many distros. But you know the amount of nohz_full= users.

Indeed! ;-)

Thanx, Paul

Quoted 26 lines · Paul E. McKenney

Quoted 25 lines · Frederic Weisbecker

Quoted 12 lines · Paul E. McKenney

Quoted 11 lines · Frederic Weisbecker

Quoted 9 lines · Christoph Lameter

Quoted 4 lines · Frederic Weisbecker

Quoted 2 lines

I was figuring that a fair number of the kthreads might eventually be using this, not just for the grace-period kthreads.

Ok makes sense. But can we just rename the cpumask to housekeeping_mask?

That would imply that all no-nohz processors are housekeeping? So all processors with a tick are housekeeping?

Quoted 7 lines

Quoted 2 lines

Well, now that I think about it again, I would really like to keep housekeeping to CPU 0 when nohz_full= is passed.

When CONFIG_NO_HZ_FULL_SYSIDLE=y, then housekeeping kthreads are bound to CPU 0. However, doing this causes significant slowdowns according to Fengguang's testing, so when CONFIG_NO_HZ_FULL_SYSIDLE=n, I bind the housekeeping kthreads to the set of non-nohz_full CPUs.

But did he see these slowdowns with nohz_full= parameter passed? I doubt he tested that. And I'm not sure that people who need full dynticks will run the usecases that trigger slowdowns with grace period kthreads.

Quoted 8 lines

Quoted 2 lines

I also doubt that people will often omit other CPUs than CPU 0 nohz_full= range.

Agreed, this is only a problem when people run workloads for which NO_HZ_FULL is not well-suited. Which is why I settled on designating the non-nohz_full= CPUs as the housekeeping CPUs -- people wanting to run general workloads not suited to NO_HZ_FULL probably won't specify nohz_full=. If they don't, then any CPU can be a housekeeping CPU.

Right. So affining GP kthread to all non-nohz-full CPU works in all case. It's convenient but it requires some plumbing:

* add a housekeeping cpumask and implement housekeeping_affine on top
* add kthread_bind_cpumask()

So what I propose is to skip these complications and just do:

if (tick_nohz_full_enabled()) // means that somebody passed nohz_full= kernel parameter
kthread_bind_cpu(GP kthread, 0)

Moreover Thomas didn't like the idea of extending housekeeping duty further CPU 0, arguing that it's too early for that. He meant that for timekeeping but the idea is expandable.

Quoted 30 lines

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

Quoted 2 lines

Could we make that set configurable? Ideally I'd like to have the ability restrict the housekeeping to one processor.

Ah, I'm curious about your usecase. But I think we can do that. And we should.

In fact I think that Paul could keep affining grace period kthread to CPU 0 for the sole case when we have nohz_full= parameter passed.

I think the performance issues reported to him refer to CONFIG_NO_HZ_FULL=y config without nohz_full= parameter passed. That's the most important to address.

Optimizing the "nohz_full= passed" case is probably not very useful and worse it complicate things a lot.

What do you think Paul? Can we simplify things that way? I'm pretty sure that nobody cares about optimizing the nohz_full= case. That would really simplify things to stick to CPU 0.

Quoted 6 lines

Quoted 3 lines

When we have CONFIG_NO_HZ_FULL_SYSIDLE=y, agreed. In that case, having housekeeping CPUs on CPUs other than CPU 0 means that you never reach full-system-idle state.

That said I expect CONFIG_NO_HZ_FULL_SYSIDLE=y to be always enable for those who run NO_HZ_FULL in the long run.

Hmmm... That probably means that we need boot-time parameters to make sysidle detection really happen. Otherwise, many users will get a nasty surprise once CONFIG_NO_HZ_FULL_SYSIDLE=y is enabled on systems that really aren't running HPC or RT workloads.

Quoted 4 lines

I suppose that I could confine SYSIDLE's attention to the nohz_full= CPUs -- that might actually make things work nicely in all cases with no configuration of any sort required. I will need to give this some thought.

Exactly, nohz_full= gives all the information we need for sysidle.

Quoted 12 lines

Quoted 7 lines

Quoted 4 lines

But in other cases, we appear to need more than one housekeeping CPU. This is especially the case when people run general workloads on systems that have NO_HZ_FULL=y, which appears to be a significant fraction of the systems these days.

Yeah NO_HZ_FULL=y is likely to be enabled in many distros. But you know the amount of nohz_full= users.

Indeed! ;-)

Thanx, Paul

Quoted 36 lines · Frederic Weisbecker

Quoted 26 lines · Paul E. McKenney

Quoted 25 lines · Frederic Weisbecker

Quoted 12 lines · Paul E. McKenney

Quoted 11 lines · Frederic Weisbecker

Quoted 9 lines · Christoph Lameter

Quoted 4 lines · Frederic Weisbecker

Quoted 2 lines

I was figuring that a fair number of the kthreads might eventually be using this, not just for the grace-period kthreads.

Ok makes sense. But can we just rename the cpumask to housekeeping_mask?

That would imply that all no-nohz processors are housekeeping? So all processors with a tick are housekeeping?

Quoted 7 lines

Quoted 2 lines

Well, now that I think about it again, I would really like to keep housekeeping to CPU 0 when nohz_full= is passed.

When CONFIG_NO_HZ_FULL_SYSIDLE=y, then housekeeping kthreads are bound to CPU 0. However, doing this causes significant slowdowns according to Fengguang's testing, so when CONFIG_NO_HZ_FULL_SYSIDLE=n, I bind the housekeeping kthreads to the set of non-nohz_full CPUs.

But did he see these slowdowns with nohz_full= parameter passed? I doubt he tested that. And I'm not sure that people who need full dynticks will run the usecases that trigger slowdowns with grace period kthreads.

Quoted 8 lines

Quoted 2 lines

I also doubt that people will often omit other CPUs than CPU 0 nohz_full= range.

Agreed, this is only a problem when people run workloads for which NO_HZ_FULL is not well-suited. Which is why I settled on designating the non-nohz_full= CPUs as the housekeeping CPUs -- people wanting to run general workloads not suited to NO_HZ_FULL probably won't specify nohz_full=. If they don't, then any CPU can be a housekeeping CPU.

Quoted 5 lines

Right. So affining GP kthread to all non-nohz-full CPU works in all case. It's convenient but it requires some plumbing:

* add a housekeeping cpumask and implement housekeeping_affine on top
* add kthread_bind_cpumask()

Yep.

Quoted 2 lines

So what I propose is to skip these complications and just do:

Quoted 5 lines

if (tick_nohz_full_enabled()) // means that somebody passed nohz_full= kernel parameter
kthread_bind_cpu(GP kthread, 0)

Moreover Thomas didn't like the idea of extending housekeeping duty further CPU 0, arguing that it's too early for that. He meant that for timekeeping but the idea is expandable.

Although I agree that we can get away with a single timekeeping CPU, I don't believe that we get away with having only a single housekeeping CPU.

Quoted 2 lines

That would imply that all no-nohz processors are housekeeping? So all processors with a tick are housekeeping?

Well, now that I think about it again, I would really like to keep housekeeping to CPU 0 when nohz_full= is passed.

Quoted 2 lines

Could we make that set configurable? Ideally I'd like to have the ability restrict the housekeeping to one processor.

Ah, I'm curious about your usecase. But I think we can do that. And we should.

Quoted 5 lines · Frederic Weisbecker

Quoted 2 lines

That would imply that all no-nohz processors are housekeeping? So all processors with a tick are housekeeping?

Well, now that I think about it again, I would really like to keep housekeeping to CPU 0 when nohz_full= is passed.

Yeah.

Quoted 3 lines

Quoted 2 lines

Could we make that set configurable? Ideally I'd like to have the ability restrict the housekeeping to one processor.

Quoted 1 line

Ah, I'm curious about your usecase. But I think we can do that. And we should.

The use case is pretty straightforward because we are trying to keep as much OS noise as possible off most processors. Processor 0 is the sacrificial lamb that will be used for all OS processing and hopefully all high latency operations will occur there. Processors 1-X have a tick but we still try to keep latencies sane. And then there is X-Y where tick is off.

Quoted 13 lines · Christoph Lameter

Quoted 5 lines · Frederic Weisbecker

Quoted 2 lines

That would imply that all no-nohz processors are housekeeping? So all processors with a tick are housekeeping?

Well, now that I think about it again, I would really like to keep housekeeping to CPU 0 when nohz_full= is passed.

Yeah.

Quoted 3 lines

Quoted 2 lines

Could we make that set configurable? Ideally I'd like to have the ability restrict the housekeeping to one processor.

Quoted 8 lines

Quoted 1 line

Ah, I'm curious about your usecase. But I think we can do that. And we should.

The use case is pretty straightforward because we are trying to keep as much OS noise as possible off most processors. Processor 0 is the sacrificial lamb that will be used for all OS processing and hopefully all high latency operations will occur there. Processors 1-X have a tick but we still try to keep latencies sane. And then there is X-Y where tick is off.

Ok. I don't entirely get why you need 1-X but I can easily imagine some non-latency-critical stuff running there.

Paul proposed "housekeeping=". If we ever go there, I'd rather vote for "sacrifical_lamb="

Quoted 23 lines · Frederic Weisbecker

Quoted 13 lines · Christoph Lameter

Quoted 5 lines · Frederic Weisbecker

Quoted 2 lines

That would imply that all no-nohz processors are housekeeping? So all processors with a tick are housekeeping?

Well, now that I think about it again, I would really like to keep housekeeping to CPU 0 when nohz_full= is passed.

Yeah.

Quoted 3 lines

Quoted 2 lines

Could we make that set configurable? Ideally I'd like to have the ability restrict the housekeeping to one processor.

Quoted 8 lines

Quoted 1 line

Ah, I'm curious about your usecase. But I think we can do that. And we should.

The use case is pretty straightforward because we are trying to keep as much OS noise as possible off most processors. Processor 0 is the sacrificial lamb that will be used for all OS processing and hopefully all high latency operations will occur there. Processors 1-X have a tick but we still try to keep latencies sane. And then there is X-Y where tick is off.

Quoted 4 lines

Ok. I don't entirely get why you need 1-X but I can easily imagine some non-latency-critical stuff running there.

Paul proposed "housekeeping=". If we ever go there, I'd rather vote for "sacrifical_lamb="

Given Christoph's desire for only one housekeeping CPU, I guess the counting version makes the most sense, so that "housekeeping=N" designates the first N non-nohz CPUs in numerical order as housekeeping CPUs. If there are fewer than N non-nohz CPUs, you get a splat at boot time and your request is capped at the number of non-nohz CPUs.

Seem reasonable?

Thanx, Paul

Quoted 35 lines · Paul E. McKenney

Quoted 23 lines · Frederic Weisbecker

Quoted 13 lines · Christoph Lameter

Quoted 5 lines · Frederic Weisbecker

Quoted 2 lines

That would imply that all no-nohz processors are housekeeping? So all processors with a tick are housekeeping?

Well, now that I think about it again, I would really like to keep housekeeping to CPU 0 when nohz_full= is passed.

Yeah.

Quoted 3 lines

Quoted 2 lines

Could we make that set configurable? Ideally I'd like to have the ability restrict the housekeeping to one processor.

Quoted 8 lines

Quoted 1 line

Ah, I'm curious about your usecase. But I think we can do that. And we should.

The use case is pretty straightforward because we are trying to keep as much OS noise as possible off most processors. Processor 0 is the sacrificial lamb that will be used for all OS processing and hopefully all high latency operations will occur there. Processors 1-X have a tick but we still try to keep latencies sane. And then there is X-Y where tick is off.

Quoted 4 lines

Ok. I don't entirely get why you need 1-X but I can easily imagine some non-latency-critical stuff running there.

Paul proposed "housekeeping=". If we ever go there, I'd rather vote for "sacrifical_lamb="

Given Christoph's desire for only one housekeeping CPU, I guess the counting version makes the most sense, so that "housekeeping=N" designates the first N non-nohz CPUs in numerical order as housekeeping CPUs. If there are fewer than N non-nohz CPUs, you get a splat at boot time and your request is capped at the number of non-nohz CPUs.

Quoted 1 line

Seem reasonable?

I wonder if it's wouldn't be rather reasonable to affine housekeeping to all non-nohz-full CPUs by default and then people who want finergrained housekeeping can affine manually kthreads from userspace.

That implies to bind without PF_NO_SETAFFINIT but that's easy enough to do.

Quoted 5 lines

I wonder if it's wouldn't be rather reasonable to affine housekeeping to all non-nohz-full CPUs by default and then people who want finergrained housekeeping can affine manually kthreads from userspace.

That implies to bind without PF_NO_SETAFFINIT but that's easy enough to do.

Works for me!

The use case is pretty straightforward because we are trying to keep as much OS noise as possible off most processors. Processor 0 is the sacrificial lamb that will be used for all OS processing and hopefully all high latency operations will occur there. Processors 1-X have a tick but we still try to keep latencies sane. And then there is X-Y where tick is off.

Quoted 6 lines · Christoph Lameter

The use case is pretty straightforward because we are trying to keep as much OS noise as possible off most processors. Processor 0 is the sacrificial lamb that will be used for all OS processing and hopefully all high latency operations will occur there. Processors 1-X have a tick but we still try to keep latencies sane. And then there is X-Y where tick is off.

So the problem is -- and this isn't too hard to create -- when we saturate/overload cpu0 with just the housekeeping. At that point it would be good if we could scale and use multiple housekeepers.

Quoted 3 lines · Frederic Weisbecker

Quoted 2 lines

I was figuring that a fair number of the kthreads might eventually be using this, not just for the grace-period kthreads.

Quoted 1 line

Ok makes sense. But can we just rename the cpumask to housekeeping_mask?

That would imply that all no-nohz processors are housekeeping? So all processors with a tick are housekeeping?

Could we make that set configurable? Ideally I'd like to have the ability restrict the housekeeping to one processor.

The CONFIG_PROVE_RCU_DELAY Kconfig parameter doesn't appear to be very effective at finding race conditions, so this commit removes it.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Andi Kleen <[email protected]>

---
 kernel/rcu/tree.c                                       | 5 -----
 kernel/rcu/update.c                                     | 3 ---

 tools/testing/selftests/rcutorture/configs/rcu/TREE01   | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE02   | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE02-T | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE03   | 1 -

 tools/testing/selftests/rcutorture/configs/rcu/TREE04   | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE05   | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE06   | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE07   | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE08   | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE08-T | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE09   | 1 -
 13 files changed, 19 deletions(-)

Paul,

Quoted 8 lines · Paul E. McKenney

The CONFIG_PROVE_RCU_DELAY Kconfig parameter doesn't appear to be very effective at finding race conditions, so this commit removes it.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Andi Kleen <[email protected]>

---
 kernel/rcu/tree.c                                       | 5 -----
 kernel/rcu/update.c                                     | 3 ---

The commit explanation implies that this patch would also remove the entry for PROVE_RCU_DELAY in lib/Kconfig.debug. Is there any reason to keep it after this patch?

Quoted 4 lines

 tools/testing/selftests/rcutorture/configs/rcu/TREE01   | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE02   | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE02-T | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE03   | 1 -

Quoted 8 lines

 tools/testing/selftests/rcutorture/configs/rcu/TREE04   | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE05   | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE06   | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE07   | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE08   | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE08-T | 1 -
 tools/testing/selftests/rcutorture/configs/rcu/TREE09   | 1 -
 13 files changed, 19 deletions(-)

This doesn't touch a few other references in tools/testing/selftests/rcutorture/. I have no idea how these tests work, so I assume these references are still needed. Is that correct?

Hello!

This series provides miscellaneous fixes:

1. Document deadlock-avoidance information in rcu_read_unlock()'s
docbook comment header.

2. Remove obsolete references to TINY_PREEMPT_RCU.

3. Add deadlock explanation to local_irq_save() call in

	__lock_task_sighand().

4. Make the rcu_node arrays be static const char * const,
courtesy of Fabian Frederick.

5. Remove redundant ACCESS_ONCE() from tick_do_timer_cpu under

	#ifdef CONFIG_NO_HZ_FULL.

6. Eliminate read-modify-write ACCESS_ONCE() calls.

7. Loosen __call_rcu()'s rcu_head alignment constraint to handle

	m68k's 16-bit alignment.

8. Allow post-unlock reference for rt_mutex.

9. Check both root and current rcu_node structures when setting up
future grace periods, courtesy of Pranith Kumar.

10. Simplify priority boosting by putting rt_mutex in rcu_node
structure.

11. Bind grace-period kthreads to no-NO_HZ_FULL CPUs instead of the
timekeeping CPU, at least for CONFIG_NO_HZ_FULL_SYSIDLE=n.

12. Don't use NMIs to dump other CPUs' stacks.

13. Use __this_cpu_read() instead of per_cpu_ptr(), courtesy of Shan Wei.

14. Remove CONFIG_PROVE_RCU_DELAY.

15. Fix __rcu_reclaim to use true/false instead of 1/0.

16. Fix sparse warning in rcu_initiate_boost(), courtesy of Pranith
Kumar.

17. Fix sparse warning in rcu_report_unblock_qs_rnp(), again courtesy
of Pranith Kumar.

Thanx, Paul

------------------------------------------------------------------------

 b/include/linux/init_task.h                               |    9 --
 b/include/linux/rcupdate.h                                |   45 ++++++++--
 b/include/linux/sched.h                                   |    6 -
 b/include/linux/tick.h                                    |   19 ++++
 b/init/Kconfig                                            |    2 
 b/kernel/rcu/rcu.h                                        |    8 +
 b/kernel/rcu/srcu.c                                       |    4 
 b/kernel/rcu/tree.c                                       |   59 ++++++--------
 b/kernel/rcu/tree.h                                       |    8 +
 b/kernel/rcu/tree_plugin.h                                |   52 +++++++-----
 b/kernel/rcu/update.c                                     |    3 
 b/kernel/signal.c                                         |    4 
 b/kernel/time/tick-sched.c                                |   10 ++
 b/tools/testing/selftests/rcutorture/configs/rcu/TREE01   |    1 
 b/tools/testing/selftests/rcutorture/configs/rcu/TREE02   |    1 
 b/tools/testing/selftests/rcutorture/configs/rcu/TREE02-T |    1 
 b/tools/testing/selftests/rcutorture/configs/rcu/TREE03   |    1 
 b/tools/testing/selftests/rcutorture/configs/rcu/TREE04   |    1 
 b/tools/testing/selftests/rcutorture/configs/rcu/TREE05   |    1 
 b/tools/testing/selftests/rcutorture/configs/rcu/TREE06   |    1 
 b/tools/testing/selftests/rcutorture/configs/rcu/TREE07   |    1 
 b/tools/testing/selftests/rcutorture/configs/rcu/TREE08   |    1 
 b/tools/testing/selftests/rcutorture/configs/rcu/TREE08-T |    1 
 b/tools/testing/selftests/rcutorture/configs/rcu/TREE09   |    1 
 24 files changed, 147 insertions(+), 93 deletions(-)

.