Re: [Xen-devel] [PATCH v3] x86/cpu: Sync any remaining RCU callbacks before CPU up/down

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.03.2020 16:33, Igor Druzhinin wrote:
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -305,7 +305,6 @@ static int enter_state(u32 state)
>      cpufreq_add_cpu(0);
>  
>   enable_cpu:
> -    rcu_barrier();
>      mtrr_aps_sync_begin();
>      enable_nonboot_cpus();
>      mtrr_aps_sync_end();

I take it you remove the invocation here because of being redundant
with the cpu_up() in enable_nonboot_cpus(). Is this safe / correct
in all cases? For one, it's not obvious to me that
mtrr_aps_sync_begin() couldn't rely on RCU syncing to have happened.
And then enable_nonboot_cpus() may not call cpu_up() at all,
because of the park_offline_cpus-based early loop continuation in
the function.

> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -85,11 +85,7 @@ long cpu_up_helper(void *data)
>      int ret = cpu_up(cpu);
>  
>      if ( ret == -EBUSY )
> -    {
> -        /* On EBUSY, flush RCU work and have one more go. */
> -        rcu_barrier();
>          ret = cpu_up(cpu);
> -    }
>  
>      if ( !ret && !opt_smt &&
>           cpu_data[cpu].compute_unit_id == INVALID_CUID &&
> @@ -110,11 +106,7 @@ long cpu_down_helper(void *data)
>      int cpu = (unsigned long)data;
>      int ret = cpu_down(cpu);
>      if ( ret == -EBUSY )
> -    {
> -        /* On EBUSY, flush RCU work and have one more go. */
> -        rcu_barrier();
>          ret = cpu_down(cpu);
> -    }

In both cases I think the comments would better be retained (in
an adjusted shape).

Jan

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