Re: [tip:timers/nohz] nohz: Move full nohz kick to its own IPI

Re: [tip:timers/nohz] nohz: Move full nohz kick to its own IPI

[Peter Zijlstra]

On Wed, Apr 16, 2014 at 12:40:01AM -0700, tip-bot for Frederic Weisbecker wrote:
> Commit-ID:  72aacf0259bb7d53b7a3b5b2f7bf982acaa52b61
> Gitweb:     http://git.kernel.org/tip/72aacf0259bb7d53b7a3b5b2f7bf982acaa52b61
> Author:     Frederic Weisbecker <fweisbec@gmail.com>
> AuthorDate: Tue, 18 Mar 2014 21:12:53 +0100
> Committer:  Frederic Weisbecker <fweisbec@gmail.com>
> CommitDate: Thu, 3 Apr 2014 18:05:21 +0200
> 
> nohz: Move full nohz kick to its own IPI
> 
> Now that we have smp_queue_function_single() which can be used to
> safely queue IPIs when interrupts are disabled and 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.
> 
> Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jens Axboe <axboe@fb.com>
> 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>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

So I suspect this is the patch that makes Ingo's machines unhappy, they
appear to get stuck thusly:

[10513.382910] RIP: 0010:[<ffffffff8112b7da>]  [<ffffffff8112b7da>] generic_exec_single+0x9a/0x180

[10513.481704]  [<ffffffff8112c092>] smp_queue_function_single+0x42/0xa0
[10513.488251]  [<ffffffff81126ce0>] tick_nohz_full_kick_cpu+0x50/0x80
[10513.494661]  [<ffffffff810f4b0e>] enqueue_task_fair+0x59e/0x6c0
[10513.506469]  [<ffffffff810e3d6a>] enqueue_task+0x3a/0x60
[10513.511836]  [<ffffffff810e8ac3>] __migrate_task+0x123/0x150
[10513.523535]  [<ffffffff810e8b0d>] migration_cpu_stop+0x1d/0x30
[10513.529401]  [<ffffffff81143460>] cpu_stopper_thread+0x70/0x120

I'm not entirely sure how yet, but this is by far the most likely
candidate. Ingo, if you still have the vmlinuz matching this trace (your
hang2.txt) could you have a peek where that RIP lands?

If that is indeed the csd_lock() function, then this is it and
something's buggered.

> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index c9007f2..4771063 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();

FWIW that barrier is complete crap ;-)

> -			smp_send_reschedule(rq->cpu);
> +			tick_nohz_full_kick_cpu(rq->cpu);
>  		}
>         }
>  #endif
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 9f8af69..582d3f6 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -230,6 +230,27 @@ void tick_nohz_full_kick(void)
>  		irq_work_queue(&__get_cpu_var(nohz_full_kick_work));
>  }
>  
> +static void nohz_full_kick_queue(struct queue_single_data *qsd)
> +{
> +	__tick_nohz_full_check();
> +}
> +
> +static DEFINE_PER_CPU(struct queue_single_data, nohz_full_kick_qsd) = {
> +	.func = nohz_full_kick_queue,
> +};
> +
> +void tick_nohz_full_kick_cpu(int cpu)
> +{
> +	if (!tick_nohz_full_cpu(cpu))
> +		return;
> +
> +	if (cpu == smp_processor_id()) {
> +		irq_work_queue(&__get_cpu_var(nohz_full_kick_work));
> +	} else {
> +		smp_queue_function_single(cpu, &per_cpu(nohz_full_kick_qsd, cpu));
> +	}
> +}

Should we instead do irq_work_queue_on() ?

Re: [tip:timers/nohz] nohz: Move full nohz kick to its own IPI

[Peter Zijlstra]

On Mon, May 05, 2014 at 02:37:06PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 16, 2014 at 12:40:01AM -0700, tip-bot for Frederic Weisbecker wrote:
> > Commit-ID:  72aacf0259bb7d53b7a3b5b2f7bf982acaa52b61
> > Gitweb:     http://git.kernel.org/tip/72aacf0259bb7d53b7a3b5b2f7bf982acaa52b61
> > Author:     Frederic Weisbecker <fweisbec@gmail.com>
> > AuthorDate: Tue, 18 Mar 2014 21:12:53 +0100
> > Committer:  Frederic Weisbecker <fweisbec@gmail.com>
> > CommitDate: Thu, 3 Apr 2014 18:05:21 +0200
> > 
> > nohz: Move full nohz kick to its own IPI
> > 
> > Now that we have smp_queue_function_single() which can be used to
> > safely queue IPIs when interrupts are disabled and 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.
> > 
> > Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Jens Axboe <axboe@fb.com>
> > 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>
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> 
> So I suspect this is the patch that makes Ingo's machines unhappy, they
> appear to get stuck thusly:
> 
> [10513.382910] RIP: 0010:[<ffffffff8112b7da>]  [<ffffffff8112b7da>] generic_exec_single+0x9a/0x180
> 
> [10513.481704]  [<ffffffff8112c092>] smp_queue_function_single+0x42/0xa0
> [10513.488251]  [<ffffffff81126ce0>] tick_nohz_full_kick_cpu+0x50/0x80
> [10513.494661]  [<ffffffff810f4b0e>] enqueue_task_fair+0x59e/0x6c0
> [10513.506469]  [<ffffffff810e3d6a>] enqueue_task+0x3a/0x60
> [10513.511836]  [<ffffffff810e8ac3>] __migrate_task+0x123/0x150
> [10513.523535]  [<ffffffff810e8b0d>] migration_cpu_stop+0x1d/0x30
> [10513.529401]  [<ffffffff81143460>] cpu_stopper_thread+0x70/0x120
> 
> I'm not entirely sure how yet, but this is by far the most likely
> candidate. Ingo, if you still have the vmlinuz matching this trace (your
> hang2.txt) could you have a peek where that RIP lands?
> 
> If that is indeed the csd_lock() function, then this is it and
> something's buggered.

On a kernel build from your .config the +0x9a is indeed very close to
that wait loop; of course 0x9a isn't even an instruction boundary for me
so its all a bit of a guess.

Re: [tip:timers/nohz] nohz: Move full nohz kick to its own IPI

[Frederic Weisbecker]

On Mon, May 05, 2014 at 02:37:06PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 16, 2014 at 12:40:01AM -0700, tip-bot for Frederic Weisbecker wrote:
> > Commit-ID:  72aacf0259bb7d53b7a3b5b2f7bf982acaa52b61
> > Gitweb:     http://git.kernel.org/tip/72aacf0259bb7d53b7a3b5b2f7bf982acaa52b61
> > Author:     Frederic Weisbecker <fweisbec@gmail.com>
> > AuthorDate: Tue, 18 Mar 2014 21:12:53 +0100
> > Committer:  Frederic Weisbecker <fweisbec@gmail.com>
> > CommitDate: Thu, 3 Apr 2014 18:05:21 +0200
> > 
> > nohz: Move full nohz kick to its own IPI
> > 
> > Now that we have smp_queue_function_single() which can be used to
> > safely queue IPIs when interrupts are disabled and 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.
> > 
> > Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Jens Axboe <axboe@fb.com>
> > 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>
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> 
> So I suspect this is the patch that makes Ingo's machines unhappy, they
> appear to get stuck thusly:
> 
> [10513.382910] RIP: 0010:[<ffffffff8112b7da>]  [<ffffffff8112b7da>] generic_exec_single+0x9a/0x180
> 
> [10513.481704]  [<ffffffff8112c092>] smp_queue_function_single+0x42/0xa0
> [10513.488251]  [<ffffffff81126ce0>] tick_nohz_full_kick_cpu+0x50/0x80
> [10513.494661]  [<ffffffff810f4b0e>] enqueue_task_fair+0x59e/0x6c0
> [10513.506469]  [<ffffffff810e3d6a>] enqueue_task+0x3a/0x60
> [10513.511836]  [<ffffffff810e8ac3>] __migrate_task+0x123/0x150
> [10513.523535]  [<ffffffff810e8b0d>] migration_cpu_stop+0x1d/0x30
> [10513.529401]  [<ffffffff81143460>] cpu_stopper_thread+0x70/0x120
> 
> I'm not entirely sure how yet, but this is by far the most likely
> candidate. Ingo, if you still have the vmlinuz matching this trace (your
> hang2.txt) could you have a peek where that RIP lands?
> 
> If that is indeed the csd_lock() function, then this is it and
> something's buggered.

