From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755343AbbDJQeA (ORCPT ); Fri, 10 Apr 2015 12:34:00 -0400 Received: from mail-am1on0097.outbound.protection.outlook.com ([157.56.112.97]:13952 "EHLO emea01-am1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751554AbbDJQd6 (ORCPT ); Fri, 10 Apr 2015 12:33:58 -0400 Message-ID: <5527FB62.1010209@ezchip.com> Date: Fri, 10 Apr 2015 12:33:38 -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> In-Reply-To: <20150410015842.GG18314@lerouge> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [12.216.194.146] X-ClientProxiedBy: DM2PR11CA0013.namprd11.prod.outlook.com (25.160.91.23) To VI1PR02MB0784.eurprd02.prod.outlook.com (25.162.14.146) Authentication-Results: infradead.org; dkim=none (message not signed) header.d=none; X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:VI1PR02MB0784; X-Forefront-Antispam-Report: BMV:1;SFV:NSPM;SFS:(10009020)(6049001)(6009001)(377454003)(479174004)(24454002)(76176999)(2950100001)(50986999)(1411001)(50466002)(87976001)(92566002)(83506001)(122386002)(80316001)(15975445007)(54356999)(87266999)(36756003)(46102003)(59896002)(66066001)(110136001)(19580395003)(64126003)(65956001)(77156002)(47776003)(42186005)(62966003)(86362001)(23746002)(65816999)(77096005)(33656002)(18886065003);DIR:OUT;SFP:1101;SCL:1;SRVR:VI1PR02MB0784;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:VI1PR02MB0784;BCL:0;PCL:0;RULEID:;SRVR:VI1PR02MB0784; X-Forefront-PRVS: 054231DC40 X-OriginatorOrg: ezchip.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Apr 2015 16:33:54.6648 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR02MB0784 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. 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. Or, you can add an extra bool that says "hey, the cpumask is valid", and that at least makes the register function's work unambiguous. But, you then never consult that bool field again, which seems a little odd as part of the published API structure. Or, we could create a new register function just for use with clients that want to specify the cpumask at registration time, though that seems a little clumsy. Or, we could say that you can't set the cpumask at registration time, but only by later calling the update_cpumask function. But this seems somewhat unfortunate too, particularly since "cpumask" is sitting right there in a function where every other field is controlled by the client. Or, we can go back to my original suggestion of a cpumask pointer. You raised the issue of potential racing between client cpumask updates and smpboot subsystem updates, but I think it's a red herring -- basically, if the client sets/clears a bit while a cpu is coming online, it's unspecified whether that cpu ends up with a thread or not; but we don't really care because the client ends up calling the "update_cpumask" function after we're done updating, and that forces all the threads to be properly parked or unparked. The last option seems like the cleanest if you prefer using "struct cpumask *" rather than a valid_cpu function pointer. But let me spin a version of the patch using "struct cpumask *" and you and Thomas can chime in with which one you prefer (or if you prefer a different model). -- Chris Metcalf, EZChip Semiconductor http://www.ezchip.com