Message-ID: <552BE99A.1050607@ezchip.com>
Date:	Mon, 13 Apr 2015 12:06:50 -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/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:
>>> 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.

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
>> 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.

As you saw, I adopted the "struct cpumask *" approach in my current
(v7) patchset last Friday:

https://lkml.org/lkml/2015/4/10/750

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 
allocated
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()
routines here:

https://lkml.org/lkml/2015/4/9/792

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:

https://lkml.org/lkml/2015/3/24/953

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:

http://git.kernel.org/cgit/linux/kernel/git/cmetcalf/linux-tile.git/log/

if you want to look more closely.

Thanks!

-- 
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