[PATCH] x86/IRQ: fix create_irq() after c/s 24068:6928172f7ded

[PATCH] x86/IRQ: fix create_irq() after c/s 24068:6928172f7ded

[Jan Beulich]

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

init_one_irq_desc() must be called with interrupts enabled (as it may
call functions from the xmalloc() group). Rather than mis-using
vector_lock to also protect the finding of an unused IRQ, make this
lockless through using cmpxchg(), and obtain the lock only around the
actual assignment of the vector.

Also fold find_unassigned_irq() into its only caller.

It is, btw, questionable whether create_irq() calling
__assign_irq_vector() (rather than assign_irq_vector()) is actually
correct - desc->affinity appears to not get initialized properly in
this case.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -151,16 +151,6 @@ int __init bind_irq_vector(int irq, int 
     return ret;
 }
 
-static inline int find_unassigned_irq(void)
-{
-    int irq;
-
-    for (irq = nr_irqs_gsi; irq < nr_irqs; irq++)
-        if (irq_to_desc(irq)->arch.used == IRQ_UNUSED)
-            return irq;
-    return -ENOSPC;
-}
-
 /*
  * Dynamic irq allocate and deallocation for MSI
  */
@@ -170,19 +160,28 @@ int create_irq(void)
     int irq, ret;
     struct irq_desc *desc;
 
-    spin_lock_irqsave(&vector_lock, flags);
+    for (irq = nr_irqs_gsi; irq < nr_irqs; irq++)
+    {
+        desc = irq_to_desc(irq);
+        if (cmpxchg(&desc->arch.used, IRQ_UNUSED, IRQ_RESERVED) == IRQ_UNUSED)
+           break;
+    }
+
+    if (irq >= nr_irqs)
+         return -ENOSPC;
 
-    irq = find_unassigned_irq();
-    if (irq < 0)
-         goto out;
-    desc = irq_to_desc(irq);
     ret = init_one_irq_desc(desc);
     if (!ret)
+    {
+        spin_lock_irqsave(&vector_lock, flags);
         ret = __assign_irq_vector(irq, desc, TARGET_CPUS);
+        spin_unlock_irqrestore(&vector_lock, flags);
+    }
     if (ret < 0)
+    {
+        desc->arch.used = IRQ_UNUSED;
         irq = ret;
-out:
-     spin_unlock_irqrestore(&vector_lock, flags);
+    }
 
     return irq;
 }
--- a/xen/include/asm-x86/irq.h
+++ b/xen/include/asm-x86/irq.h
@@ -39,12 +39,13 @@ struct irq_cfg {
         unsigned move_cleanup_count;
         vmask_t *used_vectors;
         u8 move_in_progress : 1;
-        u8 used: 1;
+        s8 used;
 };
 
 /* For use with irq_cfg.used */
 #define IRQ_UNUSED      (0)
 #define IRQ_USED        (1)
+#define IRQ_RESERVED    (-1)
 
 #define IRQ_VECTOR_UNASSIGNED (-1)
 




