Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753060AbbD0QIC (ORCPT ); Mon, 27 Apr 2015 12:08:02 -0400 Received: from mail-vn0-f48.google.com ([209.85.216.48]:37586 "EHLO mail-vn0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752184AbbD0QIA (ORCPT ); Mon, 27 Apr 2015 12:08:00 -0400 Date: Mon, 27 Apr 2015 12:07:56 -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 V8] workqueue: Allow modifying low level unbound workqueue cpumask Message-ID: <20150427160756.GE1499@htj.duckdns.org> References: <1430128720-3021-1-git-send-email-laijs@cn.fujitsu.com> <1430128720-3021-4-git-send-email-laijs@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1430128720-3021-4-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 Content-Length: 3520 Lines: 106 Hello, Lai. Overall, it looks good, just a couple more nits. On Mon, Apr 27, 2015 at 05:58:40PM +0800, Lai Jiangshan wrote: > The oreder-workquue is ignore from the low level unbound workqueue Ordered workqueues are ignored > cpumask, it will be handled in near future. > > All the (default & per-nodes') pwqs are mandatorily controlled by default & per-node > the low level cpumask. If the user configured cpumask doesn't overlap > with the low level cpumask, the low level cpumask will be used for the > wq instead. > > The default wq_unbound_cpumask is still cpu_possible_mask due to the workqueue > subsystem doesn't know what is the best default value for the runtime, the > system manager or other subsystem which knows the sufficient information should set > it when needed. Please re-flow the paragraph. Also, ultimately, we want this to consider isolcpus, right? > --- a/include/linux/workqueue.h > +++ b/include/linux/workqueue.h > @@ -424,6 +424,7 @@ struct workqueue_attrs *alloc_workqueue_attrs(gfp_t gfp_mask); > void free_workqueue_attrs(struct workqueue_attrs *attrs); > int apply_workqueue_attrs(struct workqueue_struct *wq, > const struct workqueue_attrs *attrs); > +int workqueue_set_unbound_cpumask(cpumask_var_t cpumask); Why is this a public function? > --- a/kernel/workqueue.c > +++ b/kernel/workqueue.c > @@ -3548,13 +3549,18 @@ apply_wqattrs_prepare(struct workqueue_struct *wq, > * If something goes wrong during CPU up/down, we'll fall back to > * the default pwq covering whole @attrs->cpumask. Always create > * it even if we don't use it immediately. > + * > + * If the user configured cpumask doesn't overlap with the > + * wq_unbound_cpumask, we fallback to the wq_unbound_cpumask. > */ > + if (unlikely(cpumask_empty(new_attrs->cpumask))) > + cpumask_copy(new_attrs->cpumask, wq_unbound_cpumask); Please see below. > ctx->dfl_pwq = alloc_unbound_pwq(wq, new_attrs); > if (!ctx->dfl_pwq) > goto out_free; > > for_each_node(node) { > - if (wq_calc_node_cpumask(attrs, node, -1, tmp_attrs->cpumask)) { > + if (wq_calc_node_cpumask(new_attrs, node, -1, tmp_attrs->cpumask)) { > ctx->pwq_tbl[node] = alloc_unbound_pwq(wq, tmp_attrs); > if (!ctx->pwq_tbl[node]) > goto out_free; > @@ -3564,7 +3570,10 @@ apply_wqattrs_prepare(struct workqueue_struct *wq, > } > } > > + /* save the user configured attrs */ > + cpumask_and(new_attrs->cpumask, attrs->cpumask, cpu_possible_mask); Wouldn't this make a lot more sense above when copying @attrs into @new_attrs? The comment there even says "make a copy of @attrs and sanitize it". Copy to @new_attrs, mask with wq_unbound_cpumask and fall back to wq_unbound_cpumask if empty. > +static int workqueue_apply_unbound_cpumask(void) > +{ ... > + list_for_each_entry_safe(ctx, n, &ctxs, list) { Is the following list_del() necessary? The list is never used again, right? > + list_del(&ctx->list); > + if (!ret) > + apply_wqattrs_commit(ctx); > + apply_wqattrs_cleanup(ctx); > + } > + > + return ret; > +} ... > +int workqueue_set_unbound_cpumask(cpumask_var_t cpumask) > +{ ... > +} > +EXPORT_SYMBOL_GPL(workqueue_set_unbound_cpumask); Again, why is this exported? Who's the expected user? Thanks. -- tejun -- 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/