[PATCH] irq: Exclude percpu IRQs from being fixed up

[PATCH] irq: Exclude percpu IRQs from being fixed up

[Zhang, Fengzhe]

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

irq: Exclude percpu IRQs from being fixed up

Xen spin unlock uses spurious ipi "lock_kicker_irq" to wake up blocked vCPUs waiting on that lock. This irq should always be disabled. However, when Dom0 is shuting down, function fixup_irqs is called which unmasks all irqs. Function unmask_irq effectively re-enables lock_kicker_irq and its irq handler is invoked which reports bug and crashes Dom0.

This patch sets IRQ_PER_CPU flag in irq desc and excludes percpu IRQs from being fixed up when taking CPUs down.

Signed-off-by: Fengzhe Zhang <fengzhe.zhang@intel.com>

diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
index 977d8b4..f0f9450 100644
--- a/arch/x86/kernel/irq_64.c
+++ b/arch/x86/kernel/irq_64.c
@@ -80,6 +80,9 @@ void fixup_irqs(void)
 		if (irq == 2)
 			continue;
 
+		if (desc->status & IRQ_PER_CPU)
+			continue;
+
 		/* interrupt's are disabled at this point */
 		spin_lock(&desc->lock);
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index f34e231..26bc55a 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -727,10 +727,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 				goto out_thread;
 		} else
 			compat_irq_chip_set_default_handler(desc);
-#if defined(CONFIG_IRQ_PER_CPU)
+
 		if (new->flags & IRQF_PERCPU)
 			desc->status |= IRQ_PER_CPU;
-#endif
 
 		desc->status &= ~(IRQ_AUTODETECT | IRQ_WAITING | IRQ_ONESHOT |
 				  IRQ_INPROGRESS | IRQ_SPURIOUS_DISABLED);

[-- Attachment #2: lock_kicker_irq_bug.patch --]
[-- Type: application/octet-stream, Size: 1502 bytes --]

irq: Exclude percpu IRQs from being fixed up

Xen spin unlock uses spurious ipi "lock_kicker_irq" to wake up blocked vCPUs waiting on that lock. This irq should always be disabled. However, when Dom0 is shuting down, function fixup_irqs is called which unmasks all irqs. Function unmask_irq effectively re-enables lock_kicker_irq and its irq handler is invoked which reports bug and crashes Dom0.

This patch sets IRQ_PER_CPU flag in irq desc and excludes percpu IRQs from being fixed up when taking CPUs down.

Signed-off-by: Fengzhe Zhang <fengzhe.zhang@intel.com>

diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
index 977d8b4..f0f9450 100644
--- a/arch/x86/kernel/irq_64.c
+++ b/arch/x86/kernel/irq_64.c
@@ -80,6 +80,9 @@ void fixup_irqs(void)
 		if (irq == 2)
 			continue;
 
+		if (desc->status & IRQ_PER_CPU)
+			continue;
+
 		/* interrupt's are disabled at this point */
 		spin_lock(&desc->lock);
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index f34e231..26bc55a 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -727,10 +727,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 				goto out_thread;
 		} else
 			compat_irq_chip_set_default_handler(desc);
-#if defined(CONFIG_IRQ_PER_CPU)
+
 		if (new->flags & IRQF_PERCPU)
 			desc->status |= IRQ_PER_CPU;
-#endif
 
 		desc->status &= ~(IRQ_AUTODETECT | IRQ_WAITING | IRQ_ONESHOT |
 				  IRQ_INPROGRESS | IRQ_SPURIOUS_DISABLED);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] irq: Exclude percpu IRQs from being fixed up

[Jan Beulich]

>>> On 16.02.11 at 15:26, "Zhang, Fengzhe" <fengzhe.zhang@intel.com> wrote:
> irq: Exclude percpu IRQs from being fixed up
> 
> Xen spin unlock uses spurious ipi "lock_kicker_irq" to wake up blocked vCPUs 
> waiting on that lock. This irq should always be disabled. However, when Dom0 
> is shuting down, function fixup_irqs is called which unmasks all irqs. 
> Function unmask_irq effectively re-enables lock_kicker_irq and its irq handler 
> is invoked which reports bug and crashes Dom0.
> 
> This patch sets IRQ_PER_CPU flag in irq desc and excludes percpu IRQs from 
> being fixed up when taking CPUs down.
> 
> Signed-off-by: Fengzhe Zhang <fengzhe.zhang@intel.com>
> 
> diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
> index 977d8b4..f0f9450 100644
> --- a/arch/x86/kernel/irq_64.c
> +++ b/arch/x86/kernel/irq_64.c
> @@ -80,6 +80,9 @@ void fixup_irqs(void)
>  		if (irq == 2)
>  			continue;
>  
> +		if (desc->status & IRQ_PER_CPU)
> +			continue;
> +
>  		/* interrupt's are disabled at this point */
>  		spin_lock(&desc->lock);
>  
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index f34e231..26bc55a 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -727,10 +727,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, 
> struct irqaction *new)
>  				goto out_thread;
>  		} else
>  			compat_irq_chip_set_default_handler(desc);
> -#if defined(CONFIG_IRQ_PER_CPU)

Why? XEN should select IRQ_PER_CPU instead in its Kconfig.

Jan

> +
>  		if (new->flags & IRQF_PERCPU)
>  			desc->status |= IRQ_PER_CPU;
> -#endif
>  
>  		desc->status &= ~(IRQ_AUTODETECT | IRQ_WAITING | IRQ_ONESHOT |
>  				  IRQ_INPROGRESS | IRQ_SPURIOUS_DISABLED);

Re: [PATCH] irq: Exclude percpu IRQs from being fixed up

[Fengzhe Zhang]

On 2011/2/16 22:53, Jan Beulich wrote:
>>>> On 16.02.11 at 15:26, "Zhang, Fengzhe"<fengzhe.zhang@intel.com>  wrote:
>> irq: Exclude percpu IRQs from being fixed up
>>
>> Xen spin unlock uses spurious ipi "lock_kicker_irq" to wake up blocked vCPUs
>> waiting on that lock. This irq should always be disabled. However, when Dom0
>> is shuting down, function fixup_irqs is called which unmasks all irqs.
>> Function unmask_irq effectively re-enables lock_kicker_irq and its irq handler
>> is invoked which reports bug and crashes Dom0.
>>
>> This patch sets IRQ_PER_CPU flag in irq desc and excludes percpu IRQs from
>> being fixed up when taking CPUs down.
>>
>> Signed-off-by: Fengzhe Zhang<fengzhe.zhang@intel.com>
>>
>> diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
>> index 977d8b4..f0f9450 100644
>> --- a/arch/x86/kernel/irq_64.c
>> +++ b/arch/x86/kernel/irq_64.c
>> @@ -80,6 +80,9 @@ void fixup_irqs(void)
>>   		if (irq == 2)
>>   			continue;
>>
>> +		if (desc->status&  IRQ_PER_CPU)
>> +			continue;
>> +
>>   		/* interrupt's are disabled at this point */
>>   		spin_lock(&desc->lock);
>>
>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>> index f34e231..26bc55a 100644
>> --- a/kernel/irq/manage.c
>> +++ b/kernel/irq/manage.c
>> @@ -727,10 +727,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc,
>> struct irqaction *new)
>>   				goto out_thread;
>>   		} else
>>   			compat_irq_chip_set_default_handler(desc);
>> -#if defined(CONFIG_IRQ_PER_CPU)
>
> Why? XEN should select IRQ_PER_CPU instead in its Kconfig.
>
> Jan
>
IRQ_PER_CPU switch is not found in current Kconfig. I'm not sure if this 
feature is going to be brought back in the short term. I remove the 
ifdef to set IRQ_PER_CPU flag in desc by default but still leave the IRQ 
handling logic unchanged. This is a temporary solution to fix system 
crash on poweroff. And this is the fix with minimum impact among the 
several solutions we tried.

-Fengzhe

>> +
>>   		if (new->flags&  IRQF_PERCPU)
>>   			desc->status |= IRQ_PER_CPU;
>> -#endif
>>
>>   		desc->status&= ~(IRQ_AUTODETECT | IRQ_WAITING | IRQ_ONESHOT |
>>   				  IRQ_INPROGRESS | IRQ_SPURIOUS_DISABLED);
>
>

Re: [PATCH] irq: Exclude percpu IRQs from being fixed up

[Jan Beulich]

