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
