[PATCH v3 0/3] x86/irq: fixes for CPU hot{,un}plug

[PATCH v3 0/3] x86/irq: fixes for CPU hot{,un}plug

[Roger Pau Monne]

Hello,

The following series aim to fix interrupt handling when doing CPU
plug/unplug operations.  Without this series running:

cpus=`xl info max_cpu_id`
while [ 1 ]; do
    for i in `seq 1 $cpus`; do
        xen-hptool cpu-offline $i;
        xen-hptool cpu-online $i;
    done
done

Quite quickly results in interrupts getting lost and "No irq handler for
vector" messages on the Xen console.  Drivers in dom0 also start getting
interrupt timeouts and the system becomes unusable.

After applying the series running the loop over night still result in a
fully usable system, no  "No irq handler for vector" messages at all, no
interrupt loses reported by dom0.  Test with x2apic-mode={mixed,cluster}.

I've attempted to document all code as good as I could, interrupt
handling has some unexpected corner cases that are hard to diagnose and
reason about.

Some XenRT testing is undergoing to ensure no breakages.

Thanks, Roger.

Roger Pau Monne (3):
  x86/irq: deal with old_cpu_mask for interrupts in movement in
    fixup_irqs()
  x86/irq: handle moving interrupts in _assign_irq_vector()
  x86/irq: forward pending interrupts to new destination in fixup_irqs()

 xen/arch/x86/include/asm/apic.h |   5 +
 xen/arch/x86/irq.c              | 163 +++++++++++++++++++++++++-------
 2 files changed, 132 insertions(+), 36 deletions(-)

-- 
2.45.2

[PATCH v3 1/3] x86/irq: deal with old_cpu_mask for interrupts in movement in fixup_irqs()

[Roger Pau Monne]

Given the current logic it's possible for ->arch.old_cpu_mask to get out of
sync: if a CPU set in old_cpu_mask is offlined and then onlined
again without old_cpu_mask having been updated the data in the mask will no
longer be accurate, as when brought back online the CPU will no longer have
old_vector configured to handle the old interrupt source.

If there's an interrupt movement in progress, and the to be offlined CPU (which
is the call context) is in the old_cpu_mask clear it and update the mask, so it
doesn't contain stale data.

Note that when the system is going down fixup_irqs() will be called by
smp_send_stop() from CPU 0 with a mask with only CPU 0 on it, effectively
asking to move all interrupts to the current caller (CPU 0) which is the only
CPU to remain online.  In that case we don't care to migrate interrupts that
are in the process of being moved, as it's likely we won't be able to move all
interrupts to CPU 0 due to vector shortage anyway.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Adjust commit message.
 - Add comment about excluding smp_send_stop() case.
 - Use cpu_online().
---
 xen/arch/x86/irq.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 263e502bc0f6..d305aed317f2 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2526,7 +2526,7 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
     for ( irq = 0; irq < nr_irqs; irq++ )
     {
         bool break_affinity = false, set_affinity = true;
-        unsigned int vector;
+        unsigned int vector, cpu = smp_processor_id();
         cpumask_t *affinity = this_cpu(scratch_cpumask);
 
         if ( irq == 2 )
@@ -2569,6 +2569,33 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
                                affinity);
         }
 
+        if ( desc->arch.move_in_progress &&
+             /*
+              * Only attempt to adjust the mask if the current CPU is going
+              * offline, otherwise the whole system is going down and leaving
+              * stale data in the masks is fine.
+              */
+             !cpu_online(cpu) &&
+             cpumask_test_cpu(cpu, desc->arch.old_cpu_mask) )
+        {
+            /*
+             * This CPU is going offline, remove it from ->arch.old_cpu_mask
+             * and possibly release the old vector if the old mask becomes
+             * empty.
+             *
+             * Note cleaning ->arch.old_cpu_mask is required if the CPU is
+             * brought offline and then online again, as when re-onlined the
+             * per-cpu vector table will no longer have ->arch.old_vector
+             * setup, and hence ->arch.old_cpu_mask would be stale.
+             */
+            cpumask_clear_cpu(cpu, desc->arch.old_cpu_mask);
+            if ( cpumask_empty(desc->arch.old_cpu_mask) )
+            {
+                desc->arch.move_in_progress = 0;
+                release_old_vec(desc);
+            }
+        }
+
         /*
          * Avoid shuffling the interrupt around as long as current target CPUs
          * are a subset of the input mask.  What fixup_irqs() cares about is
-- 
2.45.2

Re: [PATCH v3 for-4.19 1/3] x86/irq: deal with old_cpu_mask for interrupts in movement in fixup_irqs()

[Jan Beulich]

On 13.06.2024 18:56, Roger Pau Monne wrote:
> Given the current logic it's possible for ->arch.old_cpu_mask to get out of
> sync: if a CPU set in old_cpu_mask is offlined and then onlined
> again without old_cpu_mask having been updated the data in the mask will no
> longer be accurate, as when brought back online the CPU will no longer have
> old_vector configured to handle the old interrupt source.
> 
> If there's an interrupt movement in progress, and the to be offlined CPU (which
> is the call context) is in the old_cpu_mask clear it and update the mask, so it
> doesn't contain stale data.

Perhaps a comma before "clear" might further help reading. Happy to
add while committing.

> Note that when the system is going down fixup_irqs() will be called by
> smp_send_stop() from CPU 0 with a mask with only CPU 0 on it, effectively
> asking to move all interrupts to the current caller (CPU 0) which is the only
> CPU to remain online.  In that case we don't care to migrate interrupts that
> are in the process of being moved, as it's likely we won't be able to move all
> interrupts to CPU 0 due to vector shortage anyway.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v3 for-4.19 1/3] x86/irq: deal with old_cpu_mask for interrupts in movement in fixup_irqs()

[Oleksii K.]

On Mon, 2024-06-17 at 15:18 +0200, Jan Beulich wrote:
> On 13.06.2024 18:56, Roger Pau Monne wrote:
> > Given the current logic it's possible for ->arch.old_cpu_mask to
> > get out of
> > sync: if a CPU set in old_cpu_mask is offlined and then onlined
> > again without old_cpu_mask having been updated the data in the mask
> > will no
> > longer be accurate, as when brought back online the CPU will no
> > longer have
> > old_vector configured to handle the old interrupt source.
> > 
> > If there's an interrupt movement in progress, and the to be
> > offlined CPU (which
> > is the call context) is in the old_cpu_mask clear it and update the
> > mask, so it
> > doesn't contain stale data.
> 
> Perhaps a comma before "clear" might further help reading. Happy to
> add while committing.
> 
> > Note that when the system is going down fixup_irqs() will be called
> > by
> > smp_send_stop() from CPU 0 with a mask with only CPU 0 on it,
> > effectively
> > asking to move all interrupts to the current caller (CPU 0) which
> > is the only
> > CPU to remain online.  In that case we don't care to migrate
> > interrupts that
> > are in the process of being moved, as it's likely we won't be able
> > to move all
> > interrupts to CPU 0 due to vector shortage anyway.
> > 
> > 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

Re: [PATCH v3 for-4.19 1/3] x86/irq: deal with old_cpu_mask for interrupts in movement in fixup_irqs()

[Roger Pau Monné]

On Mon, Jun 17, 2024 at 03:18:42PM +0200, Jan Beulich wrote:
> On 13.06.2024 18:56, Roger Pau Monne wrote:
> > Given the current logic it's possible for ->arch.old_cpu_mask to get out of
> > sync: if a CPU set in old_cpu_mask is offlined and then onlined
> > again without old_cpu_mask having been updated the data in the mask will no
> > longer be accurate, as when brought back online the CPU will no longer have
> > old_vector configured to handle the old interrupt source.
> > 
> > If there's an interrupt movement in progress, and the to be offlined CPU (which
> > is the call context) is in the old_cpu_mask clear it and update the mask, so it
> > doesn't contain stale data.
> 
> Perhaps a comma before "clear" might further help reading. Happy to
> add while committing.

Maybe, I'm trying to think of other ways to word the sentence in order
to make is simpler, but I'm out of ideas.

> > Note that when the system is going down fixup_irqs() will be called by
> > smp_send_stop() from CPU 0 with a mask with only CPU 0 on it, effectively
> > asking to move all interrupts to the current caller (CPU 0) which is the only
> > CPU to remain online.  In that case we don't care to migrate interrupts that
> > are in the process of being moved, as it's likely we won't be able to move all
> > interrupts to CPU 0 due to vector shortage anyway.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks, Roger.

[PATCH v3 2/3] x86/irq: handle moving interrupts in _assign_irq_vector()

[Roger Pau Monne]

Currently there's logic in fixup_irqs() that attempts to prevent
_assign_irq_vector() from failing, as fixup_irqs() is required to evacuate all
interrupts from the CPUs not present in the input mask.  The current logic in
fixup_irqs() is incomplete, as it doesn't deal with interrupts that have
move_cleanup_count > 0 and a non-empty ->arch.old_cpu_mask field.

Instead of attempting to fixup the interrupt descriptor in fixup_irqs() so that
_assign_irq_vector() cannot fail, introduce logic in _assign_irq_vector()
to deal with interrupts that have either move_{in_progress,cleanup_count} set
and no remaining online CPUs in ->arch.cpu_mask.

If _assign_irq_vector() is requested to move an interrupt in the state
described above, first attempt to see if ->arch.old_cpu_mask contains any valid
CPUs that could be used as fallback, and if that's the case do move the
interrupt back to the previous destination.  Note this is easier because the
vector hasn't been released yet, so there's no need to allocate and setup a new
vector on the destination.

Due to the logic in fixup_irqs() that clears offline CPUs from
->arch.old_cpu_mask (and releases the old vector if the mask becomes empty) it
shouldn't be possible to get into _assign_irq_vector() with
->arch.move_{in_progress,cleanup_count} set but no online CPUs in
->arch.old_cpu_mask.

However if ->arch.move_{in_progress,cleanup_count} is set and the interrupt has
also changed affinity, it's possible the members of ->arch.old_cpu_mask are no
longer part of the affinity set, move the interrupt to a different CPU part of
the provided mask and keep the current ->arch.old_{cpu_mask,vector} for the
pending interrupt movement to be completed.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Adjust comments.
 - Clean old vector from used_vectors mask.

Changes since v1:
 - Further refine the logic in _assign_irq_vector().
---
 xen/arch/x86/irq.c | 99 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 70 insertions(+), 29 deletions(-)

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index d305aed317f2..f36962fc1dc3 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -544,7 +544,58 @@ static int _assign_irq_vector(struct irq_desc *desc, const cpumask_t *mask)
     }
 
     if ( desc->arch.move_in_progress || desc->arch.move_cleanup_count )
-        return -EAGAIN;
+    {
+        /*
+         * If the current destination is online refuse to shuffle.  Retry after
+         * the in-progress movement has finished.
+         */
+        if ( cpumask_intersects(desc->arch.cpu_mask, &cpu_online_map) )
+            return -EAGAIN;
+
+        /*
+         * Due to the logic in fixup_irqs() that clears offlined CPUs from
+         * ->arch.old_cpu_mask it shouldn't be possible to get here with
+         * ->arch.move_{in_progress,cleanup_count} set and no online CPUs in
+         * ->arch.old_cpu_mask.
+         */
+        ASSERT(valid_irq_vector(desc->arch.old_vector));
+        ASSERT(cpumask_intersects(desc->arch.old_cpu_mask, &cpu_online_map));
+
+        if ( cpumask_intersects(desc->arch.old_cpu_mask, mask) )
+        {
+            /*
+             * Fallback to the old destination if moving is in progress and the
+             * current destination is to be offlined.  This is only possible if
+             * the CPUs in old_cpu_mask intersect with the affinity mask passed
+             * in the 'mask' parameter.
+             */
+            desc->arch.vector = desc->arch.old_vector;
+            cpumask_and(desc->arch.cpu_mask, desc->arch.old_cpu_mask, mask);
+
+            /* Undo any possibly done cleanup. */
+            for_each_cpu(cpu, desc->arch.cpu_mask)
+                per_cpu(vector_irq, cpu)[desc->arch.vector] = irq;
+
+            /* Cancel the pending move and release the current vector. */
+            desc->arch.old_vector = IRQ_VECTOR_UNASSIGNED;
+            cpumask_clear(desc->arch.old_cpu_mask);
+            desc->arch.move_in_progress = 0;
+            desc->arch.move_cleanup_count = 0;
+            if ( desc->arch.used_vectors )
+            {
+                ASSERT(test_bit(old_vector, desc->arch.used_vectors));
+                clear_bit(old_vector, desc->arch.used_vectors);
+            }
+
+            return 0;
+        }
+
+        /*
+         * There's an interrupt movement in progress but the destination(s) in
+         * ->arch.old_cpu_mask are not suitable given the 'mask' parameter, go
+         * through the full logic to find a new vector in a suitable CPU.
+         */
+    }
 
     err = -ENOSPC;
 
@@ -600,7 +651,24 @@ next:
         current_vector = vector;
         current_offset = offset;
 
-        if ( valid_irq_vector(old_vector) )
+        if ( desc->arch.move_in_progress || desc->arch.move_cleanup_count )
+        {
+            ASSERT(!cpumask_intersects(desc->arch.cpu_mask, &cpu_online_map));
+            /*
+             * Special case when evacuating an interrupt from a CPU to be
+             * offlined and the interrupt was already in the process of being
+             * moved.  Leave ->arch.old_{vector,cpu_mask} as-is and just
+             * replace ->arch.{cpu_mask,vector} with the new destination.
+             * Cleanup will be done normally for the old fields, just release
+             * the current vector here.
+             */
+            if ( desc->arch.used_vectors )
+            {
+                ASSERT(test_bit(old_vector, desc->arch.used_vectors));
+                clear_bit(old_vector, desc->arch.used_vectors);
+            }
+        }
+        else if ( valid_irq_vector(old_vector) )
         {
             cpumask_and(desc->arch.old_cpu_mask, desc->arch.cpu_mask,
                         &cpu_online_map);
@@ -2607,33 +2675,6 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
             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;
-        }
-
         if ( !cpumask_intersects(mask, desc->affinity) )
         {
             break_affinity = true;
-- 
2.45.2

Re: [PATCH v3 for-4.19 2/3] x86/irq: handle moving interrupts in _assign_irq_vector()

[Jan Beulich]

On 13.06.2024 18:56, Roger Pau Monne wrote:
> Currently there's logic in fixup_irqs() that attempts to prevent
> _assign_irq_vector() from failing, as fixup_irqs() is required to evacuate all
> interrupts from the CPUs not present in the input mask.  The current logic in
> fixup_irqs() is incomplete, as it doesn't deal with interrupts that have
> move_cleanup_count > 0 and a non-empty ->arch.old_cpu_mask field.
> 
> Instead of attempting to fixup the interrupt descriptor in fixup_irqs() so that
> _assign_irq_vector() cannot fail, introduce logic in _assign_irq_vector()
> to deal with interrupts that have either move_{in_progress,cleanup_count} set
> and no remaining online CPUs in ->arch.cpu_mask.
> 
> If _assign_irq_vector() is requested to move an interrupt in the state
> described above, first attempt to see if ->arch.old_cpu_mask contains any valid
> CPUs that could be used as fallback, and if that's the case do move the
> interrupt back to the previous destination.  Note this is easier because the
> vector hasn't been released yet, so there's no need to allocate and setup a new
> vector on the destination.
> 
> Due to the logic in fixup_irqs() that clears offline CPUs from
> ->arch.old_cpu_mask (and releases the old vector if the mask becomes empty) it
> shouldn't be possible to get into _assign_irq_vector() with
> ->arch.move_{in_progress,cleanup_count} set but no online CPUs in
> ->arch.old_cpu_mask.
> 
> However if ->arch.move_{in_progress,cleanup_count} is set and the interrupt has
> also changed affinity, it's possible the members of ->arch.old_cpu_mask are no
> longer part of the affinity set, move the interrupt to a different CPU part of
> the provided mask and keep the current ->arch.old_{cpu_mask,vector} for the
> pending interrupt movement to be completed.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -544,7 +544,58 @@ static int _assign_irq_vector(struct irq_desc *desc, const cpumask_t *mask)
>      }
>  
>      if ( desc->arch.move_in_progress || desc->arch.move_cleanup_count )
> -        return -EAGAIN;
> +    {
> +        /*
> +         * If the current destination is online refuse to shuffle.  Retry after
> +         * the in-progress movement has finished.
> +         */
> +        if ( cpumask_intersects(desc->arch.cpu_mask, &cpu_online_map) )
> +            return -EAGAIN;
> +
> +        /*
> +         * Due to the logic in fixup_irqs() that clears offlined CPUs from
> +         * ->arch.old_cpu_mask it shouldn't be possible to get here with
> +         * ->arch.move_{in_progress,cleanup_count} set and no online CPUs in
> +         * ->arch.old_cpu_mask.
> +         */
> +        ASSERT(valid_irq_vector(desc->arch.old_vector));
> +        ASSERT(cpumask_intersects(desc->arch.old_cpu_mask, &cpu_online_map));
> +
> +        if ( cpumask_intersects(desc->arch.old_cpu_mask, mask) )
> +        {
> +            /*
> +             * Fallback to the old destination if moving is in progress and the
> +             * current destination is to be offlined.  This is only possible if
> +             * the CPUs in old_cpu_mask intersect with the affinity mask passed
> +             * in the 'mask' parameter.
> +             */
> +            desc->arch.vector = desc->arch.old_vector;

I'm a little puzzled that you use desc->arch.old_vector here, but ...

> +            cpumask_and(desc->arch.cpu_mask, desc->arch.old_cpu_mask, mask);
> +
> +            /* Undo any possibly done cleanup. */
> +            for_each_cpu(cpu, desc->arch.cpu_mask)
> +                per_cpu(vector_irq, cpu)[desc->arch.vector] = irq;
> +
> +            /* Cancel the pending move and release the current vector. */
> +            desc->arch.old_vector = IRQ_VECTOR_UNASSIGNED;
> +            cpumask_clear(desc->arch.old_cpu_mask);
> +            desc->arch.move_in_progress = 0;
> +            desc->arch.move_cleanup_count = 0;
> +            if ( desc->arch.used_vectors )
> +            {
> +                ASSERT(test_bit(old_vector, desc->arch.used_vectors));
> +                clear_bit(old_vector, desc->arch.used_vectors);

... old_vector here. Since we have the latter, uniformly using it might
be more consistent. I realize though that irq_to_vector() has cases where
it wouldn't return desc->arch.old_vector; I think, however, that in those
case we can't make it here. Still I'm not going to insist on making the
adjustment. Happy to make it though while committing, should you agree.

Also I'm not happy to see another instance of this pattern appear. In
x86-specific code this is inefficient, as {set,clear}_bit resolve to the
same insn as test_and_{set,clear}_bit(). Therefore imo more efficient
would be

                if (!test_and_clear_bit(old_vector, desc->arch.used_vectors))
                    ASSERT_UNREACHABLE();

(and then the two if()s folded).

I've been meaning to propose a patch to the other similar sites ...

Jan

Re: [PATCH v3 for-4.19 2/3] x86/irq: handle moving interrupts in _assign_irq_vector()

[Oleksii K.]

On Mon, 2024-06-17 at 15:31 +0200, Jan Beulich wrote:
> On 13.06.2024 18:56, Roger Pau Monne wrote:
> > Currently there's logic in fixup_irqs() that attempts to prevent
> > _assign_irq_vector() from failing, as fixup_irqs() is required to
> > evacuate all
> > interrupts from the CPUs not present in the input mask.  The
> > current logic in
> > fixup_irqs() is incomplete, as it doesn't deal with interrupts that
> > have
> > move_cleanup_count > 0 and a non-empty ->arch.old_cpu_mask field.
> > 
> > Instead of attempting to fixup the interrupt descriptor in
> > fixup_irqs() so that
> > _assign_irq_vector() cannot fail, introduce logic in
> > _assign_irq_vector()
> > to deal with interrupts that have either
> > move_{in_progress,cleanup_count} set
> > and no remaining online CPUs in ->arch.cpu_mask.
> > 
> > If _assign_irq_vector() is requested to move an interrupt in the
> > state
> > described above, first attempt to see if ->arch.old_cpu_mask
> > contains any valid
> > CPUs that could be used as fallback, and if that's the case do move
> > the
> > interrupt back to the previous destination.  Note this is easier
> > because the
> > vector hasn't been released yet, so there's no need to allocate and
> > setup a new
> > vector on the destination.
> > 
> > Due to the logic in fixup_irqs() that clears offline CPUs from
> > ->arch.old_cpu_mask (and releases the old vector if the mask
> > becomes empty) it
> > shouldn't be possible to get into _assign_irq_vector() with
> > ->arch.move_{in_progress,cleanup_count} set but no online CPUs in
> > ->arch.old_cpu_mask.
> > 
> > However if ->arch.move_{in_progress,cleanup_count} is set and the
> > interrupt has
> > also changed affinity, it's possible the members of -
> > >arch.old_cpu_mask are no
> > longer part of the affinity set, move the interrupt to a different
> > CPU part of
> > the provided mask and keep the current ->arch.old_{cpu_mask,vector}
> > for the
> > pending interrupt movement to be completed.
> > 
> > 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
> 
> > --- a/xen/arch/x86/irq.c
> > +++ b/xen/arch/x86/irq.c
> > @@ -544,7 +544,58 @@ static int _assign_irq_vector(struct irq_desc
> > *desc, const cpumask_t *mask)
> >      }
> >  
> >      if ( desc->arch.move_in_progress || desc-
> > >arch.move_cleanup_count )
> > -        return -EAGAIN;
> > +    {
> > +        /*
> > +         * If the current destination is online refuse to
> > shuffle.  Retry after
> > +         * the in-progress movement has finished.
> > +         */
> > +        if ( cpumask_intersects(desc->arch.cpu_mask,
> > &cpu_online_map) )
> > +            return -EAGAIN;
> > +
> > +        /*
> > +         * Due to the logic in fixup_irqs() that clears offlined
> > CPUs from
> > +         * ->arch.old_cpu_mask it shouldn't be possible to get
> > here with
> > +         * ->arch.move_{in_progress,cleanup_count} set and no
> > online CPUs in
> > +         * ->arch.old_cpu_mask.
> > +         */
> > +        ASSERT(valid_irq_vector(desc->arch.old_vector));
> > +        ASSERT(cpumask_intersects(desc->arch.old_cpu_mask,
> > &cpu_online_map));
> > +
> > +        if ( cpumask_intersects(desc->arch.old_cpu_mask, mask) )
> > +        {
> > +            /*
> > +             * Fallback to the old destination if moving is in
> > progress and the
> > +             * current destination is to be offlined.  This is
> > only possible if
> > +             * the CPUs in old_cpu_mask intersect with the
> > affinity mask passed
> > +             * in the 'mask' parameter.
> > +             */
> > +            desc->arch.vector = desc->arch.old_vector;
> 
> I'm a little puzzled that you use desc->arch.old_vector here, but ...
> 
> > +            cpumask_and(desc->arch.cpu_mask, desc-
> > >arch.old_cpu_mask, mask);
> > +
> > +            /* Undo any possibly done cleanup. */
> > +            for_each_cpu(cpu, desc->arch.cpu_mask)
> > +                per_cpu(vector_irq, cpu)[desc->arch.vector] = irq;
> > +
> > +            /* Cancel the pending move and release the current
> > vector. */
> > +            desc->arch.old_vector = IRQ_VECTOR_UNASSIGNED;
> > +            cpumask_clear(desc->arch.old_cpu_mask);
> > +            desc->arch.move_in_progress = 0;
> > +            desc->arch.move_cleanup_count = 0;
> > +            if ( desc->arch.used_vectors )
> > +            {
> > +                ASSERT(test_bit(old_vector, desc-
> > >arch.used_vectors));
> > +                clear_bit(old_vector, desc->arch.used_vectors);
> 
> ... old_vector here. Since we have the latter, uniformly using it
> might
> be more consistent. I realize though that irq_to_vector() has cases
> where
> it wouldn't return desc->arch.old_vector; I think, however, that in
> those
> case we can't make it here. Still I'm not going to insist on making
> the
> adjustment. Happy to make it though while committing, should you
> agree.
> 
> Also I'm not happy to see another instance of this pattern appear. In
> x86-specific code this is inefficient, as {set,clear}_bit resolve to
> the
> same insn as test_and_{set,clear}_bit(). Therefore imo more efficient
> would be
> 
>                 if (!test_and_clear_bit(old_vector, desc-
> >arch.used_vectors))
>                     ASSERT_UNREACHABLE();
> 
> (and then the two if()s folded).
> 
> I've been meaning to propose a patch to the other similar sites ...
> 
> Jan

Re: [PATCH v3 for-4.19 2/3] x86/irq: handle moving interrupts in _assign_irq_vector()

[Roger Pau Monné]

On Mon, Jun 17, 2024 at 03:31:13PM +0200, Jan Beulich wrote:
> On 13.06.2024 18:56, Roger Pau Monne wrote:
> > Currently there's logic in fixup_irqs() that attempts to prevent
> > _assign_irq_vector() from failing, as fixup_irqs() is required to evacuate all
> > interrupts from the CPUs not present in the input mask.  The current logic in
> > fixup_irqs() is incomplete, as it doesn't deal with interrupts that have
> > move_cleanup_count > 0 and a non-empty ->arch.old_cpu_mask field.
> > 
> > Instead of attempting to fixup the interrupt descriptor in fixup_irqs() so that
> > _assign_irq_vector() cannot fail, introduce logic in _assign_irq_vector()
> > to deal with interrupts that have either move_{in_progress,cleanup_count} set
> > and no remaining online CPUs in ->arch.cpu_mask.
> > 
> > If _assign_irq_vector() is requested to move an interrupt in the state
> > described above, first attempt to see if ->arch.old_cpu_mask contains any valid
> > CPUs that could be used as fallback, and if that's the case do move the
> > interrupt back to the previous destination.  Note this is easier because the
> > vector hasn't been released yet, so there's no need to allocate and setup a new
> > vector on the destination.
> > 
> > Due to the logic in fixup_irqs() that clears offline CPUs from
> > ->arch.old_cpu_mask (and releases the old vector if the mask becomes empty) it
> > shouldn't be possible to get into _assign_irq_vector() with
> > ->arch.move_{in_progress,cleanup_count} set but no online CPUs in
> > ->arch.old_cpu_mask.
> > 
> > However if ->arch.move_{in_progress,cleanup_count} is set and the interrupt has
> > also changed affinity, it's possible the members of ->arch.old_cpu_mask are no
> > longer part of the affinity set, move the interrupt to a different CPU part of
> > the provided mask and keep the current ->arch.old_{cpu_mask,vector} for the
> > pending interrupt movement to be completed.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> > --- a/xen/arch/x86/irq.c
> > +++ b/xen/arch/x86/irq.c
> > @@ -544,7 +544,58 @@ static int _assign_irq_vector(struct irq_desc *desc, const cpumask_t *mask)
> >      }
> >  
> >      if ( desc->arch.move_in_progress || desc->arch.move_cleanup_count )
> > -        return -EAGAIN;
> > +    {
> > +        /*
> > +         * If the current destination is online refuse to shuffle.  Retry after
> > +         * the in-progress movement has finished.
> > +         */
> > +        if ( cpumask_intersects(desc->arch.cpu_mask, &cpu_online_map) )
> > +            return -EAGAIN;
> > +
> > +        /*
> > +         * Due to the logic in fixup_irqs() that clears offlined CPUs from
> > +         * ->arch.old_cpu_mask it shouldn't be possible to get here with
> > +         * ->arch.move_{in_progress,cleanup_count} set and no online CPUs in
> > +         * ->arch.old_cpu_mask.
> > +         */
> > +        ASSERT(valid_irq_vector(desc->arch.old_vector));
> > +        ASSERT(cpumask_intersects(desc->arch.old_cpu_mask, &cpu_online_map));
> > +
> > +        if ( cpumask_intersects(desc->arch.old_cpu_mask, mask) )
> > +        {
> > +            /*
> > +             * Fallback to the old destination if moving is in progress and the
> > +             * current destination is to be offlined.  This is only possible if
> > +             * the CPUs in old_cpu_mask intersect with the affinity mask passed
> > +             * in the 'mask' parameter.
> > +             */
> > +            desc->arch.vector = desc->arch.old_vector;
> 
> I'm a little puzzled that you use desc->arch.old_vector here, but ...

old_vector can't be used here, as old_vector == desc->arch.vector at
this point.

The name of the variable is IMO a bit misleading, as it's value only
becomes the old_vector once a new vector is assigned.  It would be
more appropriate to name it current_vector IMO.

> > +            cpumask_and(desc->arch.cpu_mask, desc->arch.old_cpu_mask, mask);
> > +
> > +            /* Undo any possibly done cleanup. */
> > +            for_each_cpu(cpu, desc->arch.cpu_mask)
> > +                per_cpu(vector_irq, cpu)[desc->arch.vector] = irq;
> > +
> > +            /* Cancel the pending move and release the current vector. */
> > +            desc->arch.old_vector = IRQ_VECTOR_UNASSIGNED;
> > +            cpumask_clear(desc->arch.old_cpu_mask);
> > +            desc->arch.move_in_progress = 0;
> > +            desc->arch.move_cleanup_count = 0;
> > +            if ( desc->arch.used_vectors )
> > +            {
> > +                ASSERT(test_bit(old_vector, desc->arch.used_vectors));
> > +                clear_bit(old_vector, desc->arch.used_vectors);
> 
> ... old_vector here. Since we have the latter, uniformly using it might
> be more consistent.

Keep in mind that old_vector a cache of the value of desc->arch.vector
at the start of the function.

> I realize though that irq_to_vector() has cases where
> it wouldn't return desc->arch.old_vector; I think, however, that in those
> case we can't make it here. Still I'm not going to insist on making the
> adjustment. Happy to make it though while committing, should you agree.
> 
> Also I'm not happy to see another instance of this pattern appear. In
> x86-specific code this is inefficient, as {set,clear}_bit resolve to the
> same insn as test_and_{set,clear}_bit(). Therefore imo more efficient
> would be
> 
>                 if (!test_and_clear_bit(old_vector, desc->arch.used_vectors))
>                     ASSERT_UNREACHABLE();
> 
> (and then the two if()s folded).
> 
> I've been meaning to propose a patch to the other similar sites ...

Oh, indeed.  IIRC I've copied this pattern from somewhere else without
noticing.  I'm happy for you to fold the test_and_clear_bit() call
into the parent if condition.

Thanks, Roger.

[PATCH v3 3/3] x86/irq: forward pending interrupts to new destination in fixup_irqs()

[Roger Pau Monne]

fixup_irqs() is used to evacuate interrupts from to be offlined CPUs.  Given
the CPU is to become offline, the normal migration logic used by Xen where the
vector in the previous target(s) is left configured until the interrupt is
received on the new destination is not suitable.

Instead attempt to do as much as possible in order to prevent loosing
interrupts.  If fixup_irqs() is called from the CPU to be offlined (as is
currently the case) attempt to forward pending vectors when interrupts that
target the current CPU are migrated to a different destination.

Additionally, for interrupts that have already been moved from the current CPU
prior to the call to fixup_irqs() but that haven't been delivered to the new
destination (iow: interrupts with move_in_progress set and the current CPU set
in ->arch.old_cpu_mask) also check whether the previous vector is pending and
forward it to the new destination.

This allows us to remove the window with interrupts enabled at the bottom of
fixup_irqs().  Such window wasn't safe anyway: references to the CPU to become
offline are removed from interrupts masks, but the per-CPU vector_irq[] array
is not updated to reflect those changes (as the CPU is going offline anyway).

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Remove interrupt enabled window from fixup_irqs().
 - Adjust comments and commit message.

Changes since v1:
 - Rename to apic_irr_read().
---
 xen/arch/x86/include/asm/apic.h |  5 ++++
 xen/arch/x86/irq.c              | 42 ++++++++++++++++++++++++++++-----
 2 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/include/asm/apic.h b/xen/arch/x86/include/asm/apic.h
index d1cb001fb4ab..7bd66dc6e151 100644
--- a/xen/arch/x86/include/asm/apic.h
+++ b/xen/arch/x86/include/asm/apic.h
@@ -132,6 +132,11 @@ static inline bool apic_isr_read(uint8_t vector)
             (vector & 0x1f)) & 1;
 }
 
+static inline bool apic_irr_read(unsigned int vector)
+{
+    return apic_read(APIC_IRR + (vector / 32 * 0x10)) & (1U << (vector % 32));
+}
+
 static inline u32 get_apic_id(void)
 {
     u32 id = apic_read(APIC_ID);
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index f36962fc1dc3..a2b04c793292 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2593,7 +2593,7 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
 
     for ( irq = 0; irq < nr_irqs; irq++ )
     {
-        bool break_affinity = false, set_affinity = true;
+        bool break_affinity = false, set_affinity = true, check_irr = false;
         unsigned int vector, cpu = smp_processor_id();
         cpumask_t *affinity = this_cpu(scratch_cpumask);
 
@@ -2646,6 +2646,25 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
              !cpu_online(cpu) &&
              cpumask_test_cpu(cpu, desc->arch.old_cpu_mask) )
         {
+            /*
+             * This to be offlined CPU was the target of an interrupt that's
+             * been moved, and the new destination target hasn't yet
+             * acknowledged any interrupt from it.
+             *
+             * We know the interrupt is configured to target the new CPU at
+             * this point, so we can check IRR for any pending vectors and
+             * forward them to the new destination.
+             *
+             * Note that for the other case of an interrupt movement being in
+             * progress (move_cleanup_count being non-zero) we know the new
+             * destination has already acked at least one interrupt from this
+             * source, and hence there's no need to forward any stale
+             * interrupts.
+             */
+            if ( apic_irr_read(desc->arch.old_vector) )
+                send_IPI_mask(cpumask_of(cpumask_any(desc->arch.cpu_mask)),
+                              desc->arch.vector);
+
             /*
              * This CPU is going offline, remove it from ->arch.old_cpu_mask
              * and possibly release the old vector if the old mask becomes
@@ -2686,11 +2705,27 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
         if ( desc->handler->disable )
             desc->handler->disable(desc);
 
+        /*
+         * If the current CPU is going offline and is (one of) the target(s) of
+         * the interrupt, signal to check whether there are any pending vectors
+         * to be handled in the local APIC after the interrupt has been moved.
+         */
+        if ( !cpu_online(cpu) && cpumask_test_cpu(cpu, desc->arch.cpu_mask) )
+            check_irr = true;
+
         if ( desc->handler->set_affinity )
             desc->handler->set_affinity(desc, affinity);
         else if ( !(warned++) )
             set_affinity = false;
 
+        if ( check_irr && apic_irr_read(vector) )
+            /*
+             * Forward pending interrupt to the new destination, this CPU is
+             * going offline and otherwise the interrupt would be lost.
+             */
+            send_IPI_mask(cpumask_of(cpumask_any(desc->arch.cpu_mask)),
+                          desc->arch.vector);
+
         if ( desc->handler->enable )
             desc->handler->enable(desc);
 
@@ -2707,11 +2742,6 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
             printk("Broke affinity for IRQ%u, new: {%*pbl}\n",
                    irq, CPUMASK_PR(affinity));
     }
-
-    /* That doesn't seem sufficient.  Give it 1ms. */
-    local_irq_enable();
-    mdelay(1);
-    local_irq_disable();
 }
 
 void fixup_eoi(void)
-- 
2.45.2

Re: [PATCH v3 3/3] x86/irq: forward pending interrupts to new destination in fixup_irqs()

[Jan Beulich]

On 13.06.2024 18:56, Roger Pau Monne wrote:
> fixup_irqs() is used to evacuate interrupts from to be offlined CPUs.  Given
> the CPU is to become offline, the normal migration logic used by Xen where the
> vector in the previous target(s) is left configured until the interrupt is
> received on the new destination is not suitable.
> 
> Instead attempt to do as much as possible in order to prevent loosing
> interrupts.  If fixup_irqs() is called from the CPU to be offlined (as is
> currently the case)

Except (again) for smp_send_stop().

> attempt to forward pending vectors when interrupts that
> target the current CPU are migrated to a different destination.
> 
> Additionally, for interrupts that have already been moved from the current CPU
> prior to the call to fixup_irqs() but that haven't been delivered to the new
> destination (iow: interrupts with move_in_progress set and the current CPU set
> in ->arch.old_cpu_mask) also check whether the previous vector is pending and
> forward it to the new destination.
> 
> This allows us to remove the window with interrupts enabled at the bottom of
> fixup_irqs().  Such window wasn't safe anyway: references to the CPU to become
> offline are removed from interrupts masks, but the per-CPU vector_irq[] array
> is not updated to reflect those changes (as the CPU is going offline anyway).
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>[...]
> @@ -2686,11 +2705,27 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
>          if ( desc->handler->disable )
>              desc->handler->disable(desc);
>  
> +        /*
> +         * If the current CPU is going offline and is (one of) the target(s) of
> +         * the interrupt, signal to check whether there are any pending vectors
> +         * to be handled in the local APIC after the interrupt has been moved.
> +         */
> +        if ( !cpu_online(cpu) && cpumask_test_cpu(cpu, desc->arch.cpu_mask) )
> +            check_irr = true;
> +
>          if ( desc->handler->set_affinity )
>              desc->handler->set_affinity(desc, affinity);
>          else if ( !(warned++) )
>              set_affinity = false;
>  
> +        if ( check_irr && apic_irr_read(vector) )
> +            /*
> +             * Forward pending interrupt to the new destination, this CPU is
> +             * going offline and otherwise the interrupt would be lost.
> +             */
> +            send_IPI_mask(cpumask_of(cpumask_any(desc->arch.cpu_mask)),
> +                          desc->arch.vector);

Hmm, IRR may become set right after the IRR read (unlike in the other cases,
where new IRQs ought to be surfacing only at the new destination). Doesn't
this want moving ...

>          if ( desc->handler->enable )
>              desc->handler->enable(desc);

... past the actual affinity change?

Jan

Re: [PATCH v3 3/3] x86/irq: forward pending interrupts to new destination in fixup_irqs()

[Roger Pau Monné]

On Mon, Jun 17, 2024 at 03:41:12PM +0200, Jan Beulich wrote:
> On 13.06.2024 18:56, Roger Pau Monne wrote:
> > fixup_irqs() is used to evacuate interrupts from to be offlined CPUs.  Given
> > the CPU is to become offline, the normal migration logic used by Xen where the
> > vector in the previous target(s) is left configured until the interrupt is
> > received on the new destination is not suitable.
> > 
> > Instead attempt to do as much as possible in order to prevent loosing
> > interrupts.  If fixup_irqs() is called from the CPU to be offlined (as is
> > currently the case)
> 
> Except (again) for smp_send_stop().

I guess I haven't worded this properly, the point I was trying to make
is that in the context of a CPU unplug fixup_irqs() is always called
from the CPU that's going offline.

What about:

"If fixup_irqs() is called from the CPU to be offlined (as is
currently the case for CPU hot unplug) ..."

> > attempt to forward pending vectors when interrupts that
> > target the current CPU are migrated to a different destination.
> > 
> > Additionally, for interrupts that have already been moved from the current CPU
> > prior to the call to fixup_irqs() but that haven't been delivered to the new
> > destination (iow: interrupts with move_in_progress set and the current CPU set
> > in ->arch.old_cpu_mask) also check whether the previous vector is pending and
> > forward it to the new destination.
> > 
> > This allows us to remove the window with interrupts enabled at the bottom of
> > fixup_irqs().  Such window wasn't safe anyway: references to the CPU to become
> > offline are removed from interrupts masks, but the per-CPU vector_irq[] array
> > is not updated to reflect those changes (as the CPU is going offline anyway).
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >[...]
> > @@ -2686,11 +2705,27 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
> >          if ( desc->handler->disable )
> >              desc->handler->disable(desc);
> >  
> > +        /*
> > +         * If the current CPU is going offline and is (one of) the target(s) of
> > +         * the interrupt, signal to check whether there are any pending vectors
> > +         * to be handled in the local APIC after the interrupt has been moved.
> > +         */
> > +        if ( !cpu_online(cpu) && cpumask_test_cpu(cpu, desc->arch.cpu_mask) )
> > +            check_irr = true;
> > +
> >          if ( desc->handler->set_affinity )
> >              desc->handler->set_affinity(desc, affinity);
> >          else if ( !(warned++) )
> >              set_affinity = false;
> >  
> > +        if ( check_irr && apic_irr_read(vector) )
> > +            /*
> > +             * Forward pending interrupt to the new destination, this CPU is
> > +             * going offline and otherwise the interrupt would be lost.
> > +             */
> > +            send_IPI_mask(cpumask_of(cpumask_any(desc->arch.cpu_mask)),
> > +                          desc->arch.vector);
> 
> Hmm, IRR may become set right after the IRR read (unlike in the other cases,
> where new IRQs ought to be surfacing only at the new destination). Doesn't
> this want moving ...
> 
> >          if ( desc->handler->enable )
> >              desc->handler->enable(desc);
> 
> ... past the actual affinity change?

Hm, but the ->enable() hook is just unmasking the interrupt, the
actual affinity change is done in ->set_affinity(), and hence after
the call to ->set_affinity() no further interrupts should be delivered
to the CPU regardless of whether the source is masked?

Or is it possible for the device/interrupt controller to not switch to
use the new destination until the interrupt is unmasked, and hence
could have pending masked interrupts still using the old destination?
IIRC For MSI-X it's required that the device updates the destination
target once the entry is unmasked.

I'm happy to move it after the ->enable() hook, but would like to
better understand why this is required.

Thanks, Roger.

Re: [PATCH v3 3/3] x86/irq: forward pending interrupts to new destination in fixup_irqs()

[Jan Beulich]

On 18.06.2024 13:30, Roger Pau Monné wrote:
> On Mon, Jun 17, 2024 at 03:41:12PM +0200, Jan Beulich wrote:
>> On 13.06.2024 18:56, Roger Pau Monne wrote:
>>> fixup_irqs() is used to evacuate interrupts from to be offlined CPUs.  Given
>>> the CPU is to become offline, the normal migration logic used by Xen where the
>>> vector in the previous target(s) is left configured until the interrupt is
>>> received on the new destination is not suitable.
>>>
>>> Instead attempt to do as much as possible in order to prevent loosing
>>> interrupts.  If fixup_irqs() is called from the CPU to be offlined (as is
>>> currently the case)
>>
>> Except (again) for smp_send_stop().
> 
> I guess I haven't worded this properly, the point I was trying to make
> is that in the context of a CPU unplug fixup_irqs() is always called
> from the CPU that's going offline.
> 
> What about:
> 
> "If fixup_irqs() is called from the CPU to be offlined (as is
> currently the case for CPU hot unplug) ..."

Sounds good to me.

>>> @@ -2686,11 +2705,27 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
>>>          if ( desc->handler->disable )
>>>              desc->handler->disable(desc);
>>>  
>>> +        /*
>>> +         * If the current CPU is going offline and is (one of) the target(s) of
>>> +         * the interrupt, signal to check whether there are any pending vectors
>>> +         * to be handled in the local APIC after the interrupt has been moved.
>>> +         */
>>> +        if ( !cpu_online(cpu) && cpumask_test_cpu(cpu, desc->arch.cpu_mask) )
>>> +            check_irr = true;
>>> +
>>>          if ( desc->handler->set_affinity )
>>>              desc->handler->set_affinity(desc, affinity);
>>>          else if ( !(warned++) )
>>>              set_affinity = false;
>>>  
>>> +        if ( check_irr && apic_irr_read(vector) )
>>> +            /*
>>> +             * Forward pending interrupt to the new destination, this CPU is
>>> +             * going offline and otherwise the interrupt would be lost.
>>> +             */
>>> +            send_IPI_mask(cpumask_of(cpumask_any(desc->arch.cpu_mask)),
>>> +                          desc->arch.vector);
>>
>> Hmm, IRR may become set right after the IRR read (unlike in the other cases,
>> where new IRQs ought to be surfacing only at the new destination). Doesn't
>> this want moving ...
>>
>>>          if ( desc->handler->enable )
>>>              desc->handler->enable(desc);
>>
>> ... past the actual affinity change?
> 
> Hm, but the ->enable() hook is just unmasking the interrupt, the
> actual affinity change is done in ->set_affinity(), and hence after
> the call to ->set_affinity() no further interrupts should be delivered
> to the CPU regardless of whether the source is masked?
> 
> Or is it possible for the device/interrupt controller to not switch to
> use the new destination until the interrupt is unmasked, and hence
> could have pending masked interrupts still using the old destination?
> IIRC For MSI-X it's required that the device updates the destination
> target once the entry is unmasked.

That's all not relevant here, I think. IRR gets set when an interrupt is
signaled, no matter whether it's masked. It's its handling which the
masking would prevent, i.e. the "moving" of the set bit from IRR to ISR.
Plus we run with IRQs off here anyway if I'm not mistaken, so no
interrupt can be delivered to the local CPU. IOW whatever IRR bits it
has set (including ones becoming set between the IRR read and the actual
vector change), those would never be serviced. Hence the reading of the
bit ought to occur after the vector change: It's only then that we know
the IRR bit corresponding to the old vector can't become set anymore.

And even then we're assuming that no interrupt signals might still be
"on their way" from the IO-APIC or a posted MSI message write by a
device to the LAPIC (I have no idea how to properly fence that, or
whether there are guarantees for this to never occur).

Jan

Re: [PATCH v3 3/3] x86/irq: forward pending interrupts to new destination in fixup_irqs()

[Roger Pau Monné]

On Tue, Jun 18, 2024 at 04:34:50PM +0200, Jan Beulich wrote:
> On 18.06.2024 13:30, Roger Pau Monné wrote:
> > On Mon, Jun 17, 2024 at 03:41:12PM +0200, Jan Beulich wrote:
> >> On 13.06.2024 18:56, Roger Pau Monne wrote:
> >>> fixup_irqs() is used to evacuate interrupts from to be offlined CPUs.  Given
> >>> the CPU is to become offline, the normal migration logic used by Xen where the
> >>> vector in the previous target(s) is left configured until the interrupt is
> >>> received on the new destination is not suitable.
> >>>
> >>> Instead attempt to do as much as possible in order to prevent loosing
> >>> interrupts.  If fixup_irqs() is called from the CPU to be offlined (as is
> >>> currently the case)
> >>
> >> Except (again) for smp_send_stop().
> > 
> > I guess I haven't worded this properly, the point I was trying to make
> > is that in the context of a CPU unplug fixup_irqs() is always called
> > from the CPU that's going offline.
> > 
> > What about:
> > 
> > "If fixup_irqs() is called from the CPU to be offlined (as is
> > currently the case for CPU hot unplug) ..."
> 
> Sounds good to me.
> 
> >>> @@ -2686,11 +2705,27 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
> >>>          if ( desc->handler->disable )
> >>>              desc->handler->disable(desc);
> >>>  
> >>> +        /*
> >>> +         * If the current CPU is going offline and is (one of) the target(s) of
> >>> +         * the interrupt, signal to check whether there are any pending vectors
> >>> +         * to be handled in the local APIC after the interrupt has been moved.
> >>> +         */
> >>> +        if ( !cpu_online(cpu) && cpumask_test_cpu(cpu, desc->arch.cpu_mask) )
> >>> +            check_irr = true;
> >>> +
> >>>          if ( desc->handler->set_affinity )
> >>>              desc->handler->set_affinity(desc, affinity);
> >>>          else if ( !(warned++) )
> >>>              set_affinity = false;
> >>>  
> >>> +        if ( check_irr && apic_irr_read(vector) )
> >>> +            /*
> >>> +             * Forward pending interrupt to the new destination, this CPU is
> >>> +             * going offline and otherwise the interrupt would be lost.
> >>> +             */
> >>> +            send_IPI_mask(cpumask_of(cpumask_any(desc->arch.cpu_mask)),
> >>> +                          desc->arch.vector);
> >>
> >> Hmm, IRR may become set right after the IRR read (unlike in the other cases,
> >> where new IRQs ought to be surfacing only at the new destination). Doesn't
> >> this want moving ...
> >>
> >>>          if ( desc->handler->enable )
> >>>              desc->handler->enable(desc);
> >>
> >> ... past the actual affinity change?
> > 
> > Hm, but the ->enable() hook is just unmasking the interrupt, the
> > actual affinity change is done in ->set_affinity(), and hence after
> > the call to ->set_affinity() no further interrupts should be delivered
> > to the CPU regardless of whether the source is masked?
> > 
> > Or is it possible for the device/interrupt controller to not switch to
> > use the new destination until the interrupt is unmasked, and hence
> > could have pending masked interrupts still using the old destination?
> > IIRC For MSI-X it's required that the device updates the destination
> > target once the entry is unmasked.
> 
> That's all not relevant here, I think. IRR gets set when an interrupt is
> signaled, no matter whether it's masked.

I'm kind of lost here, what does signaling mean in this context?

I would expect the interrupt vector to not get set in IRR if the MSI-X
entry is masked, as at that point the state of the address/data fields
might not be consistent (that's the whole point of masking it right?)

> It's its handling which the
> masking would prevent, i.e. the "moving" of the set bit from IRR to ISR.

My understanding was that the masking would prevent the message write to
the APIC from happening, and hence no vector should get set in IRR.

> Plus we run with IRQs off here anyway if I'm not mistaken, so no
> interrupt can be delivered to the local CPU. IOW whatever IRR bits it
> has set (including ones becoming set between the IRR read and the actual
> vector change), those would never be serviced. Hence the reading of the
> bit ought to occur after the vector change: It's only then that we know
> the IRR bit corresponding to the old vector can't become set anymore.

Right, and the vector change happens in ->set_affinity(), not
->enable().  See for example set_msi_affinity() and the
write_msi_msg(), that's where the vector gets changed.

> And even then we're assuming that no interrupt signals might still be
> "on their way" from the IO-APIC or a posted MSI message write by a
> device to the LAPIC (I have no idea how to properly fence that, or
> whether there are guarantees for this to never occur).

Yeah, those I expect would be completed in the window between the
write of the new vector/destination and the reading of IRR.

Thanks, Roger.

Re: [PATCH v3 3/3] x86/irq: forward pending interrupts to new destination in fixup_irqs()

[Jan Beulich]

On 18.06.2024 16:50, Roger Pau Monné wrote:
> On Tue, Jun 18, 2024 at 04:34:50PM +0200, Jan Beulich wrote:
>> On 18.06.2024 13:30, Roger Pau Monné wrote:
>>> On Mon, Jun 17, 2024 at 03:41:12PM +0200, Jan Beulich wrote:
>>>> On 13.06.2024 18:56, Roger Pau Monne wrote:
>>>>> @@ -2686,11 +2705,27 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
>>>>>          if ( desc->handler->disable )
>>>>>              desc->handler->disable(desc);
>>>>>  
>>>>> +        /*
>>>>> +         * If the current CPU is going offline and is (one of) the target(s) of
>>>>> +         * the interrupt, signal to check whether there are any pending vectors
>>>>> +         * to be handled in the local APIC after the interrupt has been moved.
>>>>> +         */
>>>>> +        if ( !cpu_online(cpu) && cpumask_test_cpu(cpu, desc->arch.cpu_mask) )
>>>>> +            check_irr = true;
>>>>> +
>>>>>          if ( desc->handler->set_affinity )
>>>>>              desc->handler->set_affinity(desc, affinity);
>>>>>          else if ( !(warned++) )
>>>>>              set_affinity = false;
>>>>>  
>>>>> +        if ( check_irr && apic_irr_read(vector) )
>>>>> +            /*
>>>>> +             * Forward pending interrupt to the new destination, this CPU is
>>>>> +             * going offline and otherwise the interrupt would be lost.
>>>>> +             */
>>>>> +            send_IPI_mask(cpumask_of(cpumask_any(desc->arch.cpu_mask)),
>>>>> +                          desc->arch.vector);
>>>>
>>>> Hmm, IRR may become set right after the IRR read (unlike in the other cases,
>>>> where new IRQs ought to be surfacing only at the new destination). Doesn't
>>>> this want moving ...
>>>>
>>>>>          if ( desc->handler->enable )
>>>>>              desc->handler->enable(desc);
>>>>
>>>> ... past the actual affinity change?
>>>
>>> Hm, but the ->enable() hook is just unmasking the interrupt, the
>>> actual affinity change is done in ->set_affinity(), and hence after
>>> the call to ->set_affinity() no further interrupts should be delivered
>>> to the CPU regardless of whether the source is masked?
>>>
>>> Or is it possible for the device/interrupt controller to not switch to
>>> use the new destination until the interrupt is unmasked, and hence
>>> could have pending masked interrupts still using the old destination?
>>> IIRC For MSI-X it's required that the device updates the destination
>>> target once the entry is unmasked.
>>
>> That's all not relevant here, I think. IRR gets set when an interrupt is
>> signaled, no matter whether it's masked.
> 
> I'm kind of lost here, what does signaling mean in this context?
> 
> I would expect the interrupt vector to not get set in IRR if the MSI-X
> entry is masked, as at that point the state of the address/data fields
> might not be consistent (that's the whole point of masking it right?)
> 
>> It's its handling which the
>> masking would prevent, i.e. the "moving" of the set bit from IRR to ISR.
> 
> My understanding was that the masking would prevent the message write to
> the APIC from happening, and hence no vector should get set in IRR.

Hmm, yes, looks like I was confused. The masking is at the source side
(IO-APIC RTE, MSI-X entry, or - if supported - in the MSI capability).
So the sole case to worry about is MSI without mask-bit support then.

>> Plus we run with IRQs off here anyway if I'm not mistaken, so no
>> interrupt can be delivered to the local CPU. IOW whatever IRR bits it
>> has set (including ones becoming set between the IRR read and the actual
>> vector change), those would never be serviced. Hence the reading of the
>> bit ought to occur after the vector change: It's only then that we know
>> the IRR bit corresponding to the old vector can't become set anymore.
> 
> Right, and the vector change happens in ->set_affinity(), not
> ->enable().  See for example set_msi_affinity() and the
> write_msi_msg(), that's where the vector gets changed.
> 
>> And even then we're assuming that no interrupt signals might still be
>> "on their way" from the IO-APIC or a posted MSI message write by a
>> device to the LAPIC (I have no idea how to properly fence that, or
>> whether there are guarantees for this to never occur).
> 
> Yeah, those I expect would be completed in the window between the
> write of the new vector/destination and the reading of IRR.

Except we have no idea on the latencies.

Jan

Re: [PATCH v3 3/3] x86/irq: forward pending interrupts to new destination in fixup_irqs()

[Roger Pau Monné]

On Tue, Jun 18, 2024 at 06:30:22PM +0200, Jan Beulich wrote:
> On 18.06.2024 16:50, Roger Pau Monné wrote:
> > On Tue, Jun 18, 2024 at 04:34:50PM +0200, Jan Beulich wrote:
> >> On 18.06.2024 13:30, Roger Pau Monné wrote:
> >>> On Mon, Jun 17, 2024 at 03:41:12PM +0200, Jan Beulich wrote:
> >>>> On 13.06.2024 18:56, Roger Pau Monne wrote:
> >>>>> @@ -2686,11 +2705,27 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
> >>>>>          if ( desc->handler->disable )
> >>>>>              desc->handler->disable(desc);
> >>>>>  
> >>>>> +        /*
> >>>>> +         * If the current CPU is going offline and is (one of) the target(s) of
> >>>>> +         * the interrupt, signal to check whether there are any pending vectors
> >>>>> +         * to be handled in the local APIC after the interrupt has been moved.
> >>>>> +         */
> >>>>> +        if ( !cpu_online(cpu) && cpumask_test_cpu(cpu, desc->arch.cpu_mask) )
> >>>>> +            check_irr = true;
> >>>>> +
> >>>>>          if ( desc->handler->set_affinity )
> >>>>>              desc->handler->set_affinity(desc, affinity);
> >>>>>          else if ( !(warned++) )
> >>>>>              set_affinity = false;
> >>>>>  
> >>>>> +        if ( check_irr && apic_irr_read(vector) )
> >>>>> +            /*
> >>>>> +             * Forward pending interrupt to the new destination, this CPU is
> >>>>> +             * going offline and otherwise the interrupt would be lost.
> >>>>> +             */
> >>>>> +            send_IPI_mask(cpumask_of(cpumask_any(desc->arch.cpu_mask)),
> >>>>> +                          desc->arch.vector);
> >>>>
> >>>> Hmm, IRR may become set right after the IRR read (unlike in the other cases,
> >>>> where new IRQs ought to be surfacing only at the new destination). Doesn't
> >>>> this want moving ...
> >>>>
> >>>>>          if ( desc->handler->enable )
> >>>>>              desc->handler->enable(desc);
> >>>>
> >>>> ... past the actual affinity change?
> >>>
> >>> Hm, but the ->enable() hook is just unmasking the interrupt, the
> >>> actual affinity change is done in ->set_affinity(), and hence after
> >>> the call to ->set_affinity() no further interrupts should be delivered
> >>> to the CPU regardless of whether the source is masked?
> >>>
> >>> Or is it possible for the device/interrupt controller to not switch to
> >>> use the new destination until the interrupt is unmasked, and hence
> >>> could have pending masked interrupts still using the old destination?
> >>> IIRC For MSI-X it's required that the device updates the destination
> >>> target once the entry is unmasked.
> >>
> >> That's all not relevant here, I think. IRR gets set when an interrupt is
> >> signaled, no matter whether it's masked.
> > 
> > I'm kind of lost here, what does signaling mean in this context?
> > 
> > I would expect the interrupt vector to not get set in IRR if the MSI-X
> > entry is masked, as at that point the state of the address/data fields
> > might not be consistent (that's the whole point of masking it right?)
> > 
> >> It's its handling which the
> >> masking would prevent, i.e. the "moving" of the set bit from IRR to ISR.
> > 
> > My understanding was that the masking would prevent the message write to
> > the APIC from happening, and hence no vector should get set in IRR.
> 
> Hmm, yes, looks like I was confused. The masking is at the source side
> (IO-APIC RTE, MSI-X entry, or - if supported - in the MSI capability).
> So the sole case to worry about is MSI without mask-bit support then.

Yeah, and for MSI without masking bit support we don't care doing the
IRR check before or after the ->enable() hook, as that's a no-op in
that case.  The write to the MSI address/data fields has already been
done, and hence the issue would be exclusively with draining any
in-flight writes to the APIC doorbell (what you mention below).

> >> Plus we run with IRQs off here anyway if I'm not mistaken, so no
> >> interrupt can be delivered to the local CPU. IOW whatever IRR bits it
> >> has set (including ones becoming set between the IRR read and the actual
> >> vector change), those would never be serviced. Hence the reading of the
> >> bit ought to occur after the vector change: It's only then that we know
> >> the IRR bit corresponding to the old vector can't become set anymore.
> > 
> > Right, and the vector change happens in ->set_affinity(), not
> > ->enable().  See for example set_msi_affinity() and the
> > write_msi_msg(), that's where the vector gets changed.
> > 
> >> And even then we're assuming that no interrupt signals might still be
> >> "on their way" from the IO-APIC or a posted MSI message write by a
> >> device to the LAPIC (I have no idea how to properly fence that, or
> >> whether there are guarantees for this to never occur).
> > 
> > Yeah, those I expect would be completed in the window between the
> > write of the new vector/destination and the reading of IRR.
> 
> Except we have no idea on the latencies.

There isn't much else we can do? Even the current case where we add
the 1ms window at the end of the shuffling could still suffer from
this issue because we don't know the latencies.  IOW: I don't think
this is any worse than the current approach.

Thanks, Roger.

Re: [PATCH v3 3/3] x86/irq: forward pending interrupts to new destination in fixup_irqs()

[Jan Beulich]

On 19.06.2024 09:05, Roger Pau Monné wrote:
> On Tue, Jun 18, 2024 at 06:30:22PM +0200, Jan Beulich wrote:
>> On 18.06.2024 16:50, Roger Pau Monné wrote:
>>> On Tue, Jun 18, 2024 at 04:34:50PM +0200, Jan Beulich wrote:
>>>> On 18.06.2024 13:30, Roger Pau Monné wrote:
>>>>> On Mon, Jun 17, 2024 at 03:41:12PM +0200, Jan Beulich wrote:
>>>>>> On 13.06.2024 18:56, Roger Pau Monne wrote:
>>>>>>> @@ -2686,11 +2705,27 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
>>>>>>>          if ( desc->handler->disable )
>>>>>>>              desc->handler->disable(desc);
>>>>>>>  
>>>>>>> +        /*
>>>>>>> +         * If the current CPU is going offline and is (one of) the target(s) of
>>>>>>> +         * the interrupt, signal to check whether there are any pending vectors
>>>>>>> +         * to be handled in the local APIC after the interrupt has been moved.
>>>>>>> +         */
>>>>>>> +        if ( !cpu_online(cpu) && cpumask_test_cpu(cpu, desc->arch.cpu_mask) )
>>>>>>> +            check_irr = true;
>>>>>>> +
>>>>>>>          if ( desc->handler->set_affinity )
>>>>>>>              desc->handler->set_affinity(desc, affinity);
>>>>>>>          else if ( !(warned++) )
>>>>>>>              set_affinity = false;
>>>>>>>  
>>>>>>> +        if ( check_irr && apic_irr_read(vector) )
>>>>>>> +            /*
>>>>>>> +             * Forward pending interrupt to the new destination, this CPU is
>>>>>>> +             * going offline and otherwise the interrupt would be lost.
>>>>>>> +             */
>>>>>>> +            send_IPI_mask(cpumask_of(cpumask_any(desc->arch.cpu_mask)),
>>>>>>> +                          desc->arch.vector);
>>>>>>
>>>>>> Hmm, IRR may become set right after the IRR read (unlike in the other cases,
>>>>>> where new IRQs ought to be surfacing only at the new destination). Doesn't
>>>>>> this want moving ...
>>>>>>
>>>>>>>          if ( desc->handler->enable )
>>>>>>>              desc->handler->enable(desc);
>>>>>>
>>>>>> ... past the actual affinity change?
>>>>>
>>>>> Hm, but the ->enable() hook is just unmasking the interrupt, the
>>>>> actual affinity change is done in ->set_affinity(), and hence after
>>>>> the call to ->set_affinity() no further interrupts should be delivered
>>>>> to the CPU regardless of whether the source is masked?
>>>>>
>>>>> Or is it possible for the device/interrupt controller to not switch to
>>>>> use the new destination until the interrupt is unmasked, and hence
>>>>> could have pending masked interrupts still using the old destination?
>>>>> IIRC For MSI-X it's required that the device updates the destination
>>>>> target once the entry is unmasked.
>>>>
>>>> That's all not relevant here, I think. IRR gets set when an interrupt is
>>>> signaled, no matter whether it's masked.
>>>
>>> I'm kind of lost here, what does signaling mean in this context?
>>>
>>> I would expect the interrupt vector to not get set in IRR if the MSI-X
>>> entry is masked, as at that point the state of the address/data fields
>>> might not be consistent (that's the whole point of masking it right?)
>>>
>>>> It's its handling which the
>>>> masking would prevent, i.e. the "moving" of the set bit from IRR to ISR.
>>>
>>> My understanding was that the masking would prevent the message write to
>>> the APIC from happening, and hence no vector should get set in IRR.
>>
>> Hmm, yes, looks like I was confused. The masking is at the source side
>> (IO-APIC RTE, MSI-X entry, or - if supported - in the MSI capability).
>> So the sole case to worry about is MSI without mask-bit support then.
> 
> Yeah, and for MSI without masking bit support we don't care doing the
> IRR check before or after the ->enable() hook, as that's a no-op in
> that case.  The write to the MSI address/data fields has already been
> done, and hence the issue would be exclusively with draining any
> in-flight writes to the APIC doorbell (what you mention below).

Except that both here ...

>>>> Plus we run with IRQs off here anyway if I'm not mistaken, so no
>>>> interrupt can be delivered to the local CPU. IOW whatever IRR bits it
>>>> has set (including ones becoming set between the IRR read and the actual
>>>> vector change), those would never be serviced. Hence the reading of the
>>>> bit ought to occur after the vector change: It's only then that we know
>>>> the IRR bit corresponding to the old vector can't become set anymore.
>>>
>>> Right, and the vector change happens in ->set_affinity(), not
>>> ->enable().  See for example set_msi_affinity() and the
>>> write_msi_msg(), that's where the vector gets changed.
>>>
>>>> And even then we're assuming that no interrupt signals might still be
>>>> "on their way" from the IO-APIC or a posted MSI message write by a
>>>> device to the LAPIC (I have no idea how to properly fence that, or
>>>> whether there are guarantees for this to never occur).
>>>
>>> Yeah, those I expect would be completed in the window between the
>>> write of the new vector/destination and the reading of IRR.
>>
>> Except we have no idea on the latencies.
> 
> There isn't much else we can do? Even the current case where we add
> the 1ms window at the end of the shuffling could still suffer from
> this issue because we don't know the latencies.  IOW: I don't think
> this is any worse than the current approach.

... and here, the later we read IRR, the better the chances we don't miss
anything. Even the no-op ->enable() isn't a no-op execution-wise. In fact
it (quite pointlessly[1]) is an indirect call to irq_enable_none(). I'm
actually inclined to suggest that we try to even further delay the IRR
read, certainly past the cpumask_copy(), maybe even past the spin_unlock()
(latching CPU and vector into local variables, along with the latching of
->affinity that's already there).

Jan

[1] While back when that was written the main goal probably was to avoid
conditionals on what may be deemed fast paths, I wonder whether nowadays
the main goal wouldn't be to avoid indirect calls when we (pretty) easily
can.

Re: [PATCH v3 3/3] x86/irq: forward pending interrupts to new destination in fixup_irqs()

[Roger Pau Monné]

On Wed, Jun 19, 2024 at 09:24:41AM +0200, Jan Beulich wrote:
> On 19.06.2024 09:05, Roger Pau Monné wrote:
> > On Tue, Jun 18, 2024 at 06:30:22PM +0200, Jan Beulich wrote:
> >> On 18.06.2024 16:50, Roger Pau Monné wrote:
> >>> On Tue, Jun 18, 2024 at 04:34:50PM +0200, Jan Beulich wrote:
> >>>> On 18.06.2024 13:30, Roger Pau Monné wrote:
> >>>>> On Mon, Jun 17, 2024 at 03:41:12PM +0200, Jan Beulich wrote:
> >>>>>> On 13.06.2024 18:56, Roger Pau Monne wrote:
> >>>>>>> @@ -2686,11 +2705,27 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
> >>>>>>>          if ( desc->handler->disable )
> >>>>>>>              desc->handler->disable(desc);
> >>>>>>>  
> >>>>>>> +        /*
> >>>>>>> +         * If the current CPU is going offline and is (one of) the target(s) of
> >>>>>>> +         * the interrupt, signal to check whether there are any pending vectors
> >>>>>>> +         * to be handled in the local APIC after the interrupt has been moved.
> >>>>>>> +         */
> >>>>>>> +        if ( !cpu_online(cpu) && cpumask_test_cpu(cpu, desc->arch.cpu_mask) )
> >>>>>>> +            check_irr = true;
> >>>>>>> +
> >>>>>>>          if ( desc->handler->set_affinity )
> >>>>>>>              desc->handler->set_affinity(desc, affinity);
> >>>>>>>          else if ( !(warned++) )
> >>>>>>>              set_affinity = false;
> >>>>>>>  
> >>>>>>> +        if ( check_irr && apic_irr_read(vector) )
> >>>>>>> +            /*
> >>>>>>> +             * Forward pending interrupt to the new destination, this CPU is
> >>>>>>> +             * going offline and otherwise the interrupt would be lost.
> >>>>>>> +             */
> >>>>>>> +            send_IPI_mask(cpumask_of(cpumask_any(desc->arch.cpu_mask)),
> >>>>>>> +                          desc->arch.vector);
> >>>>>>
> >>>>>> Hmm, IRR may become set right after the IRR read (unlike in the other cases,
> >>>>>> where new IRQs ought to be surfacing only at the new destination). Doesn't
> >>>>>> this want moving ...
> >>>>>>
> >>>>>>>          if ( desc->handler->enable )
> >>>>>>>              desc->handler->enable(desc);
> >>>>>>
> >>>>>> ... past the actual affinity change?
> >>>>>
> >>>>> Hm, but the ->enable() hook is just unmasking the interrupt, the
> >>>>> actual affinity change is done in ->set_affinity(), and hence after
> >>>>> the call to ->set_affinity() no further interrupts should be delivered
> >>>>> to the CPU regardless of whether the source is masked?
> >>>>>
> >>>>> Or is it possible for the device/interrupt controller to not switch to
> >>>>> use the new destination until the interrupt is unmasked, and hence
> >>>>> could have pending masked interrupts still using the old destination?
> >>>>> IIRC For MSI-X it's required that the device updates the destination
> >>>>> target once the entry is unmasked.
> >>>>
> >>>> That's all not relevant here, I think. IRR gets set when an interrupt is
> >>>> signaled, no matter whether it's masked.
> >>>
> >>> I'm kind of lost here, what does signaling mean in this context?
> >>>
> >>> I would expect the interrupt vector to not get set in IRR if the MSI-X
> >>> entry is masked, as at that point the state of the address/data fields
> >>> might not be consistent (that's the whole point of masking it right?)
> >>>
> >>>> It's its handling which the
> >>>> masking would prevent, i.e. the "moving" of the set bit from IRR to ISR.
> >>>
> >>> My understanding was that the masking would prevent the message write to
> >>> the APIC from happening, and hence no vector should get set in IRR.
> >>
> >> Hmm, yes, looks like I was confused. The masking is at the source side
> >> (IO-APIC RTE, MSI-X entry, or - if supported - in the MSI capability).
> >> So the sole case to worry about is MSI without mask-bit support then.
> > 
> > Yeah, and for MSI without masking bit support we don't care doing the
> > IRR check before or after the ->enable() hook, as that's a no-op in
> > that case.  The write to the MSI address/data fields has already been
> > done, and hence the issue would be exclusively with draining any
> > in-flight writes to the APIC doorbell (what you mention below).
> 
> Except that both here ...
> 
> >>>> Plus we run with IRQs off here anyway if I'm not mistaken, so no
> >>>> interrupt can be delivered to the local CPU. IOW whatever IRR bits it
> >>>> has set (including ones becoming set between the IRR read and the actual
> >>>> vector change), those would never be serviced. Hence the reading of the
> >>>> bit ought to occur after the vector change: It's only then that we know
> >>>> the IRR bit corresponding to the old vector can't become set anymore.
> >>>
> >>> Right, and the vector change happens in ->set_affinity(), not
> >>> ->enable().  See for example set_msi_affinity() and the
> >>> write_msi_msg(), that's where the vector gets changed.
> >>>
> >>>> And even then we're assuming that no interrupt signals might still be
> >>>> "on their way" from the IO-APIC or a posted MSI message write by a
> >>>> device to the LAPIC (I have no idea how to properly fence that, or
> >>>> whether there are guarantees for this to never occur).
> >>>
> >>> Yeah, those I expect would be completed in the window between the
> >>> write of the new vector/destination and the reading of IRR.
> >>
> >> Except we have no idea on the latencies.
> > 
> > There isn't much else we can do? Even the current case where we add
> > the 1ms window at the end of the shuffling could still suffer from
> > this issue because we don't know the latencies.  IOW: I don't think
> > this is any worse than the current approach.
> 
> ... and here, the later we read IRR, the better the chances we don't miss
> anything. Even the no-op ->enable() isn't a no-op execution-wise. In fact
> it (quite pointlessly[1]) is an indirect call to irq_enable_none(). I'm
> actually inclined to suggest that we try to even further delay the IRR
> read, certainly past the cpumask_copy(), maybe even past the spin_unlock()
> (latching CPU and vector into local variables, along with the latching of
> ->affinity that's already there).

Moving past cpumask_copy() would be OK.  Moving past the spin_unlock()
I'm not so sure.  Isn't it possible that once the desc is unlocked the
interrupt starts another movement, and hence by the time the forwarded
vector is injected the irq desc has already moved to yet a different
CPU?

I don't think this is realistic given the window between the
spin_unlock() and the injection of the vector, but could be
possible.

If you are fine with moving past the cpumask_copy() but before the
spin_unlock() I can post an updated version with that.

Thanks, Roger.

Re: [PATCH v3 3/3] x86/irq: forward pending interrupts to new destination in fixup_irqs()

[Jan Beulich]

On 19.06.2024 10:32, Roger Pau Monné wrote:
> On Wed, Jun 19, 2024 at 09:24:41AM +0200, Jan Beulich wrote:
>> On 19.06.2024 09:05, Roger Pau Monné wrote:
>>> On Tue, Jun 18, 2024 at 06:30:22PM +0200, Jan Beulich wrote:
>>>> On 18.06.2024 16:50, Roger Pau Monné wrote:
>>>>> On Tue, Jun 18, 2024 at 04:34:50PM +0200, Jan Beulich wrote:
>>>>>> On 18.06.2024 13:30, Roger Pau Monné wrote:
>>>>>>> On Mon, Jun 17, 2024 at 03:41:12PM +0200, Jan Beulich wrote:
>>>>>>>> On 13.06.2024 18:56, Roger Pau Monne wrote:
>>>>>>>>> @@ -2686,11 +2705,27 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
>>>>>>>>>          if ( desc->handler->disable )
>>>>>>>>>              desc->handler->disable(desc);
>>>>>>>>>  
>>>>>>>>> +        /*
>>>>>>>>> +         * If the current CPU is going offline and is (one of) the target(s) of
>>>>>>>>> +         * the interrupt, signal to check whether there are any pending vectors
>>>>>>>>> +         * to be handled in the local APIC after the interrupt has been moved.
>>>>>>>>> +         */
>>>>>>>>> +        if ( !cpu_online(cpu) && cpumask_test_cpu(cpu, desc->arch.cpu_mask) )
>>>>>>>>> +            check_irr = true;
>>>>>>>>> +
>>>>>>>>>          if ( desc->handler->set_affinity )
>>>>>>>>>              desc->handler->set_affinity(desc, affinity);
>>>>>>>>>          else if ( !(warned++) )
>>>>>>>>>              set_affinity = false;
>>>>>>>>>  
>>>>>>>>> +        if ( check_irr && apic_irr_read(vector) )
>>>>>>>>> +            /*
>>>>>>>>> +             * Forward pending interrupt to the new destination, this CPU is
>>>>>>>>> +             * going offline and otherwise the interrupt would be lost.
>>>>>>>>> +             */
>>>>>>>>> +            send_IPI_mask(cpumask_of(cpumask_any(desc->arch.cpu_mask)),
>>>>>>>>> +                          desc->arch.vector);
>>>>>>>>
>>>>>>>> Hmm, IRR may become set right after the IRR read (unlike in the other cases,
>>>>>>>> where new IRQs ought to be surfacing only at the new destination). Doesn't
>>>>>>>> this want moving ...
>>>>>>>>
>>>>>>>>>          if ( desc->handler->enable )
>>>>>>>>>              desc->handler->enable(desc);
>>>>>>>>
>>>>>>>> ... past the actual affinity change?
>>>>>>>
>>>>>>> Hm, but the ->enable() hook is just unmasking the interrupt, the
>>>>>>> actual affinity change is done in ->set_affinity(), and hence after
>>>>>>> the call to ->set_affinity() no further interrupts should be delivered
>>>>>>> to the CPU regardless of whether the source is masked?
>>>>>>>
>>>>>>> Or is it possible for the device/interrupt controller to not switch to
>>>>>>> use the new destination until the interrupt is unmasked, and hence
>>>>>>> could have pending masked interrupts still using the old destination?
>>>>>>> IIRC For MSI-X it's required that the device updates the destination
>>>>>>> target once the entry is unmasked.
>>>>>>
>>>>>> That's all not relevant here, I think. IRR gets set when an interrupt is
>>>>>> signaled, no matter whether it's masked.
>>>>>
>>>>> I'm kind of lost here, what does signaling mean in this context?
>>>>>
>>>>> I would expect the interrupt vector to not get set in IRR if the MSI-X
>>>>> entry is masked, as at that point the state of the address/data fields
>>>>> might not be consistent (that's the whole point of masking it right?)
>>>>>
>>>>>> It's its handling which the
>>>>>> masking would prevent, i.e. the "moving" of the set bit from IRR to ISR.
>>>>>
>>>>> My understanding was that the masking would prevent the message write to
>>>>> the APIC from happening, and hence no vector should get set in IRR.
>>>>
>>>> Hmm, yes, looks like I was confused. The masking is at the source side
>>>> (IO-APIC RTE, MSI-X entry, or - if supported - in the MSI capability).
>>>> So the sole case to worry about is MSI without mask-bit support then.
>>>
>>> Yeah, and for MSI without masking bit support we don't care doing the
>>> IRR check before or after the ->enable() hook, as that's a no-op in
>>> that case.  The write to the MSI address/data fields has already been
>>> done, and hence the issue would be exclusively with draining any
>>> in-flight writes to the APIC doorbell (what you mention below).
>>
>> Except that both here ...
>>
>>>>>> Plus we run with IRQs off here anyway if I'm not mistaken, so no
>>>>>> interrupt can be delivered to the local CPU. IOW whatever IRR bits it
>>>>>> has set (including ones becoming set between the IRR read and the actual
>>>>>> vector change), those would never be serviced. Hence the reading of the
>>>>>> bit ought to occur after the vector change: It's only then that we know
>>>>>> the IRR bit corresponding to the old vector can't become set anymore.
>>>>>
>>>>> Right, and the vector change happens in ->set_affinity(), not
>>>>> ->enable().  See for example set_msi_affinity() and the
>>>>> write_msi_msg(), that's where the vector gets changed.
>>>>>
>>>>>> And even then we're assuming that no interrupt signals might still be
>>>>>> "on their way" from the IO-APIC or a posted MSI message write by a
>>>>>> device to the LAPIC (I have no idea how to properly fence that, or
>>>>>> whether there are guarantees for this to never occur).
>>>>>
>>>>> Yeah, those I expect would be completed in the window between the
>>>>> write of the new vector/destination and the reading of IRR.
>>>>
>>>> Except we have no idea on the latencies.
>>>
>>> There isn't much else we can do? Even the current case where we add
>>> the 1ms window at the end of the shuffling could still suffer from
>>> this issue because we don't know the latencies.  IOW: I don't think
>>> this is any worse than the current approach.
>>
>> ... and here, the later we read IRR, the better the chances we don't miss
>> anything. Even the no-op ->enable() isn't a no-op execution-wise. In fact
>> it (quite pointlessly[1]) is an indirect call to irq_enable_none(). I'm
>> actually inclined to suggest that we try to even further delay the IRR
>> read, certainly past the cpumask_copy(), maybe even past the spin_unlock()
>> (latching CPU and vector into local variables, along with the latching of
>> ->affinity that's already there).
> 
> Moving past cpumask_copy() would be OK.  Moving past the spin_unlock()
> I'm not so sure.  Isn't it possible that once the desc is unlocked the
> interrupt starts another movement, and hence by the time the forwarded
> vector is injected the irq desc has already moved to yet a different
> CPU?
> 
> I don't think this is realistic given the window between the
> spin_unlock() and the injection of the vector, but could be
> possible.

Hmm, yes, in principle this is possible, if we ignore the fact that bringing
down a CPU is done in stop-machine context (i.e. IRQs are off everywhere and
initiating further movement being impossible). But perhaps indeed better not
to bake in dependencies on that.

> If you are fine with moving past the cpumask_copy() but before the
> spin_unlock() I can post an updated version with that.

Yes, please do.

Jan

Re: [PATCH v3 0/3] x86/irq: fixes for CPU hot{,un}plug

[Roger Pau Monné]

Sorry, forgot to add the for-4.19 tag and Cc Oleksii.

Since we have taken the start of the series, we might as well take the
remaining patches (if other x86 maintainers agree) and attempt to
hopefully fix all the interrupt issues with CPU hotplug/unplug.

FTR: there are further issues when doing CPU hotplug/unplug from a PVH
dom0, but those are out of the scope for 4.19, as I haven't even
started to diagnose what's going on.

Thanks, Roger.

On Thu, Jun 13, 2024 at 06:56:14PM +0200, Roger Pau Monne wrote:
> Hello,
> 
> The following series aim to fix interrupt handling when doing CPU
> plug/unplug operations.  Without this series running:
> 
> cpus=`xl info max_cpu_id`
> while [ 1 ]; do
>     for i in `seq 1 $cpus`; do
>         xen-hptool cpu-offline $i;
>         xen-hptool cpu-online $i;
>     done
> done
> 
> Quite quickly results in interrupts getting lost and "No irq handler for
> vector" messages on the Xen console.  Drivers in dom0 also start getting
> interrupt timeouts and the system becomes unusable.
> 
> After applying the series running the loop over night still result in a
> fully usable system, no  "No irq handler for vector" messages at all, no
> interrupt loses reported by dom0.  Test with x2apic-mode={mixed,cluster}.
> 
> I've attempted to document all code as good as I could, interrupt
> handling has some unexpected corner cases that are hard to diagnose and
> reason about.
> 
> Some XenRT testing is undergoing to ensure no breakages.
> 
> Thanks, Roger.
> 
> Roger Pau Monne (3):
>   x86/irq: deal with old_cpu_mask for interrupts in movement in
>     fixup_irqs()
>   x86/irq: handle moving interrupts in _assign_irq_vector()
>   x86/irq: forward pending interrupts to new destination in fixup_irqs()
> 
>  xen/arch/x86/include/asm/apic.h |   5 +
>  xen/arch/x86/irq.c              | 163 +++++++++++++++++++++++++-------
>  2 files changed, 132 insertions(+), 36 deletions(-)
> 
> -- 
> 2.45.2
> 

Re: [PATCH v3 0/3] x86/irq: fixes for CPU hot{,un}plug

[Oleksii K.]

On Fri, 2024-06-14 at 09:28 +0200, Roger Pau Monné wrote:
> Sorry, forgot to add the for-4.19 tag and Cc Oleksii.
> 
> Since we have taken the start of the series, we might as well take
> the
> remaining patches (if other x86 maintainers agree) and attempt to
> hopefully fix all the interrupt issues with CPU hotplug/unplug.
> 
> FTR: there are further issues when doing CPU hotplug/unplug from a
> PVH
> dom0, but those are out of the scope for 4.19, as I haven't even
> started to diagnose what's going on.
And this issues were before the current patch series was introduced?

~ Oleksii
> 
> Thanks, Roger.
> 
> On Thu, Jun 13, 2024 at 06:56:14PM +0200, Roger Pau Monne wrote:
> > Hello,
> > 
> > The following series aim to fix interrupt handling when doing CPU
> > plug/unplug operations.  Without this series running:
> > 
> > cpus=`xl info max_cpu_id`
> > while [ 1 ]; do
> >     for i in `seq 1 $cpus`; do
> >         xen-hptool cpu-offline $i;
> >         xen-hptool cpu-online $i;
> >     done
> > done
> > 
> > Quite quickly results in interrupts getting lost and "No irq
> > handler for
> > vector" messages on the Xen console.  Drivers in dom0 also start
> > getting
> > interrupt timeouts and the system becomes unusable.
> > 
> > After applying the series running the loop over night still result
> > in a
> > fully usable system, no  "No irq handler for vector" messages at
> > all, no
> > interrupt loses reported by dom0.  Test with x2apic-
> > mode={mixed,cluster}.
> > 
> > I've attempted to document all code as good as I could, interrupt
> > handling has some unexpected corner cases that are hard to diagnose
> > and
> > reason about.
> > 
> > Some XenRT testing is undergoing to ensure no breakages.
> > 
> > Thanks, Roger.
> > 
> > Roger Pau Monne (3):
> >   x86/irq: deal with old_cpu_mask for interrupts in movement in
> >     fixup_irqs()
> >   x86/irq: handle moving interrupts in _assign_irq_vector()
> >   x86/irq: forward pending interrupts to new destination in
> > fixup_irqs()
> > 
> >  xen/arch/x86/include/asm/apic.h |   5 +
> >  xen/arch/x86/irq.c              | 163 +++++++++++++++++++++++++---
> > ----
> >  2 files changed, 132 insertions(+), 36 deletions(-)
> > 
> > -- 
> > 2.45.2
> > 

Re: [PATCH v3 0/3] x86/irq: fixes for CPU hot{,un}plug

[Roger Pau Monné]

On Fri, Jun 14, 2024 at 01:52:59PM +0200, Oleksii K. wrote:
> On Fri, 2024-06-14 at 09:28 +0200, Roger Pau Monné wrote:
> > Sorry, forgot to add the for-4.19 tag and Cc Oleksii.
> > 
> > Since we have taken the start of the series, we might as well take
> > the
> > remaining patches (if other x86 maintainers agree) and attempt to
> > hopefully fix all the interrupt issues with CPU hotplug/unplug.
> > 
> > FTR: there are further issues when doing CPU hotplug/unplug from a
> > PVH
> > dom0, but those are out of the scope for 4.19, as I haven't even
> > started to diagnose what's going on.
> And this issues were before the current patch series was introduced?

Sure, the issues with PVH dom0 cpu hotplug/unplug are additional to
the ones fixed here.

Thanks, Roger.