| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20160325085202.GB15235@gmail.com>
Date: Fri, 25 Mar 2016 09:52:02 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Frederic Weisbecker <fweisbec@...il.com>
Cc: LKML <linux-kernel@...r.kernel.org>,
Peter Zijlstra <peterz@...radead.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Thomas Gleixner <tglx@...utronix.de>,
Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 2/3] nohz: Convert tick dependency mask to atomic_t
* Frederic Weisbecker <fweisbec@...il.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@...ux-foundation.org>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Signed-off-by: Frederic Weisbecker <fweisbec@...il.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(¤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
Powered by blists - more mailing lists