[PATCH 0/6] nohz: Move nohz kick out of scheduler IPI, v8

[PATCH 0/6] nohz: Move nohz kick out of scheduler IPI, v8

[Frederic Weisbecker]

So this set happens to have more changes than expected because I
found out that the timer list full nohz kick also relies on the scheduler
IPI. So I had to convert it as well, hence a few more change splitups.

Changes:

* Check also raised lists from irq_work_needs_cpu() (thanks Peterz)

* Comment on SMP functions priority in
  generic_smp_call_function_single_interrupt() because IPI callbacks there
  are often synchronous as opposed to irq works. (thanks Peterz)

* s/native_send_call_func_single_ipi/native_send_call_func_single_ipi
  (thanks Eric Dumazet)

* Only build irq_work_queue_on() on CONFIG_SMP, fixes some broken builds

* Convert full nohz timer list kick

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

Thanks,
	Frederic
---

Frederic Weisbecker (6):
      irq_work: Split raised and lazy lists
      irq_work: Implement remote queueing
      nohz: Support nohz full remote kick
      nohz: Switch to nohz full remote kick on timer enqueue
      nohz: Use nohz own full kick on 2nd task enqueue
      nohz: Use IPI implicit full barrier against rq->nr_running r/w


 include/linux/irq_work.h |  5 ++++
 include/linux/tick.h     |  9 +++++-
 kernel/irq_work.c        | 76 ++++++++++++++++++++++++++++++------------------
 kernel/sched/core.c      | 22 ++++++++------
 kernel/sched/sched.h     | 12 ++++++--
 kernel/smp.c             |  9 ++++++
 kernel/time/tick-sched.c | 10 ++++---
 7 files changed, 97 insertions(+), 46 deletions(-)

[PATCH 1/6] irq_work: Split raised and lazy lists

[Frederic Weisbecker]

An irq work can be handled from two places: from the tick if the work
carries the "lazy" flag and the tick is periodic, or from a self IPI.

We merge all these works in a single list and we use some per cpu latch
to avoid raising a self-IPI when one is already pending.

Now we could do away with this ugly latch if only the list was only made of
non-lazy works. Just enqueueing a work on the empty list would be enough
to know if we need to raise an IPI or not.

Also we are going to implement remote irq work queuing. Then the per CPU
latch will need to become atomic in the global scope. That's too bad
because, here as well, just enqueueing a work on an empty list of
non-lazy works would be enough to know if we need to raise an IPI or not.

So lets take a way out of this: split the works in two distinct lists,
one for the works that can be handled by the next tick and another
one for those handled by the IPI. Just checking if the latter is empty
when we queue a new work is enough to know if we need to raise an IPI.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/irq_work.c | 53 ++++++++++++++++++++++++-----------------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/kernel/irq_work.c b/kernel/irq_work.c
index a82170e..126f254 100644
--- a/kernel/irq_work.c
+++ b/kernel/irq_work.c
@@ -19,8 +19,8 @@
 #include <asm/processor.h>
 
 
-static DEFINE_PER_CPU(struct llist_head, irq_work_list);
-static DEFINE_PER_CPU(int, irq_work_raised);
+static DEFINE_PER_CPU(struct llist_head, raised_list);
+static DEFINE_PER_CPU(struct llist_head, lazy_list);
 
 /*
  * Claim the entry so that no one else will poke at it.
@@ -70,15 +70,13 @@ bool irq_work_queue(struct irq_work *work)
 	/* Queue the entry and raise the IPI if needed. */
 	preempt_disable();
 
-	llist_add(&work->llnode, &__get_cpu_var(irq_work_list));
-
-	/*
-	 * If the work is not "lazy" or the tick is stopped, raise the irq
-	 * work interrupt (if supported by the arch), otherwise, just wait
-	 * for the next tick.
-	 */
-	if (!(work->flags & IRQ_WORK_LAZY) || tick_nohz_tick_stopped()) {
-		if (!this_cpu_cmpxchg(irq_work_raised, 0, 1))
+	/* If the work is "lazy", handle it from next tick if any */
+	if (work->flags & IRQ_WORK_LAZY) {
+		if (llist_add(&work->llnode, &__get_cpu_var(lazy_list)) &&
+		    tick_nohz_tick_stopped())
+			arch_irq_work_raise();
+	} else {
+		if (llist_add(&work->llnode, &__get_cpu_var(raised_list)))
 			arch_irq_work_raise();
 	}
 
@@ -90,10 +88,11 @@ EXPORT_SYMBOL_GPL(irq_work_queue);
 
 bool irq_work_needs_cpu(void)
 {
-	struct llist_head *this_list;
+	struct llist_head *raised, *lazy;
 
-	this_list = &__get_cpu_var(irq_work_list);
-	if (llist_empty(this_list))
+	raised = &__get_cpu_var(raised_list);
+	lazy = &__get_cpu_var(lazy_list);
+	if (llist_empty(raised) && llist_empty(lazy))
 		return false;
 
 	/* All work should have been flushed before going offline */
@@ -102,28 +101,18 @@ bool irq_work_needs_cpu(void)
 	return true;
 }
 
-static void __irq_work_run(void)
+static void irq_work_run_list(struct llist_head *list)
 {
 	unsigned long flags;
 	struct irq_work *work;
-	struct llist_head *this_list;
 	struct llist_node *llnode;
 
-
-	/*
-	 * Reset the "raised" state right before we check the list because
-	 * an NMI may enqueue after we find the list empty from the runner.
-	 */
-	__this_cpu_write(irq_work_raised, 0);
-	barrier();
-
-	this_list = &__get_cpu_var(irq_work_list);
-	if (llist_empty(this_list))
-		return;
-
 	BUG_ON(!irqs_disabled());
 
-	llnode = llist_del_all(this_list);
+	if (llist_empty(list))
+		return;
+
+	llnode = llist_del_all(list);
 	while (llnode != NULL) {
 		work = llist_entry(llnode, struct irq_work, llnode);
 
@@ -148,6 +137,12 @@ static void __irq_work_run(void)
 	}
 }
 
+static void __irq_work_run(void)
+{
+	irq_work_run_list(&__get_cpu_var(raised_list));
+	irq_work_run_list(&__get_cpu_var(lazy_list));
+}
+
 /*
  * Run the irq_work entries on this cpu. Requires to be ran from hardirq
  * context with local IRQs disabled.
-- 
1.8.3.1

[PATCH 2/6] irq_work: Implement remote queueing

[Frederic Weisbecker]

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

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Acked-by: Peter Zijlstra <peterz@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 include/linux/irq_work.h |  5 +++++
 kernel/irq_work.c        | 25 ++++++++++++++++++++++++-
 kernel/smp.c             |  9 +++++++++
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/include/linux/irq_work.h b/include/linux/irq_work.h
index 19ae05d..bf9422c 100644
--- a/include/linux/irq_work.h
+++ b/include/linux/irq_work.h
@@ -33,6 +33,11 @@ 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);
+
+#ifdef CONFIG_SMP
+bool irq_work_queue_on(struct irq_work *work, int cpu);
+#endif
+
 void irq_work_run(void);
 void irq_work_sync(struct irq_work *work);
 
diff --git a/kernel/irq_work.c b/kernel/irq_work.c
index 126f254..4b0a890 100644
--- a/kernel/irq_work.c
+++ b/kernel/irq_work.c
@@ -16,6 +16,7 @@
 #include <linux/tick.h>
 #include <linux/cpu.h>
 #include <linux/notifier.h>
+#include <linux/smp.h>
 #include <asm/processor.h>
 
 
@@ -55,12 +56,34 @@ void __weak arch_irq_work_raise(void)
 	 */
 }
 
