Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752868AbbDPQtF (ORCPT ); Thu, 16 Apr 2015 12:49:05 -0400 Received: from mail-wi0-f181.google.com ([209.85.212.181]:34934 "EHLO mail-wi0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753744AbbDPQsy (ORCPT ); Thu, 16 Apr 2015 12:48:54 -0400 Date: Thu, 16 Apr 2015 18:48:50 +0200 From: Frederic Weisbecker To: Chris Metcalf Cc: Don Zickus , Ingo Molnar , Andrew Morton , Andrew Jones , chai wen , Ulrich Obergfell , Fabian Frederick , Aaron Tomlin , Ben Zhang , Christoph Lameter , Gilad Ben-Yossef , Steven Rostedt , linux-kernel@vger.kernel.org, Jonathan Corbet , linux-doc@vger.kernel.org, Thomas Gleixner , Peter Zijlstra Subject: Re: [PATCH v8 1/3] smpboot: allow excluding cpus from the smpboot threads Message-ID: <20150416164848.GC16270@lerouge> References: <20150413215423.GA6121@lerouge> <1429040253-7054-1-git-send-email-cmetcalf@ezchip.com> <20150416152808.GA16270@lerouge> <552FDA2E.1090406@ezchip.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <552FDA2E.1090406@ezchip.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2616 Lines: 60 On Thu, Apr 16, 2015 at 11:50:06AM -0400, Chris Metcalf wrote: > On 4/16/2015 11:28 AM, Frederic Weisbecker wrote: > >>+ /* Unpark any threads that were voluntarily parked. */ > >>>+ if (ht->cpumask) { > >>>+ cpumask_andnot(&tmp_mask, cpu_online_mask, ht->cpumask); > >>>+ for_each_cpu(cpu, &tmp_mask) { > >>>+ struct task_struct *tsk = *per_cpu_ptr(ht->store, cpu); > >>>+ if (tsk) > >>>+ kthread_unpark(tsk); > >>>+ } > >>>+ } > >Why do you need to do that? smpboot_destroy_threads() doesn't work on parked threads? > >But kthread_stop() does an explicit unparking. > > Yes, this part left me scratching my head. Experimentally, this was necessary. > I saw the unpark in kthread_stop() but it didn't make things work properly. > Currently it looks like parked threads are only in that state while cores are > being offlined, and then they are killed individually, so it seems likely that > this particular path hasn't been tested before. I'm not sure I understand. You mean that kthreads can be parked only when cores they are affine to are offline? Also I'm scratching my head around kthread_stop() when called on kthreads that are parked on offline cores. I don't see how they can wake up and do the kthread->exited completion since they are only affine to that offline core. But I likely overlooked something. > > >+/* Statically allocated and used under smpboot_threads_lock. */ > >+static struct cpumask tmp_mask; > >+ > >Better allocate the cpumask on need rather than have it resident on memory. > >struct cpumask can be large. Plus we need to worry about locking it. > > > > I was trying to avoid the need to make functions return errors for the > extremely unlikely case of ENOMEM. No one is going to check that error > return in practice anyway; programmers are lazy. It seemed easy to > allocate one mask statically and use it under the lock; even large systems aren't > likely to burn more than a couple hundred bytes of .bss for this. Sure, but I guess it's a common practice to allocate temporary cpumasks. I can't see much "static struct cpumask" around that are used for temporary stuffs. > > But, if you'd prefer using allocation and the error-return model, I can > certainly change the code to do that. There is always a caller to return -ENOMEM to ;-) > > -- > 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/