| Message ID | 5CD2D38D020000780022CD00@prv1-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: IRQ management adjustments | expand |
On Wed, May 08, 2019 at 07:03:09AM -0600, 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. > > 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> Thanks, Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> One comment below. > --- > v2: Add/use valid_irq_vector(). > v1b: Also update vector_irq[] in the code added to fixup_irqs(). > > --- a/xen/arch/x86/irq.c > +++ b/xen/arch/x86/irq.c > @@ -99,6 +99,11 @@ 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 trace_irq_mask(u32 event, int irq, int vector, cpumask_t *mask) > { > struct { > @@ -242,6 +247,22 @@ void destroy_irq(unsigned int irq) > xfree(action); > } > > +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 __clear_irq_vector(int irq) > { > int cpu, vector, old_vector; > @@ -285,14 +306,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; > } > @@ -517,12 +531,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 ) Maybe you could use valid_irq_vector here, or compare against IRQ_VECTOR_UNASSIGNED? The fact that IRQ_VECTOR_UNASSIGNED is a negative value is an implementation detail that shouldn't be exposed directly in the code IMO. Roger.
>>> On 13.05.19 at 11:04, <roger.pau@citrix.com> wrote: > On Wed, May 08, 2019 at 07:03:09AM -0600, 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. >> >> 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> > > Thanks, > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. >> @@ -517,12 +531,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 ) > > Maybe you could use valid_irq_vector here, or compare against > IRQ_VECTOR_UNASSIGNED? Not in this patch, but I'd like to widen the use of valid_irq_vector() subsequently, which would likely also include this case. Jan
--- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -99,6 +99,11 @@ 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 trace_irq_mask(u32 event, int irq, int vector, cpumask_t *mask) { struct { @@ -242,6 +247,22 @@ void destroy_irq(unsigned int irq) xfree(action); } +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 __clear_irq_vector(int irq) { int cpu, vector, old_vector; @@ -285,14 +306,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; } @@ -517,12 +531,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; @@ -691,14 +714,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); @@ -2391,6 +2408,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) ) { @@ -2409,15 +2453,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. */
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> --- v2: Add/use valid_irq_vector(). v1b: Also update vector_irq[] in the code added to fixup_irqs().