+#ifdef CONFIG_SMP
 /*
- * Enqueue the irq_work @entry unless it's already pending
+ * Enqueue the irq_work @work on @cpu unless it's already pending
  * somewhere.
  *
  * Can be re-enqueued while the callback is still in progress.
  */
+bool irq_work_queue_on(struct irq_work *work, int cpu)
+{
+	/* All work should have been flushed before going offline */
+	WARN_ON_ONCE(cpu_is_offline(cpu));
+
+	/* Arch remote IPI send/receive backend aren't NMI safe */
+	WARN_ON_ONCE(in_nmi());
+
+	/* Only queue if not already pending */
+	if (!irq_work_claim(work))
+		return false;
+
+	if (llist_add(&work->llnode, &per_cpu(raised_list, cpu)))
+		arch_send_call_function_single_ipi(cpu);
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(irq_work_queue_on);
+#endif
+
+/* Enqueue the irq work @work on the current CPU */
 bool irq_work_queue(struct irq_work *work)
 {
 	/* Only queue if not already pending */
diff --git a/kernel/smp.c b/kernel/smp.c
index 06d574e..6e9ff62 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -3,6 +3,7 @@
  *
  * (C) Jens Axboe <jens.axboe@oracle.com> 2008
  */
+#include <linux/irq_work.h>
 #include <linux/rcupdate.h>
 #include <linux/rculist.h>
 #include <linux/kernel.h>
@@ -198,6 +199,14 @@ void generic_smp_call_function_single_interrupt(void)
 		csd->func(csd->info);
 		csd_unlock(csd);
 	}
+
+	/*
+	 * Handle irq works queued remotely by irq_work_queue_on().
+	 * Smp functions above are typically synchronous so they
+	 * better run first since some other CPUs may be busy waiting
+	 * for them.
+	 */
+	irq_work_run();
 }
 
 /*
-- 
1.8.3.1

[PATCH 3/6] nohz: Support nohz full remote kick

[Frederic Weisbecker]

Remotely kicking a full nohz CPU in order to make it re-evaluate its
next tick is currently implemented using the scheduler IPI.

However this bloats a scheduler fast path with an off-topic feature.
The scheduler tick was abused here for its cool "callable
anywhere/anytime" properties.

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

So lets implement remote kick on top of irq work. This is going to
be used when a new event requires the next tick to be recalculated:
more than 1 task competing on the CPU, timer armed, ...

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 include/linux/tick.h     |  9 ++++++++-
 kernel/time/tick-sched.c | 10 ++++++----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index b84773c..8a4987f 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -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) { }
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 6558b7a..3d63944 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -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)
-- 
1.8.3.1

[PATCH 4/6] nohz: Switch to nohz full remote kick on timer enqueue

[Frederic Weisbecker]

When a new timer is enqueued on a full dynticks target, that CPU must
re-evaluate the next tick to handle the timer correctly.

This is currently performed through the scheduler IPI. Meanwhile this
happens at the cost of off-topic workarounds in that fast path to make
it call irq_exit().

As we plan to remove this hack off the scheduler IPI, lets use
the nohz full kick instead. Pretty much any IPI fits for that job
as long at it calls irq_exit(). The nohz full kick just happens to be
handy and readily available here.

If it happens to be too much an overkill in the future, we can still
turn that timer kick into an empty IPI.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/sched/core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index d9d8ece..03dc7e9 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -620,10 +620,16 @@ static void wake_up_idle_cpu(int cpu)
 
 static bool wake_up_full_nohz_cpu(int cpu)
 {
+	/*
+	 * We just need the target to call irq_exit() and re-evaluate
+	 * the next tick. The nohz full kick at least implies that.
+	 * If needed we can still optimize that later with an
+	 * empty IRQ.
+	 */
 	if (tick_nohz_full_cpu(cpu)) {
 		if (cpu != smp_processor_id() ||
 		    tick_nohz_tick_stopped())
-			smp_send_reschedule(cpu);
+			tick_nohz_full_kick_cpu(cpu);
 		return true;
 	}
 
-- 
1.8.3.1

[PATCH 5/6] nohz: Use nohz own full kick on 2nd task enqueue

[Frederic Weisbecker]

Now that we have a nohz full remote kick based on irq work, lets use
it 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.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/sched/core.c  | 5 +----
 kernel/sched/sched.h | 2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 03dc7e9..5d25274 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1506,9 +1506,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;
 
 	/*
@@ -1525,7 +1523,6 @@ void scheduler_ipi(void)
 	 * somewhat pessimize the simple resched case.
 	 */
 	irq_enter();
-	tick_nohz_full_check();
 	sched_ttwu_pending();
 
 	/*
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 456e492..6089e00 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -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
-- 
1.8.3.1

[PATCH 6/6] nohz: Use IPI implicit full barrier against rq->nr_running r/w

[Frederic Weisbecker]

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

Acked-by: Peter Zijlstra <peterz@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/sched/core.c  |  9 +++++----
 kernel/sched/sched.h | 10 ++++++++--
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 5d25274..96e3aea 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -676,10 +676,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;
 
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 6089e00..219bfbd 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -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);
 		}
        }
-- 
1.8.3.1

Re: [PATCH 6/6] nohz: Use IPI implicit full barrier against rq->nr_running r/w

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 261 bytes --]

On Tue, Jun 10, 2014 at 05:15:09PM +0200, Frederic Weisbecker wrote:
> +       /*
> +	* More than one running task need preemption.
> +	* nr_running update is assumed to be visible
> +	* after IPI is sent from wakers.
> +	*/

That looks like whitespace damage.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/6] nohz: Move nohz kick out of scheduler IPI, v8

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 1000 bytes --]

On Tue, Jun 10, 2014 at 05:15:03PM +0200, Frederic Weisbecker wrote:
> So this set happens to have more changes than expected because I
> found out that the timer list full nohz kick also relies on the scheduler
> IPI. So I had to convert it as well, hence a few more change splitups.
> 
> Changes:
> 
> * Check also raised lists from irq_work_needs_cpu() (thanks Peterz)
> 
> * Comment on SMP functions priority in
>   generic_smp_call_function_single_interrupt() because IPI callbacks there
>   are often synchronous as opposed to irq works. (thanks Peterz)
> 
> * s/native_send_call_func_single_ipi/native_send_call_func_single_ipi
>   (thanks Eric Dumazet)
> 
> * Only build irq_work_queue_on() on CONFIG_SMP, fixes some broken builds
> 
> * Convert full nohz timer list kick
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> 	timers/nohz-irq-work-v6
> 

I think you finally nailed it ;-)

Acked-by: Peter Zijlstra <peterz@infradead.org>

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/6] nohz: Move nohz kick out of scheduler IPI, v8

[Frederic Weisbecker]

On Tue, Jun 10, 2014 at 05:48:39PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 10, 2014 at 05:15:03PM +0200, Frederic Weisbecker wrote:
> > So this set happens to have more changes than expected because I
> > found out that the timer list full nohz kick also relies on the scheduler
> > IPI. So I had to convert it as well, hence a few more change splitups.
> > 
> > Changes:
> > 
> > * Check also raised lists from irq_work_needs_cpu() (thanks Peterz)
> > 
> > * Comment on SMP functions priority in
> >   generic_smp_call_function_single_interrupt() because IPI callbacks there
> >   are often synchronous as opposed to irq works. (thanks Peterz)
> > 
> > * s/native_send_call_func_single_ipi/native_send_call_func_single_ipi
> >   (thanks Eric Dumazet)
> > 
> > * Only build irq_work_queue_on() on CONFIG_SMP, fixes some broken builds
> > 
> > * Convert full nohz timer list kick
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> > 	timers/nohz-irq-work-v6
> > 
> 
> I think you finally nailed it ;-)
> 
> Acked-by: Peter Zijlstra <peterz@infradead.org>

Yesss! :)

Re: [PATCH 6/6] nohz: Use IPI implicit full barrier against rq->nr_running r/w

[Frederic Weisbecker]

On Tue, Jun 10, 2014 at 05:48:13PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 10, 2014 at 05:15:09PM +0200, Frederic Weisbecker wrote:
> > +       /*
> > +	* More than one running task need preemption.
> > +	* nr_running update is assumed to be visible
> > +	* after IPI is sent from wakers.
> > +	*/
> 
> That looks like whitespace damage.

Ah, $EDITOR failed again while showing off. I'll fix that for the pull request.

Thanks.

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Stephen Warren]

On 06/10/2014 09:15 AM, Frederic Weisbecker wrote:
> irq 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 reevaluate 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.

I'm running next-20140624 on an ARM system, and this patch causes CPU
hot(un)plug to Oops for me; the following fires:

void irq_work_run(void)
{
	BUG_ON(!in_irq());

I found that Linus's master (8b8f5d971584 "Merge tag 'compress-3.16-rc3'
of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core")
works fine. I found that this commit inside the tip(?) tree works fine
(478850160636 "irq_work: Implement remote queueing"). However, if I
merge the two together, I hit that BUG_ON.

I think the issue is:

This commit adds a call from
generic_smp_call_function_single_interrupt() to irq_work_run().

Srivatsa's patch adds a call from hotplug_cfd() to
flush_smp_call_function_queue() to, which I imagine happens in
non-interrupt context. Note that this patch moves most of the body of
generic_smp_call_function_single_interrupt() into
flush_smp_call_function_queue().

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Stephen Warren]

On 06/24/2014 02:33 PM, Stephen Warren wrote:
> On 06/10/2014 09:15 AM, Frederic Weisbecker wrote:
>> irq 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 reevaluate 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.
> 
> I'm running next-20140624 on an ARM system, and this patch causes CPU
> hot(un)plug to Oops for me; the following fires:
> 
> void irq_work_run(void)
> {
> 	BUG_ON(!in_irq());
> 
> I found that Linus's master (8b8f5d971584 "Merge tag 'compress-3.16-rc3'
> of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core")
> works fine. I found that this commit inside the tip(?) tree works fine
> (478850160636 "irq_work: Implement remote queueing"). However, if I
> merge the two together, I hit that BUG_ON.

I forgot to mention that the conflicting commit from Linus' tree is
8d056c48e486 "CPU hotplug, smp: flush any pending IPI callbacks before
CPU offline".

> I think the issue is:
> 
> This commit adds a call from
> generic_smp_call_function_single_interrupt() to irq_work_run().
> 
> Srivatsa's patch adds a call from hotplug_cfd() to
> flush_smp_call_function_queue() to, which I imagine happens in
> non-interrupt context. Note that this patch moves most of the body of
> generic_smp_call_function_single_interrupt() into
> flush_smp_call_function_queue().

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 1695 bytes --]

