Jan Beulich
2011-Nov-04 11:52 UTC
[Xen-devel] [PATCH] 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)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Juergen Gross
2011-Nov-04 13:01 UTC
Re: [Xen-devel] [PATCH] x86/IRQ: fix create_irq() after c/s 24068:6928172f7ded
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2011-Nov-04 14:41 UTC
Re: [Xen-devel] [PATCH] x86/IRQ: fix create_irq() after c/s 24068:6928172f7ded
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Nov-04 15:05 UTC
Re: [Xen-devel] [PATCH] x86/IRQ: fix create_irq() after c/s 24068:6928172f7ded
>>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2011-Nov-04 16:34 UTC
Re: [Xen-devel] [PATCH] x86/IRQ: fix create_irq() after c/s 24068:6928172f7ded
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 >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel