From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751988AbcGRPLz (ORCPT ); Mon, 18 Jul 2016 11:11:55 -0400 Received: from foss.arm.com ([217.140.101.70]:58819 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751810AbcGRPLy (ORCPT ); Mon, 18 Jul 2016 11:11:54 -0400 Date: Mon, 18 Jul 2016 16:11:46 +0100 From: Morten Rasmussen To: Vincent Guittot Cc: Dietmar Eggemann , Peter Zijlstra , "mingo@redhat.com" , Yuyang Du , mgalbraith@suse.de, linux-kernel Subject: Re: [PATCH v2 06/13] sched: Store maximum per-cpu capacity in root domain Message-ID: <20160718151146.GA6719@e105550-lin.cambridge.arm.com> References: <1466615004-3503-7-git-send-email-morten.rasmussen@arm.com> <578646A8.1070607@arm.com> <20160713163723.GC21816@e105550-lin.cambridge.arm.com> <20160714151518.GD21816@e105550-lin.cambridge.arm.com> <20160715114615.GG21816@e105550-lin.cambridge.arm.com> <20160715160253.GH21816@e105550-lin.cambridge.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jul 18, 2016 at 02:48:42PM +0200, Vincent Guittot wrote: > On 15 July 2016 at 18:02, Morten Rasmussen wrote: > > On Fri, Jul 15, 2016 at 03:39:05PM +0200, Vincent Guittot wrote: > >> On 15 July 2016 at 13:46, Morten Rasmussen wrote: > >> > On Thu, Jul 14, 2016 at 04:15:20PM +0100, Morten Rasmussen wrote: > >> >> On Thu, Jul 14, 2016 at 03:25:36PM +0200, Vincent Guittot wrote: > >> >> > On 13 July 2016 at 18:37, Morten Rasmussen wrote: > >> >> > > Also, for SMT max capacity is less than 1024 already. No? > >> >> > > >> >> > Yes, it is. I haven't looked in details but i think that we could use > >> >> > a capacity of 1024 for SMT with changes that have been done on how to > >> >> > evaluate if a sched_group is overloaded or not. > >> >> > >> >> Changing SMT is a bit more invasive that I had hoped for for this patch > >> >> set. I will see if we can make it work with the current SMT capacities. > >> >> > >> >> > > >> >> > > But we may be able to cater for this in wake_cap() somehow. I can have a > >> >> > > look if Vincent doesn't like this patch. > >> >> > > >> >> > IMO, rd->max_cpu_capacity field doesn't seem to be required for now . > >> >> > >> >> No problem. I will try to get rid of it. I will drop the "arm:" patches > >> >> as well as they would have to be extended to guarantee a max capacity of > >> >> 1024 and we most likely will have to change it again when Juri's DT > >> >> solution hopefully gets merged. > >> > > >> > I have had a closer look at wake_cap() again. Getting rid of > >> > rd->max_cpu_capacity isn't as easy as I thought. > >> > > >> > The fundamental problem is that all we have in wake_cap() is the waking > >> > cpu and previous cpu ids which isn't sufficient to determine whether we > >> > have an asymmetric capacity system or not. A capacity <1024 can either a > >> > little cpu or an SMT thread. We need a third piece of information, which > >> > can be either the highest cpu capacity available in the cpu, or a > >> > flag/variable/function telling us whether we are on an SMT system. > >> > > >> > I see the following solutions to the problem: > >> > > >> > 1. Have a system-wide max_cpu_capacity (as proposed in this patch) which > >> > can let us detect SMT systems as max_cpu_capacity < 1024 implies SMT. > >> > > >> > 2. Change SMT thread capacity to 1024 so we implicitly know that max > >> > capacity is always 1024. As said above, this is a very invasive change > >> > as it would mean that we no longer distinguish between SMP and SMT. > >> > smt_gain and SD_SHARE_CPUCAPACITY would no longer have any effect and > >> > can be ripped out. I would prefer not create a dependency on such a > >> > massive change. We can do the experiment afterwards if needed. > >> > > >> > 3. Detect SMT in wake_cap(). This requires access to the sched_domain > >> > hierarchy as the SD_SHARE_CPUCAPACITY is the only way to detect SMT, > >> > AFAIK, apart from looping through the capacities of all cpus in the > >> > system basically computing max_cpu_capacity each time. > >> > wake_cap() is currently called before rcu_read_lock() that gives us > >> > access to the sched_domain hierarchy. I would have to postpone the > >> > wake_cap() call to being inside the lock and introduce another lookup in > >> > the sched_domain hierarchy which would be executed on every wake-up on > >> > all systems. IMHO, that is a bit ugly. > >> > > >> > I don't really like any of the solutions, but of those three I would go > >> > for the current solution (1) as it is very minimal both in the amount of > >> > code touched/affected and overhead. We can kill it later if we have a > >> > better one, no problem for me. > >> > >> I had solution 2 in mind. I haven't looked deeply the impact but I > >> thought that the main remaining blocking point is in > >> update_numa_stats where it use the fact that the capacity is less than > >> 1024 vat SMT level to compute task_capacity and set has_free_capacity > >> only if we have less than 1 task per core. > >> smt_gain would not be used anymore > > > > Isn't group capacities of also smaller and hence influence load > > balancing decisions? > > It should not because the capacity is now only used to compare groups > together and no more with the 1024 value You may very well be right. It is definitely worth a look, a lot of code can be ripped out if we can move SMT threads to have default capacity. > > > > > I was hoping that we could decouple a full audit of the load-balance > > code from this relatively simple patch set by staying with 1 for now. I > > worry that the changing SMT capacity can turn into a major task. Just > > proving that there is no regressions even if we know it should be, is a > > lot of work. > > Yes, you are probably right on that point I will put together v3 still containing 1. Thanks, Morten