Re: [Xen-devel] [PATCH v3 4/5] x86/smp: use a dedicated scratch cpumask in send_IPI_mask

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.02.2020 11:46, Roger Pau Monne wrote:
> Using scratch_cpumask in send_IPI_mask is not safe because it can be
> called from interrupt context, and hence Xen would have to make sure
> all the users of the scratch cpumask disable interrupts while using
> it.
> 
> Instead introduce a new cpumask to be used by send_IPI_mask, and
> disable interrupts while using it.

The alternative of also adding ...

> --- a/xen/arch/x86/smp.c
> +++ b/xen/arch/x86/smp.c
> @@ -59,6 +59,7 @@ static void send_IPI_shortcut(unsigned int shortcut, int 
> vector,
>      apic_write(APIC_ICR, cfg);
>  }
>  
> +DECLARE_PER_CPU(cpumask_var_t, send_ipi_cpumask);
>  /*
>   * send_IPI_mask(cpumask, vector): sends @vector IPI to CPUs in @cpumask,
>   * excluding the local CPU. @cpumask may be empty.
> @@ -67,7 +68,21 @@ static void send_IPI_shortcut(unsigned int shortcut, int 
> vector,
>  void send_IPI_mask(const cpumask_t *mask, int vector)
>  {
>      bool cpus_locked = false;
> -    cpumask_t *scratch = this_cpu(scratch_cpumask);
> +    cpumask_t *scratch = this_cpu(send_ipi_cpumask);
> +    unsigned long flags;
> +
> +    if ( in_mc() || in_nmi() )

... in_irq() here was considered, and discarded because of? With
this you'd not need the second CPU mask and you'd also be able
to avoid disabling an re-enabling IRQs.

In order to not disturb the partial sentence, a small request on
the previous hunk as well: Could you add a blank line after the
new definition, please?

> +    {
> +        /*
> +         * When in #NMI or #MC context fallback to the old (and simpler) IPI

Note that conventional notation indeed is #MC but just NMI (applies
here, in the description, and also elsewhere in the series).

> @@ -81,7 +96,15 @@ void send_IPI_mask(const cpumask_t *mask, int vector)
>           local_irq_is_enabled() && (cpus_locked = get_cpu_maps()) &&
>           (park_offline_cpus ||
>            cpumask_equal(&cpu_online_map, &cpu_present_map)) )
> +    {
> +        /*
> +         * send_IPI_mask can be called from interrupt context, and hence we
> +         * need to disable interrupts in order to protect the per-cpu
> +         * send_ipi_cpumask while being used.
> +         */
> +        local_irq_save(flags);
>          cpumask_or(scratch, mask, cpumask_of(smp_processor_id()));
> +    }
>      else
>      {
>          if ( cpus_locked )
> @@ -89,6 +112,7 @@ void send_IPI_mask(const cpumask_t *mask, int vector)
>              put_cpu_maps();
>              cpus_locked = false;
>          }
> +        local_irq_save(flags);
>          cpumask_clear(scratch);
>      }
>  
> @@ -97,6 +121,7 @@ void send_IPI_mask(const cpumask_t *mask, int vector)
>      else
>          alternative_vcall(genapic.send_IPI_mask, mask, vector);
>  
> +    local_irq_restore(flags);

Wouldn't it be better to re-enable interrupts in the "else" branch
visible in context ahead of the call?

Jan

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