lkml.org 
[lkml]   [2012]   [Jul]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 22/40] autonuma: teach CFS about autonuma affinity
    On 06/28/2012 08:56 AM, Andrea Arcangeli wrote:

    > @@ -2621,6 +2622,8 @@ find_idlest_cpu(struct sched_group *group, struct task_struct *p, int this_cpu)
    > load = weighted_cpuload(i);
    >
    > if (load< min_load || (load == min_load&& i == this_cpu)) {
    > + if (!task_autonuma_cpu(p, i))
    > + continue;
    > min_load = load;
    > idlest = i;
    > }

    Is it right to only consider CPUs on the "right" NUMA
    node, or do we want to harvest idle time elsewhere as
    a last resort?

    After your change the comment above find_idlest_cpu
    no longer matches what the function does!

    if (load < min_load || (load == min_load && i ==
    this_cpu)) {
    min_load = load;
    idlest = i;
    }

    Would it make sense for task_autonuma_cpu(p, i) to be
    inside the if ( ) braces, since that is what you are
    trying to do?

    if ((load < min_load || (load == min_load &&
    i == this_cpu)) && task_autonuma_cpu(p, i)) {

    > @@ -2639,24 +2642,27 @@ static int select_idle_sibling(struct task_struct *p, int target)

    These bits make sense.

    > /*
    > * Otherwise, iterate the domains and find an elegible idle cpu.
    > */
    > + idle_target = false;
    > sd = rcu_dereference(per_cpu(sd_llc, target));
    > for_each_lower_domain(sd) {
    > sg = sd->groups;
    > @@ -2670,9 +2676,18 @@ static int select_idle_sibling(struct task_struct *p, int target)
    > goto next;
    > }
    >
    > - target = cpumask_first_and(sched_group_cpus(sg),
    > - tsk_cpus_allowed(p));
    > - goto done;
    > + for_each_cpu_and(i, sched_group_cpus(sg),
    > + tsk_cpus_allowed(p)) {
    > + /* Find autonuma cpu only in idle group */
    > + if (task_autonuma_cpu(p, i)) {
    > + target = i;
    > + goto done;
    > + }
    > + if (!idle_target) {
    > + idle_target = true;
    > + target = i;
    > + }
    > + }

    There already is a for loop right above this:

    for_each_cpu(i, sched_group_cpus(sg)) {
    if (!idle_cpu(i))
    goto next;
    }

    It appears to loop over all the CPUs in a sched group, but
    not really. If the first CPU in the sched group is idle,
    it will fall through.

    If the first CPU in the sched group is not idle, we move
    on to the next sched group, instead of looking at the
    other CPUs in the sched group.

    Peter, Ingo, what is the original code in select_idle_sibling
    supposed to do?
    That original for_each_cpu loop would make more sense if
    it actually looped over each cpu in the group.

    Then we could remember two targets. One idle target, and
    one autonuma-compliant idle target.

    If, after looping over the CPUs, we find no autonuma-compliant
    target, we use the other idle target.

    Does that make sense?

    Am I overlooking something about how the way select_idle_sibling
    is supposed to work?

    > @@ -3195,6 +3217,8 @@ static int move_one_task(struct lb_env *env)
    > {
    > struct task_struct *p, *n;
    >
    > + env->flags |= LBF_NUMA;
    > +numa_repeat:
    > list_for_each_entry_safe(p, n,&env->src_rq->cfs_tasks, se.group_node) {
    > if (throttled_lb_pair(task_group(p), env->src_rq->cpu, env->dst_cpu))
    > continue;
    > @@ -3209,8 +3233,14 @@ static int move_one_task(struct lb_env *env)
    > * stats here rather than inside move_task().
    > */
    > schedstat_inc(env->sd, lb_gained[env->idle]);
    > + env->flags&= ~LBF_NUMA;
    > return 1;
    > }
    > + if (env->flags& LBF_NUMA) {
    > + env->flags&= ~LBF_NUMA;
    > + goto numa_repeat;
    > + }
    > +
    > return 0;
    > }

    Would it make sense to remember the first non-autonuma-compliant
    task that can be moved, and keep searching for one that fits
    autonuma's criteria further down the line?

    Then, if you fail to find a good autonuma task in the first
    iteration, you do not have to loop over the list a second time.

    > @@ -3235,6 +3265,8 @@ static int move_tasks(struct lb_env *env)
    > if (env->imbalance<= 0)
    > return 0;
    >
    > + env->flags |= LBF_NUMA;
    > +numa_repeat:

    Same here. Loops are bad enough, and it looks like it would
    only cost one pointer on the stack to avoid numa_repeat :)

    --
    All rights reversed


    \
     
     \ /
      Last update: 2012-07-01 20:41    [W:0.030 / U:6.820 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site