Re: Re: [PATCH] [BUGFIX] crash/ioapic: Prevent crash_kexec() fromdeadlocking of ioapic_lock

[Yoshihiro YUNOMAE]

Hi Ingo,

Thank you for fixing typos!
OK, I'll fix them and rename to ioapic_zap_locks().

Thank you again!

Yoshihiro YUNOMAE

(2013/08/19 18:46), Ingo Molnar wrote:
> * Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@xxxxxxxxxxx> wrote:
>
> > Prevent crash_kexec() from deadlocking of ioapic_lock.
>
> s/of/on
>
> > When crash_kexec() is executed on a cpu, the cpu will get
> > ioapic_lock in disable_IO_APIC(). So if the cpu gets NMI
> > while locking ioapic_lock, a deadlock wiil happen. In
>
> s/will
>
> > this patch, ioapic_lock is initialized before
> > disable_IO_APIC().
> >
> > To confirm this deadlocking, you'll set up as follows:
>
> s/deadlocking/deadlock
>
> > 1. Add mdelay(1000) after raw_spin_lock_irqsave() in
> >     native_ioapic_set_affinity()@arch/x86/kernel/apic/io_apic.c
> >
> >     Although the deadlocking can occur without this modification, it will
> >    increase the potential of the deadlocking problem.
>
> s/deadlocking/deadlock
>
> > 2. Build and install the kernel
> >
> > 3. Set up the OS which will run panic() and kexec when NMI is injected
> >      # echo "kernel.unknown_nmi_panic=1" >> /etc/sysctl.conf
> >      # vim /etc/default/grub
> >        add "nmi_watchdog=0 crashkernel=256M" in GRUB_CMDLINE_LINUX line
> >      # grub2-mkconfig
> >
> > 4. Reboot the OS
> >
> > 5. Run following command for each vcpu on the guest
> >      # while true; do echo <CPU num> > /proc/irq/<IO-APIC-edge or IO-APIC-fasteoi>/smp_affinitity; done;
> >     By running this command, cpus will get ioapic_lock for setting affinity.
> >
> > 6. Inject NMI (push a dump button or execute 'virsh inject-nmi <domain>' if you
> >     use VM)
> >     After injecting NMI, panic() is called in an nmi-handler context.
> >     Then, kexec will normally run in panic(), but the operation will be stopped
> >     by deadlock of ioapic_lock in crash_kexec()->machine_crash_shutdown()->
>
> s/of/on
>
> >     native_machine_crash_shutdown()->disable_IO_APIC()->clear_IO_APIC()->
> >     clear_IO_APIC_pin()->ioapic_read_entry().
>
> I suppose we could do this patch if it's a common
> occurance.
>
> A few minor details need fixing:
>
> > Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@xxxxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> > Cc: x86@xxxxxxxxxx
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> > Cc: Seiji Aguchi <seiji.aguchi@xxxxxxx>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > Cc: Sebastian Andrzej Siewior <sebastian@xxxxxxxxxxxxx>
> > Cc: Joerg Roedel <joro@xxxxxxxxxx>
> > Cc: Zhang Yanfei <zhangyanfei@xxxxxxxxxxxxxx>
> > Cc: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> > Cc: Gleb Natapov <gleb@xxxxxxxxxx>
> > Cc: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> >   arch/x86/include/asm/apic.h    |    2 ++
> >   arch/x86/kernel/apic/io_apic.c |    5 +++++
> >   arch/x86/kernel/crash.c        |    4 ++++
> >   3 files changed, 11 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> > index f8119b5..ddb06af 100644
> > --- a/arch/x86/include/asm/apic.h
> > +++ b/arch/x86/include/asm/apic.h
> > @@ -715,4 +715,6 @@ static inline void exiting_ack_irq(void)
> >   	ack_APIC_irq();
> >   }
> >
> > +extern void ioapic_lock_init(void);
> > +
> >   #endif /* _ASM_X86_APIC_H */
> > diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> > index 9ed796c..2816c07 100644
> > --- a/arch/x86/kernel/apic/io_apic.c
> > +++ b/arch/x86/kernel/apic/io_apic.c
> > @@ -1534,6 +1534,11 @@ void intel_ir_io_apic_print_entries(unsigned int apic,
> >   	}
> >   }
> >
> > +void ioapic_lock_init(void)
> > +{
> > +	raw_spin_lock_init(&ioapic_lock);
> > +}
>
> Please name this ioapic_zap_locks() to make clear that this
> is crash handling related.
>
> > +
> >   __apicdebuginit(void) print_IO_APIC(int ioapic_idx)
> >   {
> >   	union IO_APIC_reg_00 reg_00;
> > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> > index 74467fe..ea039d5 100644
> > --- a/arch/x86/kernel/crash.c
> > +++ b/arch/x86/kernel/crash.c
> > @@ -129,6 +129,10 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
> >
> >   	lapic_shutdown();
> >   #if defined(CONFIG_X86_IO_APIC)
>
> Please enhance this #ifdef while at it.
>
> > +	/*
> > +	 * Prevent crash_kexec() from deadlocking of ioapic_lock.
> > +	 */
>
> s/of/on.
>
> Also, single-line comment can go /* here */.
>
> > +	ioapic_lock_init();
> >   	disable_IO_APIC();
> >   #endif
> >   #ifdef CONFIG_HPET_TIMER
>
> Thanks,
>
> 	Ingo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae.ez@xxxxxxxxxxx


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/