Re: [PATCH v2 5/7] x86/irq: deal with old_cpu_mask for interrupts in movement in fixup_irqs()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.06.2024 16:20, Roger Pau Monne wrote:
> Given the current logic it's possible for ->arch.old_cpu_mask to get out of
> sync: if a CPU set in old_cpu_mask is offlined and then onlined
> again without old_cpu_mask having been updated the data in the mask will no
> longer be accurate, as when brought back online the CPU will no longer have
> old_vector configured to handle the old interrupt source.
> 
> If there's an interrupt movement in progress, and the to be offlined CPU 
> (which
> is the call context) is in the old_cpu_mask clear it and update the mask, so 
> it
> doesn't contain stale data.

This imo is too __cpu_disable()-centric. In the code you cover the
smp_send_stop() case afaict, where it's all _other_ CPUs which are being
offlined. As we're not meaning to bring CPUs online again in that case,
dealing with the situation likely isn't needed. Yet the description should
imo at least make clear that the case was considered.

> @@ -2589,6 +2589,28 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
>                                 affinity);
>          }
>  
> +        if ( desc->arch.move_in_progress &&
> +             !cpumask_test_cpu(cpu, &cpu_online_map) &&

This part of the condition is, afaict, what covers (excludes) the
smp_send_stop() case. Might be nice to have a brief comment here, thus
also clarifying ...

> +             cpumask_test_cpu(cpu, desc->arch.old_cpu_mask) )
> +        {
> +            /*
> +             * This CPU is going offline, remove it from ->arch.old_cpu_mask
> +             * and possibly release the old vector if the old mask becomes
> +             * empty.
> +             *
> +             * Note cleaning ->arch.old_cpu_mask is required if the CPU is
> +             * brought offline and then online again, as when re-onlined the
> +             * per-cpu vector table will no longer have ->arch.old_vector
> +             * setup, and hence ->arch.old_cpu_mask would be stale.
> +             */
> +            cpumask_clear_cpu(cpu, desc->arch.old_cpu_mask);
> +            if ( cpumask_empty(desc->arch.old_cpu_mask) )
> +            {
> +                desc->arch.move_in_progress = 0;
> +                release_old_vec(desc);
> +            }

... that none of this is really wanted or necessary in that other case.
Assuming my understanding above is correct, the code change itself is
once again
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
yet here I'm uncertain whether to offer on-commit editing, as it's not
really clear to me whether there's a dependencies on patch 4.

Jan