Messages in this thread | | | Date | Wed, 03 Sep 2014 14:41:31 +0530 | From | Preeti U Murthy <> | Subject | Re: [PATCH v5 08/12] sched: move cfs task on a CPU with higher capacity |
| |
On 09/01/2014 02:15 PM, Vincent Guittot wrote: > On 30 August 2014 19:50, Preeti U Murthy <preeti@linux.vnet.ibm.com> wrote: >> Hi Vincent, >>> index 18db43e..60ae1ce 100644 >>> --- a/kernel/sched/fair.c >>> +++ b/kernel/sched/fair.c >>> @@ -6049,6 +6049,14 @@ static bool update_sd_pick_busiest(struct lb_env *env, >>> return true; >>> } >>> >>> + /* >>> + * The group capacity is reduced probably because of activity from other >>> + * sched class or interrupts which use part of the available capacity >>> + */ >>> + if ((sg->sgc->capacity_orig * 100) > (sgs->group_capacity * >>> + env->sd->imbalance_pct)) >> >> Wouldn't the check on avg_load let us know if we are packing more tasks >> in this group than its capacity ? Isn't that the metric we are more >> interested in? > > With this patch, we don't want to pack but we prefer to spread the > task on another CPU than the one which handles the interruption if > latter uses a significant part of the CPU capacity. > >> >>> + return true; >>> + >>> return false; >>> } >>> >>> @@ -6534,13 +6542,23 @@ static int need_active_balance(struct lb_env *env) >>> struct sched_domain *sd = env->sd; >>> >>> if (env->idle == CPU_NEWLY_IDLE) { >>> + int src_cpu = env->src_cpu; >>> >>> /* >>> * ASYM_PACKING needs to force migrate tasks from busy but >>> * higher numbered CPUs in order to pack all tasks in the >>> * lowest numbered CPUs. >>> */ >>> - if ((sd->flags & SD_ASYM_PACKING) && env->src_cpu > env->dst_cpu) >>> + if ((sd->flags & SD_ASYM_PACKING) && src_cpu > env->dst_cpu) >>> + return 1; >>> + >>> + /* >>> + * If the CPUs share their cache and the src_cpu's capacity is >>> + * reduced because of other sched_class or IRQs, we trig an >>> + * active balance to move the task >>> + */ >>> + if ((capacity_orig_of(src_cpu) * 100) > (capacity_of(src_cpu) * >>> + sd->imbalance_pct)) >>> return 1; >>> } >>> >>> @@ -6643,6 +6661,8 @@ static int load_balance(int this_cpu, struct rq *this_rq, >>> >>> schedstat_add(sd, lb_imbalance[idle], env.imbalance); >>> >>> + env.src_cpu = busiest->cpu; >>> + >>> ld_moved = 0; >>> if (busiest->nr_running > 1) { >>> /* >>> @@ -6652,7 +6672,6 @@ static int load_balance(int this_cpu, struct rq *this_rq, >>> * correctly treated as an imbalance. >>> */ >>> env.flags |= LBF_ALL_PINNED; >>> - env.src_cpu = busiest->cpu; >>> env.src_rq = busiest; >>> env.loop_max = min(sysctl_sched_nr_migrate, busiest->nr_running); >>> >>> @@ -7359,10 +7378,12 @@ static void nohz_idle_balance(struct rq *this_rq, enum cpu_idle_type idle) >>> >>> /* >>> * Current heuristic for kicking the idle load balancer in the presence >>> - * of an idle cpu is the system. >>> + * of an idle cpu in the system. >>> * - This rq has more than one task. >>> - * - At any scheduler domain level, this cpu's scheduler group has multiple >>> - * busy cpu's exceeding the group's capacity. >>> + * - This rq has at least one CFS task and the capacity of the CPU is >>> + * significantly reduced because of RT tasks or IRQs. >>> + * - At parent of LLC scheduler domain level, this cpu's scheduler group has >>> + * multiple busy cpu. >>> * - For SD_ASYM_PACKING, if the lower numbered cpu's in the scheduler >>> * domain span are idle. >>> */ >>> @@ -7372,9 +7393,10 @@ static inline int nohz_kick_needed(struct rq *rq) >>> struct sched_domain *sd; >>> struct sched_group_capacity *sgc; >>> int nr_busy, cpu = rq->cpu; >>> + bool kick = false; >>> >>> if (unlikely(rq->idle_balance)) >>> - return 0; >>> + return false; >>> >>> /* >>> * We may be recently in ticked or tickless idle mode. At the first >>> @@ -7388,38 +7410,45 @@ static inline int nohz_kick_needed(struct rq *rq) >>> * balancing. >>> */ >>> if (likely(!atomic_read(&nohz.nr_cpus))) >>> - return 0; >>> + return false; >>> >>> if (time_before(now, nohz.next_balance)) >>> - return 0; >>> + return false; >>> >>> if (rq->nr_running >= 2) >> >> Will this check ^^ not catch those cases which this patch is targeting? > > This patch is not about how many tasks are running but if the capacity > of the CPU is reduced because of side activity like interruptions > which are only visible in the capacity value (with IRQ_TIME_ACCOUNTING > config) but not in nr_running. > Even if the capacity is reduced because of RT tasks, nothing ensures > that the RT task will be running when the tick fires > > Regards, > Vincent >> >> Regards >> Preeti U Murthy >> >>> - goto need_kick; >>> + return true; >>> >>> rcu_read_lock(); >>> sd = rcu_dereference(per_cpu(sd_busy, cpu)); >>> - >>> if (sd) { >>> sgc = sd->groups->sgc; >>> nr_busy = atomic_read(&sgc->nr_busy_cpus); >>> >>> - if (nr_busy > 1) >>> - goto need_kick_unlock; >>> + if (nr_busy > 1) { >>> + kick = true; >>> + goto unlock; >>> + } >>> + >>> } >>> >>> - sd = rcu_dereference(per_cpu(sd_asym, cpu)); >>> + sd = rcu_dereference(rq->sd); >>> + if (sd) { >>> + if ((rq->cfs.h_nr_running >= 1) && >>> + ((rq->cpu_capacity * sd->imbalance_pct) < >>> + (rq->cpu_capacity_orig * 100))) {
Ok I understand your explanation above. But I was wondering if you would need to add this check around rq->cfs.h_nr_running >= 1 in the above two cases as well.
I have actually raised this concern over whether we should be using rq->nr_running or cfs_rq->nr_running while we do load balancing in reply to your patch3. While all our load measurements are about the cfs_rq load, we use rq->nr_running, which may include tasks from other sched classes as well. We divide them to get average load per task during load balancing which is wrong, isn't it? Similarly during nohz_kick_needed(), we trigger load balancing based on rq->nr_running again.
In this patch too, even if you know that the cpu is being dominated by tasks that do not belong to cfs class, you would be triggering a futile load balance if there are no fair tasks to move.
Regards Preeti U Murthy
| |