Aye!

> 
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > index c9007f2..4771063 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();
> 
> FWIW that barrier is complete crap ;-)

Yeah, I'm queing the removal of that now :)

> 
> > -			smp_send_reschedule(rq->cpu);
> > +			tick_nohz_full_kick_cpu(rq->cpu);
> >  		}
> >         }
> >  #endif
> > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> > index 9f8af69..582d3f6 100644
> > --- a/kernel/time/tick-sched.c
> > +++ b/kernel/time/tick-sched.c
> > @@ -230,6 +230,27 @@ void tick_nohz_full_kick(void)
> >  		irq_work_queue(&__get_cpu_var(nohz_full_kick_work));
> >  }
> >  
> > +static void nohz_full_kick_queue(struct queue_single_data *qsd)
> > +{
> > +	__tick_nohz_full_check();
> > +}
> > +
> > +static DEFINE_PER_CPU(struct queue_single_data, nohz_full_kick_qsd) = {
> > +	.func = nohz_full_kick_queue,
> > +};
> > +
> > +void tick_nohz_full_kick_cpu(int cpu)
> > +{
> > +	if (!tick_nohz_full_cpu(cpu))
> > +		return;
> > +
> > +	if (cpu == smp_processor_id()) {
> > +		irq_work_queue(&__get_cpu_var(nohz_full_kick_work));
> > +	} else {
> > +		smp_queue_function_single(cpu, &per_cpu(nohz_full_kick_qsd, cpu));
> > +	}
> > +}
> 
> Should we instead do irq_work_queue_on() ?

I would really much prefer that yeah. But if we do that, expect some added overhead on the local
irq_work_queue() path though. irq_work_raise can't use local cmpxchg ops anymore.

Or we can have a different pending raise system for remote irq work.

I can try something.

Re: [tip:timers/nohz] nohz: Move full nohz kick to its own IPI

[Peter Zijlstra]

On Mon, May 05, 2014 at 04:52:59PM +0200, Frederic Weisbecker wrote:
> > Should we instead do irq_work_queue_on() ?
> 
> I would really much prefer that yeah. But if we do that, expect some added overhead on the local
> irq_work_queue() path though. irq_work_raise can't use local cmpxchg ops anymore.
> 
> Or we can have a different pending raise system for remote irq work.
> 
> I can try something.

Loosing that local cmpxchg shouldn't be a problem, I don't  thnk this is
a particularly hot path.

Re: [tip:timers/nohz] nohz: Move full nohz kick to its own IPI

[Frederic Weisbecker]

On Mon, May 05, 2014 at 03:31:13PM +0200, Peter Zijlstra wrote:
> On Mon, May 05, 2014 at 02:37:06PM +0200, Peter Zijlstra wrote:
> > On Wed, Apr 16, 2014 at 12:40:01AM -0700, tip-bot for Frederic Weisbecker wrote:
> > > Commit-ID:  72aacf0259bb7d53b7a3b5b2f7bf982acaa52b61
> > > Gitweb:     http://git.kernel.org/tip/72aacf0259bb7d53b7a3b5b2f7bf982acaa52b61
> > > Author:     Frederic Weisbecker <fweisbec@gmail.com>
> > > AuthorDate: Tue, 18 Mar 2014 21:12:53 +0100
> > > Committer:  Frederic Weisbecker <fweisbec@gmail.com>
> > > CommitDate: Thu, 3 Apr 2014 18:05:21 +0200
> > > 
> > > nohz: Move full nohz kick to its own IPI
> > > 
> > > Now that we have smp_queue_function_single() which can be used to
> > > safely queue IPIs when interrupts are disabled and 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.
> > > 
> > > Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Ingo Molnar <mingo@kernel.org>
> > > Cc: Jens Axboe <axboe@fb.com>
> > > 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>
> > > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > 
> > So I suspect this is the patch that makes Ingo's machines unhappy, they
> > appear to get stuck thusly:
> > 
> > [10513.382910] RIP: 0010:[<ffffffff8112b7da>]  [<ffffffff8112b7da>] generic_exec_single+0x9a/0x180
> > 
> > [10513.481704]  [<ffffffff8112c092>] smp_queue_function_single+0x42/0xa0
> > [10513.488251]  [<ffffffff81126ce0>] tick_nohz_full_kick_cpu+0x50/0x80
> > [10513.494661]  [<ffffffff810f4b0e>] enqueue_task_fair+0x59e/0x6c0
> > [10513.506469]  [<ffffffff810e3d6a>] enqueue_task+0x3a/0x60
> > [10513.511836]  [<ffffffff810e8ac3>] __migrate_task+0x123/0x150
> > [10513.523535]  [<ffffffff810e8b0d>] migration_cpu_stop+0x1d/0x30
> > [10513.529401]  [<ffffffff81143460>] cpu_stopper_thread+0x70/0x120
> > 
> > I'm not entirely sure how yet, but this is by far the most likely
> > candidate. Ingo, if you still have the vmlinuz matching this trace (your
> > hang2.txt) could you have a peek where that RIP lands?
> > 
> > If that is indeed the csd_lock() function, then this is it and
> > something's buggered.
> 
> On a kernel build from your .config the +0x9a is indeed very close to
> that wait loop; of course 0x9a isn't even an instruction boundary for me
> so its all a bit of a guess.

Note the current ordering:

    cmpxchg(&qsd->pending, 0, 1)       get ipi
    csd_lock(qsd->csd)                 xchg(&qsd->pending, 1)
    send ipi                           csd_unlock(qsd->csd)


So there shouldn't be racing updaters. Also ipi sender shouldn't
race with ipi receiver, the update shouldn't always eventually see
the unlock happening.

OTOH the ordering above is anything but intuitive. In fact csd_lock()
is on the way here. smp_queue_function_single() doesn't need it at
all. So it's just yet another risk for a deadlock.

One more reason why I would much prefer irq_work_queue_on(). But
I'm also not very happy with the possible result since we are going
to maintain two different APIs set doing almost the same thing with
just slightly different constraints or semantics :-(

Re: [tip:timers/nohz] nohz: Move full nohz kick to its own IPI

[Frederic Weisbecker]

On Mon, May 05, 2014 at 04:58:15PM +0200, Peter Zijlstra wrote:
> On Mon, May 05, 2014 at 04:52:59PM +0200, Frederic Weisbecker wrote:
> > > Should we instead do irq_work_queue_on() ?
> > 
> > I would really much prefer that yeah. But if we do that, expect some added overhead on the local
> > irq_work_queue() path though. irq_work_raise can't use local cmpxchg ops anymore.
> > 
> > Or we can have a different pending raise system for remote irq work.
> > 
> > I can try something.
> 
> Loosing that local cmpxchg shouldn't be a problem, I don't  thnk this is
> a particularly hot path.

Then the conversion is easy since most of the irq work code should
already work for remote queuing.

I'll come up with a patch soon.

Thanks.

Re: [tip:timers/nohz] nohz: Move full nohz kick to its own IPI

[Peter Zijlstra]

> Note the current ordering:
> 
>     cmpxchg(&qsd->pending, 0, 1)       get ipi
>     csd_lock(qsd->csd)                 xchg(&qsd->pending, 1)
>     send ipi                           csd_unlock(qsd->csd)
> 
> 
> So there shouldn't be racing updaters. Also ipi sender shouldn't
> race with ipi receiver, the update shouldn't always eventually see
> the unlock happening.

Yeah, I've not spotted how this particular train wreck happens either.

The problem is reproduction, it took me 9 hours to confirm I could
reproduce the problem on my machine. So how long to I run it with this
patch reverted to show its gone..

Re: [tip:timers/nohz] nohz: Move full nohz kick to its own IPI

[Frederic Weisbecker]

On Mon, May 05, 2014 at 05:12:28PM +0200, Peter Zijlstra wrote:
> > Note the current ordering:
> > 
> >     cmpxchg(&qsd->pending, 0, 1)       get ipi
> >     csd_lock(qsd->csd)                 xchg(&qsd->pending, 1)
> >     send ipi                           csd_unlock(qsd->csd)
> > 
> > 
> > So there shouldn't be racing updaters. Also ipi sender shouldn't
> > race with ipi receiver, the update shouldn't always eventually see
> > the unlock happening.
> 
> Yeah, I've not spotted how this particular train wreck happens either.
> 
> The problem is reproduction, it took me 9 hours to confirm I could
> reproduce the problem on my machine. So how long to I run it with this
> patch reverted to show its gone..

Maybe it could be favoured cpu hotplug. Anyway converting to irq_work should
fix it.

Re: [tip:timers/nohz] nohz: Move full nohz kick to its own IPI

[Peter Zijlstra]

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

On Mon, May 05, 2014 at 05:34:08PM +0200, Frederic Weisbecker wrote:
> On Mon, May 05, 2014 at 05:12:28PM +0200, Peter Zijlstra wrote:
> > > Note the current ordering:
> > > 
> > >     cmpxchg(&qsd->pending, 0, 1)       get ipi
> > >     csd_lock(qsd->csd)                 xchg(&qsd->pending, 1)
> > >     send ipi                           csd_unlock(qsd->csd)
> > > 
> > > 
> > > So there shouldn't be racing updaters. Also ipi sender shouldn't
> > > race with ipi receiver, the update shouldn't always eventually see
> > > the unlock happening.
> > 
> > Yeah, I've not spotted how this particular train wreck happens either.
> > 
> > The problem is reproduction, it took me 9 hours to confirm I could
> > reproduce the problem on my machine. So how long to I run it with this
> > patch reverted to show its gone..
> 
> Maybe it could be favoured cpu hotplug. Anyway converting to irq_work should
> fix it.

Ingo needs a commit msg for the revert of this patch; do you think you
have time to look into _why_ this patch is broken and write such a
thing?

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

Re: [tip:timers/nohz] nohz: Move full nohz kick to its own IPI

[Frederic Weisbecker]

On Wed, May 07, 2014 at 05:17:35PM +0200, Peter Zijlstra wrote:
> On Mon, May 05, 2014 at 05:34:08PM +0200, Frederic Weisbecker wrote:
> > On Mon, May 05, 2014 at 05:12:28PM +0200, Peter Zijlstra wrote:
> > > > Note the current ordering:
> > > > 
> > > >     cmpxchg(&qsd->pending, 0, 1)       get ipi
> > > >     csd_lock(qsd->csd)                 xchg(&qsd->pending, 1)
> > > >     send ipi                           csd_unlock(qsd->csd)
> > > > 
> > > > 
> > > > So there shouldn't be racing updaters. Also ipi sender shouldn't
> > > > race with ipi receiver, the update shouldn't always eventually see
> > > > the unlock happening.
> > > 
> > > Yeah, I've not spotted how this particular train wreck happens either.
> > > 
> > > The problem is reproduction, it took me 9 hours to confirm I could
> > > reproduce the problem on my machine. So how long to I run it with this
> > > patch reverted to show its gone..
> > 
> > Maybe it could be favoured cpu hotplug. Anyway converting to irq_work should
> > fix it.
> 
> Ingo needs a commit msg for the revert of this patch; do you think you
> have time to look into _why_ this patch is broken and write such a
> thing?

I can try but I need to reproduce it. Do you have any clue on how to do so?
Also which HEAD were you guys using?

Thanks.

Re: [tip:timers/nohz] nohz: Move full nohz kick to its own IPI

[Peter Zijlstra]

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

On Wed, May 07, 2014 at 05:29:24PM +0200, Frederic Weisbecker wrote:
> On Wed, May 07, 2014 at 05:17:35PM +0200, Peter Zijlstra wrote:
> > On Mon, May 05, 2014 at 05:34:08PM +0200, Frederic Weisbecker wrote:
> > > On Mon, May 05, 2014 at 05:12:28PM +0200, Peter Zijlstra wrote:
> > > > > Note the current ordering:
> > > > > 
> > > > >     cmpxchg(&qsd->pending, 0, 1)       get ipi
> > > > >     csd_lock(qsd->csd)                 xchg(&qsd->pending, 1)
> > > > >     send ipi                           csd_unlock(qsd->csd)
> > > > > 
> > > > > 
> > > > > So there shouldn't be racing updaters. Also ipi sender shouldn't
> > > > > race with ipi receiver, the update shouldn't always eventually see
> > > > > the unlock happening.
> > > > 
> > > > Yeah, I've not spotted how this particular train wreck happens either.
> > > > 
> > > > The problem is reproduction, it took me 9 hours to confirm I could
> > > > reproduce the problem on my machine. So how long to I run it with this
> > > > patch reverted to show its gone..
> > > 
> > > Maybe it could be favoured cpu hotplug. Anyway converting to irq_work should
> > > fix it.
> > 
> > Ingo needs a commit msg for the revert of this patch; do you think you
> > have time to look into _why_ this patch is broken and write such a
> > thing?
> 
> I can try but I need to reproduce it. Do you have any clue on how to do so?
> Also which HEAD were you guys using?

Ha!, so I was running a tip/master with that commit in -- a few days
ago, v3.15-rc4-1644-g5c658b0cdf22 might've been it.

Then I ran it on my dual socket AMD interlagos, with:

while :; make O=allyesconfig-build/ clean; make O=allyesconfig-build/
-j96 -s; done

for 9 hours, and then got empty RCU stall warns and a bricked machine.

I might still have the .config, but I don't think there was anything
particularly odd about the config other than having NOHZ_FULL enabled.

The only way I found this patch was by staring at some RCU stall warns
Ingo managed to get, sometimes they actually got backtraces in them
apparently.

According to Ingo the bigger the machine the faster it reproduces, but
reproduction times, even for these 32 cpu machines, are in the many
hours range.

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

Re: [tip:timers/nohz] nohz: Move full nohz kick to its own IPI

[Frederic Weisbecker]

On Wed, May 07, 2014 at 05:37:36PM +0200, Peter Zijlstra wrote:
> On Wed, May 07, 2014 at 05:29:24PM +0200, Frederic Weisbecker wrote:
> > On Wed, May 07, 2014 at 05:17:35PM +0200, Peter Zijlstra wrote:
> > > On Mon, May 05, 2014 at 05:34:08PM +0200, Frederic Weisbecker wrote:
> > > > On Mon, May 05, 2014 at 05:12:28PM +0200, Peter Zijlstra wrote:
> > > > > > Note the current ordering:
> > > > > > 
> > > > > >     cmpxchg(&qsd->pending, 0, 1)       get ipi
> > > > > >     csd_lock(qsd->csd)                 xchg(&qsd->pending, 1)
> > > > > >     send ipi                           csd_unlock(qsd->csd)
> > > > > > 
> > > > > > 
> > > > > > So there shouldn't be racing updaters. Also ipi sender shouldn't
> > > > > > race with ipi receiver, the update shouldn't always eventually see
> > > > > > the unlock happening.
> > > > > 
> > > > > Yeah, I've not spotted how this particular train wreck happens either.
> > > > > 
> > > > > The problem is reproduction, it took me 9 hours to confirm I could
> > > > > reproduce the problem on my machine. So how long to I run it with this
> > > > > patch reverted to show its gone..
> > > > 
> > > > Maybe it could be favoured cpu hotplug. Anyway converting to irq_work should
> > > > fix it.
> > > 
> > > Ingo needs a commit msg for the revert of this patch; do you think you
> > > have time to look into _why_ this patch is broken and write such a
> > > thing?
> > 
> > I can try but I need to reproduce it. Do you have any clue on how to do so?
> > Also which HEAD were you guys using?
> 
> Ha!, so I was running a tip/master with that commit in -- a few days
> ago, v3.15-rc4-1644-g5c658b0cdf22 might've been it.
> 
> Then I ran it on my dual socket AMD interlagos, with:
> 
> while :; make O=allyesconfig-build/ clean; make O=allyesconfig-build/
> -j96 -s; done
> 
> for 9 hours, and then got empty RCU stall warns and a bricked machine.
> 
> I might still have the .config, but I don't think there was anything
> particularly odd about the config other than having NOHZ_FULL enabled.
> 
> The only way I found this patch was by staring at some RCU stall warns
> Ingo managed to get, sometimes they actually got backtraces in them
> apparently.
> 
> According to Ingo the bigger the machine the faster it reproduces, but
> reproduction times, even for these 32 cpu machines, are in the many
> hours range.

Ok then, I'll try something.

But note that those commits aren't upstream yet and they are in a seperate
branch tip:timers/nohz with no other non-upstream commits.

And I work alone on this branch.

So we can as well zap these commits and replace them with the irq_work_on()
conversion (still preparing that).

Re: [tip:timers/nohz] nohz: Move full nohz kick to its own IPI

[Peter Zijlstra]

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

On Wed, May 07, 2014 at 06:05:08PM +0200, Frederic Weisbecker wrote:
> So we can as well zap these commits and replace them with the irq_work_on()
> conversion (still preparing that).

I hear you, but I think Ingo doesn't want to rebase the tree because its
public, but maybe he can make an exception..

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

Re: [tip:timers/nohz] nohz: Move full nohz kick to its own IPI

[Ingo Molnar]

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

> On Wed, May 07, 2014 at 06:05:08PM +0200, Frederic Weisbecker wrote:
> > So we can as well zap these commits and replace them with the irq_work_on()
> > conversion (still preparing that).
> 
> I hear you, but I think Ingo doesn't want to rebase the tree because its
> public, but maybe he can make an exception..

Since tip:timers/nohz only contains these commits:

 fc1781cc66b1 Merge branch 'timers/nohz-ipi-for-tip-v3' of git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks into timers/nohz
 72aacf0259bb nohz: Move full nohz kick to its own IPI
 55d77c215c74 smp: Non busy-waiting IPI queue

we can zap them all, or even keep 55d77c215c74.

Frederic, which one would you prefer?

Thanks,

	Ingo

Re: [tip:timers/nohz] nohz: Move full nohz kick to its own IPI

[Frederic Weisbecker]

On Wed, May 07, 2014 at 09:07:14PM +0200, Ingo Molnar wrote:
> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Wed, May 07, 2014 at 06:05:08PM +0200, Frederic Weisbecker wrote:
> > > So we can as well zap these commits and replace them with the irq_work_on()
> > > conversion (still preparing that).
> > 
> > I hear you, but I think Ingo doesn't want to rebase the tree because its
> > public, but maybe he can make an exception..
> 
> Since tip:timers/nohz only contains these commits:
> 
>  fc1781cc66b1 Merge branch 'timers/nohz-ipi-for-tip-v3' of git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks into timers/nohz
>  72aacf0259bb nohz: Move full nohz kick to its own IPI
>  55d77c215c74 smp: Non busy-waiting IPI queue
> 
> we can zap them all, or even keep 55d77c215c74.
> 
> Frederic, which one would you prefer?

I think we can zap them all. But since I'm not yet sure how the new solution, based on irq work,
is going to be received, do you mind if we just wait for the new solution to be acked by Peterz?

If that's ok for you, I'll then do a pull request that includes the solution and zaps the old two
commits.

Thanks!

Re: [tip:timers/nohz] nohz: Move full nohz kick to its own IPI

[Ingo Molnar]

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

> On Wed, May 07, 2014 at 09:07:14PM +0200, Ingo Molnar wrote:
> > 
> > * Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > On Wed, May 07, 2014 at 06:05:08PM +0200, Frederic Weisbecker wrote:
> > > > So we can as well zap these commits and replace them with the irq_work_on()
> > > > conversion (still preparing that).
> > > 
> > > I hear you, but I think Ingo doesn't want to rebase the tree because its
> > > public, but maybe he can make an exception..
> > 
> > Since tip:timers/nohz only contains these commits:
> > 
> >  fc1781cc66b1 Merge branch 'timers/nohz-ipi-for-tip-v3' of git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks into timers/nohz
> >  72aacf0259bb nohz: Move full nohz kick to its own IPI
> >  55d77c215c74 smp: Non busy-waiting IPI queue
> > 
> > we can zap them all, or even keep 55d77c215c74.
> > 
> > Frederic, which one would you prefer?
> 
> I think we can zap them all. But since I'm not yet sure how the new 
> solution, based on irq work, is going to be received, do you mind if 
> we just wait for the new solution to be acked by Peterz?
> 
> If that's ok for you, I'll then do a pull request that includes the 
> solution and zaps the old two commits.

It's all isolated to timers/nohz, not mixed up with anything else, so 
zapping it is easy (I already zapped the second commit) and you can 
simply use latest -Linus to base your new solution on.

Thanks,

	Ingo