On Tue, Jun 24, 2014 at 02:33:41PM -0600, Stephen Warren wrote:
> On 06/10/2014 09:15 AM, Frederic Weisbecker wrote:
> > irq 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 reevaluate 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.
> 
> I'm running next-20140624 on an ARM system, and this patch causes CPU
> hot(un)plug to Oops for me; the following fires:
> 
> void irq_work_run(void)
> {
> 	BUG_ON(!in_irq());
> 
> I found that Linus's master (8b8f5d971584 "Merge tag 'compress-3.16-rc3'
> of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core")
> works fine. I found that this commit inside the tip(?) tree works fine
> (478850160636 "irq_work: Implement remote queueing"). However, if I
> merge the two together, I hit that BUG_ON.
> 
> I think the issue is:
> 
> This commit adds a call from
> generic_smp_call_function_single_interrupt() to irq_work_run().
> 
> Srivatsa's patch adds a call from hotplug_cfd() to
> flush_smp_call_function_queue() to, which I imagine happens in
> non-interrupt context. Note that this patch moves most of the body of
> generic_smp_call_function_single_interrupt() into
> flush_smp_call_function_queue().

Right you are.. I think I'll just remove the BUG_ON(), Frederic?

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 2720 bytes --]

On Wed, Jun 25, 2014 at 07:12:34AM +0200, Peter Zijlstra wrote:
> On Tue, Jun 24, 2014 at 02:33:41PM -0600, Stephen Warren wrote:
> > On 06/10/2014 09:15 AM, Frederic Weisbecker wrote:
> > > irq 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 reevaluate 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.
> > 
> > I'm running next-20140624 on an ARM system, and this patch causes CPU
> > hot(un)plug to Oops for me; the following fires:
> > 
> > void irq_work_run(void)
> > {
> > 	BUG_ON(!in_irq());
> > 
> > I found that Linus's master (8b8f5d971584 "Merge tag 'compress-3.16-rc3'
> > of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core")
> > works fine. I found that this commit inside the tip(?) tree works fine
> > (478850160636 "irq_work: Implement remote queueing"). However, if I
> > merge the two together, I hit that BUG_ON.
> > 
> > I think the issue is:
> > 
> > This commit adds a call from
> > generic_smp_call_function_single_interrupt() to irq_work_run().
> > 
> > Srivatsa's patch adds a call from hotplug_cfd() to
> > flush_smp_call_function_queue() to, which I imagine happens in
> > non-interrupt context. Note that this patch moves most of the body of
> > generic_smp_call_function_single_interrupt() into
> > flush_smp_call_function_queue().
> 
> Right you are.. I think I'll just remove the BUG_ON(), Frederic?

Something a little so like:

---
Subject: irq_work: Remove BUG_ON in irq_work_run_list()
From: Peter Zijlstra <peterz@infradead.org>
Date: Wed Jun 25 07:13:07 CEST 2014

Because of a collision with 8d056c48e486 ("CPU hotplug, smp: flush any
pending IPI callbacks before CPU offline"), which ends up calling
hotplug_cfd()->flush_smp_call_function_queue()->run_irq_work(), which
is not from IRQ context.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Reported-by: Stephen Warren <swarren@wwwdotorg.org>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 kernel/irq_work.c |    2 --
 1 file changed, 2 deletions(-)

--- a/kernel/irq_work.c
+++ b/kernel/irq_work.c
@@ -130,8 +130,6 @@ static void irq_work_run_list(struct lli
 	struct irq_work *work;
 	struct llist_node *llnode;
 
-	BUG_ON(!irqs_disabled());
-
 	if (llist_empty(list))
 		return;
 

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Srivatsa S. Bhat]

On 06/25/2014 10:47 AM, Peter Zijlstra wrote:
> On Wed, Jun 25, 2014 at 07:12:34AM +0200, Peter Zijlstra wrote:
>> On Tue, Jun 24, 2014 at 02:33:41PM -0600, Stephen Warren wrote:
>>> On 06/10/2014 09:15 AM, Frederic Weisbecker wrote:
>>>> irq work currently only supports local callbacks. However its code
>>>> is mostly ready to run remote callbacks and we have some potential user.

[...]

>> Right you are.. I think I'll just remove the BUG_ON(), Frederic?
> 
> Something a little so like:
> 
> ---
> Subject: irq_work: Remove BUG_ON in irq_work_run_list()

I think this should be irq_work_run(), see below...

> From: Peter Zijlstra <peterz@infradead.org>
> Date: Wed Jun 25 07:13:07 CEST 2014
> 
> Because of a collision with 8d056c48e486 ("CPU hotplug, smp: flush any
> pending IPI callbacks before CPU offline"), which ends up calling
> hotplug_cfd()->flush_smp_call_function_queue()->run_irq_work(), which


s/run_irq_work/irq_work_run


> is not from IRQ context.
> 
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Reported-by: Stephen Warren <swarren@wwwdotorg.org>
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> ---
>  kernel/irq_work.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> --- a/kernel/irq_work.c
> +++ b/kernel/irq_work.c
> @@ -130,8 +130,6 @@ static void irq_work_run_list(struct lli
>  	struct irq_work *work;
>  	struct llist_node *llnode;
>  
> -	BUG_ON(!irqs_disabled());
> -

I don't think irqs_disabled() is the problematic condition, since
hotplug_cfg() invokes irq_work_run() from CPU_DYING context (which has
irqs disabled). I guess you meant to remove the in_irq() check inside
irq_work_run() instead?

Regards,
Srivatsa S. Bhat

>  	if (llist_empty(list))
>  		return;
>  
> 

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Peter Zijlstra]

On Wed, Jun 25, 2014 at 12:07:05PM +0530, Srivatsa S. Bhat wrote:
> I don't think irqs_disabled() is the problematic condition, since
> hotplug_cfg() invokes irq_work_run() from CPU_DYING context (which has
> irqs disabled). I guess you meant to remove the in_irq() check inside
> irq_work_run() instead?

Yes, clearly I should not get up at 6am.. :-) Let me go do a new one.

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Peter Zijlstra]

On Wed, Jun 25, 2014 at 11:36:57AM +0200, Peter Zijlstra wrote:
> On Wed, Jun 25, 2014 at 12:07:05PM +0530, Srivatsa S. Bhat wrote:
> > I don't think irqs_disabled() is the problematic condition, since
> > hotplug_cfg() invokes irq_work_run() from CPU_DYING context (which has
> > irqs disabled). I guess you meant to remove the in_irq() check inside
> > irq_work_run() instead?
> 
> Yes, clearly I should not get up at 6am.. :-) Let me go do a new one.

---
Subject: irq_work: Remove BUG_ON in irq_work_run()
From: Peter Zijlstra <peterz@infradead.org>
Date: Wed Jun 25 07:13:07 CEST 2014

