diff mbox series

[v3,01/15] x86/IRQ: deal with move-in-progress state in fixup_irqs()

Message ID 5CDE90950200007800230069@prv1-mh.provo.novell.com (mailing list archive)
State New, archived
Headers show
Series x86: IRQ management adjustments | expand

Commit Message

Jan Beulich May 17, 2019, 10:44 a.m. UTC 
The flag being set may prevent affinity changes, as these often imply
assignment of a new vector. When there's no possible destination left
for the IRQ, the clearing of the flag needs to happen right from
fixup_irqs().

Additionally _assign_irq_vector() needs to avoid setting the flag when
there's no online CPU left in what gets put into ->arch.old_cpu_mask.
The old vector can be released right away in this case.

Also extend the log message about broken affinity to include the new
affinity as well, allowing to notice issues with affinity changes not
actually having taken place. Swap the if/else-if order there at the
same time to reduce the amount of conditions checked.

At the same time replace two open coded instances of the new helper
function.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
v3: Move release_old_vec() further up (so a later patch won't need to).
    Re-base.
v2: Add/use valid_irq_vector().
v1b: Also update vector_irq[] in the code added to fixup_irqs().

Comments

Andrew Cooper July 3, 2019, 3:39 p.m. UTC | #1 
On 17/05/2019 11:44, Jan Beulich wrote:
> The flag being set may prevent affinity changes, as these often imply
> assignment of a new vector. When there's no possible destination left
> for the IRQ, the clearing of the flag needs to happen right from
> fixup_irqs().
>
> Additionally _assign_irq_vector() needs to avoid setting the flag when
> there's no online CPU left in what gets put into ->arch.old_cpu_mask.
> The old vector can be released right away in this case.

This suggests that it is a bugfix, but it isn't clear what happens when
things go wrong.

> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2418,15 +2462,18 @@ void fixup_irqs(const cpumask_t *mask, b
>          if ( desc->handler->enable )
>              desc->handler->enable(desc);
>  
> +        cpumask_copy(&affinity, desc->affinity);
> +
>          spin_unlock(&desc->lock);
>  
>          if ( !verbose )
>              continue;
>  
> -        if ( break_affinity && set_affinity )
> -            printk("Broke affinity for irq %i\n", irq);
> -        else if ( !set_affinity )
> -            printk("Cannot set affinity for irq %i\n", irq);
> +        if ( !set_affinity )
> +            printk("Cannot set affinity for IRQ%u\n", irq);
> +        else if ( break_affinity )
> +            printk("Broke affinity for IRQ%u, new: %*pb\n",
> +                   irq, nr_cpu_ids, &affinity);

While I certainly prefer this version, I should point out that you
refused to accept my patches like this, and for consistency with the
rest of the codebase, you should be using cpumask_bits().

~Andrew
Jan Beulich July 4, 2019, 9:32 a.m. UTC | #2 
On 03.07.2019 17:39, Andrew Cooper wrote:
> On 17/05/2019 11:44, Jan Beulich wrote:
>> The flag being set may prevent affinity changes, as these often imply
>> assignment of a new vector. When there's no possible destination left
>> for the IRQ, the clearing of the flag needs to happen right from
>> fixup_irqs().
>>
>> Additionally _assign_irq_vector() needs to avoid setting the flag when
>> there's no online CPU left in what gets put into ->arch.old_cpu_mask.
>> The old vector can be released right away in this case.
> 
> This suggests that it is a bugfix, but it isn't clear what happens when
> things go wrong.

The vector cleanup wouldn't ever trigger, as the IRQ wouldn't get
raised anymore to any of its prior target CPUs. Hence the immediate
cleanup that gets done in that case. I thought the 2nd sentence
would make this clear. If it doesn't, do you have a suggestion on
how to improve the text?

>> --- a/xen/arch/x86/irq.c
>> +++ b/xen/arch/x86/irq.c
>> @@ -2418,15 +2462,18 @@ void fixup_irqs(const cpumask_t *mask, b
>>           if ( desc->handler->enable )
>>               desc->handler->enable(desc);
>>   
>> +        cpumask_copy(&affinity, desc->affinity);
>> +
>>           spin_unlock(&desc->lock);
>>   
>>           if ( !verbose )
>>               continue;
>>   
>> -        if ( break_affinity && set_affinity )
>> -            printk("Broke affinity for irq %i\n", irq);
>> -        else if ( !set_affinity )
>> -            printk("Cannot set affinity for irq %i\n", irq);
>> +        if ( !set_affinity )
>> +            printk("Cannot set affinity for IRQ%u\n", irq);
>> +        else if ( break_affinity )
>> +            printk("Broke affinity for IRQ%u, new: %*pb\n",
>> +                   irq, nr_cpu_ids, &affinity);
> 
> While I certainly prefer this version, I should point out that you
> refused to accept my patches like this, and for consistency with the
> rest of the codebase, you should be using cpumask_bits().

Oh, indeed. I guess I had converted a debugging only printk() into
this one without noticing the necessary tidying, the more that
elsewhere in the series I'm actually doing so already.

Jan
diff mbox series

Patch

--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -99,6 +99,27 @@  void unlock_vector_lock(void)
     spin_unlock(&vector_lock);
 }
 
+static inline bool valid_irq_vector(unsigned int vector)
+{
+    return vector >= FIRST_DYNAMIC_VECTOR && vector <= LAST_HIPRIORITY_VECTOR;
+}
+
+static void release_old_vec(struct irq_desc *desc)
+{
+    unsigned int vector = desc->arch.old_vector;
+
+    desc->arch.old_vector = IRQ_VECTOR_UNASSIGNED;
+    cpumask_clear(desc->arch.old_cpu_mask);
+
+    if ( !valid_irq_vector(vector) )
+        ASSERT_UNREACHABLE();
+    else if ( desc->arch.used_vectors )
+    {
+        ASSERT(test_bit(vector, desc->arch.used_vectors));
+        clear_bit(vector, desc->arch.used_vectors);
+    }
+}
+
 static void trace_irq_mask(uint32_t event, int irq, int vector,
                            const cpumask_t *mask)
 {
@@ -288,14 +309,7 @@  static void __clear_irq_vector(int irq)
         per_cpu(vector_irq, cpu)[old_vector] = ~irq;
     }
 
-    desc->arch.old_vector = IRQ_VECTOR_UNASSIGNED;
-    cpumask_clear(desc->arch.old_cpu_mask);
-
-    if ( desc->arch.used_vectors )
-    {
-        ASSERT(test_bit(old_vector, desc->arch.used_vectors));
-        clear_bit(old_vector, desc->arch.used_vectors);
-    }
+    release_old_vec(desc);
 
     desc->arch.move_in_progress = 0;
 }
@@ -520,12 +534,21 @@  next:
         /* Found one! */
         current_vector = vector;
         current_offset = offset;
-        if (old_vector > 0) {
-            desc->arch.move_in_progress = 1;
-            cpumask_copy(desc->arch.old_cpu_mask, desc->arch.cpu_mask);
+
+        if ( old_vector > 0 )
+        {
+            cpumask_and(desc->arch.old_cpu_mask, desc->arch.cpu_mask,
+                        &cpu_online_map);
             desc->arch.old_vector = desc->arch.vector;
+            if ( !cpumask_empty(desc->arch.old_cpu_mask) )
+                desc->arch.move_in_progress = 1;
+            else
+                /* This can happen while offlining a CPU. */
+                release_old_vec(desc);
         }
+
         trace_irq_mask(TRC_HW_IRQ_ASSIGN_VECTOR, irq, vector, &tmp_mask);
+
         for_each_cpu(new_cpu, &tmp_mask)
             per_cpu(vector_irq, new_cpu)[vector] = irq;
         desc->arch.vector = vector;
@@ -694,14 +717,8 @@  void irq_move_cleanup_interrupt(struct c
 
         if ( desc->arch.move_cleanup_count == 0 )
         {
-            desc->arch.old_vector = IRQ_VECTOR_UNASSIGNED;
-            cpumask_clear(desc->arch.old_cpu_mask);
-
-            if ( desc->arch.used_vectors )
-            {
-                ASSERT(test_bit(vector, desc->arch.used_vectors));
-                clear_bit(vector, desc->arch.used_vectors);
-            }
+            ASSERT(vector == desc->arch.old_vector);
+            release_old_vec(desc);
         }
 unlock:
         spin_unlock(&desc->lock);
@@ -2400,6 +2417,33 @@  void fixup_irqs(const cpumask_t *mask, b
             continue;
         }
 
+        /*
+         * In order for the affinity adjustment below to be successful, we
+         * need __assign_irq_vector() to succeed. This in particular means
+         * clearing desc->arch.move_in_progress if this would otherwise
+         * prevent the function from succeeding. Since there's no way for the
+         * flag to get cleared anymore when there's no possible destination
+         * left (the only possibility then would be the IRQs enabled window
+         * after this loop), there's then also no race with us doing it here.
+         *
+         * Therefore the logic here and there need to remain in sync.
+         */
+        if ( desc->arch.move_in_progress &&
+             !cpumask_intersects(mask, desc->arch.cpu_mask) )
+        {
+            unsigned int cpu;
+
+            cpumask_and(&affinity, desc->arch.old_cpu_mask, &cpu_online_map);
+
+            spin_lock(&vector_lock);
+            for_each_cpu(cpu, &affinity)
+                per_cpu(vector_irq, cpu)[desc->arch.old_vector] = ~irq;
+            spin_unlock(&vector_lock);
+
+            release_old_vec(desc);
+            desc->arch.move_in_progress = 0;
+        }
+
         cpumask_and(&affinity, &affinity, mask);
         if ( cpumask_empty(&affinity) )
         {
@@ -2418,15 +2462,18 @@  void fixup_irqs(const cpumask_t *mask, b
         if ( desc->handler->enable )
             desc->handler->enable(desc);
 
+        cpumask_copy(&affinity, desc->affinity);
+
         spin_unlock(&desc->lock);
 
         if ( !verbose )
             continue;
 
-        if ( break_affinity && set_affinity )
-            printk("Broke affinity for irq %i\n", irq);
-        else if ( !set_affinity )
-            printk("Cannot set affinity for irq %i\n", irq);
+        if ( !set_affinity )
+            printk("Cannot set affinity for IRQ%u\n", irq);
+        else if ( break_affinity )
+            printk("Broke affinity for IRQ%u, new: %*pb\n",
+                   irq, nr_cpu_ids, &affinity);
     }
 
     /* That doesn't seem sufficient.  Give it 1ms. */