>>> On 17.02.11 at 01:15, Fengzhe Zhang <fengzhe.zhang@intel.com> wrote:
> On 2011/2/16 22:53, Jan Beulich wrote:
>>>>> On 16.02.11 at 15:26, "Zhang, Fengzhe"<fengzhe.zhang@intel.com>  wrote:
>>> irq: Exclude percpu IRQs from being fixed up
>>>
>>> Xen spin unlock uses spurious ipi "lock_kicker_irq" to wake up blocked vCPUs
>>> waiting on that lock. This irq should always be disabled. However, when Dom0
>>> is shuting down, function fixup_irqs is called which unmasks all irqs.
>>> Function unmask_irq effectively re-enables lock_kicker_irq and its irq 
> handler
>>> is invoked which reports bug and crashes Dom0.
>>>
>>> This patch sets IRQ_PER_CPU flag in irq desc and excludes percpu IRQs from
>>> being fixed up when taking CPUs down.
>>>
>>> Signed-off-by: Fengzhe Zhang<fengzhe.zhang@intel.com>
>>>
>>> diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
>>> index 977d8b4..f0f9450 100644
>>> --- a/arch/x86/kernel/irq_64.c
>>> +++ b/arch/x86/kernel/irq_64.c
>>> @@ -80,6 +80,9 @@ void fixup_irqs(void)
>>>   		if (irq == 2)
>>>   			continue;
>>>
>>> +		if (desc->status&  IRQ_PER_CPU)
>>> +			continue;
>>> +
>>>   		/* interrupt's are disabled at this point */
>>>   		spin_lock(&desc->lock);
>>>
>>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>>> index f34e231..26bc55a 100644
>>> --- a/kernel/irq/manage.c
>>> +++ b/kernel/irq/manage.c
>>> @@ -727,10 +727,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc,
>>> struct irqaction *new)
>>>   				goto out_thread;
>>>   		} else
>>>   			compat_irq_chip_set_default_handler(desc);
>>> -#if defined(CONFIG_IRQ_PER_CPU)
>>
>> Why? XEN should select IRQ_PER_CPU instead in its Kconfig.
>>
>> Jan
>>
> IRQ_PER_CPU switch is not found in current Kconfig. I'm not sure if this 

kernel/irq/Kconfig (introduced as a generic option in 2.6.38-rc2). In
prior kernel you'd have to add a respective Kconfig item in
drivers/xen/Kconfig.

> feature is going to be brought back in the short term. I remove the 
> ifdef to set IRQ_PER_CPU flag in desc by default but still leave the IRQ 
> handling logic unchanged. This is a temporary solution to fix system 
> crash on poweroff. And this is the fix with minimum impact among the 
> several solutions we tried.

But it's more a hack than a fix. And making per-CPU IRQs properly
treated as such isn't a bad idea in any case, I would say.

Jan

Re: [PATCH] irq: Exclude percpu IRQs from being fixed up

[Fengzhe Zhang]

On 2011/2/17 15:52, Jan Beulich wrote:
>>>> On 17.02.11 at 01:15, Fengzhe Zhang<fengzhe.zhang@intel.com>  wrote:
>> On 2011/2/16 22:53, Jan Beulich wrote:
>>>>>> On 16.02.11 at 15:26, "Zhang, Fengzhe"<fengzhe.zhang@intel.com>   wrote:
>>>> irq: Exclude percpu IRQs from being fixed up
>>>>
>>>> Xen spin unlock uses spurious ipi "lock_kicker_irq" to wake up blocked vCPUs
>>>> waiting on that lock. This irq should always be disabled. However, when Dom0
>>>> is shuting down, function fixup_irqs is called which unmasks all irqs.
>>>> Function unmask_irq effectively re-enables lock_kicker_irq and its irq
>> handler
>>>> is invoked which reports bug and crashes Dom0.
>>>>
>>>> This patch sets IRQ_PER_CPU flag in irq desc and excludes percpu IRQs from
>>>> being fixed up when taking CPUs down.
>>>>
>>>> Signed-off-by: Fengzhe Zhang<fengzhe.zhang@intel.com>
>>>>
>>>> diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
>>>> index 977d8b4..f0f9450 100644
>>>> --- a/arch/x86/kernel/irq_64.c
>>>> +++ b/arch/x86/kernel/irq_64.c
>>>> @@ -80,6 +80,9 @@ void fixup_irqs(void)
>>>>    		if (irq == 2)
>>>>    			continue;
>>>>
>>>> +		if (desc->status&   IRQ_PER_CPU)
>>>> +			continue;
>>>> +
>>>>    		/* interrupt's are disabled at this point */
>>>>    		spin_lock(&desc->lock);
>>>>
>>>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>>>> index f34e231..26bc55a 100644
>>>> --- a/kernel/irq/manage.c
>>>> +++ b/kernel/irq/manage.c
>>>> @@ -727,10 +727,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc,
>>>> struct irqaction *new)
>>>>    				goto out_thread;
>>>>    		} else
>>>>    			compat_irq_chip_set_default_handler(desc);
>>>> -#if defined(CONFIG_IRQ_PER_CPU)
>>>
>>> Why? XEN should select IRQ_PER_CPU instead in its Kconfig.
>>>
>>> Jan
>>>
>> IRQ_PER_CPU switch is not found in current Kconfig. I'm not sure if this
>
> kernel/irq/Kconfig (introduced as a generic option in 2.6.38-rc2). In
> prior kernel you'd have to add a respective Kconfig item in
> drivers/xen/Kconfig.
>
>> feature is going to be brought back in the short term. I remove the
>> ifdef to set IRQ_PER_CPU flag in desc by default but still leave the IRQ
>> handling logic unchanged. This is a temporary solution to fix system
>> crash on poweroff. And this is the fix with minimum impact among the
>> several solutions we tried.
>
> But it's more a hack than a fix. And making per-CPU IRQs properly