Because of a collision with 8d056c48e486 ("CPU hotplug, smp: flush any
pending IPI callbacks before CPU offline"), which ends up calling
hotplug_cfd()->flush_smp_call_function_queue()->irq_work_run(), which
is not from IRQ context.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Reported-by: Stephen Warren <swarren@wwwdotorg.org>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/n/tip-busatzs2gvz4v62258agipuf@git.kernel.org
---
 kernel/irq_work.c |   12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

Index: linux-2.6/kernel/irq_work.c
===================================================================
--- linux-2.6.orig/kernel/irq_work.c
+++ linux-2.6/kernel/irq_work.c
@@ -160,21 +160,11 @@ static void irq_work_run_list(struct lli
 	}
 }
 
-static void __irq_work_run(void)
+static void irq_work_run(void)
 {
 	irq_work_run_list(&__get_cpu_var(raised_list));
 	irq_work_run_list(&__get_cpu_var(lazy_list));
 }
-
-/*
- * Run the irq_work entries on this cpu. Requires to be ran from hardirq
- * context with local IRQs disabled.
- */
-void irq_work_run(void)
-{
-	BUG_ON(!in_irq());
-	__irq_work_run();
-}
 EXPORT_SYMBOL_GPL(irq_work_run);
 
 /*

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Srivatsa S. Bhat]

On 06/25/2014 03:09 PM, Peter Zijlstra wrote:
> On Wed, Jun 25, 2014 at 11:36:57AM +0200, Peter Zijlstra wrote:
>> On Wed, Jun 25, 2014 at 12:07:05PM +0530, Srivatsa S. Bhat wrote:
>>> I don't think irqs_disabled() is the problematic condition, since
>>> hotplug_cfg() invokes irq_work_run() from CPU_DYING context (which has
>>> irqs disabled). I guess you meant to remove the in_irq() check inside
>>> irq_work_run() instead?
>>
>> Yes, clearly I should not get up at 6am.. :-) Let me go do a new one.
> 
> ---
> Subject: irq_work: Remove BUG_ON in irq_work_run()
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Wed Jun 25 07:13:07 CEST 2014
> 
> Because of a collision with 8d056c48e486 ("CPU hotplug, smp: flush any
> pending IPI callbacks before CPU offline"), which ends up calling
> hotplug_cfd()->flush_smp_call_function_queue()->irq_work_run(), which
> is not from IRQ context.
> 
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Reported-by: Stephen Warren <swarren@wwwdotorg.org>
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> Link: http://lkml.kernel.org/n/tip-busatzs2gvz4v62258agipuf@git.kernel.org
> ---
>  kernel/irq_work.c |   12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> Index: linux-2.6/kernel/irq_work.c
> ===================================================================
> --- linux-2.6.orig/kernel/irq_work.c
> +++ linux-2.6/kernel/irq_work.c
> @@ -160,21 +160,11 @@ static void irq_work_run_list(struct lli
>  	}
>  }
> 
> -static void __irq_work_run(void)

Hmm, irq_work_cpu_notify() calls __irq_work_run() in the CPU_DYING
phase, to by-pass BUG_ON(!in_irq()). How about doing the same thing
from hotplug_cfd() as well?

> +static void irq_work_run(void)
>  {
>  	irq_work_run_list(&__get_cpu_var(raised_list));
>  	irq_work_run_list(&__get_cpu_var(lazy_list));
>  }
> -
> -/*
> - * Run the irq_work entries on this cpu. Requires to be ran from hardirq
> - * context with local IRQs disabled.
> - */
> -void irq_work_run(void)
> -{
> -	BUG_ON(!in_irq());
> -	__irq_work_run();
> -}
>  EXPORT_SYMBOL_GPL(irq_work_run);
> 
>  /*
> 

Regards,
Srivatsa S. Bhat

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Srivatsa S. Bhat]

On 06/25/2014 03:20 PM, Srivatsa S. Bhat wrote:
> On 06/25/2014 03:09 PM, Peter Zijlstra wrote:
>> On Wed, Jun 25, 2014 at 11:36:57AM +0200, Peter Zijlstra wrote:
>>> On Wed, Jun 25, 2014 at 12:07:05PM +0530, Srivatsa S. Bhat wrote:
>>>> I don't think irqs_disabled() is the problematic condition, since
>>>> hotplug_cfg() invokes irq_work_run() from CPU_DYING context (which has
>>>> irqs disabled). I guess you meant to remove the in_irq() check inside
>>>> irq_work_run() instead?
>>>
>>> Yes, clearly I should not get up at 6am.. :-) Let me go do a new one.
>>
>> ---
>> Subject: irq_work: Remove BUG_ON in irq_work_run()
>> From: Peter Zijlstra <peterz@infradead.org>
>> Date: Wed Jun 25 07:13:07 CEST 2014
>>
>> Because of a collision with 8d056c48e486 ("CPU hotplug, smp: flush any
>> pending IPI callbacks before CPU offline"), which ends up calling
>> hotplug_cfd()->flush_smp_call_function_queue()->irq_work_run(), which
>> is not from IRQ context.
>>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Reported-by: Stephen Warren <swarren@wwwdotorg.org>
>> Signed-off-by: Peter Zijlstra <peterz@infradead.org>
>> Link: http://lkml.kernel.org/n/tip-busatzs2gvz4v62258agipuf@git.kernel.org
>> ---
>>  kernel/irq_work.c |   12 +-----------
>>  1 file changed, 1 insertion(+), 11 deletions(-)
>>
>> Index: linux-2.6/kernel/irq_work.c
>> ===================================================================
>> --- linux-2.6.orig/kernel/irq_work.c
>> +++ linux-2.6/kernel/irq_work.c
>> @@ -160,21 +160,11 @@ static void irq_work_run_list(struct lli
>>  	}
>>  }
>>
>> -static void __irq_work_run(void)
> 
> Hmm, irq_work_cpu_notify() calls __irq_work_run() in the CPU_DYING
> phase, to by-pass BUG_ON(!in_irq()). How about doing the same thing
> from hotplug_cfd() as well?
> 

Wait, that was a stupid idea. hotplug_cfd() already invokes irq_work_run
indirectly via flush_smp_call_function_queue(). So irq_work_cpu_notify()
doesn't need to invoke it again, AFAIU. So perhaps we can get rid of
irq_work_cpu_notify() altogether?

Regards,
Srivatsa S. Bhat

>> +static void irq_work_run(void)
>>  {
>>  	irq_work_run_list(&__get_cpu_var(raised_list));
>>  	irq_work_run_list(&__get_cpu_var(lazy_list));
>>  }
>> -
>> -/*
>> - * Run the irq_work entries on this cpu. Requires to be ran from hardirq
>> - * context with local IRQs disabled.
>> - */
>> -void irq_work_run(void)
>> -{
>> -	BUG_ON(!in_irq());
>> -	__irq_work_run();
>> -}
>>  EXPORT_SYMBOL_GPL(irq_work_run);
>>
>>  /*
>>
> 

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Peter Zijlstra]

On Wed, Jun 25, 2014 at 03:24:11PM +0530, Srivatsa S. Bhat wrote:
> Wait, that was a stupid idea. hotplug_cfd() already invokes irq_work_run
> indirectly via flush_smp_call_function_queue(). So irq_work_cpu_notify()
> doesn't need to invoke it again, AFAIU. So perhaps we can get rid of
> irq_work_cpu_notify() altogether?

Just so...

getting up at 6am and sitting in an airport terminal doesn't seem to
agree with me; any more silly fail here?

---
Subject: irq_work: Remove BUG_ON in irq_work_run()
From: Peter Zijlstra <peterz@infradead.org>
Date: Wed Jun 25 07:13:07 CEST 2014

Because of a collision with 8d056c48e486 ("CPU hotplug, smp: flush any
pending IPI callbacks before CPU offline"), which ends up calling
hotplug_cfd()->flush_smp_call_function_queue()->irq_work_run(), which
is not from IRQ context.

And since that already calls irq_work_run() from the hotplug path,
remove our entire hotplug handling.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Reported-by: Stephen Warren <swarren@wwwdotorg.org>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/n/tip-busatzs2gvz4v62258agipuf@git.kernel.org
---
 kernel/irq_work.c |   48 +++++-------------------------------------------
 1 file changed, 5 insertions(+), 43 deletions(-)

Index: linux-2.6/kernel/irq_work.c
===================================================================
--- linux-2.6.orig/kernel/irq_work.c
+++ linux-2.6/kernel/irq_work.c
@@ -160,20 +160,14 @@ static void irq_work_run_list(struct lli
 	}
 }
 
-static void __irq_work_run(void)
-{
-	irq_work_run_list(&__get_cpu_var(raised_list));
-	irq_work_run_list(&__get_cpu_var(lazy_list));
-}
-
 /*
- * Run the irq_work entries on this cpu. Requires to be ran from hardirq
- * context with local IRQs disabled.
+ * hotplug calls this through:
+ *  hotplug_cfs() -> flush_smp_call_function_queue()
  */
