[PATCH 0/3] nohz: Convert tick dependency mask to atomic_t

[PATCH 0/3] nohz: Convert tick dependency mask to atomic_t

[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(-)

[PATCH 1/3] atomic: Introduce atomic_fetch_or

[Frederic Weisbecker]

This is deemed to replace the type generic fetch_or() which brings a lot
of issues such as macro induced block variable aliasing and sloppy types.
Not to mention fetch_or() doesn't refer to any namespace, adding even
more confusion.

So lets provide an atomic_t version. Current and next users of fetch_or
are thus encouraged to use atomic_t.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 include/linux/atomic.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/linux/atomic.h b/include/linux/atomic.h
index df4f369..3d64c08 100644
--- a/include/linux/atomic.h
+++ b/include/linux/atomic.h
@@ -559,6 +559,27 @@ static inline int atomic_dec_if_positive(atomic_t *v)
 #endif
 
 /**
+ * atomic_fetch_or - perform *p |= mask and return old value of *p
+ * @p: pointer to atomic_t
+ * @mask: mask to OR on the atomic_t
+ */
+#ifndef atomic_fetch_or
+static inline int atomic_fetch_or(atomic_t *p, int mask)
+{
+	int old, val = atomic_read(p);
+
+	for (;;) {
+		old = atomic_cmpxchg(p, val, val | mask);
+		if (old == val)
+			break;
+		val = old;
+	}
+
+	return old;
+}
+#endif
+
+/**
  * fetch_or - perform *ptr |= mask and return old value of *ptr
  * @ptr: pointer to value
  * @mask: mask to OR on the value
-- 
2.7.0

[tip:core/urgent] locking/atomic: Introduce atomic_fetch_or()

[tip-bot for Frederic Weisbecker]

Commit-ID:  5acba71e18833b9d06686b3751598bfa263a3ac3
Gitweb:     http://git.kernel.org/tip/5acba71e18833b9d06686b3751598bfa263a3ac3
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Thu, 24 Mar 2016 15:37:59 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 29 Mar 2016 11:52:11 +0200

locking/atomic: Introduce atomic_fetch_or()

This is deemed to replace the type generic fetch_or() which brings a lot
of issues such as macro induced block variable aliasing and sloppy types.
Not to mention fetch_or() doesn't refer to any namespace, adding even
more confusion.

So lets provide an atomic_t version. Current and next users of fetch_or()
are thus encouraged to use atomic_t.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/1458830281-4255-2-git-send-email-fweisbec@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/atomic.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/linux/atomic.h b/include/linux/atomic.h
index df4f369..3d64c08 100644
--- a/include/linux/atomic.h
+++ b/include/linux/atomic.h
@@ -559,6 +559,27 @@ static inline int atomic_dec_if_positive(atomic_t *v)
 #endif
 
 /**
+ * atomic_fetch_or - perform *p |= mask and return old value of *p
+ * @p: pointer to atomic_t
+ * @mask: mask to OR on the atomic_t
+ */
+#ifndef atomic_fetch_or
+static inline int atomic_fetch_or(atomic_t *p, int mask)
+{
+	int old, val = atomic_read(p);
+
+	for (;;) {
+		old = atomic_cmpxchg(p, val, val | mask);
+		if (old == val)
+			break;
+		val = old;
+	}
+
+	return old;
+}
+#endif
+
+/**
  * fetch_or - perform *ptr |= mask and return old value of *ptr
  * @ptr: pointer to value
  * @mask: mask to OR on the value

[PATCH 2/3] nohz: Convert tick dependency mask to atomic_t

[Frederic Weisbecker]

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 <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 include/linux/sched.h    |  4 ++--
 kernel/time/tick-sched.c | 61 ++++++++++++++++++++++++------------------------
 kernel/time/tick-sched.h |  2 +-
 3 files changed, 33 insertions(+), 34 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 34495d2..a43a593 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -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 */
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 084b79f..58e3310 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -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:
diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
index eb4e325..bf38226 100644
--- a/kernel/time/tick-sched.h
+++ b/kernel/time/tick-sched.h
@@ -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);
-- 
2.7.0