Yes, that's true. Switching on IRQ_PER_CPU can make this patch neat. But 
that isn't my call.

-Fengzhe

> treated as such isn't a bad idea in any case, I would say.
>
> Jan
>

Re: Re: [PATCH] irq: Exclude percpu IRQs from being fixed up

[Ian Campbell]

On Thu, 2011-02-17 at 07:52 +0000, Jan Beulich wrote:
> >>> On 17.02.11 at 01:15, Fengzhe Zhang <fengzhe.zhang@intel.com> wrote:
> > IRQ_PER_CPU switch is not found in current Kconfig. I'm not sure if this 
> 
> kernel/irq/Kconfig (introduced as a generic option in 2.6.38-rc2). In
> prior kernel you'd have to add a respective Kconfig item in
> drivers/xen/Kconfig.

Also this should be fixed in mainline _before_ being considered for
backporting to the xen/stable-2.6.32.x branch, otherwise it will simply
come back one day when the stable branch moves forward...

> > feature is going to be brought back in the short term. I remove the 
> > ifdef to set IRQ_PER_CPU flag in desc by default but still leave the IRQ 
> > handling logic unchanged. This is a temporary solution to fix system 
> > crash on poweroff. And this is the fix with minimum impact among the 
> > several solutions we tried.
> 
> But it's more a hack than a fix.

Agreed, it seems to take a very narrow approach to a specific failure
without looking at the bigger picture.

>  And making per-CPU IRQs properly
> treated as such isn't a bad idea in any case, I would say.

In addition changing generic code, which also has an impact on native,
in this way needs a lot more rationale in the commit message as to why
it is correct for both Xen and native. Most importantly it needs to go
via the x86 maintainers and not the Xen maintainers.

It also need to be made very clear why the semantics which are required
for this specific lock (lock_kicker_irq) are correct and desirable for
_every_  IRQ_PER_CPU (aka IRQF_PERCPU) lock on x86. The description of
this patch does not do this.

How does this change tie in with the existing mainline IRQF_NO_SUSPEND
flag (which Xen uses on these IPI IRQs) and the IRQF_FORCE_RESUME flag
currently in the tip tree (intended for 2.6.39, I believe)?

Ian.

RE: [PATCH] irq: Exclude percpu IRQs from being fixed up

[Li, Xin]

> >>>
> >> IRQ_PER_CPU switch is not found in current Kconfig. I'm not sure if this
> >
> > kernel/irq/Kconfig (introduced as a generic option in 2.6.38-rc2). In
> > prior kernel you'd have to add a respective Kconfig item in
> > drivers/xen/Kconfig.
> >
> >> feature is going to be brought back in the short term. I remove the
> >> ifdef to set IRQ_PER_CPU flag in desc by default but still leave the IRQ
> >> handling logic unchanged. This is a temporary solution to fix system
> >> crash on poweroff. And this is the fix with minimum impact among the
> >> several solutions we tried.
> >
> > But it's more a hack than a fix. And making per-CPU IRQs properly
> 
> Yes, that's true. Switching on IRQ_PER_CPU can make this patch neat. But
> that isn't my call.

Jeremy, would you consider the change to fix this issue?
Thanks!
-Xin

Re: RE: [PATCH] irq: Exclude percpu IRQs from being fixed up

[Ian Campbell]