-void irq_work_run(void)
+static void irq_work_run(void)
 {
-	BUG_ON(!in_irq());
-	__irq_work_run();
+	irq_work_run_list(&__get_cpu_var(raised_list));
+	irq_work_run_list(&__get_cpu_var(lazy_list));
 }
 EXPORT_SYMBOL_GPL(irq_work_run);
 
@@ -189,35 +183,3 @@ void irq_work_sync(struct irq_work *work
 		cpu_relax();
 }
 EXPORT_SYMBOL_GPL(irq_work_sync);
-
-#ifdef CONFIG_HOTPLUG_CPU
-static int irq_work_cpu_notify(struct notifier_block *self,
-			       unsigned long action, void *hcpu)
-{
-	long cpu = (long)hcpu;
-
-	switch (action) {
-	case CPU_DYING:
-		/* Called from stop_machine */
-		if (WARN_ON_ONCE(cpu != smp_processor_id()))
-			break;
-		__irq_work_run();
-		break;
-	default:
-		break;
-	}
-	return NOTIFY_OK;
-}
-
-static struct notifier_block cpu_notify;
-
-static __init int irq_work_init_cpu_notifier(void)
-{
-	cpu_notify.notifier_call = irq_work_cpu_notify;
-	cpu_notify.priority = 0;
-	register_cpu_notifier(&cpu_notify);
-	return 0;
-}
-device_initcall(irq_work_init_cpu_notifier);
-
-#endif /* CONFIG_HOTPLUG_CPU */

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Srivatsa S. Bhat]

On 06/25/2014 03:49 PM, Peter Zijlstra wrote:
> On Wed, Jun 25, 2014 at 03:24:11PM +0530, Srivatsa S. Bhat wrote:
>> Wait, that was a stupid idea. hotplug_cfd() already invokes irq_work_run
>> indirectly via flush_smp_call_function_queue(). So irq_work_cpu_notify()
>> doesn't need to invoke it again, AFAIU. So perhaps we can get rid of
>> irq_work_cpu_notify() altogether?
> 
> Just so...
> 
> getting up at 6am and sitting in an airport terminal doesn't seem to
> agree with me;

Haha :-)

> any more silly fail here?
> 

A few minor nits below..

> ---
> Subject: irq_work: Remove BUG_ON in irq_work_run()
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Wed Jun 25 07:13:07 CEST 2014
> 
> Because of a collision with 8d056c48e486 ("CPU hotplug, smp: flush any
> pending IPI callbacks before CPU offline"), which ends up calling
> hotplug_cfd()->flush_smp_call_function_queue()->irq_work_run(), which
> is not from IRQ context.
> 
> And since that already calls irq_work_run() from the hotplug path,
> remove our entire hotplug handling.
> 
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Reported-by: Stephen Warren <swarren@wwwdotorg.org>
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> Link: http://lkml.kernel.org/n/tip-busatzs2gvz4v62258agipuf@git.kernel.org
> ---
>  kernel/irq_work.c |   48 +++++-------------------------------------------
>  1 file changed, 5 insertions(+), 43 deletions(-)
> 
> Index: linux-2.6/kernel/irq_work.c
> ===================================================================
> --- linux-2.6.orig/kernel/irq_work.c
> +++ linux-2.6/kernel/irq_work.c
> @@ -160,20 +160,14 @@ static void irq_work_run_list(struct lli
>  	}
>  }
> 
> -static void __irq_work_run(void)
> -{
> -	irq_work_run_list(&__get_cpu_var(raised_list));
> -	irq_work_run_list(&__get_cpu_var(lazy_list));
> -}
> -
>  /*
> - * Run the irq_work entries on this cpu. Requires to be ran from hardirq
> - * context with local IRQs disabled.
> + * hotplug calls this through:
> + *  hotplug_cfs() -> flush_smp_call_function_queue()

s/hotplug_cfs/hotplug_cfd

>   */
> -void irq_work_run(void)
> +static void irq_work_run(void)