Re: [PATCH 2/3] nohz: Convert tick dependency mask to atomic_t

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> 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 <torvalds@linux-foundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
>  include/linux/sched.h    |  4 ++--
>  kernel/time/tick-sched.c | 61 ++++++++++++++++++++++++------------------------
>  kernel/time/tick-sched.h |  2 +-
>  3 files changed, 33 insertions(+), 34 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 34495d2..a43a593 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -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 */
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 084b79f..58e3310 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -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:
> diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
> index eb4e325..bf38226 100644
> --- a/kernel/time/tick-sched.h
> +++ b/kernel/time/tick-sched.h
> @@ -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.

Thanks,

	Ingo

Re: [PATCH 2/3] nohz: Convert tick dependency mask to atomic_t

[Frederic Weisbecker]

On Fri, Mar 25, 2016 at 09:52:02AM +0100, Ingo Molnar wrote:
> > diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
> > index eb4e325..bf38226 100644
> > --- a/kernel/time/tick-sched.h
> > +++ b/kernel/time/tick-sched.h
> > @@ -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.

Thanks, I much prefer it that way too!

Should I do a pull request or can I let you apply these?

[tip:core/urgent] timers/nohz: Convert tick dependency mask to atomic_t

[tip-bot for Frederic Weisbecker]

Commit-ID:  f009a7a767e792d5ab0b46c08d46236ea5271dd9
Gitweb:     http://git.kernel.org/tip/f009a7a767e792d5ab0b46c08d46236ea5271dd9
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Thu, 24 Mar 2016 15:38:00 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 29 Mar 2016 11:52:11 +0200

timers/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 <torvalds@linux-foundation.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/1458830281-4255-3-git-send-email-fweisbec@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/sched.h    |  4 ++--
 kernel/time/tick-sched.c | 61 ++++++++++++++++++++++++------------------------
 kernel/time/tick-sched.h |  2 +-
 3 files changed, 33 insertions(+), 34 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 60bba7e..52c4847 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -720,7 +720,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];
@@ -1549,7 +1549,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 */
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 084b79f..58e3310 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -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:
diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
index eb4e325..bf38226 100644
--- a/kernel/time/tick-sched.h
+++ b/kernel/time/tick-sched.h
@@ -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);

[PATCH 3/3] Revert "atomic: Export fetch_or()"

[Frederic Weisbecker]

This reverts commit 5fd7a09cfb8c6852f596c1f8c891c6158395250e.

Now that we have introduced atomic_fetch_or(), fetch_or() is only used
by the scheduler in order to deal with thread_info flags which type
can vary across architectures.

Lets confine fetch_or() back to the scheduler so that we encourage
future users to use the more robust and well typed atomic_t version
instead.

While at it, fetch_or() gets robustified, pasting improvements from a
previous patch by Ingo Molnar that avoids needless expression
re-evaluations in the loop.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 include/linux/atomic.h | 21 ---------------------
 kernel/sched/core.c    | 18 ++++++++++++++++++
 2 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/include/linux/atomic.h b/include/linux/atomic.h
index 3d64c08..506c353 100644
--- a/include/linux/atomic.h
+++ b/include/linux/atomic.h
@@ -579,27 +579,6 @@ static inline int atomic_fetch_or(atomic_t *p, int mask)
 }
 #endif
 
