Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1575025 > unrolled thread
| Started by | Gabriel C <nix.or.die@gmail.com> |
|---|---|
| First post | 2017-02-06 18:40 +0100 |
| Last post | 2017-02-07 23:10 +0100 |
| Articles | 4 on this page of 24 — 8 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Gabriel C <nix.or.die@gmail.com> - 2017-02-06 18:40 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Greg KH <gregkh@linuxfoundation.org> - 2017-02-06 18:50 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) "Ruslan Ruslichenko -X (rruslich - GLOBALLOGIC INC at Cisco)" <rruslich@cisco.com> - 2017-02-06 20:10 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Gabriel C <nix.or.die@gmail.com> - 2017-02-06 21:40 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Linus Torvalds <torvalds@linux-foundation.org> - 2017-02-07 00:10 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Thomas Gleixner <tglx@linutronix.de> - 2017-02-07 22:10 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Thomas Gleixner <tglx@linutronix.de> - 2017-02-07 22:30 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Gabriel C <nix.or.die@gmail.com> - 2017-02-11 00:20 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Gabriel C <nix.or.die@gmail.com> - 2017-02-11 02:50 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Thomas Gleixner <tglx@linutronix.de> - 2017-02-11 10:10 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Gabriel C <nix.or.die@gmail.com> - 2017-02-11 14:10 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Borislav Petkov <bp@alien8.de> - 2017-02-11 15:30 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Gabriel C <nix.or.die@gmail.com> - 2017-02-11 22:00 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Borislav Petkov <bp@alien8.de> - 2017-02-11 22:40 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Gabriel C <nix.or.die@gmail.com> - 2017-02-12 21:30 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Borislav Petkov <bp@alien8.de> - 2017-02-12 22:20 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Gabriel C <nix.or.die@gmail.com> - 2017-02-12 23:30 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Borislav Petkov <bp@alien8.de> - 2017-02-13 01:40 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Gabriel C <nix.or.die@gmail.com> - 2017-02-13 02:30 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Mike Galbraith <efault@gmx.de> - 2017-02-13 03:40 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Thomas Gleixner <tglx@linutronix.de> - 2017-02-13 09:50 +0100
[tip:timers/urgent] tick/broadcast: Prevent deadlock on tick_broadcast_lock tip-bot for Mike Galbraith <tipbot@zytor.com> - 2017-02-13 10:00 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Borislav Petkov <bp@alien8.de> - 2017-02-13 11:30 +0100
Re: Linux 4.9.6 ( Restore IO-APIC irq_chip retrigger callback , breaks my box ) Thomas Gleixner <tglx@linutronix.de> - 2017-02-07 23:10 +0100
Page 2 of 2 — ← Prev page 1 [2]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-02-13 09:50 +0100 |
| Message-ID | <takuC-1iW-13@gated-at.bofh.it> |
| In reply to | #1579362 |
On Mon, 13 Feb 2017, Mike Galbraith wrote:
> kernel/time/tick-broadcast.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> --- a/kernel/time/tick-broadcast.c
> +++ b/kernel/time/tick-broadcast.c
> @@ -357,6 +357,7 @@ void tick_broadcast_control(enum tick_br
> struct clock_event_device *bc, *dev;
> struct tick_device *td;
> int cpu, bc_stopped;
> + unsigned long flags;
>
> td = this_cpu_ptr(&tick_cpu_device);
> dev = td->evtdev;
> @@ -370,7 +371,7 @@ void tick_broadcast_control(enum tick_br
> if (!tick_device_is_functional(dev))
> return;
>
> - raw_spin_lock(&tick_broadcast_lock);
> + raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
> cpu = smp_processor_id();
> bc = tick_broadcast_device.evtdev;
> bc_stopped = cpumask_empty(tick_broadcast_mask);
> @@ -420,7 +421,7 @@ void tick_broadcast_control(enum tick_br
> tick_broadcast_setup_oneshot(bc);
> }
> }
> - raw_spin_unlock(&tick_broadcast_lock);
> + raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
That cures the lockdep splat, but the comment above
tick_broadcast_control() says:
* Called with interrupts disabled, so clockevents_lock is not
* required here because the local clock event device cannot go away
* under us.
So if we want to relax the calling convention, then we need to take the
lock early. Otherwise it's unsafe to fiddle with the local clock event
device.
The calling convention was broken with the following commit:
29d7bbada98e intel_idle: Remove superfluous SMP fuction call
So we could fix it at the call site, but making the core more robust is the
better solution.
I'll fix it up.
Thanks,
tglx
[toc] | [prev] | [next] | [standalone]
| From | tip-bot for Mike Galbraith <tipbot@zytor.com> |
|---|---|
| Date | 2017-02-13 10:00 +0100 |
| Subject | [tip:timers/urgent] tick/broadcast: Prevent deadlock on tick_broadcast_lock |
| Message-ID | <takEi-1mF-9@gated-at.bofh.it> |
| In reply to | #1579362 |
Commit-ID: 202461e2f3c15dbfb05825d29ace0d20cdf55fa4
Gitweb: http://git.kernel.org/tip/202461e2f3c15dbfb05825d29ace0d20cdf55fa4
Author: Mike Galbraith <efault@gmx.de>
AuthorDate: Mon, 13 Feb 2017 03:31:55 +0100
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 13 Feb 2017 09:49:31 +0100
tick/broadcast: Prevent deadlock on tick_broadcast_lock
tick_broadcast_lock is taken from interrupt context, but the following call
chain takes the lock without disabling interrupts:
[ 12.703736] _raw_spin_lock+0x3b/0x50
[ 12.703738] tick_broadcast_control+0x5a/0x1a0
[ 12.703742] intel_idle_cpu_online+0x22/0x100
[ 12.703744] cpuhp_invoke_callback+0x245/0x9d0
[ 12.703752] cpuhp_thread_fun+0x52/0x110
[ 12.703754] smpboot_thread_fn+0x276/0x320
So the following deadlock can happen:
lock(tick_broadcast_lock);
<Interrupt>
lock(tick_broadcast_lock);
intel_idle_cpu_online() is the only place which violates the calling
convention of tick_broadcast_control(). This was caused by the removal of
the smp function call in course of the cpu hotplug rework.
Instead of slapping local_irq_disable/enable() at the call site, we can
relax the calling convention and handle it in the core code, which makes
the whole machinery more robust.
Fixes: 29d7bbada98e ("intel_idle: Remove superfluous SMP fuction call")
Reported-by: Gabriel C <nix.or.die@gmail.com>
Signed-off-by: Mike Galbraith <efault@gmx.de>
Cc: Ruslan Ruslichenko <rruslich@cisco.com>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: lwn@lwn.net
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Anna-Maria Gleixner <anna-maria@linutronix.de>
Cc: Sebastian Siewior <bigeasy@linutronix.de>
Cc: stable <stable@vger.kernel.org>
Link: http://lkml.kernel.org/r/1486953115.5912.4.camel@gmx.de
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
kernel/time/tick-broadcast.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 3109204..17ac99b 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -347,17 +347,16 @@ static void tick_handle_periodic_broadcast(struct clock_event_device *dev)
*
* Called when the system enters a state where affected tick devices
* might stop. Note: TICK_BROADCAST_FORCE cannot be undone.
- *
- * Called with interrupts disabled, so clockevents_lock is not
- * required here because the local clock event device cannot go away
- * under us.
*/
void tick_broadcast_control(enum tick_broadcast_mode mode)
{
struct clock_event_device *bc, *dev;
struct tick_device *td;
int cpu, bc_stopped;
+ unsigned long flags;
+ /* Protects also the local clockevent device. */
+ raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
td = this_cpu_ptr(&tick_cpu_device);
dev = td->evtdev;
@@ -365,12 +364,11 @@ void tick_broadcast_control(enum tick_broadcast_mode mode)
* Is the device not affected by the powerstate ?
*/
if (!dev || !(dev->features & CLOCK_EVT_FEAT_C3STOP))
- return;
+ goto out;
if (!tick_device_is_functional(dev))
- return;
+ goto out;
- raw_spin_lock(&tick_broadcast_lock);
cpu = smp_processor_id();
bc = tick_broadcast_device.evtdev;
bc_stopped = cpumask_empty(tick_broadcast_mask);
@@ -420,7 +418,8 @@ void tick_broadcast_control(enum tick_broadcast_mode mode)
tick_broadcast_setup_oneshot(bc);
}
}
- raw_spin_unlock(&tick_broadcast_lock);
+out:
+ raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
}
EXPORT_SYMBOL_GPL(tick_broadcast_control);
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@alien8.de> |
|---|---|
| Date | 2017-02-13 11:30 +0100 |
| Message-ID | <tam3o-2qv-29@gated-at.bofh.it> |
| In reply to | #1579344 |
On Mon, Feb 13, 2017 at 02:26:20AM +0100, Gabriel C wrote:
> I didn't tested your patch yet but did a boot with mce=off and nomce
> which seems to not really works since is still want to mc_device_add()
> even when off.
mc_device_add() is microcode loader's ->add_dev() subsys pointer and
that's not from mce. From mce you should be seeing only (with the debug
patch applied):
[ 1.717508] mce: mcheck_init_device: entry
[ 1.718769] mce: Unable to init device /dev/mcelog (rc: -5)
> See :
>
> http://ftp.frugalware.org/pub/other/people/crazy/kernel/t/crash_mce_off.jpg
That looks like core 13 got the NMI from the watchdog at
if (wait)
csd_lock_wait(csd);
IINM and from what I could correlate to the asm it generates here,
RIP points to that READ_ONCE there in smp_cond_load_acquire() in
smp_call_function_single() which is called by collect_cpu_info() of the
microcode loader to get the microcode-relevant info from the CPU.
So this is simply a bystander CPU which got interrupted.
> I'll build an .10-rc8 with your patch tomorrow .. is somewhat late now here :)
Ok.
> Another thing is .. there seems to be a real bug in tsc code .
>
> I've build an -rc8 with a lot more debug options on an now I see the following :
Right before I went to bed I thought of telling you to enable lockdep :-)
Good. :-)
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-02-07 23:10 +0100 |
| Message-ID | <t8m7w-c4-9@gated-at.bofh.it> |
| In reply to | #1575241 |
On Mon, 6 Feb 2017, Linus Torvalds wrote:
> That said, it also strikes me that the implicated
> irq_chip_retrigger_hierarchy() function looks really very suspicious
> indeed.
>
> Most of the other users don't seem to traverse the parent all the way
> until they find something. They just do the operation in the parent,
> and if the parent needs it, it might then do it in _its_ parent and so
> on.
The whole point of the hierarchy is that we have decoupled the stacked
chips so the ioapic does not know whether it is connected to the irq
remapping unit or to the vector domain directly.
> So I'm wondering if that for-loop triggers a stack overflow on your
> setup somehow, just because that irq_retrigger() call is now truly
> recursive, and hasn't been turned into tail-calls.
It would only be recursive if some level down the hierarchy would use the
same callback.
The ioapic is always on top of its hierarchy and its either connected to
the vector domain directly, which is the last level in the hierarchy and
implements the real retrigger callback or to the irq remapping unit which
does not have a retrigger callback. So it's not a recursion problem AFAICT,
but lets try and just use the apic callback directly as we did before the
whole hierarchy rework. That's wrong for other reasons, but that does not
matter in that particular case. Patch below.
We have the same situation with the MSI interrupt domains which all use
irq_chip_retrigger_hierarchy() function as their retrigger callback, which
does not seem to have the same effect on Gabriels machine.
I have the feeling that this commit unearthes some other subtle wreckage in
the interrupt machinery which gets not triggered otherwise.
Thanks,
tglx
8<-------------------
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 52f352b063fd..3b6e5f3f099d 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1867,6 +1867,8 @@ static int ioapic_set_affinity(struct irq_data *irq_data,
return ret;
}
+extern int apic_retrigger_irq(struct irq_data *irq_data);
+
static struct irq_chip ioapic_chip __read_mostly = {
.name = "IO-APIC",
.irq_startup = startup_ioapic_irq,
@@ -1875,7 +1877,7 @@ static struct irq_chip ioapic_chip __read_mostly = {
.irq_ack = irq_chip_ack_parent,
.irq_eoi = ioapic_ack_level,
.irq_set_affinity = ioapic_set_affinity,
- .irq_retrigger = irq_chip_retrigger_hierarchy,
+ .irq_retrigger = apic_retrigger_irq,
.flags = IRQCHIP_SKIP_SET_WAKE,
};
@@ -1887,7 +1889,7 @@ static struct irq_chip ioapic_ir_chip __read_mostly = {
.irq_ack = irq_chip_ack_parent,
.irq_eoi = ioapic_ir_ack_level,
.irq_set_affinity = ioapic_set_affinity,
- .irq_retrigger = irq_chip_retrigger_hierarchy,
+ .irq_retrigger = apic_retrigger_irq,
.flags = IRQCHIP_SKIP_SET_WAKE,
};
diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index 5d30c5e42bb1..ce9b93e19266 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -496,7 +496,7 @@ void setup_vector_irq(int cpu)
__setup_vector_irq(cpu);
}
-static int apic_retrigger_irq(struct irq_data *irq_data)
+int apic_retrigger_irq(struct irq_data *irq_data)
{
struct apic_chip_data *data = apic_chip_data(irq_data);
unsigned long flags;
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web