Andrew Cooper
2011-Sep-02 16:35 UTC
[Xen-devel] [PATCH 0 of 3] IRQ: Part 1 of the irq code cleanup
Presented are some basic changes Patch 1 removes some bit-rotten code Patch 2 reduces the data overhead of the irq code Patch 3 removes a loop from the irq cleanup code Signed-off-by Andrew Cooper <andrew.cooper3@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Andrew Cooper
2011-Sep-02 16:35 UTC
[Xen-devel] [PATCH 1 of 3] IRQ: Remove bit-rotten code
irq_desc.depth is a write only variable.
LEGACY_IRQ_FROM_VECTOR(vec) is never referenced.
Signed-off-by Andrew Cooper <andrew.cooper3@citrix.com>
diff -r 227130622561 -r 5a1826139750 xen/arch/x86/io_apic.c
--- a/xen/arch/x86/io_apic.c Thu Aug 25 12:03:14 2011 +0100
+++ b/xen/arch/x86/io_apic.c Fri Sep 02 17:33:17 2011 +0100
@@ -1955,7 +1955,6 @@ static void __init check_timer(void)
if ((ret = bind_irq_vector(0, vector, mask_all)))
printk(KERN_ERR"..IRQ0 is not set correctly with ioapic!!!,
err:%d\n", ret);
- irq_desc[0].depth = 0;
irq_desc[0].status &= ~IRQ_DISABLED;
/*
diff -r 227130622561 -r 5a1826139750 xen/arch/x86/irq.c
--- a/xen/arch/x86/irq.c Thu Aug 25 12:03:14 2011 +0100
+++ b/xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100
@@ -178,7 +178,6 @@ static void dynamic_irq_cleanup(unsigned
desc->handler->shutdown(irq);
action = desc->action;
desc->action = NULL;
- desc->depth = 1;
desc->msi_desc = NULL;
desc->handler = &no_irq_type;
desc->chip_data->used_vectors=NULL;
@@ -278,7 +277,6 @@ static void __init init_one_irq_desc(str
desc->status = IRQ_DISABLED;
desc->handler = &no_irq_type;
desc->action = NULL;
- desc->depth = 1;
desc->msi_desc = NULL;
spin_lock_init(&desc->lock);
cpus_setall(desc->affinity);
@@ -736,7 +734,6 @@ void __init release_irq(unsigned int irq
spin_lock_irqsave(&desc->lock,flags);
action = desc->action;
desc->action = NULL;
- desc->depth = 1;
desc->status |= IRQ_DISABLED;
desc->handler->shutdown(irq);
spin_unlock_irqrestore(&desc->lock,flags);
@@ -764,7 +761,6 @@ int __init setup_irq(unsigned int irq, s
}
desc->action = new;
- desc->depth = 0;
desc->status &= ~IRQ_DISABLED;
desc->handler->startup(irq);
@@ -1343,7 +1339,6 @@ int pirq_guest_bind(struct vcpu *v, stru
cpus_clear(action->cpu_eoi_map);
init_timer(&action->eoi_timer, irq_guest_eoi_timer_fn, desc, 0);
- desc->depth = 0;
desc->status |= IRQ_GUEST;
desc->status &= ~IRQ_DISABLED;
desc->handler->startup(irq);
@@ -1459,7 +1454,6 @@ static irq_guest_action_t *__pirq_guest_
BUG_ON(action->in_flight != 0);
/* Disabling IRQ before releasing the desc_lock avoids an IRQ storm. */
- desc->depth = 1;
desc->status |= IRQ_DISABLED;
desc->handler->disable(irq);
diff -r 227130622561 -r 5a1826139750 xen/include/asm-x86/irq.h
--- a/xen/include/asm-x86/irq.h Thu Aug 25 12:03:14 2011 +0100
+++ b/xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100
@@ -19,7 +19,6 @@
#define MSI_IRQ(irq) ((irq) >= nr_irqs_gsi && (irq) <
nr_irqs)
#define LEGACY_VECTOR(irq) ((irq) + FIRST_LEGACY_VECTOR)
-#define LEGACY_IRQ_FROM_VECTOR(vec) ((vec) - FIRST_LEGACY_VECTOR)
#define irq_to_desc(irq) (&irq_desc[irq])
#define irq_cfg(irq) (&irq_cfg[irq])
diff -r 227130622561 -r 5a1826139750 xen/include/xen/irq.h
--- a/xen/include/xen/irq.h Thu Aug 25 12:03:14 2011 +0100
+++ b/xen/include/xen/irq.h Fri Sep 02 17:33:17 2011 +0100
@@ -72,7 +72,6 @@ typedef struct irq_desc {
hw_irq_controller *handler;
struct msi_desc *msi_desc;
struct irqaction *action; /* IRQ action list */
- unsigned int depth; /* nested irq disables */
struct irq_cfg *chip_data;
int irq;
spinlock_t lock;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Andrew Cooper
2011-Sep-02 16:35 UTC
[Xen-devel] [PATCH 2 of 3] IRQ: Fold irq_status into irq_cfg
irq_status is an int for each of nr_irqs which represents a single
boolean variable. Fold it into the bitfield in irq_cfg, which saves
768 bytes per CPU with per-cpu IDTs in use.
Signed-off-by Andrew Cooper <andrew.cooper3@citrix.com>
diff -r 5a1826139750 -r cf93a1825d66 xen/arch/x86/irq.c
--- a/xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100
+++ b/xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100
@@ -39,11 +39,6 @@ boolean_param("irq-perdev-vector-map", o
u8 __read_mostly *irq_vector;
struct irq_desc __read_mostly *irq_desc = NULL;
-int __read_mostly *irq_status = NULL;
-#define IRQ_UNUSED (0)
-#define IRQ_USED (1)
-#define IRQ_RSVD (2)
-
#define IRQ_VECTOR_UNASSIGNED (0)
static DECLARE_BITMAP(used_vectors, NR_VECTORS);
@@ -117,7 +112,7 @@ static int __init __bind_irq_vector(int
ASSERT(!test_bit(vector, cfg->used_vectors));
set_bit(vector, cfg->used_vectors);
}
- irq_status[irq] = IRQ_USED;
+ cfg->used = IRQ_USED;
if (IO_APIC_IRQ(irq))
irq_vector[irq] = vector;
return 0;
@@ -139,7 +134,7 @@ static inline int find_unassigned_irq(vo
int irq;
for (irq = nr_irqs_gsi; irq < nr_irqs; irq++)
- if (irq_status[irq] == IRQ_UNUSED)
+ if (irq_cfg[irq].used == IRQ_UNUSED)
return irq;
return -ENOSPC;
}
@@ -191,8 +186,6 @@ static void dynamic_irq_cleanup(unsigned
xfree(action);
}
-static void init_one_irq_status(int irq);
-
static void __clear_irq_vector(int irq)
{
int cpu, vector;
@@ -211,7 +204,7 @@ static void __clear_irq_vector(int irq)
cfg->vector = IRQ_VECTOR_UNASSIGNED;
cpus_clear(cfg->cpu_mask);
- init_one_irq_status(irq);
+ cfg->used = IRQ_UNUSED;
if (likely(!cfg->move_in_progress))
return;
@@ -283,17 +276,13 @@ static void __init init_one_irq_desc(str
INIT_LIST_HEAD(&desc->rl_link);
}
-static void init_one_irq_status(int irq)
-{
- irq_status[irq] = IRQ_UNUSED;
-}
-
static void __init init_one_irq_cfg(struct irq_cfg *cfg)
{
cfg->vector = IRQ_VECTOR_UNASSIGNED;
cpus_clear(cfg->cpu_mask);
cpus_clear(cfg->old_cpu_mask);
cfg->used_vectors = NULL;
+ cfg->used = IRQ_UNUSED;
}
int __init init_irq_data(void)
@@ -307,15 +296,13 @@ int __init init_irq_data(void)
irq_desc = xmalloc_array(struct irq_desc, nr_irqs);
irq_cfg = xmalloc_array(struct irq_cfg, nr_irqs);
- irq_status = xmalloc_array(int, nr_irqs);
irq_vector = xmalloc_array(u8, nr_irqs_gsi);
- if ( !irq_desc || !irq_cfg || !irq_status ||! irq_vector )
+ if ( !irq_desc || !irq_cfg ||! irq_vector )
return -ENOMEM;
memset(irq_desc, 0, nr_irqs * sizeof(*irq_desc));
memset(irq_cfg, 0, nr_irqs * sizeof(*irq_cfg));
- memset(irq_status, 0, nr_irqs * sizeof(*irq_status));
memset(irq_vector, 0, nr_irqs_gsi * sizeof(*irq_vector));
for (irq = 0; irq < nr_irqs; irq++) {
@@ -325,7 +312,6 @@ int __init init_irq_data(void)
desc->chip_data = cfg;
init_one_irq_desc(desc);
init_one_irq_cfg(cfg);
- init_one_irq_status(irq);
}
/* Never allocate the hypercall vector or Linux/BSD fast-trap vector. */
@@ -444,7 +430,7 @@ next:
set_bit(vector, cfg->used_vectors);
}
- irq_status[irq] = IRQ_USED;
+ cfg->used = IRQ_USED;
if (IO_APIC_IRQ(irq))
irq_vector[irq] = vector;
err = 0;
diff -r 5a1826139750 -r cf93a1825d66 xen/include/asm-x86/irq.h
--- a/xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100
+++ b/xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100
@@ -34,8 +34,13 @@ struct irq_cfg {
unsigned move_cleanup_count;
vmask_t *used_vectors;
u8 move_in_progress : 1;
+ u8 used: 1;
};
+/* For use with irq_cfg.used */
+#define IRQ_UNUSED (0)
+#define IRQ_USED (1)
+
extern struct irq_cfg *irq_cfg;
typedef int vector_irq_t[NR_VECTORS];
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Andrew Cooper
2011-Sep-02 16:35 UTC
[Xen-devel] [PATCH 3 of 3] IRQ: Introduce old_vector to irq_cfg
Introduce old_vector to irq_cfg with the same principle as
old_cpu_mask. This removes a brute force loop from
__clear_irq_vector(), and paves the way to correct bitrotten logic
elsewhere in the irq code.
Signed-off-by Andrew Cooper <andrew.cooper3@citrix.com>
diff -r cf93a1825d66 -r 1a244d4ca6ac xen/arch/x86/io_apic.c
--- a/xen/arch/x86/io_apic.c Fri Sep 02 17:33:17 2011 +0100
+++ b/xen/arch/x86/io_apic.c Fri Sep 02 17:33:17 2011 +0100
@@ -487,11 +487,16 @@ fastcall void smp_irq_move_cleanup_inter
__get_cpu_var(vector_irq)[vector] = -1;
cfg->move_cleanup_count--;
- if ( cfg->move_cleanup_count == 0
- && cfg->used_vectors )
+ if ( cfg->move_cleanup_count == 0 )
{
- ASSERT(test_bit(vector, cfg->used_vectors));
- clear_bit(vector, cfg->used_vectors);
+ cfg->old_vector = -1;
+ cpus_clear(cfg->old_cpu_mask);
+
+ if ( cfg->used_vectors )
+ {
+ ASSERT(test_bit(vector, cfg->used_vectors));
+ clear_bit(vector, cfg->used_vectors);
+ }
}
unlock:
spin_unlock(&desc->lock);
diff -r cf93a1825d66 -r 1a244d4ca6ac xen/arch/x86/irq.c
--- a/xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100
+++ b/xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100
@@ -211,15 +211,9 @@ static void __clear_irq_vector(int irq)
cpus_and(tmp_mask, cfg->old_cpu_mask, cpu_online_map);
for_each_cpu_mask(cpu, tmp_mask) {
- for (vector = FIRST_DYNAMIC_VECTOR; vector <= LAST_DYNAMIC_VECTOR;
- vector++) {
- if (per_cpu(vector_irq, cpu)[vector] != irq)
- continue;
- TRACE_3D(TRC_HW_IRQ_MOVE_FINISH,
- irq, vector, cpu);
- per_cpu(vector_irq, cpu)[vector] = -1;
- break;
- }
+ ASSERT( per_cpu(vector_irq, cpu)[cfg->old_vector] == irq );
+ TRACE_3D(TRC_HW_IRQ_MOVE_FINISH, irq, vector, cpu);
+ per_cpu(vector_irq, cpu)[vector] = -1;
}
if ( cfg->used_vectors )
@@ -279,6 +273,7 @@ static void __init init_one_irq_desc(str
static void __init init_one_irq_cfg(struct irq_cfg *cfg)
{
cfg->vector = IRQ_VECTOR_UNASSIGNED;
+ cfg->old_vector = IRQ_VECTOR_UNASSIGNED;
cpus_clear(cfg->cpu_mask);
cpus_clear(cfg->old_cpu_mask);
cfg->used_vectors = NULL;
@@ -418,6 +413,7 @@ next:
if (old_vector) {
cfg->move_in_progress = 1;
cpus_copy(cfg->old_cpu_mask, cfg->cpu_mask);
+ cfg->old_vector = cfg->vector;
}
trace_irq_mask(TRC_HW_IRQ_ASSIGN_VECTOR, irq, vector, &tmp_mask);
for_each_cpu_mask(new_cpu, tmp_mask)
diff -r cf93a1825d66 -r 1a244d4ca6ac xen/include/asm-x86/irq.h
--- a/xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100
+++ b/xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100
@@ -28,7 +28,8 @@ typedef struct {
} vmask_t;
struct irq_cfg {
- int vector;
+ s16 vector; /* vector itself is only 8 bits, */
+ s16 old_vector; /* but we use -1 for unassigned */
cpumask_t cpu_mask;
cpumask_t old_cpu_mask;
unsigned move_cleanup_count;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
George Dunlap
2011-Sep-05 10:10 UTC
Re: [Xen-devel] [PATCH 1 of 3] IRQ: Remove bit-rotten code
Acked-by: George Dunlap <george.dunlap@eu.citrix.com> On Fri, Sep 2, 2011 at 5:35 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:> irq_desc.depth is a write only variable. > LEGACY_IRQ_FROM_VECTOR(vec) is never referenced. > > Signed-off-by Andrew Cooper <andrew.cooper3@citrix.com> > > diff -r 227130622561 -r 5a1826139750 xen/arch/x86/io_apic.c > --- a/xen/arch/x86/io_apic.c Thu Aug 25 12:03:14 2011 +0100 > +++ b/xen/arch/x86/io_apic.c Fri Sep 02 17:33:17 2011 +0100 > @@ -1955,7 +1955,6 @@ static void __init check_timer(void) > if ((ret = bind_irq_vector(0, vector, mask_all))) > printk(KERN_ERR"..IRQ0 is not set correctly with ioapic!!!, err:%d\n", ret); > > - irq_desc[0].depth = 0; > irq_desc[0].status &= ~IRQ_DISABLED; > > /* > diff -r 227130622561 -r 5a1826139750 xen/arch/x86/irq.c > --- a/xen/arch/x86/irq.c Thu Aug 25 12:03:14 2011 +0100 > +++ b/xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100 > @@ -178,7 +178,6 @@ static void dynamic_irq_cleanup(unsigned > desc->handler->shutdown(irq); > action = desc->action; > desc->action = NULL; > - desc->depth = 1; > desc->msi_desc = NULL; > desc->handler = &no_irq_type; > desc->chip_data->used_vectors=NULL; > @@ -278,7 +277,6 @@ static void __init init_one_irq_desc(str > desc->status = IRQ_DISABLED; > desc->handler = &no_irq_type; > desc->action = NULL; > - desc->depth = 1; > desc->msi_desc = NULL; > spin_lock_init(&desc->lock); > cpus_setall(desc->affinity); > @@ -736,7 +734,6 @@ void __init release_irq(unsigned int irq > spin_lock_irqsave(&desc->lock,flags); > action = desc->action; > desc->action = NULL; > - desc->depth = 1; > desc->status |= IRQ_DISABLED; > desc->handler->shutdown(irq); > spin_unlock_irqrestore(&desc->lock,flags); > @@ -764,7 +761,6 @@ int __init setup_irq(unsigned int irq, s > } > > desc->action = new; > - desc->depth = 0; > desc->status &= ~IRQ_DISABLED; > desc->handler->startup(irq); > > @@ -1343,7 +1339,6 @@ int pirq_guest_bind(struct vcpu *v, stru > cpus_clear(action->cpu_eoi_map); > init_timer(&action->eoi_timer, irq_guest_eoi_timer_fn, desc, 0); > > - desc->depth = 0; > desc->status |= IRQ_GUEST; > desc->status &= ~IRQ_DISABLED; > desc->handler->startup(irq); > @@ -1459,7 +1454,6 @@ static irq_guest_action_t *__pirq_guest_ > BUG_ON(action->in_flight != 0); > > /* Disabling IRQ before releasing the desc_lock avoids an IRQ storm. */ > - desc->depth = 1; > desc->status |= IRQ_DISABLED; > desc->handler->disable(irq); > > diff -r 227130622561 -r 5a1826139750 xen/include/asm-x86/irq.h > --- a/xen/include/asm-x86/irq.h Thu Aug 25 12:03:14 2011 +0100 > +++ b/xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100 > @@ -19,7 +19,6 @@ > #define MSI_IRQ(irq) ((irq) >= nr_irqs_gsi && (irq) < nr_irqs) > > #define LEGACY_VECTOR(irq) ((irq) + FIRST_LEGACY_VECTOR) > -#define LEGACY_IRQ_FROM_VECTOR(vec) ((vec) - FIRST_LEGACY_VECTOR) > > #define irq_to_desc(irq) (&irq_desc[irq]) > #define irq_cfg(irq) (&irq_cfg[irq]) > diff -r 227130622561 -r 5a1826139750 xen/include/xen/irq.h > --- a/xen/include/xen/irq.h Thu Aug 25 12:03:14 2011 +0100 > +++ b/xen/include/xen/irq.h Fri Sep 02 17:33:17 2011 +0100 > @@ -72,7 +72,6 @@ typedef struct irq_desc { > hw_irq_controller *handler; > struct msi_desc *msi_desc; > struct irqaction *action; /* IRQ action list */ > - unsigned int depth; /* nested irq disables */ > struct irq_cfg *chip_data; > int irq; > spinlock_t lock; > > _______________________________________________ > 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
George Dunlap
2011-Sep-05 10:14 UTC
Re: [Xen-devel] [PATCH 3 of 3] IRQ: Introduce old_vector to irq_cfg
Comments inline. On Fri, Sep 2, 2011 at 5:35 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:> Introduce old_vector to irq_cfg with the same principle as > old_cpu_mask. This removes a brute force loop from > __clear_irq_vector(), and paves the way to correct bitrotten logic > elsewhere in the irq code. > > Signed-off-by Andrew Cooper <andrew.cooper3@citrix.com> > > diff -r cf93a1825d66 -r 1a244d4ca6ac xen/arch/x86/io_apic.c > --- a/xen/arch/x86/io_apic.c Fri Sep 02 17:33:17 2011 +0100 > +++ b/xen/arch/x86/io_apic.c Fri Sep 02 17:33:17 2011 +0100 > @@ -487,11 +487,16 @@ fastcall void smp_irq_move_cleanup_inter > __get_cpu_var(vector_irq)[vector] = -1; > cfg->move_cleanup_count--; > > - if ( cfg->move_cleanup_count == 0 > - && cfg->used_vectors ) > + if ( cfg->move_cleanup_count == 0 ) > { > - ASSERT(test_bit(vector, cfg->used_vectors)); > - clear_bit(vector, cfg->used_vectors); > + cfg->old_vector = -1;Just for consistency, should this be IRQ_VECTOR_UNASSIGNED instead of -1?> + cpus_clear(cfg->old_cpu_mask); > + > + if ( cfg->used_vectors ) > + { > + ASSERT(test_bit(vector, cfg->used_vectors)); > + clear_bit(vector, cfg->used_vectors); > + } > } > unlock: > spin_unlock(&desc->lock); > diff -r cf93a1825d66 -r 1a244d4ca6ac xen/arch/x86/irq.c > --- a/xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100 > +++ b/xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100 > @@ -211,15 +211,9 @@ static void __clear_irq_vector(int irq) > > cpus_and(tmp_mask, cfg->old_cpu_mask, cpu_online_map); > for_each_cpu_mask(cpu, tmp_mask) { > - for (vector = FIRST_DYNAMIC_VECTOR; vector <= LAST_DYNAMIC_VECTOR; > - vector++) { > - if (per_cpu(vector_irq, cpu)[vector] != irq) > - continue; > - TRACE_3D(TRC_HW_IRQ_MOVE_FINISH, > - irq, vector, cpu); > - per_cpu(vector_irq, cpu)[vector] = -1; > - break; > - } > + ASSERT( per_cpu(vector_irq, cpu)[cfg->old_vector] == irq ); > + TRACE_3D(TRC_HW_IRQ_MOVE_FINISH, irq, vector, cpu); > + per_cpu(vector_irq, cpu)[vector] = -1;Do you mean cfg->old_vector here, instead of vector?> } > > if ( cfg->used_vectors ) > @@ -279,6 +273,7 @@ static void __init init_one_irq_desc(str > static void __init init_one_irq_cfg(struct irq_cfg *cfg) > { > cfg->vector = IRQ_VECTOR_UNASSIGNED; > + cfg->old_vector = IRQ_VECTOR_UNASSIGNED; > cpus_clear(cfg->cpu_mask); > cpus_clear(cfg->old_cpu_mask); > cfg->used_vectors = NULL; > @@ -418,6 +413,7 @@ next: > if (old_vector) { > cfg->move_in_progress = 1; > cpus_copy(cfg->old_cpu_mask, cfg->cpu_mask); > + cfg->old_vector = cfg->vector; > } > trace_irq_mask(TRC_HW_IRQ_ASSIGN_VECTOR, irq, vector, &tmp_mask); > for_each_cpu_mask(new_cpu, tmp_mask) > diff -r cf93a1825d66 -r 1a244d4ca6ac xen/include/asm-x86/irq.h > --- a/xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100 > +++ b/xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100 > @@ -28,7 +28,8 @@ typedef struct { > } vmask_t; > > struct irq_cfg { > - int vector; > + s16 vector; /* vector itself is only 8 bits, */ > + s16 old_vector; /* but we use -1 for unassigned */ > cpumask_t cpu_mask; > cpumask_t old_cpu_mask; > unsigned move_cleanup_count; > > _______________________________________________ > 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
Andrew Cooper
2011-Sep-05 11:43 UTC
Re: [Xen-devel] [PATCH 3 of 3] IRQ: Introduce old_vector to irq_cfg
On 05/09/11 11:14, George Dunlap wrote:> Comments inline. > > On Fri, Sep 2, 2011 at 5:35 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> Introduce old_vector to irq_cfg with the same principle as >> old_cpu_mask. This removes a brute force loop from >> __clear_irq_vector(), and paves the way to correct bitrotten logic >> elsewhere in the irq code. >> >> Signed-off-by Andrew Cooper <andrew.cooper3@citrix.com> >> >> diff -r cf93a1825d66 -r 1a244d4ca6ac xen/arch/x86/io_apic.c >> --- a/xen/arch/x86/io_apic.c Fri Sep 02 17:33:17 2011 +0100 >> +++ b/xen/arch/x86/io_apic.c Fri Sep 02 17:33:17 2011 +0100 >> @@ -487,11 +487,16 @@ fastcall void smp_irq_move_cleanup_inter >> __get_cpu_var(vector_irq)[vector] = -1; >> cfg->move_cleanup_count--; >> >> - if ( cfg->move_cleanup_count == 0 >> - && cfg->used_vectors ) >> + if ( cfg->move_cleanup_count == 0 ) >> { >> - ASSERT(test_bit(vector, cfg->used_vectors)); >> - clear_bit(vector, cfg->used_vectors); >> + cfg->old_vector = -1; > Just for consistency, should this be IRQ_VECTOR_UNASSIGNED instead of -1?Yes - I missed that. However, IRQ_VECTOR_UNASSIGNED should be -1 instead of 0, as the first 32 entries of irq_vector have 0 entries which are not unassigned.>> + cpus_clear(cfg->old_cpu_mask); >> + >> + if ( cfg->used_vectors ) >> + { >> + ASSERT(test_bit(vector, cfg->used_vectors)); >> + clear_bit(vector, cfg->used_vectors); >> + } >> } >> unlock: >> spin_unlock(&desc->lock); >> diff -r cf93a1825d66 -r 1a244d4ca6ac xen/arch/x86/irq.c >> --- a/xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100 >> +++ b/xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100 >> @@ -211,15 +211,9 @@ static void __clear_irq_vector(int irq) >> >> cpus_and(tmp_mask, cfg->old_cpu_mask, cpu_online_map); >> for_each_cpu_mask(cpu, tmp_mask) { >> - for (vector = FIRST_DYNAMIC_VECTOR; vector <= LAST_DYNAMIC_VECTOR; >> - vector++) { >> - if (per_cpu(vector_irq, cpu)[vector] != irq) >> - continue; >> - TRACE_3D(TRC_HW_IRQ_MOVE_FINISH, >> - irq, vector, cpu); >> - per_cpu(vector_irq, cpu)[vector] = -1; >> - break; >> - } >> + ASSERT( per_cpu(vector_irq, cpu)[cfg->old_vector] == irq ); >> + TRACE_3D(TRC_HW_IRQ_MOVE_FINISH, irq, vector, cpu); >> + per_cpu(vector_irq, cpu)[vector] = -1; > Do you mean cfg->old_vector here, instead of vector?No - the TRACE_3D and per_cpu lines are only diffs because of the change in whitespace when removing the loop (and this is the code which should actually remove the vector mapping). You are correct however that cfg->old_vector should be set to IRQ_VECTOR_UNASSIGNED at the end of the for_each for consistency. (In reality, you cant get to this bit of code without having a valid cfg->old_vector)>> } >> >> if ( cfg->used_vectors ) >> @@ -279,6 +273,7 @@ static void __init init_one_irq_desc(str >> static void __init init_one_irq_cfg(struct irq_cfg *cfg) >> { >> cfg->vector = IRQ_VECTOR_UNASSIGNED; >> + cfg->old_vector = IRQ_VECTOR_UNASSIGNED; >> cpus_clear(cfg->cpu_mask); >> cpus_clear(cfg->old_cpu_mask); >> cfg->used_vectors = NULL; >> @@ -418,6 +413,7 @@ next: >> if (old_vector) { >> cfg->move_in_progress = 1; >> cpus_copy(cfg->old_cpu_mask, cfg->cpu_mask); >> + cfg->old_vector = cfg->vector; >> } >> trace_irq_mask(TRC_HW_IRQ_ASSIGN_VECTOR, irq, vector, &tmp_mask); >> for_each_cpu_mask(new_cpu, tmp_mask) >> diff -r cf93a1825d66 -r 1a244d4ca6ac xen/include/asm-x86/irq.h >> --- a/xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100 >> +++ b/xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100 >> @@ -28,7 +28,8 @@ typedef struct { >> } vmask_t; >> >> struct irq_cfg { >> - int vector; >> + s16 vector; /* vector itself is only 8 bits, */ >> + s16 old_vector; /* but we use -1 for unassigned */ >> cpumask_t cpu_mask; >> cpumask_t old_cpu_mask; >> unsigned move_cleanup_count; >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel >>-- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George Dunlap
2011-Sep-05 11:45 UTC
Re: [Xen-devel] [PATCH 3 of 3] IRQ: Introduce old_vector to irq_cfg
On Mon, 2011-09-05 at 12:43 +0100, Andrew Cooper wrote:> >> diff -r cf93a1825d66 -r 1a244d4ca6ac xen/arch/x86/irq.c > >> --- a/xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100 > >> +++ b/xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100 > >> @@ -211,15 +211,9 @@ static void __clear_irq_vector(int irq) > >> > >> cpus_and(tmp_mask, cfg->old_cpu_mask, cpu_online_map); > >> for_each_cpu_mask(cpu, tmp_mask) { > >> - for (vector = FIRST_DYNAMIC_VECTOR; vector <= LAST_DYNAMIC_VECTOR; > >> - vector++) { > >> - if (per_cpu(vector_irq, cpu)[vector] != irq) > >> - continue; > >> - TRACE_3D(TRC_HW_IRQ_MOVE_FINISH, > >> - irq, vector, cpu); > >> - per_cpu(vector_irq, cpu)[vector] = -1; > >> - break; > >> - } > >> + ASSERT( per_cpu(vector_irq, cpu)[cfg->old_vector] == irq ); > >> + TRACE_3D(TRC_HW_IRQ_MOVE_FINISH, irq, vector, cpu); > >> + per_cpu(vector_irq, cpu)[vector] = -1; > > Do you mean cfg->old_vector here, instead of vector? > > No - the TRACE_3D and per_cpu lines are only diffs because of the change > in whitespace when removing the loop (and this is the code which should > actually remove the vector mapping). You are correct however that > cfg->old_vector should be set to IRQ_VECTOR_UNASSIGNED at the end of the > for_each for consistency. (In reality, you cant get to this bit of code > without having a valid cfg->old_vector)But you''re also removing the for loop, which sets vector. (I.e., there''s some bad coding in the original code, where the variable "vector" means different things in different parts of the function.) Before the patch, vector in that line will be any vector between FIRST_DYNAMIC_VECTOR and LAST_DYNAMIC_VECTOR s.t. per_cpu(vector_irq, cpu)[vector] == irq. After the patch, vector at that line will be equal to cfg->vector (set above). Since we''re looking through the cpus in cfg->old_cpu_mask, I would think that we would be clearing cfg->old_vector, would we not? In any case, it''s certain that the ASSERT() should be checking the same thing as the clearing line; i.e., either ASSERT(...[vector]==irq) and then set ...[vector]=-1, or ASSERT(...[cfg->old_vector]==irq) and then set ...[cfg->old_vector]=-1. -George> > >> } > >> > >> if ( cfg->used_vectors ) > >> @@ -279,6 +273,7 @@ static void __init init_one_irq_desc(str > >> static void __init init_one_irq_cfg(struct irq_cfg *cfg) > >> { > >> cfg->vector = IRQ_VECTOR_UNASSIGNED; > >> + cfg->old_vector = IRQ_VECTOR_UNASSIGNED; > >> cpus_clear(cfg->cpu_mask); > >> cpus_clear(cfg->old_cpu_mask); > >> cfg->used_vectors = NULL; > >> @@ -418,6 +413,7 @@ next: > >> if (old_vector) { > >> cfg->move_in_progress = 1; > >> cpus_copy(cfg->old_cpu_mask, cfg->cpu_mask); > >> + cfg->old_vector = cfg->vector; > >> } > >> trace_irq_mask(TRC_HW_IRQ_ASSIGN_VECTOR, irq, vector, &tmp_mask); > >> for_each_cpu_mask(new_cpu, tmp_mask) > >> diff -r cf93a1825d66 -r 1a244d4ca6ac xen/include/asm-x86/irq.h > >> --- a/xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100 > >> +++ b/xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100 > >> @@ -28,7 +28,8 @@ typedef struct { > >> } vmask_t; > >> > >> struct irq_cfg { > >> - int vector; > >> + s16 vector; /* vector itself is only 8 bits, */ > >> + s16 old_vector; /* but we use -1 for unassigned */ > >> cpumask_t cpu_mask; > >> cpumask_t old_cpu_mask; > >> unsigned move_cleanup_count; > >> > >> _______________________________________________ > >> 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
Andrew Cooper
2011-Sep-05 13:17 UTC
[Xen-devel] [PATCH 3 of 3] IRQ: Introduce old_vector to irq_cfg v2
IRQ: Introduce old_vector to irq_cfg v2 Introduce old_vector to irq_cfg with the same principle as old_cpu_mask. This removes a brute force loop from __clear_irq_vector(), and paves the way to correct bitrotten logic elsewhere in the irq code. Changes: * Make use of IRQ_VECTOR_UNASSIGNED instead of -1 for consistency * Correct logic in __clear_irq_vector(): should clean up cfg->old_vector rather than cfg->vector which has already been cleaned up Signed-off-by Andrew Cooper <andrew.cooper3@citrix.com> -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel