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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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) )

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

Thanks, Roger.