On Thu, 2011-04-28 at 12:05 +0100, Li, Xin wrote:
> > >>>
> > >> IRQ_PER_CPU switch is not found in current Kconfig. I'm not sure if this
> > >
> > > kernel/irq/Kconfig (introduced as a generic option in 2.6.38-rc2). In
> > > prior kernel you'd have to add a respective Kconfig item in
> > > drivers/xen/Kconfig.
> > >
> > >> feature is going to be brought back in the short term. I remove the
> > >> ifdef to set IRQ_PER_CPU flag in desc by default but still leave the IRQ
> > >> handling logic unchanged. This is a temporary solution to fix system
> > >> crash on poweroff. And this is the fix with minimum impact among the
> > >> several solutions we tried.
> > >
> > > But it's more a hack than a fix. And making per-CPU IRQs properly
> > 
> > Yes, that's true. Switching on IRQ_PER_CPU can make this patch neat. But
> > that isn't my call.
> 
> Jeremy, would you consider the change to fix this issue?

There was no response to the several issues/questions I raised in
<1297933765.16356.1344.camel@zakaz.uk.xensource.com>.

Ian.

RE: Re: [PATCH] irq: Exclude percpu IRQs from being fixed up

[Tian, Kevin]

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

> From: Ian Campbell [mailto:Ian.Campbell@citrix.com]
> Sent: Thursday, February 17, 2011 5:09 PM
> 
> On Thu, 2011-02-17 at 07:52 +0000, Jan Beulich wrote:
> > >>> On 17.02.11 at 01:15, Fengzhe Zhang <fengzhe.zhang@intel.com>
> wrote:
> > > IRQ_PER_CPU switch is not found in current Kconfig. I'm not sure if
> > > this
> >
> > kernel/irq/Kconfig (introduced as a generic option in 2.6.38-rc2). In
> > prior kernel you'd have to add a respective Kconfig item in
> > drivers/xen/Kconfig.
> 
> Also this should be fixed in mainline _before_ being considered for backporting
> to the xen/stable-2.6.32.x branch, otherwise it will simply come back one day
> when the stable branch moves forward...

yes, it's preferred if we can make it common instead of special case.

> 
> > > feature is going to be brought back in the short term. I remove the
> > > ifdef to set IRQ_PER_CPU flag in desc by default but still leave the
> > > IRQ handling logic unchanged. This is a temporary solution to fix
> > > system crash on poweroff. And this is the fix with minimum impact
> > > among the several solutions we tried.
> >
> > But it's more a hack than a fix.
> 
> Agreed, it seems to take a very narrow approach to a specific failure without
> looking at the bigger picture.

latest tip also removes the CONFIG_IRQ_PER_CPU switch now, as people think it
save little by adding #ifdef. So for current 2.6.39 pv tree, we only need the proper
check in irq.c, but we do need select this config option for .32 tree as you
suggested or backport below commit:

commit 6a58fb3bad099076f36f0f30f44507bc3275cdb6
Author: Thomas Gleixner <tglx@linutronix.de>
Date:   Tue Feb 8 15:40:05 2011 +0100

    genirq: Remove CONFIG_IRQ_PER_CPU
    
    The saving of this switch is minimal versus the ifdef mess it
    creates. Simple enable PER_CPU unconditionally and remove the config
    switch.
    
    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

> 
> >  And making per-CPU IRQs properly
> > treated as such isn't a bad idea in any case, I would say.
> 
> In addition changing generic code, which also has an impact on native, in this
> way needs a lot more rationale in the commit message as to why it is correct
> for both Xen and native. Most importantly it needs to go via the x86
> maintainers and not the Xen maintainers.

I'll try to ping kernel upstream first, but the likelihood for acceptance is 50 vs. 50.
native x86 doesn't use percpu irq so far, and thus to handle this flag is specific 
to Xen. but perhaps we can persuade them since fixup_irqs is not in critical path
and if we consider Xen as a special platform requiring percpu irq support, it's
probably good to have it.

> 
> It also need to be made very clear why the semantics which are required for
> this specific lock (lock_kicker_irq) are correct and desirable for _every_
> IRQ_PER_CPU (aka IRQF_PERCPU) lock on x86. The description of this patch
> does not do this.

I'll elaborate it when sending to the kernel side. Basically the fix is not specific to
spinlock irq. it's for all percpu type irq.

> 
> How does this change tie in with the existing mainline IRQF_NO_SUSPEND flag
> (which Xen uses on these IPI IRQs) and the IRQF_FORCE_RESUME flag currently
> in the tip tree (intended for 2.6.39, I believe)?
> 

I think they're orthogonal. fixup_irqs is required when you try to hot remove a cpu,
and in reboot/suspend path this happens before handling device suspend.

Thanks
Kevin

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel