| Message ID | 5CDE917502000078002300A8@prv1-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: IRQ management adjustments | expand |
On Fri, May 17, 2019 at 04:48:21AM -0600, Jan Beulich wrote: > fixup_irqs() skips interrupts without action. Hence such interrupts can > retain affinity to just offline CPUs. With "noirqbalance" in effect, > pirq_guest_bind() so far would have left them alone, resulting in a non- > working interrupt. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v3: New. > --- > I've not observed this problem in practice - the change is just the > result of code inspection after having noticed action-less IRQs in 'i' > debug key output pointing at all parked/offline CPUs. > > --- a/xen/arch/x86/irq.c > +++ b/xen/arch/x86/irq.c > @@ -1683,9 +1683,27 @@ int pirq_guest_bind(struct vcpu *v, stru > > desc->status |= IRQ_GUEST; > > - /* Attempt to bind the interrupt target to the correct CPU. */ > - if ( !opt_noirqbalance && (desc->handler->set_affinity != NULL) ) > - desc->handler->set_affinity(desc, cpumask_of(v->processor)); > + /* > + * Attempt to bind the interrupt target to the correct (or at least > + * some online) CPU. > + */ > + if ( desc->handler->set_affinity ) > + { > + const cpumask_t *affinity = NULL; > + > + if ( !opt_noirqbalance ) > + affinity = cpumask_of(v->processor); > + else if ( !cpumask_intersects(desc->affinity, &cpu_online_map) ) > + { > + cpumask_setall(desc->affinity); > + affinity = &cpumask_all; > + } > + else if ( !cpumask_intersects(desc->arch.cpu_mask, > + &cpu_online_map) ) I'm not sure I see the purpose of the desc->arch.cpu_mask check, wouldn't it be better to just use else and set the affinity to desc->affinity? Or it's just an optimization to avoid doing the set_affinity call if the interrupt it already bound to an online CPU? Thanks, Roger.
>>> On 20.05.19 at 13:40, <roger.pau@citrix.com> wrote: > On Fri, May 17, 2019 at 04:48:21AM -0600, Jan Beulich wrote: >> fixup_irqs() skips interrupts without action. Hence such interrupts can >> retain affinity to just offline CPUs. With "noirqbalance" in effect, >> pirq_guest_bind() so far would have left them alone, resulting in a non- >> working interrupt. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> v3: New. >> --- >> I've not observed this problem in practice - the change is just the >> result of code inspection after having noticed action-less IRQs in 'i' >> debug key output pointing at all parked/offline CPUs. >> >> --- a/xen/arch/x86/irq.c >> +++ b/xen/arch/x86/irq.c >> @@ -1683,9 +1683,27 @@ int pirq_guest_bind(struct vcpu *v, stru >> >> desc->status |= IRQ_GUEST; >> >> - /* Attempt to bind the interrupt target to the correct CPU. */ >> - if ( !opt_noirqbalance && (desc->handler->set_affinity != NULL) ) >> - desc->handler->set_affinity(desc, cpumask_of(v->processor)); >> + /* >> + * Attempt to bind the interrupt target to the correct (or at least >> + * some online) CPU. >> + */ >> + if ( desc->handler->set_affinity ) >> + { >> + const cpumask_t *affinity = NULL; >> + >> + if ( !opt_noirqbalance ) >> + affinity = cpumask_of(v->processor); >> + else if ( !cpumask_intersects(desc->affinity, &cpu_online_map) ) >> + { >> + cpumask_setall(desc->affinity); >> + affinity = &cpumask_all; >> + } >> + else if ( !cpumask_intersects(desc->arch.cpu_mask, >> + &cpu_online_map) ) > > I'm not sure I see the purpose of the desc->arch.cpu_mask check, > wouldn't it be better to just use else and set the affinity to > desc->affinity? We should avoid clobbering desc->affinity whenever possible: It reflects (see the respective patch in this series) what was requested by whatever "outside" party. > Or it's just an optimization to avoid doing the set_affinity call if > the interrupt it already bound to an online CPU? This is a second aspect here indeed - why play with the IRQ if it has a valid destination? Jan
On Mon, May 20, 2019 at 09:17:19AM -0600, Jan Beulich wrote: > >>> On 20.05.19 at 13:40, <roger.pau@citrix.com> wrote: > > On Fri, May 17, 2019 at 04:48:21AM -0600, Jan Beulich wrote: > >> fixup_irqs() skips interrupts without action. Hence such interrupts can > >> retain affinity to just offline CPUs. With "noirqbalance" in effect, > >> pirq_guest_bind() so far would have left them alone, resulting in a non- > >> working interrupt. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> --- > >> v3: New. > >> --- > >> I've not observed this problem in practice - the change is just the > >> result of code inspection after having noticed action-less IRQs in 'i' > >> debug key output pointing at all parked/offline CPUs. > >> > >> --- a/xen/arch/x86/irq.c > >> +++ b/xen/arch/x86/irq.c > >> @@ -1683,9 +1683,27 @@ int pirq_guest_bind(struct vcpu *v, stru > >> > >> desc->status |= IRQ_GUEST; > >> > >> - /* Attempt to bind the interrupt target to the correct CPU. */ > >> - if ( !opt_noirqbalance && (desc->handler->set_affinity != NULL) ) > >> - desc->handler->set_affinity(desc, cpumask_of(v->processor)); > >> + /* > >> + * Attempt to bind the interrupt target to the correct (or at least > >> + * some online) CPU. > >> + */ > >> + if ( desc->handler->set_affinity ) > >> + { > >> + const cpumask_t *affinity = NULL; > >> + > >> + if ( !opt_noirqbalance ) > >> + affinity = cpumask_of(v->processor); > >> + else if ( !cpumask_intersects(desc->affinity, &cpu_online_map) ) > >> + { > >> + cpumask_setall(desc->affinity); > >> + affinity = &cpumask_all; > >> + } > >> + else if ( !cpumask_intersects(desc->arch.cpu_mask, > >> + &cpu_online_map) ) > > > > I'm not sure I see the purpose of the desc->arch.cpu_mask check, > > wouldn't it be better to just use else and set the affinity to > > desc->affinity? > > We should avoid clobbering desc->affinity whenever possible: It > reflects (see the respective patch in this series) what was > requested by whatever "outside" party. > > > Or it's just an optimization to avoid doing the set_affinity call if > > the interrupt it already bound to an online CPU? > > This is a second aspect here indeed - why play with the IRQ if > it has a valid destination? Thanks for the clarification, that LGTM: Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Roger.
On 17/05/2019 11:48, Jan Beulich wrote: > fixup_irqs() skips interrupts without action. Hence such interrupts can > retain affinity to just offline CPUs. With "noirqbalance" in effect, > pirq_guest_bind() so far would have left them alone, resulting in a non- > working interrupt. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -1683,9 +1683,27 @@ int pirq_guest_bind(struct vcpu *v, stru desc->status |= IRQ_GUEST; - /* Attempt to bind the interrupt target to the correct CPU. */ - if ( !opt_noirqbalance && (desc->handler->set_affinity != NULL) ) - desc->handler->set_affinity(desc, cpumask_of(v->processor)); + /* + * Attempt to bind the interrupt target to the correct (or at least + * some online) CPU. + */ + if ( desc->handler->set_affinity ) + { + const cpumask_t *affinity = NULL; + + if ( !opt_noirqbalance ) + affinity = cpumask_of(v->processor); + else if ( !cpumask_intersects(desc->affinity, &cpu_online_map) ) + { + cpumask_setall(desc->affinity); + affinity = &cpumask_all; + } + else if ( !cpumask_intersects(desc->arch.cpu_mask, + &cpu_online_map) ) + affinity = desc->affinity; + if ( affinity ) + desc->handler->set_affinity(desc, affinity); + } desc->status &= ~IRQ_DISABLED; desc->handler->startup(desc);
fixup_irqs() skips interrupts without action. Hence such interrupts can retain affinity to just offline CPUs. With "noirqbalance" in effect, pirq_guest_bind() so far would have left them alone, resulting in a non- working interrupt. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v3: New. --- I've not observed this problem in practice - the change is just the result of code inspection after having noticed action-less IRQs in 'i' debug key output pointing at all parked/offline CPUs.