Re: [patch V3 05/32] stop_machine: Provide stop_machine_cpuslocked()

[Paul E. McKenney]

On Wed, May 24, 2017 at 10:15:16AM +0200, Thomas Gleixner wrote:
> From: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> 
> Some call sites of stop_machine() are within a get_online_cpus() protected
> region.
> 
> stop_machine() calls get_online_cpus() as well, which is possible in the
> current implementation but prevents converting the hotplug locking to a
> percpu rwsem.
> 
> Provide stop_machine_cpuslocked() to avoid nested calls to get_online_cpus().
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>

Acked-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>

> ---
>  include/linux/stop_machine.h |   26 +++++++++++++++++++++++---
>  kernel/stop_machine.c        |   11 +++++++----
>  2 files changed, 30 insertions(+), 7 deletions(-)
> 
> Index: b/include/linux/stop_machine.h
> ===================================================================
> --- a/include/linux/stop_machine.h
> +++ b/include/linux/stop_machine.h
> @@ -116,15 +116,29 @@ static inline int try_stop_cpus(const st
>   * @fn() runs.
>   *
>   * This can be thought of as a very heavy write lock, equivalent to
> - * grabbing every spinlock in the kernel. */
> + * grabbing every spinlock in the kernel.
> + *
> + * Protects against CPU hotplug.
> + */
>  int stop_machine(cpu_stop_fn_t fn, void *data, const struct cpumask *cpus);
> 
> +/**
> + * stop_machine_cpuslocked: freeze the machine on all CPUs and run this function
> + * @fn: the function to run
> + * @data: the data ptr for the @fn()
> + * @cpus: the cpus to run the @fn() on (NULL = any online cpu)
> + *
> + * Same as above. Must be called from with in a cpus_read_lock() protected
> + * region. Avoids nested calls to cpus_read_lock().
> + */
> +int stop_machine_cpuslocked(cpu_stop_fn_t fn, void *data, const struct cpumask *cpus);
> +
>  int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
>  				   const struct cpumask *cpus);
>  #else	/* CONFIG_SMP || CONFIG_HOTPLUG_CPU */
> 
> -static inline int stop_machine(cpu_stop_fn_t fn, void *data,
> -				 const struct cpumask *cpus)
> +static inline int stop_machine_cpuslocked(cpu_stop_fn_t fn, void *data,
> +					  const struct cpumask *cpus)
>  {
>  	unsigned long flags;
>  	int ret;
> @@ -134,6 +148,12 @@ static inline int stop_machine(cpu_stop_
>  	return ret;
>  }
> 
> +static inline int stop_machine(cpu_stop_fn_t fn, void *data,
> +			       const struct cpumask *cpus)
> +{
> +	return stop_machine_cpuslocked(fn, data, cpus);
> +}
> +
>  static inline int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
>  						 const struct cpumask *cpus)
>  {
> Index: b/kernel/stop_machine.c
> ===================================================================
> --- a/kernel/stop_machine.c
> +++ b/kernel/stop_machine.c
> @@ -552,7 +552,8 @@ static int __init cpu_stop_init(void)
>  }
>  early_initcall(cpu_stop_init);
> 
> -static int __stop_machine(cpu_stop_fn_t fn, void *data, const struct cpumask *cpus)
> +int stop_machine_cpuslocked(cpu_stop_fn_t fn, void *data,
> +			    const struct cpumask *cpus)
>  {
>  	struct multi_stop_data msdata = {
>  		.fn = fn,
> @@ -561,6 +562,8 @@ static int __stop_machine(cpu_stop_fn_t
>  		.active_cpus = cpus,
>  	};
> 
> +	lockdep_assert_cpus_held();
> +
>  	if (!stop_machine_initialized) {
>  		/*
>  		 * Handle the case where stop_machine() is called
> @@ -590,9 +593,9 @@ int stop_machine(cpu_stop_fn_t fn, void
>  	int ret;
> 
>  	/* No CPUs can come up or down during this. */
> -	get_online_cpus();
> -	ret = __stop_machine(fn, data, cpus);
> -	put_online_cpus();
> +	cpus_read_lock();
> +	ret = stop_machine_cpuslocked(fn, data, cpus);
> +	cpus_read_unlock();
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(stop_machine);
> 
>