[-- Attachment #2: x86-create-irq-locking.patch --]
[-- Type: text/plain, Size: 2472 bytes --]

x86/IRQ: fix create_irq() after c/s 24068:6928172f7ded

init_one_irq_desc() must be called with interrupts enabled (as it may
call functions from the xmalloc() group). Rather than mis-using
vector_lock to also protect the finding of an unused IRQ, make this
lockless through using cmpxchg(), and obtain the lock only around the
actual assignment of the vector.

Also fold find_unassigned_irq() into its only caller.

It is, btw, questionable whether create_irq() calling
__assign_irq_vector() (rather than assign_irq_vector()) is actually
correct - desc->affinity appears to not get initialized properly in
this case.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -151,16 +151,6 @@ int __init bind_irq_vector(int irq, int 
     return ret;
 }
 
-static inline int find_unassigned_irq(void)
-{
-    int irq;
-
-    for (irq = nr_irqs_gsi; irq < nr_irqs; irq++)
-        if (irq_to_desc(irq)->arch.used == IRQ_UNUSED)
-            return irq;
-    return -ENOSPC;
-}
-
 /*
  * Dynamic irq allocate and deallocation for MSI
  */
@@ -170,19 +160,28 @@ int create_irq(void)
     int irq, ret;
     struct irq_desc *desc;
 
-    spin_lock_irqsave(&vector_lock, flags);
+    for (irq = nr_irqs_gsi; irq < nr_irqs; irq++)
+    {
+        desc = irq_to_desc(irq);
+        if (cmpxchg(&desc->arch.used, IRQ_UNUSED, IRQ_RESERVED) == IRQ_UNUSED)
+           break;
+    }
+
+    if (irq >= nr_irqs)
+         return -ENOSPC;
 
-    irq = find_unassigned_irq();
-    if (irq < 0)
-         goto out;
-    desc = irq_to_desc(irq);
     ret = init_one_irq_desc(desc);
     if (!ret)
+    {
+        spin_lock_irqsave(&vector_lock, flags);
         ret = __assign_irq_vector(irq, desc, TARGET_CPUS);
+        spin_unlock_irqrestore(&vector_lock, flags);
+    }
     if (ret < 0)
+    {
+        desc->arch.used = IRQ_UNUSED;
         irq = ret;
-out:
-     spin_unlock_irqrestore(&vector_lock, flags);
+    }
 
     return irq;
 }
--- a/xen/include/asm-x86/irq.h
+++ b/xen/include/asm-x86/irq.h
@@ -39,12 +39,13 @@ struct irq_cfg {
         unsigned move_cleanup_count;
         vmask_t *used_vectors;
         u8 move_in_progress : 1;
-        u8 used: 1;
+        s8 used;
 };
 
 /* For use with irq_cfg.used */
 #define IRQ_UNUSED      (0)
 #define IRQ_USED        (1)
+#define IRQ_RESERVED    (-1)
 
 #define IRQ_VECTOR_UNASSIGNED (-1)
 

[-- 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] x86/IRQ: fix create_irq() after c/s 24068:6928172f7ded

[Juergen Gross]

On 11/04/2011 12:52 PM, Jan Beulich wrote:
> init_one_irq_desc() must be called with interrupts enabled (as it may
> call functions from the xmalloc() group). Rather than mis-using
> vector_lock to also protect the finding of an unused IRQ, make this
> lockless through using cmpxchg(), and obtain the lock only around the
> actual assignment of the vector.

Works for me (machine coming up again).

Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PDG ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: [PATCH] x86/IRQ: fix create_irq() after c/s 24068:6928172f7ded

[Keir Fraser]

On 04/11/2011 11:52, "Jan Beulich" <JBeulich@suse.com> wrote:

> init_one_irq_desc() must be called with interrupts enabled (as it may
> call functions from the xmalloc() group). Rather than mis-using
> vector_lock to also protect the finding of an unused IRQ, make this
> lockless through using cmpxchg(), and obtain the lock only around the
> actual assignment of the vector.

Looks fine to me.

Acked-by: Keir Fraser <keir@xen.org>

