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:
>>> @@ -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;
>>[...]
>> 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.)
> 
> I might we wrong, but I think you are missing that the to be offlined
> CPU has been removed from cpu_online_map by the time it gets passed
> to fixup_irqs().

Just on this part (I'll need to take more time to reply to other parts):
No, I've specifically paid attention to that fact. Yet for this particular
observation of mine is doesn't matter. If mask == &cpu_online_map, then
no matter what is in cpu_online_map

        cpumask_and(affinity, desc->arch.cpu_mask, &cpu_online_map);

will mask things down to a subset of cpu_online_map, and hence

        if ( !desc->action || cpumask_subset(affinity, mask) )

(effectively being

        if ( !desc->action || cpumask_subset(affinity, &cpu_online_map) )

) is nothing else than

        if ( !desc->action || true )

. Yet that doesn't feel quite right.

Jan