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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.05.2024 18:23, Roger Pau Monné wrote:
> On Thu, May 16, 2024 at 06:04:22PM +0200, Jan Beulich wrote:
>> 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.
> 
> Oh, I get it now.  Ideally we would use cpu_online_map with the to be
> removed CPU set, but that's complicated in this context.
> 
> For the purposes here we might as well avoid the AND of
> ->arch.cpu_mask with cpu_online_map and just check:
> 
> if ( !desc->action || cpumask_subset(desc->arch.cpu_mask, mask) )

Right, just that I wouldn't say "as well" - we simply may not mask with
cpu_online_map, for the reason stated in the earlier reply.

However, I remain unconvinced that we can outright drop the check of
->affinity. While I doubt cpumask_subset() was correct before, if there's
no intersection with cpu_online_map we still need to update ->affinity
too, to avoid it becoming an "impossible" setting. So I continue to think
that the logic as we have it right now may need splitting into two parts,
one dealing with IRQ movement and the other with ->affinity.

> As even if ->arch.cpu_mask has non-online CPUs set aside from the to
> be offlined CPU, it would just mean that we might be shuffling more
> than strictly necessary.

Limiting the overall benefit of your change, but yes.

>  Note this will only be an issue with cluster
> mode, physical mode must always have a single online CPU set in
> ->arch.cpu_mask.

Sure.

Jan