-/**
- * fetch_or - perform *ptr |= mask and return old value of *ptr
- * @ptr: pointer to value
- * @mask: mask to OR on the value
- *
- * cmpxchg based fetch_or, macro so it works for different integer types
- */
-#ifndef fetch_or
-#define fetch_or(ptr, mask)						\
-({	typeof(*(ptr)) __old, __val = *(ptr);				\
-	for (;;) {							\
-		__old = cmpxchg((ptr), __val, __val | (mask));		\
-		if (__old == __val)					\
-			break;						\
-		__val = __old;						\
-	}								\
-	__old;								\
-})
-#endif
-
-
 #ifdef CONFIG_GENERIC_ATOMIC64
 #include <asm-generic/atomic64.h>
 #endif
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 44db0ff..4cce134 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -321,6 +321,24 @@ static inline void init_hrtick(void)
 }
 #endif	/* CONFIG_SCHED_HRTICK */
 
+/*
+ * cmpxchg based fetch_or, macro so it works for different integer types
+ */
+#define fetch_or(ptr, mask)						\
+	({								\
+		typeof(ptr) _ptr = (ptr);				\
+		typeof(mask) _mask = (mask);				\
+		typeof(*_ptr) _old, _val = *_ptr;			\
+									\
+		for (;;) {						\
+			_old = cmpxchg(_ptr, _val, _val | _mask);	\
+			if (_old == _val)				\
+				break;					\
+			_val = _old;					\
+		}							\
+	_old;								\
+})
+
 #if defined(CONFIG_SMP) && defined(TIF_POLLING_NRFLAG)
 /*
  * Atomically set TIF_NEED_RESCHED and test for TIF_POLLING_NRFLAG,
-- 
2.7.0

[tip:core/urgent] locking/atomic, sched: Unexport fetch_or()

[tip-bot for Frederic Weisbecker]

Commit-ID:  5529578a27288d11d4d15635c258c6dde0f0fb10
Gitweb:     http://git.kernel.org/tip/5529578a27288d11d4d15635c258c6dde0f0fb10
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Thu, 24 Mar 2016 15:38:01 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 29 Mar 2016 11:52:11 +0200

locking/atomic, sched: Unexport fetch_or()

This patch functionally reverts:

  5fd7a09cfb8c ("atomic: Export fetch_or()")

During the merge Linus observed that the generic version of fetch_or()
was messy:

  " This makes the ugly "fetch_or()" macro that the scheduler used
    internally a new generic helper, and does a bad job at it. "

  e23604edac2a Merge branch 'timers-nohz-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip

Now that we have introduced atomic_fetch_or(), fetch_or() is only used
by the scheduler in order to deal with thread_info flags which type
can vary across architectures.

Lets confine fetch_or() back to the scheduler so that we encourage
future users to use the more robust and well typed atomic_t version
instead.

While at it, fetch_or() gets robustified, pasting improvements from a
previous patch by Ingo Molnar that avoids needless expression
re-evaluations in the loop.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/1458830281-4255-4-git-send-email-fweisbec@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/atomic.h | 21 ---------------------
 kernel/sched/core.c    | 18 ++++++++++++++++++
 2 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/include/linux/atomic.h b/include/linux/atomic.h
index 3d64c08..506c353 100644
--- a/include/linux/atomic.h
+++ b/include/linux/atomic.h
@@ -579,27 +579,6 @@ static inline int atomic_fetch_or(atomic_t *p, int mask)
 }
 #endif
 
-/**
- * fetch_or - perform *ptr |= mask and return old value of *ptr
- * @ptr: pointer to value
- * @mask: mask to OR on the value
- *
- * cmpxchg based fetch_or, macro so it works for different integer types
- */
-#ifndef fetch_or
-#define fetch_or(ptr, mask)						\
-({	typeof(*(ptr)) __old, __val = *(ptr);				\
-	for (;;) {							\
-		__old = cmpxchg((ptr), __val, __val | (mask));		\
-		if (__old == __val)					\
-			break;						\
-		__val = __old;						\
-	}								\
-	__old;								\
-})
-#endif
-
-
 #ifdef CONFIG_GENERIC_ATOMIC64
 #include <asm-generic/atomic64.h>
 #endif
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index d8465ee..8b489fc 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -321,6 +321,24 @@ static inline void init_hrtick(void)
 }
 #endif	/* CONFIG_SCHED_HRTICK */
 
+/*
+ * cmpxchg based fetch_or, macro so it works for different integer types
+ */
+#define fetch_or(ptr, mask)						\
+	({								\
+		typeof(ptr) _ptr = (ptr);				\
+		typeof(mask) _mask = (mask);				\
+		typeof(*_ptr) _old, _val = *_ptr;			\
+									\
+		for (;;) {						\
+			_old = cmpxchg(_ptr, _val, _val | _mask);	\
+			if (_old == _val)				\
+				break;					\
+			_val = _old;					\
+		}							\
+	_old;								\
+})
+
 #if defined(CONFIG_SMP) && defined(TIF_POLLING_NRFLAG)
 /*
  * Atomically set TIF_NEED_RESCHED and test for TIF_POLLING_NRFLAG,

Re: [PATCH 0/3] nohz: Convert tick dependency mask to atomic_t

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> 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(-)

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.

Thanks,

	Ingo

Re: [PATCH 0/3] nohz: Convert tick dependency mask to atomic_t

[Frederic Weisbecker]

On Fri, Mar 25, 2016 at 09:48:47AM +0100, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > 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(-)
> 
> 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:

       $ grep 'flags;' $(ls arch/*/include/asm/thread_info.h)
       arch/alpha/include/asm/thread_info.h:	unsigned int  flags;		/* low level flags */
       arch/arc/include/asm/thread_info.h:	unsigned long flags;			/* low level flags */
       arch/arm64/include/asm/thread_info.h:	unsigned long flags; 	     /* low level flags */
       arch/arm/include/asm/thread_info.h:	unsigned long flags; 	  /* low level flags */
       arch/avr32/include/asm/thread_info.h:	unsigned long flags;       /* low level flags */
       arch/blackfin/include/asm/thread_info.h:	 unsigned long flags;					  /* low level flags */
       arch/c6x/include/asm/thread_info.h:		 unsigned long flags;       /* low level flags */
       arch/cris/include/asm/thread_info.h:		 unsigned long flags; 	    /* low level flags */
       arch/frv/include/asm/thread_info.h:		 unsigned long flags; 	 /* low level flags */
       arch/h8300/include/asm/thread_info.h:		 unsigned long flags;	    /* low level flags */
       arch/hexagon/include/asm/thread_info.h:		 unsigned long flags;          /* low level flags */
       arch/ia64/include/asm/thread_info.h:		 __u32 flags;  		       /* thread_info flags (see TIF_*) */
       arch/m32r/include/asm/thread_info.h:		 unsigned long flags;      	    /* low level flags */
       arch/m68k/include/asm/thread_info.h:		 unsigned long flags;
       arch/metag/include/asm/thread_info.h:		 unsigned long flags;	    /* low level flags */
       arch/microblaze/include/asm/thread_info.h:	 unsigned long flags; /* low level flags */
       arch/mips/include/asm/thread_info.h:		 unsigned long flags; 	/* low level flags */
       arch/mn10300/include/asm/thread_info.h:		 unsigned long flags; 	     /* low level flags */
       arch/nios2/include/asm/thread_info.h:		 unsigned long flags; 	  /* low level flags */
       arch/openrisc/include/asm/thread_info.h:	 unsigned long flags;       /* low level flags */
       arch/parisc/include/asm/thread_info.h:		 unsigned long flags; 	       /* thread_info flags (see TIF_*) */
       arch/powerpc/include/asm/thread_info.h:		 unsigned long local_flags;    	  /* private flags for thread */
       arch/s390/include/asm/thread_info.h:		 unsigned long flags;     	   /* low level flags */
       arch/score/include/asm/thread_info.h:		 unsigned long flags; 	/* low level flags */
       arch/sh/include/asm/thread_info.h:		 unsigned long flags; 	     /* low level flags */
       arch/tile/include/asm/thread_info.h:		 unsigned long flags; 	  /* low level flags */
       arch/um/include/asm/thread_info.h:		 unsigned long flags;       /* low level flags */
       arch/unicore32/include/asm/thread_info.h:	 unsigned long flags; 	    /* low level flags */
       arch/x86/include/asm/thread_info.h:		 __u32	       flags; 	 /* low level flags */
       arch/xtensa/include/asm/thread_info.h:		 unsigned long flags;	      /* low level flags */

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.

