Messages in this thread | | | Date | Fri, 10 Apr 2015 03:58:43 +0200 | From | Frederic Weisbecker <> | Subject | Re: [PATCH] smpboot: allow excluding cpus from the smpboot threads |
| |
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 >
| |