nohz: Convert tick dependency mask to atomic_t

The tick dependency mask was intially unsigned long because this is the type on which clear_bit() operates on and fetch_or() accepts it.

But now that we have atomic_fetch_or(), we can instead use atomic_andnot() to clear the bit. This consolidates the type of our tick dependency mask, reduce its size on structures and benefit from possible architecture optimizations on atomic_t operations.

Suggested-by Linus Torvalds <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Signed-off-by Frederic Weisbecker <[email protected]>

---
 include/linux/sched.h    |  4 ++--
 kernel/time/tick-sched.c | 61 ++++++++++++++++++++++++------------------------
 kernel/time/tick-sched.h |  2 +-
 3 files changed, 33 insertions(+), 34 deletions(-)

include/linux/sched.h +2 -2

index 34495d2..a43a593 100644
@@ -719,7 +719,7 @@ struct signal_struct {
 	struct task_cputime cputime_expires;
 
 #ifdef CONFIG_NO_HZ_FULL
-	unsigned long tick_dep_mask;
+	atomic_t tick_dep_mask;
 #endif
 
 	struct list_head cpu_timers[3];

@@ -1548,7 +1548,7 @@ struct task_struct {
 #endif
 
 #ifdef CONFIG_NO_HZ_FULL
-	unsigned long tick_dep_mask;
+	atomic_t tick_dep_mask;
 #endif
 	unsigned long nvcsw, nivcsw; /* context switch counts */
 	u64 start_time;		/* monotonic time in nsec */

kernel/time/tick-sched.c +30 -31

index 084b79f..58e3310 100644
@@ -157,52 +157,50 @@ static void tick_sched_handle(struct tick_sched *ts, struct pt_regs *regs)
 cpumask_var_t tick_nohz_full_mask;
 cpumask_var_t housekeeping_mask;
 bool tick_nohz_full_running;
-static unsigned long tick_dep_mask;
+static atomic_t tick_dep_mask;
 
-static void trace_tick_dependency(unsigned long dep)
+static bool check_tick_dependency(atomic_t *dep)
 {
-	if (dep & TICK_DEP_MASK_POSIX_TIMER) {
+	int val = atomic_read(dep);
+
+	if (val & TICK_DEP_MASK_POSIX_TIMER) {
 		trace_tick_stop(0, TICK_DEP_MASK_POSIX_TIMER);
-		return;
+		return true;
 	}
 
-	if (dep & TICK_DEP_MASK_PERF_EVENTS) {
+	if (val & TICK_DEP_MASK_PERF_EVENTS) {
 		trace_tick_stop(0, TICK_DEP_MASK_PERF_EVENTS);
-		return;
+		return true;
 	}
 
-	if (dep & TICK_DEP_MASK_SCHED) {
+	if (val & TICK_DEP_MASK_SCHED) {
 		trace_tick_stop(0, TICK_DEP_MASK_SCHED);
-		return;
+		return true;
 	}
 
-	if (dep & TICK_DEP_MASK_CLOCK_UNSTABLE)
+	if (val & TICK_DEP_MASK_CLOCK_UNSTABLE) {
 		trace_tick_stop(0, TICK_DEP_MASK_CLOCK_UNSTABLE);
+		return true;
+	}
+
+	return false;
 }
 
 static bool can_stop_full_tick(struct tick_sched *ts)
 {
 	WARN_ON_ONCE(!irqs_disabled());
 
-	if (tick_dep_mask) {
-		trace_tick_dependency(tick_dep_mask);
+	if (check_tick_dependency(&tick_dep_mask))
 		return false;
-	}
 
-	if (ts->tick_dep_mask) {
-		trace_tick_dependency(ts->tick_dep_mask);
+	if (check_tick_dependency(&ts->tick_dep_mask))
 		return false;
-	}
 
-	if (current->tick_dep_mask) {
-		trace_tick_dependency(current->tick_dep_mask);
+	if (check_tick_dependency(&current->tick_dep_mask))
 		return false;
-	}
 
-	if (current->signal->tick_dep_mask) {
-		trace_tick_dependency(current->signal->tick_dep_mask);
+	if (check_tick_dependency(&current->signal->tick_dep_mask))
 		return false;
-	}
 
 	return true;
 }

@@ -259,12 +257,12 @@ static void tick_nohz_full_kick_all(void)
 	preempt_enable();
 }
 
-static void tick_nohz_dep_set_all(unsigned long *dep,
+static void tick_nohz_dep_set_all(atomic_t *dep,
 				  enum tick_dep_bits bit)
 {
-	unsigned long prev;
+	int prev;
 
-	prev = fetch_or(dep, BIT_MASK(bit));
+	prev = atomic_fetch_or(dep, BIT(bit));
 	if (!prev)
 		tick_nohz_full_kick_all();
 }

@@ -280,7 +278,7 @@ void tick_nohz_dep_set(enum tick_dep_bits bit)
 
 void tick_nohz_dep_clear(enum tick_dep_bits bit)
 {
-	clear_bit(bit, &tick_dep_mask);
+	atomic_andnot(BIT(bit), &tick_dep_mask);
 }
 
 /*

@@ -289,12 +287,12 @@ void tick_nohz_dep_clear(enum tick_dep_bits bit)
  */
 void tick_nohz_dep_set_cpu(int cpu, enum tick_dep_bits bit)
 {
-	unsigned long prev;
+	int prev;
 	struct tick_sched *ts;
 
 	ts = per_cpu_ptr(&tick_cpu_sched, cpu);
 
-	prev = fetch_or(&ts->tick_dep_mask, BIT_MASK(bit));
+	prev = atomic_fetch_or(&ts->tick_dep_mask, BIT(bit));
 	if (!prev) {
 		preempt_disable();
 		/* Perf needs local kick that is NMI safe */

@@ -313,7 +311,7 @@ void tick_nohz_dep_clear_cpu(int cpu, enum tick_dep_bits bit)
 {
 	struct tick_sched *ts = per_cpu_ptr(&tick_cpu_sched, cpu);
 
-	clear_bit(bit, &ts->tick_dep_mask);
+	atomic_andnot(BIT(bit), &ts->tick_dep_mask);
 }
 
 /*

@@ -331,7 +329,7 @@ void tick_nohz_dep_set_task(struct task_struct *tsk, enum tick_dep_bits bit)
 
 void tick_nohz_dep_clear_task(struct task_struct *tsk, enum tick_dep_bits bit)
 {
-	clear_bit(bit, &tsk->tick_dep_mask);
+	atomic_andnot(BIT(bit), &tsk->tick_dep_mask);
 }
 
 /*

@@ -345,7 +343,7 @@ void tick_nohz_dep_set_signal(struct signal_struct *sig, enum tick_dep_bits bit)
 
 void tick_nohz_dep_clear_signal(struct signal_struct *sig, enum tick_dep_bits bit)
 {
-	clear_bit(bit, &sig->tick_dep_mask);
+	atomic_andnot(BIT(bit), &sig->tick_dep_mask);
 }
 
 /*

@@ -366,7 +364,8 @@ void __tick_nohz_task_switch(void)
 	ts = this_cpu_ptr(&tick_cpu_sched);
 
 	if (ts->tick_stopped) {
-		if (current->tick_dep_mask || current->signal->tick_dep_mask)
+		if (atomic_read(&current->tick_dep_mask) ||
+		    atomic_read(&current->signal->tick_dep_mask))
 			tick_nohz_full_kick();
 	}
 out:

kernel/time/tick-sched.h +0 -0

index eb4e325..bf38226 100644

kernel/time/tick-sched.h struct tick_sched { · line 66

Quoted 13 lines

kernel/time/tick-sched.h +1 -1

index eb4e325..bf38226 100644
@@ -60,7 +60,7 @@ struct tick_sched {
 	u64				next_timer;
 	ktime_t				idle_expires;
 	int				do_timer_last;
-	unsigned long			tick_dep_mask;
+	atomic_t			tick_dep_mask;
 };
 
 extern struct tick_sched *tick_get_tick_sched(int cpu);

Yeah, so I really like this interface, because it makes it really, really obvious that only atomic_t-compatible operations can be used on the value. It's a common bug to have a long, operated on atomically via bitops - and then occasionally operated on in a non-atomic fashion, or used without taking ordering into account. Such bugs are quite hard to find.

This change also shrinks the mask from long to int, which is an another bonus, and which addresses the other objection Linus had.

As per Linus suggestion, lets convert the tick dependency mask to atomic_t. Introduce atomic_fetch_or() and confine fetch_or() back to scheduler guts.

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
timers/nohz

HEAD: 7b7e5da5733f58668181077ec394a718e08c392c

Thanks,
Frederic
---

Frederic Weisbecker (3):
atomic: Introduce atomic_fetch_or
nohz: Convert tick dependency mask to atomic_t Revert "atomic: Export fetch_or()"

 include/linux/atomic.h   | 34 +++++++++++++--------------
 include/linux/sched.h    |  4 ++--
 kernel/sched/core.c      | 18 ++++++++++++++
 kernel/time/tick-sched.c | 61 ++++++++++++++++++++++++------------------------
 kernel/time/tick-sched.h |  2 +-
 5 files changed, 68 insertions(+), 51 deletions(-)

Quoted 19 lines · Frederic Weisbecker

As per Linus suggestion, lets convert the tick dependency mask to atomic_t. Introduce atomic_fetch_or() and confine fetch_or() back to scheduler guts.

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
timers/nohz

HEAD: 7b7e5da5733f58668181077ec394a718e08c392c

Thanks,
Frederic
---

Frederic Weisbecker (3):
atomic: Introduce atomic_fetch_or
nohz: Convert tick dependency mask to atomic_t Revert "atomic: Export fetch_or()"

Quoted 6 lines

 include/linux/atomic.h   | 34 +++++++++++++--------------
 include/linux/sched.h    |  4 ++--
 kernel/sched/core.c      | 18 ++++++++++++++
 kernel/time/tick-sched.c | 61 ++++++++++++++++++++++++------------------------
 kernel/time/tick-sched.h |  2 +-
 5 files changed, 68 insertions(+), 51 deletions(-)

Could you please also convert the sched/core.c usage, so that we can get rid of the private fetch_or() definition? Please also double check that it does not result in worse code generation.

Quoted 29 lines · Ingo Molnar

Quoted 25 lines · Frederic Weisbecker

As per Linus suggestion, lets convert the tick dependency mask to atomic_t. Introduce atomic_fetch_or() and confine fetch_or() back to scheduler guts.

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
timers/nohz

HEAD: 7b7e5da5733f58668181077ec394a718e08c392c

Thanks,
Frederic
---

Frederic Weisbecker (3):
atomic: Introduce atomic_fetch_or
nohz: Convert tick dependency mask to atomic_t Revert "atomic: Export fetch_or()"

 include/linux/atomic.h   | 34 +++++++++++++--------------
 include/linux/sched.h    |  4 ++--
 kernel/sched/core.c      | 18 ++++++++++++++
 kernel/time/tick-sched.c | 61 ++++++++++++++++++++++++------------------------
 kernel/time/tick-sched.h |  2 +-
 5 files changed, 68 insertions(+), 51 deletions(-)

Quoted 3 lines

Could you please also convert the sched/core.c usage, so that we can get rid of the private fetch_or() definition? Please also double check that it does not result in worse code generation.

That involve converting thread_info::flags to atomic_t and given how much the type varies across architectures:

also given how much it is accessed (and that happens a lot in ASM as well). This conversion deserves quite a whole project on its own.

It might be possible to do it incrementally though.

Harmonizing thread_info::flags does not look easy, given how much assembly code accesses this field.

Quoted 3 lines · Ingo Molnar

Harmonizing thread_info::flags does not look easy, given how much assembly code accesses this field.

It might not be too bad.

For 32-bit architectures (which is still most of them), it's just a

   unsigned int/long -> atomic_t

and for 64-bit architectures you end up with three choices:

- it's already 32-bit (alpha, ia64, x86):

        unsigned int -> atomic_t

- little-endian long:

        atomic_t flags
        unsigned int padding;

- big-endian long (only powerpc? Maybe there's a big-endian MIPS still?)

        unsigned int padding;
        atomic_t flags;

Quoted 19 lines · Linus Torvalds

Quoted 3 lines · Ingo Molnar

Harmonizing thread_info::flags does not look easy, given how much assembly code accesses this field.

It might not be too bad.

For 32-bit architectures (which is still most of them), it's just a

   unsigned int/long -> atomic_t

and for 64-bit architectures you end up with three choices:

- it's already 32-bit (alpha, ia64, x86):

        unsigned int -> atomic_t

- little-endian long:

Quoted 7 lines

        atomic_t flags
        unsigned int padding;

- big-endian long (only powerpc? Maybe there's a big-endian MIPS still?)

        unsigned int padding;
        atomic_t flags;

Hm, that indeed sounds fairly nice and doable - I thought some architectures do have a task flag above bit 31, but that does not appear to be so ...

Right now we seem to have 27 bits defined in include/linux/sched.h, with 5 more bits left for the future. Here's their current usage histogram in the kernel source:

PF_KTHREAD : 68
PF_MEMALLOC : 65

Quoted 37 lines · Ingo Molnar

Quoted 19 lines · Linus Torvalds

Quoted 3 lines · Ingo Molnar

Harmonizing thread_info::flags does not look easy, given how much assembly code accesses this field.

It might not be too bad.

For 32-bit architectures (which is still most of them), it's just a

   unsigned int/long -> atomic_t

and for 64-bit architectures you end up with three choices:

- it's already 32-bit (alpha, ia64, x86):

        unsigned int -> atomic_t

- little-endian long:

Quoted 7 lines

        atomic_t flags
        unsigned int padding;

- big-endian long (only powerpc? Maybe there's a big-endian MIPS still?)

        unsigned int padding;
        atomic_t flags;

Hm, that indeed sounds fairly nice and doable - I thought some architectures do have a task flag above bit 31, but that does not appear to be so ...

Right now we seem to have 27 bits defined in include/linux/sched.h, with 5 more bits left for the future. Here's their current usage histogram in the kernel source:

Quoted 2 lines

PF_KTHREAD : 68
PF_MEMALLOC : 65

Argh, my reading comprehension skills suck today.

That's a totally useless analysis of task_struct::flags, while we want to convert thread_info::flags...

Quoted 43 lines · Ingo Molnar

Quoted 39 lines · Ingo Molnar

Quoted 19 lines · Linus Torvalds

Quoted 3 lines · Ingo Molnar

Harmonizing thread_info::flags does not look easy, given how much assembly code accesses this field.

It might not be too bad.

For 32-bit architectures (which is still most of them), it's just a

   unsigned int/long -> atomic_t

and for 64-bit architectures you end up with three choices:

- it's already 32-bit (alpha, ia64, x86):

        unsigned int -> atomic_t

- little-endian long:

Quoted 7 lines

        atomic_t flags
        unsigned int padding;

- big-endian long (only powerpc? Maybe there's a big-endian MIPS still?)

        unsigned int padding;
        atomic_t flags;

Hm, that indeed sounds fairly nice and doable - I thought some architectures do have a task flag above bit 31, but that does not appear to be so ...

Right now we seem to have 27 bits defined in include/linux/sched.h, with 5 more bits left for the future. Here's their current usage histogram in the kernel source:

PF_KTHREAD : 68
PF_MEMALLOC : 65

Quoted 4 lines

Argh, my reading comprehension skills suck today.

That's a totally useless analysis of task_struct::flags, while we want to convert thread_info::flags...

Actually we want to convert that one too :-)
In fact I planned to start there.

Sounds good to me! I also volunteer the x86 architecture to be the guinea pig to convert thread_info::flags to atomic_t ;-) [*]

Quoted 2 lines · Ingo Molnar

Sounds good to me! I also volunteer the x86 architecture to be the guinea pig to convert thread_info::flags to atomic_t ;-) [*]

So I'm not sure we can do this one arch at a time; all the TIF manipulators live in include/linux/thread_info.h and are shared across all archs.

That's a totally useless analysis of task_struct::flags, while we want to convert thread_info::flags...