Re: [PATCH] smpboot: allow excluding cpus from the smpboot threads
From: Chris Metcalf
Date: Mon Apr 13 2015 - 12:07:09 EST
On 04/12/2015 03:14 PM, Frederic Weisbecker wrote:
On Fri, Apr 10, 2015 at 12:33:38PM -0400, Chris Metcalf wrote:
On 04/09/2015 09:58 PM, Frederic Weisbecker wrote:
Hmm, that's not quite what he mentioned. He suggested to check whether the
On Thu, Apr 09, 2015 at 04:29:01PM -0400, Chris Metcalf wrote:
I stuck with it since Thomas mentioned valid_cpu() as part of his earlier
I'm not sure why this needs to be a callback instead of a pointer to a cpumask
@@ -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.
that smpboot can handle by itself. In fact I don't understand why you want to stick with
this valid_cpu() approach.
suggestion to just park/unpark the threads, so I was assuming he had
a preference for that approach.
CPU is valid in the unpark function, that didn't necessary imply to do it
through a callback.
Fair enough; I may have read his comment too specifically.
The problem with the code you provided, as I see it, is that the cpumask
If the field is of type "struct cpumask *", just checking NULL is enough.
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.
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()
As you saw, I adopted the "struct cpumask *" approach in my current
(v7) patchset last Friday:
There are really two ways to handle this:
1. The client owns the cpumask, and notifies the smpboot subsystem
whenever it has finished a round of changes to the cpumask so that
they can take effect. There is a technical race here where the smpboot
subsystem might look at the mask as it is being updated, but this is
OK since worst-case is a thread on a newly-brought-up core is incorrectly
parked or unparked, but that is corrected immediately when the client
calls in to say it has finished updating the mask.
2. The smpboot subsystem owns the cpumask, and it's only updated
by having the client call in to pass a new mask. This avoids the technical
race, but it does mean that the client can't update a field that it
and provided to the subsystem, which feels a bit unnatural.
Either one could be OK, but I opted for #1. What do you think of it?
Also, I want to ask Linus to pull the tile-specific changes for nohz_full
for the tile architecture. This includes a copy of the change to add the
tick_nohz_full_add_cpus_to() and tick_nohz_full_remove_cpus_from()
which I used to fix the tilegx network driver's default irq affinity mask.
There's also the support for tile's nohz_full in general, which you
commented on, but didn't provide an explicit Ack for:
If you'd like to nack either change, or better yet ack them, let me know.
I'll wait a little while before asking Linus to pull.
The tile tree stuff to be pulled for v4.1 is here:
if you want to look more closely.
Chris Metcalf, EZChip Semiconductor
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/