s/static//

>  {
> -	BUG_ON(!in_irq());
> -	__irq_work_run();
> +	irq_work_run_list(&__get_cpu_var(raised_list));
> +	irq_work_run_list(&__get_cpu_var(lazy_list));
>  }
>  EXPORT_SYMBOL_GPL(irq_work_run);

With those 2 changes, everything looks good to me.

Regards,
Srivatsa S. Bhat

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Stephen Warren]

On 06/25/2014 04:19 AM, Peter Zijlstra wrote:
> On Wed, Jun 25, 2014 at 03:24:11PM +0530, Srivatsa S. Bhat wrote:
>> Wait, that was a stupid idea. hotplug_cfd() already invokes irq_work_run
>> indirectly via flush_smp_call_function_queue(). So irq_work_cpu_notify()
>> doesn't need to invoke it again, AFAIU. So perhaps we can get rid of
>> irq_work_cpu_notify() altogether?
> 
> Just so...
> 
> getting up at 6am and sitting in an airport terminal doesn't seem to
> agree with me; any more silly fail here?
> 
> ---
> Subject: irq_work: Remove BUG_ON in irq_work_run()
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Wed Jun 25 07:13:07 CEST 2014
> 
> Because of a collision with 8d056c48e486 ("CPU hotplug, smp: flush any
> pending IPI callbacks before CPU offline"), which ends up calling
> hotplug_cfd()->flush_smp_call_function_queue()->irq_work_run(), which
> is not from IRQ context.
> 
> And since that already calls irq_work_run() from the hotplug path,
> remove our entire hotplug handling.

Tested-by: Stephen Warren <swarren@nvidia.com>

[with the s/static// already mentioned in this thread, obviously:-)]

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Peter Zijlstra]

On Wed, Jun 25, 2014 at 10:23:21AM -0600, Stephen Warren wrote:
> On 06/25/2014 04:19 AM, Peter Zijlstra wrote:
> > On Wed, Jun 25, 2014 at 03:24:11PM +0530, Srivatsa S. Bhat wrote:
> >> Wait, that was a stupid idea. hotplug_cfd() already invokes irq_work_run
> >> indirectly via flush_smp_call_function_queue(). So irq_work_cpu_notify()
> >> doesn't need to invoke it again, AFAIU. So perhaps we can get rid of
> >> irq_work_cpu_notify() altogether?
> > 
> > Just so...
> > 
> > getting up at 6am and sitting in an airport terminal doesn't seem to
> > agree with me; any more silly fail here?
> > 
> > ---
> > Subject: irq_work: Remove BUG_ON in irq_work_run()
> > From: Peter Zijlstra <peterz@infradead.org>
> > Date: Wed Jun 25 07:13:07 CEST 2014
> > 
> > Because of a collision with 8d056c48e486 ("CPU hotplug, smp: flush any
> > pending IPI callbacks before CPU offline"), which ends up calling
> > hotplug_cfd()->flush_smp_call_function_queue()->irq_work_run(), which
> > is not from IRQ context.
> > 
> > And since that already calls irq_work_run() from the hotplug path,
> > remove our entire hotplug handling.
> 
> Tested-by: Stephen Warren <swarren@nvidia.com>
> 
> [with the s/static// already mentioned in this thread, obviously:-)]

Right; I pushed out a fixed version right before loosing my tubes at the
airport :-)

https://git.kernel.org/cgit/linux/kernel/git/peterz/queue.git/commit/?h=timers/nohz&id=921d8b81281ecdca686369f52165d04fa3505bd7

I've not gotten wu build bot spam on it so it must be good ;-)

In any case, I'll add your tested-by and update later this evening.

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Srivatsa S. Bhat]

On 06/25/2014 10:08 PM, Peter Zijlstra wrote:
> On Wed, Jun 25, 2014 at 10:23:21AM -0600, Stephen Warren wrote:
>> On 06/25/2014 04:19 AM, Peter Zijlstra wrote:
>>> On Wed, Jun 25, 2014 at 03:24:11PM +0530, Srivatsa S. Bhat wrote:
>>>> Wait, that was a stupid idea. hotplug_cfd() already invokes irq_work_run
>>>> indirectly via flush_smp_call_function_queue(). So irq_work_cpu_notify()
>>>> doesn't need to invoke it again, AFAIU. So perhaps we can get rid of
>>>> irq_work_cpu_notify() altogether?
>>>
>>> Just so...
>>>
>>> getting up at 6am and sitting in an airport terminal doesn't seem to
>>> agree with me; any more silly fail here?
>>>
>>> ---
>>> Subject: irq_work: Remove BUG_ON in irq_work_run()
>>> From: Peter Zijlstra <peterz@infradead.org>
>>> Date: Wed Jun 25 07:13:07 CEST 2014
>>>
>>> Because of a collision with 8d056c48e486 ("CPU hotplug, smp: flush any
>>> pending IPI callbacks before CPU offline"), which ends up calling
>>> hotplug_cfd()->flush_smp_call_function_queue()->irq_work_run(), which
>>> is not from IRQ context.
>>>
>>> And since that already calls irq_work_run() from the hotplug path,
>>> remove our entire hotplug handling.
>>
>> Tested-by: Stephen Warren <swarren@nvidia.com>
>>
>> [with the s/static// already mentioned in this thread, obviously:-)]
> 
> Right; I pushed out a fixed version right before loosing my tubes at the
> airport :-)
> 
> https://git.kernel.org/cgit/linux/kernel/git/peterz/queue.git/commit/?h=timers/nohz&id=921d8b81281ecdca686369f52165d04fa3505bd7
> 

This version looks good.

Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Regards,
Srivatsa S. Bhat


> I've not gotten wu build bot spam on it so it must be good ;-)
> 
> In any case, I'll add your tested-by and update later this evening.
> 

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Borislav Petkov]

On Wed, Jun 25, 2014 at 10:27:24PM +0530, Srivatsa S. Bhat wrote:
> > https://git.kernel.org/cgit/linux/kernel/git/peterz/queue.git/commit/?h=timers/nohz&id=921d8b81281ecdca686369f52165d04fa3505bd7

Just hit it here too. Cherry-picking it ontop of latest tip fixes the
issue, thanks.

Tested-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Stephen Warren]

