[PATCH 0 of 3] IRQ: Part 1 of the irq code cleanup

[PATCH 0 of 3] IRQ: Part 1 of the irq code cleanup

[Andrew Cooper]

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>

[PATCH 1 of 3] IRQ: Remove bit-rotten code

[Andrew Cooper]

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;

[PATCH 2 of 3] IRQ: Fold irq_status into irq_cfg

[Andrew Cooper]

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];

[PATCH 3 of 3] IRQ: Introduce old_vector to irq_cfg

[Andrew Cooper]

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;

Re: [PATCH 1 of 3] IRQ: Remove bit-rotten code

[George Dunlap]

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
>

Re: [PATCH 3 of 3] IRQ: Introduce old_vector to irq_cfg

[George Dunlap]

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
>

Re: [PATCH 3 of 3] IRQ: Introduce old_vector to irq_cfg

[Andrew Cooper]

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

Re: [PATCH 3 of 3] IRQ: Introduce old_vector to irq_cfg

[George Dunlap]

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
> >>
> 

[PATCH 3 of 3] IRQ: Introduce old_vector to irq_cfg v2

[Andrew Cooper]

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

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


[-- Attachment #2: irq-introduce-old_vector-to-irq_cfg.patch --]
[-- Type: text/x-patch, Size: 4189 bytes --]

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>

diff -r cf93a1825d66 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	Mon Sep 05 14:12:59 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 = IRQ_VECTOR_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 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	Mon Sep 05 14:12:59 2011 +0100
@@ -39,8 +39,6 @@ boolean_param("irq-perdev-vector-map", o
 u8 __read_mostly *irq_vector;
 struct irq_desc __read_mostly *irq_desc = NULL;
 
-#define IRQ_VECTOR_UNASSIGNED (0)
-
 static DECLARE_BITMAP(used_vectors, NR_VECTORS);
 
 struct irq_cfg __read_mostly *irq_cfg = NULL;
@@ -211,15 +209,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)[cfg->old_vector] = -1;
      }
 
     if ( cfg->used_vectors )
@@ -229,6 +221,8 @@ static void __clear_irq_vector(int irq)
     }
 
     cfg->move_in_progress = 0;
+    cfg->old_vector = IRQ_VECTOR_UNASSIGNED;
+    cpus_clear(cfg->old_cpu_mask);
 }
 
 void clear_irq_vector(int irq)
@@ -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 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	Mon Sep 05 14:12:59 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;
@@ -41,6 +42,8 @@ struct irq_cfg {
 #define IRQ_UNUSED      (0)
 #define IRQ_USED        (1)
 
+#define IRQ_VECTOR_UNASSIGNED (-1)
+
 extern struct irq_cfg *irq_cfg;
 
 typedef int vector_irq_t[NR_VECTORS];

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

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