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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.05.2024 17:56, Roger Pau Monné wrote:
> On Thu, May 16, 2024 at 05:00:54PM +0200, Jan Beulich wrote:
>> 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.
> 
> Yeah, I didn't want to get into updating ->affinity in this patch, so
> decided to leave that as-is.
> 
> Note however that if we shuffle the interrupt around we should update
> ->affinity, so that the new destination is part of ->affinity?

I would put it differently: If we shuffle the IRQ around, we want to
respect ->affinity if at all possible. Only if that's impossible (all CPUs
in ->affinity offline) we may need to update ->affinity as well. Issue is
that ...

> Otherwise we could end up with the interrupt assigned to CPU(s) that
> are not part of the ->affinity mask.  Maybe that's OK, TBH I'm not
> sure I understand the purpose of the ->affinity mask, hence why I've
> decided to leave it alone in this patch.

..., as you say, it's not entirely clear what ->affinity's purpose is, and
hence whether it might be okay(ish) to leave it without any intersection
with online CPUs. If we were to permit that, I'm relatively sure though
that then other code may need updating (it'll at least need auditing).

>>> @@ -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.
> 
> Oh, so it's allowed to have the interrupt target a CPU
> (->arch.cpu_mask) that's not set in the affinity mask?

To be honest I'm not quite sure whether it's "allowed" or merely "happens
to".

Jan