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(-)
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(-)
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(¤t->tick_dep_mask))
return false;
- }
- if (current->signal->tick_dep_mask) {
- trace_tick_dependency(current->signal->tick_dep_mask);
+ if (check_tick_dependency(¤t->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(¤t->tick_dep_mask) ||
+ atomic_read(¤t->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
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 <[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/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
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 <[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/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
* Frederic Weisbecker <[email protected]> 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
* Frederic Weisbecker <[email protected]> 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 <[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(-)
>
> 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(¤t->tick_dep_mask))
> return false;
> - }
>
> - if (current->signal->tick_dep_mask) {
> - trace_tick_dependency(current->signal->tick_dep_mask);
> + if (check_tick_dependency(¤t->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(¤t->tick_dep_mask) ||
> + atomic_read(¤t->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
On Fri, Mar 25, 2016 at 09:48:47AM +0100, Ingo Molnar wrote:
>
> * Frederic Weisbecker <[email protected]> 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.
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?
* Frederic Weisbecker <[email protected]> wrote:
> On Fri, Mar 25, 2016 at 09:48:47AM +0100, Ingo Molnar wrote:
> >
> > * Frederic Weisbecker <[email protected]> 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
Commit-ID: 5acba71e18833b9d06686b3751598bfa263a3ac3
Gitweb: http://git.kernel.org/tip/5acba71e18833b9d06686b3751598bfa263a3ac3
Author: Frederic Weisbecker <[email protected]>
AuthorDate: Thu, 24 Mar 2016 15:37:59 +0100
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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
Commit-ID: f009a7a767e792d5ab0b46c08d46236ea5271dd9
Gitweb: http://git.kernel.org/tip/f009a7a767e792d5ab0b46c08d46236ea5271dd9
Author: Frederic Weisbecker <[email protected]>
AuthorDate: Thu, 24 Mar 2016 15:38:00 +0100
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Signed-off-by: Frederic Weisbecker <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[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(-)
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(¤t->tick_dep_mask))
return false;
- }
- if (current->signal->tick_dep_mask) {
- trace_tick_dependency(current->signal->tick_dep_mask);
+ if (check_tick_dependency(¤t->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(¤t->tick_dep_mask) ||
+ atomic_read(¤t->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);
Commit-ID: 5529578a27288d11d4d15635c258c6dde0f0fb10
Gitweb: http://git.kernel.org/tip/5529578a27288d11d4d15635c258c6dde0f0fb10
Author: Frederic Weisbecker <[email protected]>
AuthorDate: Thu, 24 Mar 2016 15:38:01 +0100
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Signed-off-by: Frederic Weisbecker <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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,
On Tue, Mar 29, 2016 at 4:44 AM, Ingo Molnar <[email protected]> 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
* Linus Torvalds <[email protected]> wrote:
> On Tue, Mar 29, 2016 at 4:44 AM, Ingo Molnar <[email protected]> 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(¤t->__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
* Ingo Molnar <[email protected]> wrote:
>
> * Linus Torvalds <[email protected]> wrote:
>
> > On Tue, Mar 29, 2016 at 4:44 AM, Ingo Molnar <[email protected]> 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
On Tue, Mar 29, 2016 at 03:05:14PM +0200, Ingo Molnar wrote:
>
> * Ingo Molnar <[email protected]> wrote:
>
> >
> > * Linus Torvalds <[email protected]> wrote:
> >
> > > On Tue, Mar 29, 2016 at 4:44 AM, Ingo Molnar <[email protected]> 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.
* Ingo Molnar <[email protected]> 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
* Frederic Weisbecker <[email protected]> wrote:
> On Tue, Mar 29, 2016 at 03:05:14PM +0200, Ingo Molnar wrote:
> >
> > * Ingo Molnar <[email protected]> wrote:
> >
> > >
> > > * Linus Torvalds <[email protected]> wrote:
> > >
> > > > On Tue, Mar 29, 2016 at 4:44 AM, Ingo Molnar <[email protected]> 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
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.
* Peter Zijlstra <[email protected]> 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