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

[Frederic Weisbecker]

On Fri, Apr 10, 2015 at 12:33:38PM -0400, Chris Metcalf wrote:
> On 04/09/2015 09:58 PM, Frederic Weisbecker wrote:
> >On Thu, Apr 09, 2015 at 04:29:01PM -0400, Chris Metcalf wrote:
> >>--- 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.
> 
> I stuck with it since Thomas mentioned valid_cpu() as part of his earlier
> suggestion to just park/unpark the threads, so I was assuming he had
> a preference for that approach.

Hmm, that's not quite what he mentioned. He suggested to check whether the
CPU is valid in the unpark function, that didn't necessary imply to do it
through a callback.

> 
> The problem with the code you provided, as I see it, is that the cpumask
> field being kept in the struct smp_hotplug_thread is awkward to
> initialize while keeping the default that it doesn't have to be mentioned
> in the initializer for the client's structure. To make this work, in the
> register function you have to check for a NULL pointer (for OFFSTACK)
> and then allocate and initialize to cpu_possible_mask, but in the
> !OFFSTACK case you could just require that an empty mask really means
> cpu_possible_mask, which seems like an unfortunate overloading.

If the field is of type "struct cpumask *", just checking NULL is enough.
I don't think OFFSTACK changes anything. This only changes the allocation
on the client side. But the pointer passed to the "struct smp_hotplug_thread"
is the same and that's all transparent to the smpboot subsystem.

Also if the cpumask is NULL on that struct (default), let the smpboot
subsystem attribute cpu_possible_mask to it (no need to allocate a copy).
Well this could even not be overwritten and handled by smpboot_thread_unpark()
itself.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/