Re: [PATCH] smpboot: allow excluding cpus from the smpboot threads

[Frederic Weisbecker <fweisbec@gmail.com>]

On Thu, Apr 09, 2015 at 04:29:01PM -0400, Chris Metcalf wrote:
> This change allows some cores to be excluded from running the
> smp_hotplug_thread tasks.  The motivating example for this is
> the watchdog threads, which by default we don't want to run
> on any enabled nohz_full cores.
> 
> A new smp_hotplug_thread field is introduced, "valid_cpu", which
> is an optional pointer to a function that returns per-cpu whether
> or not the given smp_hotplug_thread should run on that core; the
> function is called when deciding whether to unpark the thread.
> 
> If a change is made to which cpus are valid, the
> smpboot_repark_percpu_thread() function should be called and
> threads will be suitably parked and unparked.
> 
> Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
> ---
> Thomas, how does this look?  If this seems about right, I'll fold
> in your feedback and put out a patch set that includes the matching
> changes to the watchdog and, if Frederic will take it for the
> nohz queue to the timer tree, send it up that way.  This is just
> compile-tested so far since I have to wrap up for the time being
> and head home.  Final patch will actually be tested :-)
> 
> I took Frederic's suggested patch from a 10,000 foot viewpoint and
> modified it to stick with the valid_cpu() callback approach.
> 
> p.s. I think the smpboot_thread_schedule() declaration in 
> linux/smpboot.h is dead; there doesn't seem to be a definition.
> 
>  include/linux/smpboot.h |  4 ++++
>  kernel/smpboot.c        | 37 +++++++++++++++++++++++++++++++++----
>  2 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/smpboot.h b/include/linux/smpboot.h
> index 13e929679550..7dedbf92420e 100644
> --- a/include/linux/smpboot.h
> +++ b/include/linux/smpboot.h
> @@ -27,6 +27,8 @@ struct smpboot_thread_data;
>   * @pre_unpark:		Optional unpark function, called before the thread is
>   *			unparked (cpu online). This is not guaranteed to be
>   *			called on the target cpu of the thread. Careful!
> + * @valid_cpu:		Optional function, called when unparking the threads,
> + *			to limit the set of cpus on which threads are unparked.

I'm not sure why this needs to be a callback instead of a pointer to a cpumask
that smpboot can handle by itself. In fact I don't understand why you want to stick with
this valid_cpu() approach.

>   * @selfparking:	Thread is not parked by the park function.
>   * @thread_comm:	The base name of the thread
>   */
> @@ -41,12 +43,14 @@ struct smp_hotplug_thread {
>  	void				(*park)(unsigned int cpu);
>  	void				(*unpark)(unsigned int cpu);
>  	void				(*pre_unpark)(unsigned int cpu);
> +	int				(*valid_cpu)(unsigned int cpu);
>  	bool				selfparking;
>  	const char			*thread_comm;
>  };
>  
>  int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread);
>  void smpboot_unregister_percpu_thread(struct smp_hotplug_thread *plug_thread);
> +void smpboot_repark_percpu_thread(struct smp_hotplug_thread *plug_thread);
>  int smpboot_thread_schedule(void);
>  
>  #endif
> diff --git a/kernel/smpboot.c b/kernel/smpboot.c
> index 40190f28db35..c7dd768a4599 100644
> --- a/kernel/smpboot.c
> +++ b/kernel/smpboot.c
> @@ -218,11 +218,13 @@ int smpboot_create_threads(unsigned int cpu)
>  
>  static void smpboot_unpark_thread(struct smp_hotplug_thread *ht, unsigned int cpu)
>  {
> -	struct task_struct *tsk = *per_cpu_ptr(ht->store, cpu);
> +	if (!ht->valid_cpu || ht->valid_cpu(cpu)) {
> +		struct task_struct *tsk = *per_cpu_ptr(ht->store, cpu);
>  
> -	if (ht->pre_unpark)
> -		ht->pre_unpark(cpu);
> -	kthread_unpark(tsk);
> +		if (ht->pre_unpark)
> +			ht->pre_unpark(cpu);
> +		kthread_unpark(tsk);
> +	}
>  }
>  
>  void smpboot_unpark_threads(unsigned int cpu)
> @@ -314,3 +316,30 @@ void smpboot_unregister_percpu_thread(struct smp_hotplug_thread *plug_thread)
>  	put_online_cpus();
>  }
>  EXPORT_SYMBOL_GPL(smpboot_unregister_percpu_thread);
> +
> +/**
> + * smpboot_repark_percpu_thread - Adjust which per_cpu hotplug threads stay parked
> + * @plug_thread:	Hotplug thread descriptor
> + *
> + * After changing what the valid_cpu() callback will return, call this
> + * function to let appropriate threads park and unpark.
> + */
> +void smpboot_repark_percpu_thread(struct smp_hotplug_thread *plug_thread)

That looks to me a bit of an unecessary indirect way to tell "update cpumask".

> +{
> +	unsigned int cpu;
> +
> +	if (!plug_thread->valid_cpu)
> +		return;
> +
> +	get_online_cpus();
> +	mutex_lock(&smpboot_threads_lock);
> +	for_each_online_cpu(cpu) {
> +		if (plug_thread->valid_cpu(cpu))
> +			smpboot_unpark_thread(plug_thread, cpu);
> +		else
> +			smpboot_park_thread(plug_thread, cpu);
> +	}
> +	mutex_unlock(&smpboot_threads_lock);
> +	put_online_cpus();
> +}
> +EXPORT_SYMBOL_GPL(smpboot_repark_percpu_thread);
> -- 
> 2.1.2
>