> Also fold find_unassigned_irq() into its only caller.
> 
> It is, btw, questionable whether create_irq() calling
> __assign_irq_vector() (rather than assign_irq_vector()) is actually
> correct - desc->affinity appears to not get initialized properly in
> this case.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -151,16 +151,6 @@ int __init bind_irq_vector(int irq, int
>      return ret;
>  }
>  
> -static inline int find_unassigned_irq(void)
> -{
> -    int irq;
> -
> -    for (irq = nr_irqs_gsi; irq < nr_irqs; irq++)
> -        if (irq_to_desc(irq)->arch.used == IRQ_UNUSED)
> -            return irq;
> -    return -ENOSPC;
> -}
> -
>  /*
>   * Dynamic irq allocate and deallocation for MSI
>   */
> @@ -170,19 +160,28 @@ int create_irq(void)
>      int irq, ret;
>      struct irq_desc *desc;
>  
> -    spin_lock_irqsave(&vector_lock, flags);
> +    for (irq = nr_irqs_gsi; irq < nr_irqs; irq++)
> +    {
> +        desc = irq_to_desc(irq);
> +        if (cmpxchg(&desc->arch.used, IRQ_UNUSED, IRQ_RESERVED) ==
> IRQ_UNUSED)
> +           break;
> +    }
> +
> +    if (irq >= nr_irqs)
> +         return -ENOSPC;
>  
> -    irq = find_unassigned_irq();
> -    if (irq < 0)
> -         goto out;
> -    desc = irq_to_desc(irq);
>      ret = init_one_irq_desc(desc);
>      if (!ret)
> +    {
> +        spin_lock_irqsave(&vector_lock, flags);
>          ret = __assign_irq_vector(irq, desc, TARGET_CPUS);
> +        spin_unlock_irqrestore(&vector_lock, flags);
> +    }
>      if (ret < 0)
> +    {
> +        desc->arch.used = IRQ_UNUSED;
>          irq = ret;
> -out:
> -     spin_unlock_irqrestore(&vector_lock, flags);
> +    }
>  
>      return irq;
>  }
> --- a/xen/include/asm-x86/irq.h
> +++ b/xen/include/asm-x86/irq.h
> @@ -39,12 +39,13 @@ struct irq_cfg {
>          unsigned move_cleanup_count;
>          vmask_t *used_vectors;
>          u8 move_in_progress : 1;
> -        u8 used: 1;
> +        s8 used;
>  };
>  
>  /* For use with irq_cfg.used */
>  #define IRQ_UNUSED      (0)
>  #define IRQ_USED        (1)
> +#define IRQ_RESERVED    (-1)
>  
>  #define IRQ_VECTOR_UNASSIGNED (-1)
>  
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] x86/IRQ: fix create_irq() after c/s 24068:6928172f7ded

[Jan Beulich]

>>> On 04.11.11 at 15:41, Keir Fraser <keir@xen.org> wrote:
> On 04/11/2011 11:52, "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> init_one_irq_desc() must be called with interrupts enabled (as it may
>> call functions from the xmalloc() group). Rather than mis-using
>> vector_lock to also protect the finding of an unused IRQ, make this
>> lockless through using cmpxchg(), and obtain the lock only around the
>> actual assignment of the vector.
> 
> Looks fine to me.
> 
> Acked-by: Keir Fraser <keir@xen.org>
> 
>> Also fold find_unassigned_irq() into its only caller.
>> 
>> It is, btw, questionable whether create_irq() calling
>> __assign_irq_vector() (rather than assign_irq_vector()) is actually
>> correct - desc->affinity appears to not get initialized properly in
>> this case.

Any thought on this one? Adjusting this would have the nice side
effect of the function no longer explicitly acquiring vector_lock.

Jan

Re: [PATCH] x86/IRQ: fix create_irq() after c/s 24068:6928172f7ded

[Keir Fraser]

On 04/11/2011 15:05, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 04.11.11 at 15:41, Keir Fraser <keir@xen.org> wrote:
>> On 04/11/2011 11:52, "Jan Beulich" <JBeulich@suse.com> wrote:
>> 
>>> init_one_irq_desc() must be called with interrupts enabled (as it may
>>> call functions from the xmalloc() group). Rather than mis-using
>>> vector_lock to also protect the finding of an unused IRQ, make this
>>> lockless through using cmpxchg(), and obtain the lock only around the
>>> actual assignment of the vector.
>> 
>> Looks fine to me.
>> 
>> Acked-by: Keir Fraser <keir@xen.org>
>> 
>>> Also fold find_unassigned_irq() into its only caller.
>>> 
>>> It is, btw, questionable whether create_irq() calling
>>> __assign_irq_vector() (rather than assign_irq_vector()) is actually
>>> correct - desc->affinity appears to not get initialized properly in
>>> this case.
> 
> Any thought on this one? Adjusting this would have the nice side
> effect of the function no longer explicitly acquiring vector_lock.

I would agree it should call assign_irq_vector(). It was probably only
taking the lock itself, and thus using __assign_irq_vector(), to avoid the
irq it found in find_unassigned_irq() being stolen. That can't happen now
you reserve it via cmpxchg.

 -- Keir

> Jan
>