From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758101AbbDVTjn (ORCPT ); Wed, 22 Apr 2015 15:39:43 -0400 Received: from mail-qg0-f52.google.com ([209.85.192.52]:33655 "EHLO mail-qg0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758039AbbDVTjj (ORCPT ); Wed, 22 Apr 2015 15:39:39 -0400 Date: Wed, 22 Apr 2015 15:39:35 -0400 From: Tejun Heo To: Lai Jiangshan Cc: linux-kernel@vger.kernel.org, Christoph Lameter , Kevin Hilman , Mike Galbraith , "Paul E. McKenney" , Viresh Kumar , Frederic Weisbecker Subject: Re: [PATCH 3/3 V7] workqueue: Allow modifying low level unbound workqueue cpumask Message-ID: <20150422193935.GG10738@htj.duckdns.org> References: <1427973282-3052-1-git-send-email-laijs@cn.fujitsu.com> <1428405998-3102-1-git-send-email-laijs@cn.fujitsu.com> <1428405998-3102-3-git-send-email-laijs@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1428405998-3102-3-git-send-email-laijs@cn.fujitsu.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 Hello, Generally looks good to me. Some minor things below. On Tue, Apr 07, 2015 at 07:26:37PM +0800, Lai Jiangshan wrote: > diff --git a/kernel/workqueue.c b/kernel/workqueue.c > index cbccf5d..557612e 100644 > --- a/kernel/workqueue.c > +++ b/kernel/workqueue.c > @@ -299,7 +299,7 @@ static DEFINE_SPINLOCK(wq_mayday_lock); /* protects wq->maydays list */ > static LIST_HEAD(workqueues); /* PR: list of all workqueues */ > static bool workqueue_freezing; /* PL: have wqs started freezing? */ > > -static cpumask_var_t wq_unbound_global_cpumask; > +static cpumask_var_t wq_unbound_global_cpumask; /* PL: low level cpumask for all unbound wqs */ Are we set on this variable name? What would we lose by naming it wq_unbound_cpumask or wq_cpu_possible_mask? > @@ -3493,6 +3493,7 @@ static struct pool_workqueue *numa_pwq_tbl_install(struct workqueue_struct *wq, > struct apply_wqattrs_ctx { > struct workqueue_struct *wq; /* target to be applied */ > struct workqueue_attrs *attrs; /* configured attrs */ > + struct list_head list; /* queued for batching commit */ batch commit > struct pool_workqueue *dfl_pwq; > struct pool_workqueue *pwq_tbl[]; > }; > @@ -3517,7 +3518,8 @@ static void apply_wqattrs_cleanup(struct apply_wqattrs_ctx *ctx) > /* Allocates the attrs and pwqs for later installment */ > static struct apply_wqattrs_ctx * > apply_wqattrs_prepare(struct workqueue_struct *wq, > - const struct workqueue_attrs *attrs) > + const struct workqueue_attrs *attrs, > + cpumask_var_t unbound_cpumask) Why do we need this tho? The global mask is protected by pool mutex, right? The update function can set it to the new value and just call update and revert on failure. > @@ -3710,6 +3721,14 @@ static void wq_update_unbound_numa(struct workqueue_struct *wq, int cpu, > * wq's, the default pwq should be used. > */ > if (wq_calc_node_cpumask(wq->unbound_attrs, node, cpu_off, cpumask)) { > + /* > + * wq->unbound_attrs is the user configured attrs whose > + * cpumask is not masked with wq_unbound_global_cpumask, > + * so we make complete it. > + */ > + cpumask_and(cpumask, cpumask, wq_unbound_global_cpumask); > + if (cpumask_empty(cpumask)) > + goto use_dfl_pwq; Wouldn't it be better to apply the global cpumask before calling wq_calc_node_cpumask()? Or just move it inside wq_calc_node_cpumask? Thanks. -- tejun