On 06/25/2014 10:23 AM, Stephen Warren wrote:
> On 06/25/2014 04:19 AM, Peter Zijlstra wrote:
>> On Wed, Jun 25, 2014 at 03:24:11PM +0530, Srivatsa S. Bhat wrote:
>>> Wait, that was a stupid idea. hotplug_cfd() already invokes irq_work_run
>>> indirectly via flush_smp_call_function_queue(). So irq_work_cpu_notify()
>>> doesn't need to invoke it again, AFAIU. So perhaps we can get rid of
>>> irq_work_cpu_notify() altogether?
>>
>> Just so...
>>
>> getting up at 6am and sitting in an airport terminal doesn't seem to
>> agree with me; any more silly fail here?
>>
>> ---
>> Subject: irq_work: Remove BUG_ON in irq_work_run()
>> From: Peter Zijlstra <peterz@infradead.org>
>> Date: Wed Jun 25 07:13:07 CEST 2014
>>
>> Because of a collision with 8d056c48e486 ("CPU hotplug, smp: flush any
>> pending IPI callbacks before CPU offline"), which ends up calling
>> hotplug_cfd()->flush_smp_call_function_queue()->irq_work_run(), which
>> is not from IRQ context.
>>
>> And since that already calls irq_work_run() from the hotplug path,
>> remove our entire hotplug handling.
> 
> Tested-by: Stephen Warren <swarren@nvidia.com>
> 
> [with the s/static// already mentioned in this thread, obviously:-)]

next-20140701 still seems to fail CPU hotplug. I assume this patch
hasn't yet been applied for some reason?

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Frederic Weisbecker]

On Wed, Jun 25, 2014 at 06:38:20PM +0200, Peter Zijlstra wrote:
> On Wed, Jun 25, 2014 at 10:23:21AM -0600, Stephen Warren wrote:
> > On 06/25/2014 04:19 AM, Peter Zijlstra wrote:
> > > On Wed, Jun 25, 2014 at 03:24:11PM +0530, Srivatsa S. Bhat wrote:
> > >> Wait, that was a stupid idea. hotplug_cfd() already invokes irq_work_run
> > >> indirectly via flush_smp_call_function_queue(). So irq_work_cpu_notify()
> > >> doesn't need to invoke it again, AFAIU. So perhaps we can get rid of
> > >> irq_work_cpu_notify() altogether?
> > > 
> > > Just so...
> > > 
> > > getting up at 6am and sitting in an airport terminal doesn't seem to
> > > agree with me; any more silly fail here?
> > > 
> > > ---
> > > Subject: irq_work: Remove BUG_ON in irq_work_run()
> > > From: Peter Zijlstra <peterz@infradead.org>
> > > Date: Wed Jun 25 07:13:07 CEST 2014
> > > 
> > > Because of a collision with 8d056c48e486 ("CPU hotplug, smp: flush any
> > > pending IPI callbacks before CPU offline"), which ends up calling
> > > hotplug_cfd()->flush_smp_call_function_queue()->irq_work_run(), which
> > > is not from IRQ context.
> > > 
> > > And since that already calls irq_work_run() from the hotplug path,
> > > remove our entire hotplug handling.
> > 
> > Tested-by: Stephen Warren <swarren@nvidia.com>
> > 
> > [with the s/static// already mentioned in this thread, obviously:-)]
> 
> Right; I pushed out a fixed version right before loosing my tubes at the
> airport :-)
> 
> https://git.kernel.org/cgit/linux/kernel/git/peterz/queue.git/commit/?h=timers/nohz&id=921d8b81281ecdca686369f52165d04fa3505bd7
> 
> I've not gotten wu build bot spam on it so it must be good ;-)
> 
> In any case, I'll add your tested-by and update later this evening.

Ack! Thanks for fixing this.

In case you're AFK, do you need any help for pushing the patch or so?

Re: [PATCH 2/6] irq_work: Implement remote queueing

[Sachin Kamat]

On Wed, Jun 25, 2014 at 10:08 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Jun 25, 2014 at 10:23:21AM -0600, Stephen Warren wrote:
>> On 06/25/2014 04:19 AM, Peter Zijlstra wrote:
>> > On Wed, Jun 25, 2014 at 03:24:11PM +0530, Srivatsa S. Bhat wrote:
>> >> Wait, that was a stupid idea. hotplug_cfd() already invokes irq_work_run
>> >> indirectly via flush_smp_call_function_queue(). So irq_work_cpu_notify()
>> >> doesn't need to invoke it again, AFAIU. So perhaps we can get rid of
>> >> irq_work_cpu_notify() altogether?
>> >
>> > Just so...
>> >
>> > getting up at 6am and sitting in an airport terminal doesn't seem to
>> > agree with me; any more silly fail here?
>> >
>> > ---
>> > Subject: irq_work: Remove BUG_ON in irq_work_run()
>> > From: Peter Zijlstra <peterz@infradead.org>
>> > Date: Wed Jun 25 07:13:07 CEST 2014
>> >
>> > Because of a collision with 8d056c48e486 ("CPU hotplug, smp: flush any
>> > pending IPI callbacks before CPU offline"), which ends up calling
>> > hotplug_cfd()->flush_smp_call_function_queue()->irq_work_run(), which
>> > is not from IRQ context.
>> >
>> > And since that already calls irq_work_run() from the hotplug path,
>> > remove our entire hotplug handling.
>>
>> Tested-by: Stephen Warren <swarren@nvidia.com>
>>
>> [with the s/static// already mentioned in this thread, obviously:-)]
>
> Right; I pushed out a fixed version right before loosing my tubes at the
> airport :-)
>
> https://git.kernel.org/cgit/linux/kernel/git/peterz/queue.git/commit/?h=timers/nohz&id=921d8b81281ecdca686369f52165d04fa3505bd7

This patch fixes boot issue on Exynos5420/5800 based boards with bL
switcher enabled.

FWIW,
Tested-by: Sachin Kamat <sachin.kamat@samsung.com>

-- 
Regards,
Sachin.