Messages in this thread | | | From | Vincent Guittot <> | Date | Fri, 20 Feb 2015 14:31:08 +0100 | Subject | Re: [PATCH RESEND v9 08/10] sched: replace capacity_factor by usage |
| |
On 20 February 2015 at 12:14, Peter Zijlstra <peterz@infradead.org> wrote: > On Thu, Jan 15, 2015 at 11:09:28AM +0100, Vincent Guittot wrote: > >> Finally, the sched_group->sched_group_capacity->capacity_orig has been removed >> because it's no more used during load balance. > > Maybe do that in a separate patch to avoid cluttering this one?
ok
> >> [1] https://lkml.org/lkml/2014/8/12/295 > > Patch references are like: > 9a5d9ba6a363 ("sched/fair: Allow calculate_imbalance() to move idle cpus") >
ok, I'm going to update the reference
>> /* >> + * Check whether the capacity of the rq has been noticeably reduced by side >> + * activity. The imbalance_pct is used for the threshold. >> + * Return true is the capacity is reduced >> */ >> static inline int >> +check_cpu_capacity(struct rq *rq, struct sched_domain *sd) >> { >> + return ((rq->cpu_capacity * sd->imbalance_pct) < >> + (rq->cpu_capacity_orig * 100)); >> } > > How about cpu_has_capacity() to be consistent with the below function?
I can change the name for consistency but the 2 function are not testing the same thing , so i would not create any confusion. group_has_capacity tests if some capacity is not used whereas check_cpu_capacity/cpu_has_capacity check if the capacity has been reduced by side activity but doesn't give information about spare capacity.
> > This comment could use whitespace: > >> /* >> + * group_has_capacity returns true if the group has spare capacity that could >> + * be used by some tasks. > > We consider that a group has spare capacity if the >> + * number of task is smaller than the number of CPUs or if the usage is lower >> + * than the available capacity for CFS tasks. > > For the latter, we use a >> + * threshold to stabilize the state, to take into account the variance of the >> + * tasks' load and to return true if the available capacity in meaningful for >> + * the load balancer. > > As an example, an available capacity of 1% can appear >> + * but it doesn't make any benefit for the load balance. >> */ >> +static inline bool >> +group_has_capacity(struct lb_env *env, struct sg_lb_stats *sgs) >> { >> + if ((sgs->group_capacity * 100) > >> + (sgs->group_usage * env->sd->imbalance_pct)) >> + return true; >> >> + if (sgs->sum_nr_running < sgs->group_weight) >> + return true; >> + >> + return false; >> +} > > Would it not make sense to first do the nr_running test, its cheaper > than the multiplication thing.
good point, i will reorder the test
> >> +/* >> + * group_is_overloaded returns true if the group has more tasks than it can >> + * handle. > > We consider that a group is overloaded if the number of tasks is >> + * greater than the number of CPUs and the tasks already use all available >> + * capacity for CFS tasks. > > For the latter, we use a threshold to stabilize >> + * the state, to take into account the variance of tasks' load and to return >> + * true if available capacity is no more meaningful for load balancer >> + */ >> +static inline bool >> +group_is_overloaded(struct lb_env *env, struct sg_lb_stats *sgs) >> +{ >> + if (sgs->sum_nr_running <= sgs->group_weight) >> + return false; >> >> + if ((sgs->group_capacity * 100) < >> + (sgs->group_usage * env->sd->imbalance_pct)) >> + return true; >> >> + return false; >> } > > Maybe a note on the difference between group_is_overloaded() and > !group_has_capacity()?
ok, i will add a comment
> > As to the comment, I think it can be reduced by referring to the comment > of group_has_capacity().
ok, i 'm going to update it
> >> /* >> * In case the child domain prefers tasks go to siblings >> + * first, lower the sg capacity so that we'll try >> * and move all the excess tasks away. We lower the capacity >> * of a group only if the local group has the capacity to fit >> + * these excess tasks. > > The extra check prevents the case where >> + * you always pull from the heaviest group when it is already >> + * under-utilized (possible with a large weight task outweighs >> + * the tasks on the system). >> */ >> if (prefer_sibling && sds->local && >> + group_has_capacity(env, &sds->local_stat) && >> + (sgs->sum_nr_running > 1)) { >> + sgs->group_no_capacity = 1; >> + sgs->group_type = group_overloaded; >> + } > > Looks OK otherwise I suppose.
| |