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

[Frederic Weisbecker <fweisbec@gmail.com>]

On Mon, Apr 13, 2015 at 12:06:50PM -0400, Chris Metcalf wrote:
> >>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.

That's actually a common pattern. Check out struct timer_list,
it is allocated and pre-filled by the client. The "expires" field is
initialized by the client which then calls add_timer() to arm it.
Now if you want to modify the expiration of the timer while it's
queued, raw-modifying the "expires" field won't work much as expected.
You need to do that through mod_timer().

You seldom can directly change the field of an object while it's live
handled by another subsystem.

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

Let's see that on the thread.

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

Right, I'll have a look at this.

Thanks.

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