Date: Mon, 13 Apr 2015 23:54:25 +0200
From: Frederic Weisbecker <fweisbec@gmail.com>
To: Chris Metcalf <cmetcalf@ezchip.com>
Cc: Thomas Gleixner <tglx@linutronix.de>, Don Zickus <dzickus@redhat.com>,
        linux-kernel@vger.kernel.org, Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH] smpboot: allow excluding cpus from the smpboot threads
Message-ID: <20150413215423.GA6121@lerouge>
References: <alpine.DEB.2.11.1504082232060.3845@nanos>
 <1428611341-27563-1-git-send-email-cmetcalf@ezchip.com>
 <20150410015842.GG18314@lerouge>
 <5527FB62.1010209@ezchip.com>
 <20150412191403.GB23757@lerouge>
 <552BE99A.1050607@ezchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <552BE99A.1050607@ezchip.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3968
Lines: 95

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/