[PATCH] IRQ: fix incorrect logic in __clear_irq_vector

[PATCH] IRQ: fix incorrect logic in __clear_irq_vector

[Andrew Cooper]

In the old code, tmp_mask is the cpu_and of cfg->cpu_mask and
cpu_online_map.  However, in the usual case of moving an IRQ from one
PCPU to another because the scheduler decides its a good idea,
cfg->cpu_mask and cfg->old_cpu_mask do not intersect.  This causes the
old cpu vector_irq table to keep the irq reference when it shouldn't.

This leads to a resource leak if a domain is shut down wile an irq has
a move pending, which results in Xen's create_irq() eventually failing
with -ENOSPC when all vector_irq tables are full of stale references.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

diff -r 1f08b380d438 -r fa051d11b3de xen/arch/x86/irq.c
--- a/xen/arch/x86/irq.c	Wed Aug 10 14:43:34 2011 +0100
+++ b/xen/arch/x86/irq.c	Fri Aug 12 14:09:52 2011 +0100
@@ -216,7 +216,7 @@ static void __clear_irq_vector(int irq)
 
     if (likely(!cfg->move_in_progress))
         return;
-    for_each_cpu_mask(cpu, tmp_mask) {
+    for_each_cpu_mask(cpu, cfg->old_cpu_mask) {
         for (vector = FIRST_DYNAMIC_VECTOR; vector <= LAST_DYNAMIC_VECTOR;
                                 vector++) {
             if (per_cpu(vector_irq, cpu)[vector] != irq)

Re: [PATCH] IRQ: fix incorrect logic in __clear_irq_vector

[Andrew Cooper]

On 12/08/11 14:10, Andrew Cooper wrote:
> In the old code, tmp_mask is the cpu_and of cfg->cpu_mask and
> cpu_online_map.  However, in the usual case of moving an IRQ from one
> PCPU to another because the scheduler decides its a good idea,
> cfg->cpu_mask and cfg->old_cpu_mask do not intersect.  This causes the
> old cpu vector_irq table to keep the irq reference when it shouldn't.
>
> This leads to a resource leak if a domain is shut down wile an irq has
> a move pending, which results in Xen's create_irq() eventually failing
> with -ENOSPC when all vector_irq tables are full of stale references.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> diff -r 1f08b380d438 -r fa051d11b3de xen/arch/x86/irq.c
> --- a/xen/arch/x86/irq.c	Wed Aug 10 14:43:34 2011 +0100
> +++ b/xen/arch/x86/irq.c	Fri Aug 12 14:09:52 2011 +0100
> @@ -216,7 +216,7 @@ static void __clear_irq_vector(int irq)
>  
>      if (likely(!cfg->move_in_progress))
>          return;
> -    for_each_cpu_mask(cpu, tmp_mask) {
> +    for_each_cpu_mask(cpu, cfg->old_cpu_mask) {
>          for (vector = FIRST_DYNAMIC_VECTOR; vector <= LAST_DYNAMIC_VECTOR;
>                                  vector++) {
>              if (per_cpu(vector_irq, cpu)[vector] != irq)
Apologies for the previous spam of this patch - I failed somewhat with
patchbomb.

2 things come to mind.

1) This affects all versions of Xen since per-cpu idts were introduces,
so is a candidate for backporting to all relevant trees.

2) What would the tradeoff be with adding a "u8 old_vector" to irq_cfg? 
It would increase the size of the cfg structure but would avoid several
pieces of code which loop through all dynamic vectors and check if the
irq vector matches?

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

Re: [PATCH] IRQ: fix incorrect logic in __clear_irq_vector

[Jan Beulich]

>>> On 12.08.11 at 15:13, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 12/08/11 14:10, Andrew Cooper wrote:
>> In the old code, tmp_mask is the cpu_and of cfg->cpu_mask and
>> cpu_online_map.  However, in the usual case of moving an IRQ from one
>> PCPU to another because the scheduler decides its a good idea,
>> cfg->cpu_mask and cfg->old_cpu_mask do not intersect.  This causes the
>> old cpu vector_irq table to keep the irq reference when it shouldn't.
>>
>> This leads to a resource leak if a domain is shut down wile an irq has
>> a move pending, which results in Xen's create_irq() eventually failing
>> with -ENOSPC when all vector_irq tables are full of stale references.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> diff -r 1f08b380d438 -r fa051d11b3de xen/arch/x86/irq.c
>> --- a/xen/arch/x86/irq.c	Wed Aug 10 14:43:34 2011 +0100
>> +++ b/xen/arch/x86/irq.c	Fri Aug 12 14:09:52 2011 +0100
>> @@ -216,7 +216,7 @@ static void __clear_irq_vector(int irq)
>>  
>>      if (likely(!cfg->move_in_progress))
>>          return;
>> -    for_each_cpu_mask(cpu, tmp_mask) {
>> +    for_each_cpu_mask(cpu, cfg->old_cpu_mask) {

I think you rather want

    cpus_and(tmp_mask, cfg->old_cpu_mask, cpu_online_map);

before the loop, and keep the looping on tmp_mask. Otherwise you're
in danger of accessing offline CPUs' per-CPU data.

>>          for (vector = FIRST_DYNAMIC_VECTOR; vector <= LAST_DYNAMIC_VECTOR;
>>                                  vector++) {
>>              if (per_cpu(vector_irq, cpu)[vector] != irq)
> Apologies for the previous spam of this patch - I failed somewhat with
> patchbomb.
> 
> 2 things come to mind.
> 
> 1) This affects all versions of Xen since per-cpu idts were introduces,
> so is a candidate for backporting to all relevant trees.
> 
> 2) What would the tradeoff be with adding a "u8 old_vector" to irq_cfg? 
> It would increase the size of the cfg structure but would avoid several
> pieces of code which loop through all dynamic vectors and check if the
> irq vector matches?

The size wouldn't grow (there's already a single bit at the end, so a
new u8 would just fill what is currently padding space). I didn't,
however, spot any other loop than the one here that would benefit.

Jan

Re: Re: [PATCH] IRQ: fix incorrect logic in __clear_irq_vector

[Jan Beulich]

>>> On 12.08.11 at 16:03, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> The question now is whether sticking another u8 into irq_cfg is a good idea.
> 
> If as you thing, it wont increase the size, then I suggest that it is a
> good idea as we do speed up __clear_irq_vector.

Yes, give it a try (but as a separate patch of course).

Jan