Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933290AbbD1KMz (ORCPT ); Tue, 28 Apr 2015 06:12:55 -0400 Received: from cn.fujitsu.com ([59.151.112.132]:3672 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S933106AbbD1KMw (ORCPT ); Tue, 28 Apr 2015 06:12:52 -0400 X-IronPort-AV: E=Sophos;i="5.04,848,1406563200"; d="scan'208";a="91351629" Message-ID: <553F5DEB.1040703@cn.fujitsu.com> Date: Tue, 28 Apr 2015 18:16:11 +0800 From: Lai Jiangshan User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Tejun Heo CC: , 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 References: <1430128720-3021-1-git-send-email-laijs@cn.fujitsu.com> <1430128720-3021-4-git-send-email-laijs@cn.fujitsu.com> <20150427160756.GE1499@htj.duckdns.org> <553EE60C.1040503@cn.fujitsu.com> <553EEF5F.1090703@cn.fujitsu.com> <20150428034918.GD2152@htj.duckdns.org> In-Reply-To: <20150428034918.GD2152@htj.duckdns.org> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.167.226.103] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3925 Lines: 104 On 04/28/2015 11:49 AM, Tejun Heo wrote: > Hello, > > On Tue, Apr 28, 2015 at 10:24:31AM +0800, Lai Jiangshan wrote: >>>> 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. >> >> We need to save the user original configured attrs. >> When any time wq_unbound_cpumask is changed, we should use >> the user original configured attrs (cpumask) to re-calculate >> the pwqs and avoid losing any information. > > Sure, we can do that for new_attrs and then mask tmp_attrs further w/ > wq_unbound_cpumask, no? > > Thanks. > We need to pass new_attrs to wq_calc_node_cpumask(). If new_attrs (the first argument of wq_calc_node_cpumask()) is not masked with wq_unbound_cpumask when passed in, wq_calc_node_cpumask() will be much complicated (I tried coding it yesterday). Quote: static bool wq_calc_node_cpumask(const struct workqueue_attrs *attrs, int node, int cpu_going_down, cpumask_t *cpumask) { if (!wq_numa_enabled || attrs->no_numa) goto use_dfl; /* does @node have any online CPUs @attrs wants? */ cpumask_and(cpumask, cpumask_of_node(node), attrs->cpumask); [1] if (cpu_going_down >= 0) cpumask_clear_cpu(cpu_going_down, cpumask); if (cpumask_empty(cpumask)) goto use_dfl; /* yeap, return possible CPUs in @node that @attrs wants */ cpumask_and(cpumask, attrs->cpumask, wq_numa_possible_cpumask[node]); [2] return !cpumask_equal(cpumask, attrs->cpumask); [3] use_dfl: cpumask_copy(cpumask, attrs->cpumask); [4] return false; } If @attrs is not masked with wq_unbound_cpumask when passed in, the code needs add two maskings (with wq_unbound_cpumask) at [1] and [2]. And the code requests to get the cpumask of the default pwq at [3]&[4], thus the code need to (re-)calculate the default pwq's attrs here and doubles the code. (this calculation is already done before this function). It will make all things simple and avoid complicating the wq_calc_node_cpumask(), if wq_calc_node_cpumask() is kept unchanged but accepts only the default pwq's attrs as its first argument. The call-site in wq_update_unbound_numa() is changed in V8 to meet this requirement. @@ -3705,11 +3714,11 @@ static void wq_update_unbound_numa(struct workqueue_struct *wq, int cpu, /* * Let's determine what needs to be done. If the target cpumask is - * different from wq's, we need to compare it to @pwq's and create - * a new one if they don't match. If the target cpumask equals - * wq's, the default pwq should be used. + * different from the default pwq's, we need to compare it to @pwq's + * and create a new one if they don't match. If the target cpumask + * equals the default pwq's, the default pwq should be used. */ - if (wq_calc_node_cpumask(wq->unbound_attrs, node, cpu_off, cpumask)) { + if (wq_calc_node_cpumask(wq->dfl_pwq->pool->attrs, node, cpu_off, cpumask)) { if (cpumask_equal(cpumask, pwq->pool->attrs->cpumask)) goto out_unlock; } else { This requirement is not a new requirement. In the code before this patch, the argument @attrs for wq_calc_node_cpumask() is expected to be the default pwq's attrs which happens to be wq->unbound_attrs all the time. In the code after this patch, the argument @attrs for wq_calc_node_cpumask() is still expected to be the default pwq's attrs which may not be wq->unbound_attrs. So the requirement is not new and wq_calc_node_cpumask() is untouched, but the comment for wq_calc_node_cpumask() needs to be updated which I should have done, forgive me. Thanks, Lai. -- 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/