Re: [PATCH 0/3] nohz: Convert tick dependency mask to atomic_t

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Fri, Mar 25, 2016 at 09:48:47AM +0100, Ingo Molnar wrote:
> > 
> > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > 
> > > 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(-)
> > 
> > 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:

Ah, yes - I did a similar analysis originally and then promptly forgot about it!

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

So I suspect your original series of introducing the atomic_t interface while 
reverting back to the scheduler-specific auto-typing hack is fine after all.

> 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.

So I don't even know where to begin with that:

 - some 64-bit architectures want 32-bit flags
 - some 64-bit architectures want 64-bit flags
 - some 64-bit architectures may genuinely want more than 32 flags
 - some 64-bit architectures may want 64-bit word just because it's the fastest

... there's not a single natural data type on the C side that I can see this could 
be converted to :-/

Thanks,

	Ingo

Re: [PATCH 0/3] nohz: Convert tick dependency mask to atomic_t

[Linus Torvalds]

On Tue, Mar 29, 2016 at 4:44 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> 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;

so you could do that fairly mindlessly. You *could* even use a nasty
macro from hell to do it automatically, with the above rules in some .

Then each architecture could clean itself up and get rid of the
padding field if they want to.

              Linus

Re: [PATCH 0/3] nohz: Convert tick dependency mask to atomic_t

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Tue, Mar 29, 2016 at 4:44 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > 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;

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
  PF_EXITING                    : 49
  PF_RANDOMIZE                  : 20
  PF_VCPU                       : 18
  PF_FREEZER_SKIP               : 15
  PF_SUPERPRIV                  : 14
  PF_FSTRANS                    : 14
  PF_NOFREEZE                   : 13
  PF_WQ_WORKER                  : 12
  PF_SWAPWRITE                  : 12
  PF_MEMALLOC_NOIO              : 11
  PF_FROZEN                     : 11
  PF_NO_SETAFFINITY             : 9
  PF_LESS_THROTTLE              : 8
  PF_USED_MATH                  : 7
  PF_SUSPEND_TASK               : 7
  PF_KSWAPD                     : 7
  PF_FORKNOEXEC                 : 7
  PF_NPROC_EXCEEDED             : 6
  PF_MCE_PROCESS                : 6
  PF_MCE_EARLY                  : 6
  PF_USED_ASYNC                 : 5
  PF_SIGNALED                   : 5
  PF_EXITPIDONE                 : 5
  PF_DUMPCORE                   : 5
  PF_MUTEX_TESTER               : 1

1)

PF_MUTEX_TESTER could be gotten rid of straight away as it appears to be unused.

2)

I'd also rename the lot while touching every usage site: the PF_ 'process flag' 
namespace currently collides with:

 - the PF_ 'page flag' namespace

 - the PF_ 'protocol family' constants in the networking code

... all of which makes grepping and code reading a bit harder than it should be, 
IMHO.

Calling them 'process' flags is a misnomer anyway, these are fundamentally per 
task flags.

All in one, having them named TF_ would work for me. TF_ is a mostly unused 
namespace in generic code right now, and it would rhyme well with the existing 
TIF_ (thread_info flag) namespace.

( I guess ATF_ for 'atomic task flag' would work as well, except that the acronym
  sounds too much like a well-known government agency. Plus I guess the ASS
  acronym principle applies as well. )

3)

