Re: [Xen-devel] [PATCH] x86/flush: use APIC ALLBUT destination shorthand when possible

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.12.2019 13:44, Roger Pau Monne wrote:
> @@ -227,14 +233,47 @@ void flush_area_mask(const cpumask_t *mask, const void 
> *va, unsigned int flags)
>      if ( (flags & ~FLUSH_ORDER_MASK) &&
>           !cpumask_subset(mask, cpumask_of(cpu)) )
>      {
> +        bool cpus_locked = false;
> +
>          spin_lock(&flush_lock);
>          cpumask_and(&flush_cpumask, mask, &cpu_online_map);
>          cpumask_clear_cpu(cpu, &flush_cpumask);
>          flush_va      = va;
>          flush_flags   = flags;
> -        send_IPI_mask(&flush_cpumask, INVALIDATE_TLB_VECTOR);
> +
> +        /*
> +         * Prevent any CPU hot{un}plug while sending the IPIs if we are to 
> use
> +         * a shorthand, also refuse to use a shorthand if not all CPUs are
> +         * online or have been parked.
> +         */
> +        if ( system_state > SYS_STATE_smp_boot && !cpu_overflow &&
> +             (cpus_locked = get_cpu_maps()) &&
> +             (park_offline_cpus ||

Why is it relevant whether we park offline CPUs, or whether we've
even brought up all of the ones a system has? An IPI, in particular
a broadcast one, shouldn't have any issue getting delivered if some
of the nominal recipients don't listen, should it? (The use of
cpu_online_map that was already there above is a sign - but not a
proof, as it may itself be buggy - that the set of online CPUs
fluctuating behind this function's back ought to not be a problem.)

Further a question on lock nesting: Since the commit message
doesn't say anything in this regard, did you check there are no
TLB flush invocations with the get_cpu_maps() lock held? Even if
you did and even if there are none, I think the function should
then get a comment attached to the effect of this lock order
inversion risk. (For example, it isn't obvious to me that no user
of stop_machine() would ever want to do any kind of TLB flushing.)

Overall I wonder whether your goal couldn't be achieved without
the extra locking and without the special conditions.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel