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)
