Re: [PATCH] x86/irq: do not release irq until all cleanup is done

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.11.2022 13:35, Roger Pau Monne wrote:
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -220,27 +220,27 @@ static void _clear_irq_vector(struct irq_desc *desc)
>          clear_bit(vector, desc->arch.used_vectors);
>      }
>  
> -    desc->arch.used = IRQ_UNUSED;
> -
> -    trace_irq_mask(TRC_HW_IRQ_CLEAR_VECTOR, irq, vector, tmp_mask);

The use of tmp_mask here was ...

> +    if ( unlikely(desc->arch.move_in_progress) )
> +    {
> +        /* If we were in motion, also clear desc->arch.old_vector */
> +        old_vector = desc->arch.old_vector;
> +        cpumask_and(tmp_mask, desc->arch.old_cpu_mask, &cpu_online_map);

.. before the variable was updated here. Now you move it past this
update (to the very end of the function). I wonder whether, to keep
things simple in this change, it would be tolerable to leave the
trace_irq_mask() invocation where it was, despite cleanup not having
completed yet at that point. (The alternative would look to be to
act directly on desc->arch.old_cpu_mask in the code you re-indent,
leaving tmp_mask alone. I think that ought to be okay since nothing
else should access that mask anymore. We could even avoid altering
the field, by going from cpumask_and() to a cpu_online() check in
the for_each_cpu() body.)

>  
> -    if ( likely(!desc->arch.move_in_progress) )
> -        return;
> +        for_each_cpu(cpu, tmp_mask)
> +        {
> +            ASSERT(per_cpu(vector_irq, cpu)[old_vector] == irq);
> +            TRACE_3D(TRC_HW_IRQ_MOVE_FINISH, irq, old_vector, cpu);
> +            per_cpu(vector_irq, cpu)[old_vector] = ~irq;
> +        }
>  
> -    /* If we were in motion, also clear desc->arch.old_vector */
> -    old_vector = desc->arch.old_vector;
> -    cpumask_and(tmp_mask, desc->arch.old_cpu_mask, &cpu_online_map);
> +        release_old_vec(desc);
>  
> -    for_each_cpu(cpu, tmp_mask)
> -    {
> -        ASSERT(per_cpu(vector_irq, cpu)[old_vector] == irq);
> -        TRACE_3D(TRC_HW_IRQ_MOVE_FINISH, irq, old_vector, cpu);
> -        per_cpu(vector_irq, cpu)[old_vector] = ~irq;
> +        desc->arch.move_in_progress = 0;
>      }
>  
> -    release_old_vec(desc);
> +    write_atomic(&desc->arch.used, IRQ_UNUSED);

Now that there is an ordering requirement between these last two writes,
I think you want to add smp_wmb() after the first one (still inside the
inner scope). write_atomic() is only a volatile asm() (which the compiler
may move under certain conditions) and an access through a volatile
pointer (which isn't ordered with non-volatile memory accesses), but it
is specifically not a memory barrier.

Jan

> -    desc->arch.move_in_progress = 0;
> +    trace_irq_mask(TRC_HW_IRQ_CLEAR_VECTOR, irq, vector, tmp_mask);
>  }
>  
>  void __init clear_irq_vector(int irq)