[lkml]   [2015]   [Apr]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] smpboot: allow excluding cpus from the smpboot threads
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:
> 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:

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:

Right, I'll have a look at this.


> 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.
> Thanks!
> --
> Chris Metcalf, EZChip Semiconductor

 \ /
  Last update: 2015-04-14 00:21    [W:0.184 / U:2.380 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site