Skip to contentSkip to conversation

IRQ: Part 1 of the irq code cleanup

PATCH New Andrew Cooper 4 participants 9 messages Sep 2, 2011 – Sep 5, 2011

lore
Share & tools
lore

Andrew Cooper

author
#0 lore raw

PATCH 1/1 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 <[email protected]>

author
#1 lore raw

PATCH 1/1 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 <[email protected]>

xen/arch/x86/io_apic.c +0 -1
diff -r 227130622561 -r 5a1826139750 xen/arch/x86/io_apic.c
@@ -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;
 
     /*
xen/arch/x86/irq.c +0 -6
diff -r 227130622561 -r 5a1826139750 xen/arch/x86/irq.c
@@ -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);
xen/include/asm-x86/irq.h +0 -1
diff -r 227130622561 -r 5a1826139750 xen/include/asm-x86/irq.h
@@ -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])
xen/include/xen/irq.h +0 -1
diff -r 227130622561 -r 5a1826139750 xen/include/xen/irq.h
@@ -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;
Acked-by on v1 1/1
#2 lore raw

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

Acked-by George Dunlap <[email protected]>
Quoted 95 lines · Andrew Cooper

irq_desc.depth is a write only variable.
LEGACY_IRQ_FROM_VECTOR(vec) is never referenced.

Signed-off-by Andrew Cooper <[email protected]>

diff -r 227130622561 -r 5a1826139750 xen/arch/x86/io_apic.c

xen/arch/x86/io_apic.c    Fri Sep 02 17:33:17 2011 +0100 renamed from xen/arch/x86/io_apic.c    Thu Aug 25 12:03:14 2011 +0100 +0 -1
@@ -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

xen/arch/x86/irq.c        Fri Sep 02 17:33:17 2011 +0100 renamed from xen/arch/x86/irq.c        Thu Aug 25 12:03:14 2011 +0100 +0 -6
@@ -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

xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100 renamed from xen/include/asm-x86/irq.h Thu Aug 25 12:03:14 2011 +0100 +0 -1
@@ -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

xen/include/xen/irq.h     Fri Sep 02 17:33:17 2011 +0100 renamed from xen/include/xen/irq.h     Thu Aug 25 12:03:14 2011 +0100 +0 -0
@@ -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
[email protected]
http://lists.xensource.com/xen-devel

author
#3 lore raw

PATCH 1/1 IRQ: Fold irq_status into irq_cfg

In reply to 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 <[email protected]>

xen/arch/x86/irq.c +6 -20
diff -r 5a1826139750 -r cf93a1825d66 xen/arch/x86/irq.c
@@ -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;
xen/include/asm-x86/irq.h +5 -0
diff -r 5a1826139750 -r cf93a1825d66 xen/include/asm-x86/irq.h
@@ -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];
author
#4 lore raw

PATCH 1/1 IRQ: Introduce old_vector to irq_cfg

In reply to 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 <[email protected]>

xen/arch/x86/io_apic.c +9 -4
diff -r cf93a1825d66 -r 1a244d4ca6ac xen/arch/x86/io_apic.c
@@ -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);
xen/arch/x86/irq.c +5 -9
diff -r cf93a1825d66 -r 1a244d4ca6ac xen/arch/x86/irq.c
@@ -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)
xen/include/asm-x86/irq.h +2 -1
diff -r cf93a1825d66 -r 1a244d4ca6ac xen/include/asm-x86/irq.h
@@ -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;
#5 lore raw

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

Comments inline.

Quoted 11 lines · 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 <[email protected]>

diff -r cf93a1825d66 -r 1a244d4ca6ac xen/arch/x86/io_apic.c

xen/arch/x86/io_apic.c    Fri Sep 02 17:33:17 2011 +0100 fastcall void smp_irq_move_cleanup_inter · line 492

Quoted 10 lines
xen/arch/x86/io_apic.c    Fri Sep 02 17:33:17 2011 +0100 +2 -4
@@ -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?

Quoted 19 lines
xen/arch/x86/io_apic.c    Fri Sep 02 17:33:17 2011 +0100 +7 -0
+            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

xen/arch/x86/irq.c        Fri Sep 02 17:33:17 2011 +0100 +0 -2
@@ -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++) {

xen/arch/x86/irq.c        Fri Sep 02 17:33:17 2011 +0100 static void __clear_irq_vector(int irq) · line 216

Quoted 10 lines
xen/arch/x86/irq.c        Fri Sep 02 17:33:17 2011 +0100 +3 -7
-            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?

Quoted 38 lines
xen/arch/x86/irq.c        Fri Sep 02 17:33:17 2011 +0100 +2 -0
      }
 
     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

xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100 +2 -1
@@ -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
[email protected]
http://lists.xensource.com/xen-devel

author
#6 lore raw

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

Quoted 17 lines · George Dunlap

Comments inline.

Quoted 14 lines · 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 <[email protected]>

diff -r cf93a1825d66 -r 1a244d4ca6ac xen/arch/x86/io_apic.c

xen/arch/x86/io_apic.c Fri Sep 02 17:33:17 2011 +0100 +0 -0
@@ -487,11 +487,16 @@ fastcall void smp_irq_move_cleanup_inter
        __get_cpu_var(vector_irq)[vector] = -1;
        cfg->move_cleanup_count--;
Quoted 8 lines

xen/arch/x86/io_apic.c Fri Sep 02 17:33:17 2011 +0100 fastcall void smp_irq_move_cleanup_inter · line 492

Quoted 7 lines
xen/arch/x86/io_apic.c Fri Sep 02 17:33:17 2011 +0100 +2 -4
-        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.

Quoted 15 lines
Quoted 15 lines
xen/arch/x86/io_apic.c Fri Sep 02 17:33:17 2011 +0100 +7 -0
+            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

xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100 +0 -0
@@ -211,15 +211,9 @@ static void __clear_irq_vector(int irq)
Quoted 15 lines
Quoted 4 lines
xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100 +0 -2
    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++) {

xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100 static void __clear_irq_vector(int irq) · line 216

Quoted 10 lines
xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100 +3 -7
-            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)

Quoted 38 lines
Quoted 38 lines
xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100 +2 -0
     }
 
    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

xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100 +2 -1
@@ -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
[email protected]
http://lists.xensource.com/xen-devel

Signature · 2 lines
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com
#7 lore raw

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

Quoted 21 lines · Andrew Cooper
Quoted 5 lines
Quoted 5 lines

diff -r cf93a1825d66 -r 1a244d4ca6ac xen/arch/x86/irq.c

xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100 +0 -0
@@ -211,15 +211,9 @@ static void __clear_irq_vector(int irq)
Quoted 15 lines
Quoted 4 lines
xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100 +0 -2
    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++) {

xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100 static void __clear_irq_vector(int irq) · line 216

Quoted 10 lines
xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100 +3 -7
-            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?

Quoted 6 lines

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

Quoted 40 lines
Quoted 39 lines
Quoted 39 lines
xen/arch/x86/irq.c Fri Sep 02 17:33:17 2011 +0100 +2 -0
     }
 
    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

xen/include/asm-x86/irq.h Fri Sep 02 17:33:17 2011 +0100 +2 -1
@@ -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
[email protected]
http://lists.xensource.com/xen-devel

author
#8 lore raw

PATCH 1/1 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 <[email protected]>

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

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 <[email protected]>

xen/arch/x86/io_apic.c +9 -4
diff -r cf93a1825d66 xen/arch/x86/io_apic.c
@@ -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);
xen/arch/x86/irq.c +7 -11
diff -r cf93a1825d66 xen/arch/x86/irq.c
@@ -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)
xen/include/asm-x86/irq.h +4 -1
diff -r cf93a1825d66 xen/include/asm-x86/irq.h
@@ -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];

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel