Message-ID: <5527FB62.1010209@ezchip.com>
Date:	Fri, 10 Apr 2015 12:33:38 -0400
From:	Chris Metcalf <cmetcalf@...hip.com>
To:	Frederic Weisbecker <fweisbec@...il.com>
CC:	Thomas Gleixner <tglx@...utronix.de>,
	Don Zickus <dzickus@...hat.com>,
	<linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] smpboot: allow excluding cpus from the smpboot threads

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.

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.

Or, you can add an extra bool that says "hey, the cpumask is valid",
and that at least makes the register function's work unambiguous.
But, you then never consult that bool field again, which seems a little
odd as part of the published API structure.

Or, we could create a new register function just for use with clients
that want to specify the cpumask at registration time, though that seems
a little clumsy.

Or, we could say that you can't set the cpumask at registration time,
but only by later calling the update_cpumask function.  But this seems
somewhat unfortunate too, particularly since "cpumask" is sitting right
there in a function where every other field is controlled by the client.

Or, we can go back to my original suggestion of a cpumask pointer.
You raised the issue of potential racing between client cpumask updates and
smpboot subsystem updates, but I think it's a red herring -- basically, if
the client sets/clears a bit while a cpu is coming online, it's 
unspecified whether
that cpu ends up with a thread or not; but we don't really care because
the client ends up calling the "update_cpumask" function after we're done
updating, and that forces all the threads to be properly parked or unparked.

The last option seems like the cleanest if you prefer using "struct 
cpumask *"
rather than a valid_cpu function pointer.  But let me spin a version of the
patch using "struct cpumask *" and you and Thomas can chime in with
which one you prefer (or if you prefer a different model).

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

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

Powered by blists - more mailing lists