Messages in this thread | | | Date | Tue, 6 Feb 2018 18:33:15 +0000 | From | Patrick Bellasi <> | Subject | Re: [PATCH v4 1/3] sched/fair: add util_est on top of PELT |
| |
On 06-Feb 16:50, Peter Zijlstra wrote: > > Mostly nice, I almost applied, except too many nits below.
:)
Thanks for the really fast still useful review!
> On Tue, Feb 06, 2018 at 02:41:29PM +0000, Patrick Bellasi wrote: > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > index 7b6535987500..118f49c39b60 100644 > > --- a/kernel/sched/fair.c > > +++ b/kernel/sched/fair.c > > @@ -5193,6 +5193,20 @@ static inline void hrtick_update(struct rq *rq) > > } > > #endif > > > > +static inline unsigned long task_util(struct task_struct *p); > > +static inline unsigned long _task_util_est(struct task_struct *p); > > What's with the leading underscore? I don't see one without it.
Good point, I was actually expecting this question and I should have added it to the cover letter, sorry.
The reasoning was: the task's estimated utilization is defined as the max between PELT and the "estimation". Where "estimation" is the max between EWMA and the last ENQUEUED utilization.
Thus I was envisioning these two calls:
_task_util_est := max(EWMA, ENQUEUED) task_util_est := max(util_avg, _task_util_est)
but since now we have clients only for the first API, I've not added the second one. Still I would prefer to keep the "_" to make it clear that's and util_est's internal signal, not the actual task's estimated utilization.
Does it make sense?
Do you prefer I just remove the "_" and we will refactor it once we should add a customer for the proper task's util_est?
> > + > > +static inline void util_est_enqueue(struct task_struct *p) > > Also pass @rq from enqueue_task_fair() ? I see no point in computing > task_rq(p) if we already have the value around.
You right, that seems to make sense. I look into it and update if really sane.
> > > +{ > > + struct cfs_rq *cfs_rq = &task_rq(p)->cfs; > > + > > + if (!sched_feat(UTIL_EST)) > > + return; > > + > > + /* Update root cfs_rq's estimated utilization */ > > + cfs_rq->avg.util_est.enqueued += _task_util_est(p); > > +} > > > > +/* > > + * Check if the specified (signed) value is within a specified margin, > > + * based on the observation that: > > + * abs(x) < y := (unsigned)(x + y - 1) < (2 * y - 1) > > * Note: this only works when x+y < INT_MAX.
+1
> > > + */ > > +static inline bool within_margin(long value, unsigned int margin) > > This mixing of long and int is dodgy, do we want to consistently use int > here?
Right, perhaps better "unsigned int" for both params, isn't?
> > +{ > > + return ((unsigned int)(value + margin - 1) < (2 * margin - 1)); > > +} > > + > > +static inline void util_est_dequeue(struct task_struct *p, int flags) > > +{ > > + struct cfs_rq *cfs_rq = &task_rq(p)->cfs; > > + unsigned long util_last; > > + long last_ewma_diff; > > + unsigned long ewma; > > + long util_est = 0; > > Why long?
Right, because I've did not spot the possibility to update it when I changed the util_est type... anyway, I'll check better, but likely we don't need a long range.
> > + > > + if (!sched_feat(UTIL_EST)) > > + return; > > + > > + /* > > + * Update root cfs_rq's estimated utilization > > + * > > + * If *p is the last task then the root cfs_rq's estimated utilization > > + * of a CPU is 0 by definition. > > + */ > > + if (cfs_rq->nr_running) { > > + util_est = READ_ONCE(cfs_rq->avg.util_est.enqueued); > > Because util_est.enqueued is of type 'unsigned int'.
Indeed...
> > > + util_est -= min_t(long, util_est, _task_util_est(p)); > > + } > > + WRITE_ONCE(cfs_rq->avg.util_est.enqueued, util_est); > > long to int truncate
right!
We have util_avg related signals which are all long based, but in the scope of "utilization" tracking, and specifically for "util_est" signals, int should have a sufficient range.
> > + > > + /* > > + * Skip update of task's estimated utilization when the task has not > > + * yet completed an activation, e.g. being migrated. > > + */ > > + if (!(flags & DEQUEUE_SLEEP)) > > + return; > > + > > + ewma = READ_ONCE(p->se.avg.util_est.ewma); > > + util_last = task_util(p); > > Again, all kinds of long, while the ewma type itself is of 'unsigned > int'.
Yes, for utilization should be enough...
> > > + > > + /* > > + * Skip update of task's estimated utilization when its EWMA is > > + * already ~1% close to its last activation value. > > + */ > > + last_ewma_diff = util_last - ewma; > > + if (within_margin(last_ewma_diff, (SCHED_CAPACITY_SCALE / 100))) > > + return; > > + > > + /* > > + * Update Task's estimated utilization > > + * > > + * When *p completes an activation we can consolidate another sample > > + * about the task size. This is done by storing the last PELT value > > + * for this task and using this value to load another sample in the > > + * exponential weighted moving average: > > + * > > + * ewma(t) = w * task_util(p) + (1-w) * ewma(t-1) > > + * = w * task_util(p) + ewma(t-1) - w * ewma(t-1) > > + * = w * (task_util(p) - ewma(t-1)) + ewma(t-1) > > + * = w * ( last_ewma_diff ) + ewma(t-1) > > + * = w * (last_ewma_diff + ewma(t-1) / w) > > + * > > + * Where 'w' is the weight of new samples, which is configured to be > > + * 0.25, thus making w=1/4 ( >>= UTIL_EST_WEIGHT_SHIFT) > > + */ > > + ewma = last_ewma_diff + (ewma << UTIL_EST_WEIGHT_SHIFT); > > + ewma >>= UTIL_EST_WEIGHT_SHIFT; > > + > > + WRITE_ONCE(p->se.avg.util_est.ewma, ewma); > > + WRITE_ONCE(p->se.avg.util_est.enqueued, util_last); > > Two stores to that word... can we fix that nicely?
Good point, the single word comes from the goal to fit into the same cache line of sched_avg.
I think we can fix it by having a struct util_est on stack and then it should be possible to update the above code to do:
ue = READ_ONCE(p->se.avg.util_est) ... magic code on ue.{enqueued, ewma} ... WRITE_ONCE(p->se.avg.util_est, ue);
That should be safe on 32bit builds too, right?
> > +} > > > +static inline unsigned long _task_util_est(struct task_struct *p) > > +{ > > + return max(p->se.avg.util_est.ewma, p->se.avg.util_est.enqueued); > > +} > > Aside from the underscore thing I already noted, why is this here and > not where the fwd declaration is?
Because here is where we have already the definitions of cpu_util{_est}() and task_util()... that's to try to keep things together. Does it make sense?
> > +/* > > + * UtilEstimation. Use estimated CPU utilization. > > + */ > > +SCHED_FEAT(UTIL_EST, false) > > Since you couldn't measure it, do we wants it true?
I'm just a single tester so far, I would be more confident once someone volunteer to turn this on to give a better coverage.
Moreover, a small out-of-tree patch enabling it for mobile devices is more then acceptable for the time being ;)
Finally, we are also considering to post a follow-up to enable it via KConfig along with a PELT half-life tunable, i.e using a 16ms instead of the default 32ms. Do you think this is something can fly mainline?
Cheers Patrick
-- #include <best/regards.h>
Patrick Bellasi
| |