Mostly nice, I almost applied, except too many nits below.
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.
> +
> +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.
> +{
> + 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.
> + */
> +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?
> +{
> + 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?
> +
> + 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'.
> + 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
> +
> + /*
> + * 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'.
> +
> + /*
> + * 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?
> +}
> +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?
> +/*
> + * UtilEstimation. Use estimated CPU utilization.
> + */
> +SCHED_FEAT(UTIL_EST, false)
Since you couldn't measure it, do we wants it true?
