Re: [PATCH for-4.19] xen/x86: limit interrupt movement done by fixup_irqs()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.05.2024 15:22, Roger Pau Monne wrote:
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2527,7 +2527,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)
>  {

Evacuating is one purpose. Updating affinity, if need be, is another. I've
been wondering more than once though whether it is actually correct /
necessary for ->affinity to be updated by the function. As it stands you
don't remove the respective code, though.

> @@ -2576,7 +2576,12 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
>                  release_old_vec(desc);
>          }
>  
> -        if ( !desc->action || cpumask_subset(desc->affinity, mask) )
> +        /*
> +         * Avoid shuffling the interrupt around if it's assigned to a CPU set
> +         * that's all covered by the requested affinity mask.
> +         */
> +        cpumask_and(affinity, desc->arch.cpu_mask, &cpu_online_map);
> +        if ( !desc->action || cpumask_subset(affinity, mask) )
>          {
>              spin_unlock(&desc->lock);
>              continue;

First my understanding of how the two CPU sets are used: ->affinity is
merely a representation of where the IRQ is permitted to be handled.
->arch.cpu_mask describes all CPUs where the assigned vector is valid
(and would thus need cleaning up when a new vector is assigned). Neither
of the two needs to be a strict subset of the other.

(It's not really clear whether ->arch.cpu_mask is [supposed to be] a
subset of cpu_online_map.)

If that understanding of mine is correct, going from just ->arch.cpu_mask
doesn't look quite right to me, as that may include CPUs not in ->affinity.
As in: Things could be further limited, by also ANDing in ->affinity.

At the same time your(?) and my variant suffer from cpumask_subset()
possibly returning true despite the CPU the IRQ is presently being
handled on being the one that we're in the process of bringing down. In
that case we absolutely cannot skip the move. (I'd like to note that
there are only two possible input values of "mask" for the function. The
case I think you've been looking into is when it's &cpu_online_map. In
which case cpumask_subset() is going to always return true with your
change in place, if I'm not mistaken. That seems to make your change
questionable. Yet with that I guess I'm overlooking something.)

Plus there remains the question of whether updating ->affinity can indeed
be skipped in more cases than it is right now (prior to you patch), or
whether up front we may want to get rid of that updating (in line, I think,
with earlier changes we did elsewhere) altogether.

Jan