From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932438AbbDMQHF (ORCPT ); Mon, 13 Apr 2015 12:07:05 -0400 Received: from mail-db3on0056.outbound.protection.outlook.com ([157.55.234.56]:42185 "EHLO emea01-db3-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932333AbbDMQHA (ORCPT ); Mon, 13 Apr 2015 12:07:00 -0400 Authentication-Results: infradead.org; dkim=none (message not signed) header.d=none; Message-ID: <552BE99A.1050607@ezchip.com> Date: Mon, 13 Apr 2015 12:06:50 -0400 From: Chris Metcalf User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Frederic Weisbecker CC: Thomas Gleixner , Don Zickus , , Peter Zijlstra Subject: Re: [PATCH] smpboot: allow excluding cpus from the smpboot threads References: <1428611341-27563-1-git-send-email-cmetcalf@ezchip.com> <20150410015842.GG18314@lerouge> <5527FB62.1010209@ezchip.com> <20150412191403.GB23757@lerouge> In-Reply-To: <20150412191403.GB23757@lerouge> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [12.216.194.146] X-ClientProxiedBy: BL2PR01CA0018.prod.exchangelabs.com (10.141.66.18) To AM2PR02MB0770.eurprd02.prod.outlook.com (25.163.146.155) X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:AM2PR02MB0770; X-Forefront-Antispam-Report: BMV:1;SFV:NSPM;SFS:(10009020)(6009001)(6049001)(41574002)(76104003)(479174004)(51704005)(24454002)(377454003)(65806001)(36756003)(2950100001)(66066001)(77096005)(65956001)(23746002)(122386002)(40100003)(86362001)(46102003)(4001350100001)(93886004)(77156002)(62966003)(15975445007)(92566002)(47776003)(19580395003)(80316001)(54356999)(76176999)(65816999)(50986999)(64126003)(87976001)(1411001)(83506001)(110136001)(33656002)(87266999)(50466002)(59896002)(42186005)(18886065003);DIR:OUT;SFP:1101;SCL:1;SRVR:AM2PR02MB0770;H:[10.7.0.41];FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(5002010)(5005006);SRVR:AM2PR02MB0770;BCL:0;PCL:0;RULEID:;SRVR:AM2PR02MB0770; X-Forefront-PRVS: 0545EFAC9A X-OriginatorOrg: ezchip.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Apr 2015 16:06:57.2262 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM2PR02MB0770 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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