lkml.org 
[lkml]   [2016]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v2 06/13] sched: Store maximum per-cpu capacity in root domain
    On 15 July 2016 at 18:02, Morten Rasmussen <morten.rasmussen@arm.com> wrote:
    > On Fri, Jul 15, 2016 at 03:39:05PM +0200, Vincent Guittot wrote:
    >> On 15 July 2016 at 13:46, Morten Rasmussen <morten.rasmussen@arm.com> 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 <morten.rasmussen@arm.com> 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

    >
    > 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'm happy to look at the SMT stuff it has been on my list of outstanding
    > issues for a very long time, but I would prefer to break it into
    > multiple independent patch sets to keep them focused. I haven't had a
    > much luck with massive complicated patch sets so far ;-)

    \
     
     \ /
      Last update: 2016-07-18 15:41    [W:7.738 / U:0.056 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site