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