The current check used in fixup_irqs() to decide whether to move around
interrupts is based on the affinity mask, but such mask can have all bits set,
and hence is unlikely to be a subset of the input mask. For example if an
interrupt has an affinity mask of all 1s, any input to fixup_irqs() that's not
an all set CPU mask would cause that interrupt to be shuffled around
unconditionally.
What fixup_irqs() care about is evacuating interrupts from CPUs not set on the
input CPU mask, and for that purpose it should check whether the interrupt is
assigned to a CPU not present in the input mask. Assume that ->arch.cpu_mask
is a subset of the ->affinity mask, and keep the current logic that resets the
->affinity mask if the interrupt has to be shuffled around.
Doing the affinity movement based on ->arch.cpu_mask requires removing the
special handling to ->arch.cpu_mask done for high priority vectors, otherwise
the adjustment done to cpu_mask makes them always skip the CPU interrupt
movement.
While there also adjust the comment as to the purpose of fixup_irqs().
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
- Adjust handling of high priority vectors.
Changes since v0 (outside the series):
- Do not AND ->arch.cpu_mask with cpu_online_map.
- Keep using cpumask_subset().
- Integrate into bigger series.
---
xen/arch/x86/include/asm/irq.h | 2 +-
xen/arch/x86/irq.c | 21 +++++++++++----------
2 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
index 94f634ce10a1..5f445299be61 100644
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -168,7 +168,7 @@ void free_domain_pirqs(struct domain *d);
int map_domain_emuirq_pirq(struct domain *d, int pirq, int emuirq);
int unmap_domain_pirq_emuirq(struct domain *d, int pirq);
-/* Reset irq affinities to match the given CPU mask. */
+/* Evacuate interrupts assigned to CPUs not present in the input CPU mask. */
void fixup_irqs(const cpumask_t *mask, bool verbose);
void fixup_eoi(void);
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index d101ffeaf9f3..263e502bc0f6 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2516,7 +2516,7 @@ static int __init cf_check setup_dump_irqs(void)
}
__initcall(setup_dump_irqs);
-/* Reset irq affinities to match the given CPU mask. */
+/* Evacuate interrupts assigned to CPUs not present in the input CPU mask. */
void fixup_irqs(const cpumask_t *mask, bool verbose)
{
unsigned int irq;
@@ -2540,19 +2540,15 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
vector = irq_to_vector(irq);
if ( vector >= FIRST_HIPRIORITY_VECTOR &&
- vector <= LAST_HIPRIORITY_VECTOR )
+ vector <= LAST_HIPRIORITY_VECTOR &&
+ desc->handler == &no_irq_type )
{
- cpumask_and(desc->arch.cpu_mask, desc->arch.cpu_mask, mask);
-
/*
* This can in particular happen when parking secondary threads
* during boot and when the serial console wants to use a PCI IRQ.
*/
- if ( desc->handler == &no_irq_type )
- {
- spin_unlock(&desc->lock);
- continue;
- }
+ spin_unlock(&desc->lock);
+ continue;
}
if ( desc->arch.move_cleanup_count )
@@ -2573,7 +2569,12 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
affinity);
}
- if ( !desc->action || cpumask_subset(desc->affinity, mask) )
+ /*
+ * Avoid shuffling the interrupt around as long as current target CPUs
+ * are a subset of the input mask. What fixup_irqs() cares about is
+ * evacuating interrupts from CPUs not in the input mask.
+ */
+ if ( !desc->action || cpumask_subset(desc->arch.cpu_mask, mask) )
{
spin_unlock(&desc->lock);
continue;
--
2.44.0
On 10.06.2024 16:20, Roger Pau Monne wrote: > The current check used in fixup_irqs() to decide whether to move around > interrupts is based on the affinity mask, but such mask can have all bits set, > and hence is unlikely to be a subset of the input mask. For example if an > interrupt has an affinity mask of all 1s, any input to fixup_irqs() that's not > an all set CPU mask would cause that interrupt to be shuffled around > unconditionally. > > What fixup_irqs() care about is evacuating interrupts from CPUs not set on the > input CPU mask, and for that purpose it should check whether the interrupt is > assigned to a CPU not present in the input mask. Assume that ->arch.cpu_mask > is a subset of the ->affinity mask, and keep the current logic that resets the > ->affinity mask if the interrupt has to be shuffled around. > > Doing the affinity movement based on ->arch.cpu_mask requires removing the > special handling to ->arch.cpu_mask done for high priority vectors, otherwise > the adjustment done to cpu_mask makes them always skip the CPU interrupt > movement. > > While there also adjust the comment as to the purpose of fixup_irqs(). > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Aiui this is independent of patch 1, so could go in while we still settle on how to word things there? Jan
On Tue, 2024-06-11 at 11:59 +0200, Jan Beulich wrote: > On 10.06.2024 16:20, Roger Pau Monne wrote: > > The current check used in fixup_irqs() to decide whether to move > > around > > interrupts is based on the affinity mask, but such mask can have > > all bits set, > > and hence is unlikely to be a subset of the input mask. For > > example if an > > interrupt has an affinity mask of all 1s, any input to fixup_irqs() > > that's not > > an all set CPU mask would cause that interrupt to be shuffled > > around > > unconditionally. > > > > What fixup_irqs() care about is evacuating interrupts from CPUs not > > set on the > > input CPU mask, and for that purpose it should check whether the > > interrupt is > > assigned to a CPU not present in the input mask. Assume that - > > >arch.cpu_mask > > is a subset of the ->affinity mask, and keep the current logic that > > resets the > > ->affinity mask if the interrupt has to be shuffled around. > > > > Doing the affinity movement based on ->arch.cpu_mask requires > > removing the > > special handling to ->arch.cpu_mask done for high priority vectors, > > otherwise > > the adjustment done to cpu_mask makes them always skip the CPU > > interrupt > > movement. > > > > While there also adjust the comment as to the purpose of > > fixup_irqs(). > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> Release-Acked-By: Oleksii Kurochko <oleksii.kurochko@gmail.com> ~ Oleksii > > Aiui this is independent of patch 1, so could go in while we still > settle on > how to word things there? > > Jan >
On Tue, Jun 11, 2024 at 11:59:39AM +0200, Jan Beulich wrote: > On 10.06.2024 16:20, Roger Pau Monne wrote: > > The current check used in fixup_irqs() to decide whether to move around > > interrupts is based on the affinity mask, but such mask can have all bits set, > > and hence is unlikely to be a subset of the input mask. For example if an > > interrupt has an affinity mask of all 1s, any input to fixup_irqs() that's not > > an all set CPU mask would cause that interrupt to be shuffled around > > unconditionally. > > > > What fixup_irqs() care about is evacuating interrupts from CPUs not set on the > > input CPU mask, and for that purpose it should check whether the interrupt is > > assigned to a CPU not present in the input mask. Assume that ->arch.cpu_mask > > is a subset of the ->affinity mask, and keep the current logic that resets the > > ->affinity mask if the interrupt has to be shuffled around. > > > > Doing the affinity movement based on ->arch.cpu_mask requires removing the > > special handling to ->arch.cpu_mask done for high priority vectors, otherwise > > the adjustment done to cpu_mask makes them always skip the CPU interrupt > > movement. > > > > While there also adjust the comment as to the purpose of fixup_irqs(). > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Aiui this is independent of patch 1, so could go in while we still settle on > how to word things there? I think so, the issue patch 1 fixes is independent from the rest of the series. Thanks, Roger.
© 2016 - 2024 Red Hat, Inc.