nohz: Move nohz kick out of scheduler IPI, v4
Subsystems (7)
cover nohz: Move nohz kick out of scheduler IPI, v4
So this version now implements remote irq works using the generic IPI interrupt available in most archs, as suggested by Peterz.
Keep in mind that the first patch is really just a draft to build the mockup. It needs to be turned into an internal state set on boot or so.
git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
timers/nohz-irq-work-v2
Thanks,
Frederic
---
Frederic Weisbecker (5):
irq_work: Let arch tell us if it can raise irq work
irq_work: Force non-lazy works to the IPI
irq_work: Allow remote queueing
nohz: Move full nohz kick to its own IPI
nohz: Use IPI implicit full barrier against rq->nr_running r/w
arch/alpha/kernel/time.c | 5 +++ arch/arm/kernel/smp.c | 5 +++ arch/powerpc/kernel/time.c | 5 +++ arch/sparc/kernel/pcr.c | 5 +++ arch/x86/kernel/irq_work.c | 7 ++++ include/linux/irq_work.h | 3 ++ include/linux/tick.h | 9 ++++- kernel/irq_work.c | 83 +++++++++++++++++++++++++++++++--------------- kernel/sched/core.c | 14 ++++---- kernel/sched/sched.h | 12 +++++-- kernel/smp.c | 3 ++ kernel/time/tick-sched.c | 10 +++--- kernel/timer.c | 2 +- 13 files changed, 119 insertions(+), 44 deletions(-)
PATCH 1/5 irq_work: Let arch tell us if it can raise irq work
We prepare for executing the full nohz kick through an irq work. But if we do this as is, we'll run into conflicting tick locking: the tick holds the hrtimer lock and the nohz kick may do so too.
So we need to be able to force the execution of some irq works (more precisely the non-lazy ones) to the arch irq work interrupt if any.
As a start we need to know if the arch support sending its own self-IPIs and doesn't rely on the tick to execute the works.
This solution proposes weak function. Of course it's ugly and deemed only for a draft. The best would be to call a generic irq_work_set_raisable() only once per arch.
--- arch/alpha/kernel/time.c | 5 +++++ arch/arm/kernel/smp.c | 5 +++++ arch/powerpc/kernel/time.c | 5 +++++ arch/sparc/kernel/pcr.c | 5 +++++ arch/x86/kernel/irq_work.c | 7 +++++++ kernel/irq_work.c | 5 +++++ 6 files changed, 32 insertions(+)
arch/alpha/kernel/time.c +5 -0
@@ -65,6 +65,11 @@ void arch_irq_work_raise(void) set_irq_work_pending_flag(); } +bool arch_irq_work_can_raise (void ) +{ +return true; +} +#else /* CONFIG_IRQ_WORK */ #define test_irq_work_pending () 0
arch/arm/kernel/smp.c +5 -0
@@ -459,6 +459,11 @@ void arch_irq_work_raise(void)if (is_smp ())smp_cross_call (cpumask_of (smp_processor_id ()),IPI_IRQ_WORK ); } + +bool arch_irq_work_can_raise (void ) +{ +return is_smp (); +}#endif static const char *ipi_types[NR_IPI ] = {
arch/powerpc/kernel/time.c +5 -0
@@ -472,6 +472,11 @@ void arch_irq_work_raise(void) preempt_enable(); } +bool arch_irq_work_can_raise (void ) +{ +return true; +} +#else /* CONFIG_IRQ_WORK */ #define test_irq_work_pending () 0
arch/sparc/kernel/pcr.c +5 -0
@@ -48,6 +48,11 @@ void arch_irq_work_raise(void)set_softint (1 <<PIL_DEFERRED_PCR_WORK ); } +bool arch_irq_work_can_raise (void ) +{ +return true; +} +const struct pcr_ops *pcr_ops;EXPORT_SYMBOL_GPL (pcr_ops);
arch/x86/kernel/irq_work.c +7 -0
@@ -48,3 +48,10 @@ void arch_irq_work_raise(void)apic_wait_icr_idle ();#endif } + +#ifdef CONFIG_X86_LOCAL_APIC +bool arch_irq_work_can_raise (void ) +{ +return cpu_has_apic; +} +#endif
kernel/irq_work.c +5 -0
@@ -55,6 +55,11 @@ void __weak arch_irq_work_raise(void)*/ } +bool __weak arch_irq_work_can_raise (void ) +{ +return false; +} +/* * Enqueue the irq_work @entry unless it's already pending * somewhere.
Signature · 1 line
1.8.3.1
Peter Zijlstra
maintainerRe: [PATCH 1/5] irq_work: Let arch tell us if it can raise irq work
Quoted 3 lines · Frederic Weisbecker
We prepare for executing the full nohz kick through an irq work. But if we do this as is, we'll run into conflicting tick locking: the tick holds the hrtimer lock and the nohz kick may do so too.
It does? How does the tick end up holding that lock?
Normal hrtimer callbacks run without holding the hrtimer lock -- I made it so.
This means tick_sched_timer() is called without hrtimer lock, and I don't see it taking it anywhere in tick_sched_do_timer() or tick_sched_handle().
Re: [PATCH 1/5] irq_work: Let arch tell us if it can raise irq work
Quoted 5 lines · Peter Zijlstra
Quoted 3 lines · Frederic Weisbecker
We prepare for executing the full nohz kick through an irq work. But if we do this as is, we'll run into conflicting tick locking: the tick holds the hrtimer lock and the nohz kick may do so too.
Quoted 8 lines
It does? How does the tick end up holding that lock?
Normal hrtimer callbacks run without holding the hrtimer lock -- I made it so.
This means
tick_sched_timer()is called without hrtimer lock, and I don't see it taking it anywhere intick_sched_do_timer()ortick_sched_handle().
Check hrtimer_interrupt(), it takes the per cpu base->lock.
Peter Zijlstra
maintainerRe: [PATCH 1/5] irq_work: Let arch tell us if it can raise irq work
Quoted 8 lines · Frederic Weisbecker
Quoted 7 lines · Peter Zijlstra
Quoted 3 lines · Frederic Weisbecker
We prepare for executing the full nohz kick through an irq work. But if we do this as is, we'll run into conflicting tick locking: the tick holds the hrtimer lock and the nohz kick may do so too.
It does? How does the tick end up holding that lock?
Quoted 8 lines
Quoted 6 lines
Normal hrtimer callbacks run without holding the hrtimer lock -- I made it so.
This means
tick_sched_timer()is called without hrtimer lock, and I don't see it taking it anywhere intick_sched_do_timer()ortick_sched_handle().Check
hrtimer_interrupt(), it takes the per cpubase->lock.
check __run_hrtimer() which drops base->lock over calling ->function.
Re: [PATCH 1/5] irq_work: Let arch tell us if it can raise irq work
Quoted 12 lines · Peter Zijlstra
Quoted 11 lines · Frederic Weisbecker
Quoted 10 lines · Peter Zijlstra
Quoted 3 lines · Frederic Weisbecker
We prepare for executing the full nohz kick through an irq work. But if we do this as is, we'll run into conflicting tick locking: the tick holds the hrtimer lock and the nohz kick may do so too.
It does? How does the tick end up holding that lock?
Normal hrtimer callbacks run without holding the hrtimer lock -- I made it so.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
This means
tick_sched_timer()is called without hrtimer lock, and I don't see it taking it anywhere intick_sched_do_timer()ortick_sched_handle().Check
hrtimer_interrupt(), it takes the per cpubase->lock.check
__run_hrtimer()which dropsbase->lockover calling ->function.
Oh! I had lockdep splats a few days ago. But I think I worked too many hours on it and eventually developed some brainfarted pet assumptions all along :-(
It was probably due to some other mistakes of mine. Ok, lets try again.
PATCH 2/5 irq_work: Force non-lazy works to the IPI
In reply to Frederic WeisbeckerAs we plan to handle the full nohz IPI using irq work, we need to enforce non-lazy works outside the tick because it's called under hrtimer lock. This is not desired from the nohz callback revaluating the tick because it can take hrtimer lock itself.
--- include/linux/irq_work.h | 1 + kernel/irq_work.c | 61 +++++++++++++++++++++++++++--------------------- kernel/timer.c | 2 +- 3 files changed, 36 insertions(+), 28 deletions(-)
include/linux/irq_work.h +1 -0
@@ -34,6 +34,7 @@ void init_irq_work(struct irq_work *work, void (*func)(struct irq_work *))bool irq_work_queue (struct irq_work *work);void irq_work_run (void ); +void irq_work_run_tick (void );void irq_work_sync (struct irq_work *work);#ifdef CONFIG_IRQ_WORK
kernel/irq_work.c +34 -27
123 lines not on this page · raw message
kernel/timer.c +1 -1
@@ -1384,7 +1384,7 @@ void update_process_times(int user_tick)rcu_check_callbacks (cpu, user_tick);#ifdef CONFIG_IRQ_WORK if (in_irq ()) -irq_work_run (); +irq_work_run_tick ();#endif scheduler_tick ();run_posix_cpu_timers (p);
Signature · 1 line
1.8.3.1
PATCH 3/5 irq_work: Allow remote queueing
In reply to Frederic Weisbeckerirq work currently only supports local callbacks. However its code is mostly ready to run remote callbacks and we have some potential user.
The full nohz subsystem currently open codes its own remote irq work on top of the scheduler ipi when it wants a CPU to revaluate its next tick. However this ad hoc solution bloats the scheduler IPI.
Lets just extend the irq work subsystem to support remote queuing on top of the generic SMP IPI to handle this kind of user. This shouldn't add noticeable overhead.
--- include/linux/irq_work.h | 2 ++ kernel/irq_work.c | 17 +++++++++++++++++ kernel/smp.c | 3 +++ 3 files changed, 22 insertions(+)
include/linux/irq_work.h +2 -0
@@ -33,6 +33,8 @@ void init_irq_work(struct irq_work *work, void (*func)(struct irq_work *))#define DEFINE_IRQ_WORK (name, _f) struct irq_work name = { .func = (_f), }bool irq_work_queue (struct irq_work *work); +bool irq_work_queue_on (struct irq_work *work,int cpu); +void irq_work_run (void );void irq_work_run_tick (void );void irq_work_sync (struct irq_work *work);
kernel/irq_work.c +17 -0
@@ -66,6 +66,23 @@ bool __weak arch_irq_work_can_raise(void)* * Can be re-enqueued while the callback is still in progress. */ +bool irq_work_queue_on (struct irq_work *work,int cpu) +{ +/* Only queue if not already pending */ +if (!irq_work_claim (work)) +return false; + +/* All work should have been flushed before going offline */ +WARN_ON_ONCE (cpu_is_offline (cpu)); +WARN_ON_ONCE (work->flags &IRQ_WORK_LAZY ); + +if (llist_add (&work->llnode, &per_cpu (raised_list, cpu))) +native_send_call_func_single_ipi (cpu); + +return true; +} +EXPORT_SYMBOL_GPL (irq_work_queue_on); +bool irq_work_queue (struct irq_work *work) {unsigned long flags;
kernel/smp.c +3 -0
@@ -3,6 +3,7 @@* * (C) Jens Axboe <[email protected]> 2008 */ +#include <linux/irq_work.h> #include <linux/rcupdate.h> #include <linux/rculist.h> #include <linux/kernel.h>
@@ -198,6 +199,8 @@ void generic_smp_call_function_single_interrupt(void) csd->func (csd->info);csd_unlock (csd); } + +irq_work_run (); }/*
Signature · 1 line
1.8.3.1
PATCH 4/5 nohz: Move full nohz kick to its own IPI
In reply to Frederic WeisbeckerNow that the irq work subsystem can queue remote callbacks, it's a perfect fit to safely queue IPIs when interrupts are disabled without worrying about concurrent callers.
Lets use it for the full dynticks kick to notify a CPU that it's exiting single task mode.
This unbloats a bit the scheduler IPI that the nohz code was abusing for its cool "callable anywhere/anytime" properties.
--- include/linux/tick.h | 9 ++++++++- kernel/sched/core.c | 5 +---- kernel/sched/sched.h | 2 +- kernel/time/tick-sched.c | 10 ++++++---- 4 files changed, 16 insertions(+), 10 deletions(-)
include/linux/tick.h +8 -1
@@ -181,7 +181,13 @@ static inline bool tick_nohz_full_cpu(int cpu)extern void tick_nohz_init (void );extern void __tick_nohz_full_check (void ); -extern void tick_nohz_full_kick (void ); +extern void tick_nohz_full_kick_cpu (int cpu); + +static inline void tick_nohz_full_kick (void ) +{ +tick_nohz_full_kick_cpu (smp_processor_id ()); +} +extern void tick_nohz_full_kick_all (void );extern void __tick_nohz_task_switch (struct task_struct *tsk);#else
@@ -189,6 +195,7 @@ static inline void tick_nohz_init(void) { }static inline bool tick_nohz_full_enabled (void ) {return false; }static inline bool tick_nohz_full_cpu (int cpu) {return false; }static inline void __tick_nohz_full_check (void ) { } +static inline void tick_nohz_full_kick_cpu (int cpu) { }static inline void tick_nohz_full_kick (void ) { }static inline void tick_nohz_full_kick_all (void ) { }static inline void __tick_nohz_task_switch (struct task_struct *tsk) { }
kernel/sched/core.c +1 -4
@@ -1500,9 +1500,7 @@ void scheduler_ipi(void)*/ preempt_fold_need_resched (); -if (llist_empty (&this_rq ()->wake_list) - && !tick_nohz_full_cpu (smp_processor_id ()) - && !got_nohz_idle_kick ()) +if (llist_empty (&this_rq ()->wake_list) && !got_nohz_idle_kick ())return ;/*
@@ -1519,7 +1517,6 @@ void scheduler_ipi(void)* somewhat pessimize the simple resched case. */ irq_enter (); -tick_nohz_full_check ();sched_ttwu_pending ();/*
kernel/sched/sched.h +1 -1
@@ -1225,7 +1225,7 @@ static inline void inc_nr_running(struct rq *rq)if (tick_nohz_full_cpu (rq->cpu)) {/* Order rq->nr_running write against the IPI */ smp_wmb (); -smp_send_reschedule (rq->cpu); +tick_nohz_full_kick_cpu (rq->cpu); } }#endif
kernel/time/tick-sched.c +6 -4
@@ -224,13 +224,15 @@ static DEFINE_PER_CPU(struct irq_work, nohz_full_kick_work) = { };/* -* Kick the current CPU if it's full dynticks in order to force it to +* Kick the CPU if it's full dynticks in order to force it to * re-evaluate its dependency on the tick and restart it if necessary. */ -void tick_nohz_full_kick (void ) +void tick_nohz_full_kick_cpu (int cpu) { -if (tick_nohz_full_cpu (smp_processor_id ())) -irq_work_queue (&__get_cpu_var (nohz_full_kick_work)); +if (!tick_nohz_full_cpu (cpu)) +return ; + +irq_work_queue_on (&per_cpu (nohz_full_kick_work, cpu), cpu); }static void nohz_full_kick_ipi (void *info)
Signature · 1 line
1.8.3.1
PATCH 5/5 nohz: Use IPI implicit full barrier against rq->nr_running r/w
In reply to Frederic WeisbeckerA full dynticks CPU is allowed to stop its tick when a single task runs. Meanwhile when a new task gets enqueued, the CPU must be notified so that it restart its tick to maintain local fairness and other accounting details.
This notification is performed by way of an IPI. Then when the target receives the IPI, we expect it to see the new value of rq->nr_running.
Hence the following ordering scenario:
CPU 0 CPU 1
write rq->running get IPI smp_wmb() smp_rmb() send IPI read rq->nr_running
But Paul Mckenney says that nowadays IPIs imply a full barrier on all architectures. So we can safely remove this pair and rely on the implicit barriers that come along IPI send/receive. Lets just comment on this new assumption.
--- kernel/sched/core.c | 9 +++++---- kernel/sched/sched.h | 10 ++++++++-- 2 files changed, 13 insertions(+), 6 deletions(-)
kernel/sched/core.c +5 -4
@@ -670,10 +670,11 @@ bool sched_can_stop_tick(void) rq =this_rq (); -/* Make sure rq->nr_running update is visible after the IPI */ -smp_rmb (); - -/* More than one running task need preemption */ +/* +* More than one running task need preemption. +* nr_running update is assumed to be visible +* after IPI is sent from wakers. +*/ if (rq->nr_running >1 )return false;
kernel/sched/sched.h +8 -2
@@ -1223,8 +1223,14 @@ static inline void inc_nr_running(struct rq *rq)#ifdef CONFIG_NO_HZ_FULL if (rq->nr_running ==2 ) {if (tick_nohz_full_cpu (rq->cpu)) { -/* Order rq->nr_running write against the IPI */ -smp_wmb (); +/* +* Tick is needed if more than one task runs on a CPU. +* Send the target an IPI to kick it out of nohz mode. +* +* We assume that IPI implies full memory barrier and the +* new value of rq->nr_running is visible on reception +* from the target. +*/ tick_nohz_full_kick_cpu (rq->cpu); } }
Signature · 1 line
1.8.3.1