We could also rename the flag itself to __flags, for the following five purposes:

 - to make sure there's no lingering unconverted usage, especially in assembly
   code that tends to drop types and go by names only.

 - and to push people towards using accessors (task_flag(), set_task_flag(), 
   etc.), not the raw field.

 - accessor conversion could precede the type conversion. I.e. the new accessors
   could work on the old type as well.

 - accessors would also make it easier to extend the type to atomic64_t in the
   future, should we ever run out of 32 task flags.

 - accessors would make it easier to do per arch conversion as well.

So this:

	if (current->flags & PF_KTHREAD)

would look like this:

	if (atomic_read(&current->__task_flags) & TF_KTHREAD)

Or rather, we'd use obviously named accessors:

	if (task_flag(current, TF_KTHREAD))

plus:

	set_task_flag(current, TF_KTHREAD);

et al.

How does this sound?

Thanks,

	Ingo

Re: [PATCH 0/3] nohz: Convert tick dependency mask to atomic_t

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Tue, Mar 29, 2016 at 4:44 AM, Ingo Molnar <mingo@kernel.org> wrote:
> > >
> > > 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;
> 
> 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

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...

Thanks,

	Ingo

Re: [PATCH 0/3] nohz: Convert tick dependency mask to atomic_t

[Frederic Weisbecker]

On Tue, Mar 29, 2016 at 03:05:14PM +0200, Ingo Molnar wrote:
> 
> * Ingo Molnar <mingo@kernel.org> wrote:
> 
> > 
> > * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > 
> > > On Tue, Mar 29, 2016 at 4:44 AM, Ingo Molnar <mingo@kernel.org> wrote:
> > > >
> > > > 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;
> > 
> > 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
> 
> 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.

Re: [PATCH 0/3] nohz: Convert tick dependency mask to atomic_t

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Tue, Mar 29, 2016 at 03:05:14PM +0200, Ingo Molnar wrote:
> > 
> > * Ingo Molnar <mingo@kernel.org> wrote:
> > 
> > > 
> > > * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > > 
> > > > On Tue, Mar 29, 2016 at 4:44 AM, Ingo Molnar <mingo@kernel.org> wrote:
> > > > >
> > > > > 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;
> > > 
> > > 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
> > 
> > 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 ;-) [*]

Thanks,

	Ingo

Re: [PATCH 0/3] nohz: Convert tick dependency mask to atomic_t

[Peter Zijlstra]

On Thu, Mar 31, 2016 at 08:54:19AM +0200, Ingo Molnar wrote:
> 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.

Another thing to look out for is that set_bit() uses LOCK BTS when the
bit is not a compile time constant, we do not have an atomic_*() version
of that. Now I'm not sure if this matters, we might never end up
triggering that code path for TIF flags.

Re: [PATCH 0/3] nohz: Convert tick dependency mask to atomic_t

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Mar 31, 2016 at 08:54:19AM +0200, Ingo Molnar wrote:
> > 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.

So my thinking was that we'd #ifdef the variants.

Thanks,

	Ingo

Re: [PATCH 0/3] nohz: Convert tick dependency mask to atomic_t

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

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

So going over to arguing about thread_info::flags:

1)

We already have ti::flags accessors for most of the generic code. There's a few 
outliers (in the scheduler code...) which can be fixed.

2)

We could introduce a ARCH_HAS_ATOMIC_TIF flag to do per arch conversion: this 
would trigger #ifdefs in the accessors. When an architecture switches ti::flags to 
atomic_t, it also sets ARCH_HAS_ATOMIC_TIF.

I'd still do a flags => __flags rename of the field, to make the conversion easy 
and safe. So any arch that has ARCH_HAS_ATOMIC_TIF set provides an atomic_t 
thread_info::__flags field.

3)

The new accessors under ARCH_HAS_ATOMIC_TIF would use atomic.h functions to 
shuffle the thread-info flags.

4)

After one kernel release we could add:

	WARN_ONCE("please convert thread_info::flags to atomic_t!", 1);

to the old accessors to accelerate conversion.

5)

Eventually, once every architecture is converted, we could eliminate the old 
sched.c fetch_or() macro.

Thanks,

	Ingo