by Hongyan Xia

[permalink] [raw]

Subject: Re: [RFC PATCH v3 6/6] Propagate negative bias

On 26/05/2024 23:53, Dietmar Eggemann wrote:
> On 07/05/2024 14:50, Hongyan Xia wrote:
>> Negative bias is interesting, because dequeuing such a task will
>> actually increase utilization.
>>
>> Solve by applying PELT decay to negative biases as well. This in fact
>> can be implemented easily with some math tricks.
>>
>> Signed-off-by: Hongyan Xia <[email protected]>
>> ---
>> kernel/sched/fair.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 44 insertions(+)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 0177d7e8f364..7259a61e9ae5 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -4863,6 +4863,45 @@ static inline unsigned long task_util_est_uclamp(struct task_struct *p)
>> {
>> return max(task_util_uclamp(p), _task_util_est_uclamp(p));
>> }
>> +
>> +/*
>> + * Negative biases are tricky. If we remove them right away then dequeuing a
>> + * uclamp_max task has the interesting effect that dequeuing results in a higher
>> + * rq utilization. Solve this by applying PELT decay to the bias itself.
>> + *
>> + * Keeping track of a PELT-decayed negative bias is extra overhead. However, we
>> + * observe this interesting math property, where y is the decay factor and p is
>> + * the number of periods elapsed:
>> + *
>> + * util_new = util_old * y^p - neg_bias * y^p
>> + * = (util_old - neg_bias) * y^p
>> + *
>> + * Therefore, we simply subtract the negative bias from util_avg the moment we
>> + * dequeue, then the PELT signal itself is the total of util_avg and the decayed
>> + * negative bias, and we no longer need to track the decayed bias separately.
>> + */
>> +static void propagate_negative_bias(struct task_struct *p)
>> +{
>> + if (task_util_bias(p) < 0 && !task_on_rq_migrating(p)) {
>> + unsigned long neg_bias = -task_util_bias(p);
>> + struct sched_entity *se = &p->se;
>> + struct cfs_rq *cfs_rq;
>> +
>> + p->se.avg.util_avg_bias = 0;
>> +
>> + for_each_sched_entity(se) {
>> + u32 divider, neg_sum;
>> +
>> + cfs_rq = cfs_rq_of(se);
>> + divider = get_pelt_divider(&cfs_rq->avg);
>> + neg_sum = neg_bias * divider;
>> + sub_positive(&se->avg.util_avg, neg_bias);
>> + sub_positive(&se->avg.util_sum, neg_sum);
>> + sub_positive(&cfs_rq->avg.util_avg, neg_bias);
>> + sub_positive(&cfs_rq->avg.util_sum, neg_sum);
>> + }
>> + }
>
> So you remove the 'task bias = clamp(util_avg, uclamp_min, uclamp_max) -
> util_avg' from the se and cfs_rq util_avg' in case it's negative. I.e.
> if the task is capped hard.
>
> Looks like this is the old issue that PELT has blocked contribution
> whereas uclamp does not (runnable only).
>
> What's the rationale behind this? Is it because the task didn't get the
> runtime it needed so we can remove this (artificially accrued) util_avg?
>
> Normally we wouldn't remove blocked util_avg and let it rather decay
> periodically for cfs_rq's and at wakeup for tasks.

Sorry I may not have understood what you asked.

PELT has decaying effect whereas uclamp does not, so you will have the
effect that dequeuing a task will immediately remove the bias, but
util_avg won't be immediately gone.

In the case of uclamp_max, assuming an always-running task with
uclamp_max of 200, which means util_avg 1024 and util_avg_bias of -824.
The moment this task is dequeued, the rq uclamp util will immediately go
from 200 to 1024, and then 1024 slowly decay to 0. This patch is to
mitigate this effect, so that it will just decay from 200 to 0 without
spiking to 1024 first.

Hopefully this answers your question.

> [...]

2024-06-10 15:30:44

by Hongyan Xia

[permalink] [raw]

Subject: Re: [RFC PATCH v3 2/6] sched/uclamp: Track a new util_avg_bias signal

On 28/05/2024 12:09, Hongyan Xia wrote:
> On 26/05/2024 23:52, Dietmar Eggemann wrote:
>[...]
>>> +    old = READ_ONCE(p->se.avg.util_avg_bias);
>>> +    new = (int)clamp(util, uclamp_min, uclamp_max) - (int)util;
>>> +
>>> +    WRITE_ONCE(p->se.avg.util_avg_bias, new);
>>> +    if (!p->se.on_rq)
>>> +        return;
>>> +    WRITE_ONCE(avg->util_avg_bias, READ_ONCE(avg->util_avg_bias) +
>>> new - old);
>>> +}
>>> +#else /* !CONFIG_UCLAMP_TASK */
>>> +static void update_util_bias(struct sched_avg *avg, struct
>>> task_struct *p)
>>> +{
>>> +}
>>> +#endif
>>> +
>>> /*
>>>    * sched_entity:
>>>    *
>>> @@ -296,6 +330,8 @@ int __update_load_avg_blocked_se(u64 now, struct
>>> sched_entity *se)
>>> {
>>>       if (___update_load_sum(now, &se->avg, 0, 0, 0)) {
>>>           ___update_load_avg(&se->avg, se_weight(se));
>>> +        if (entity_is_task(se))
>>> +            update_util_bias(NULL, task_of(se));
>>
>> IMHO,
>>
>> update_util_bias(struct sched_avg *avg, struct sched_entity *se)
>>
>>      if (!entity_is_task(se))
>>          return;
>>
>>      ...
>>
>> would be easier to read.
>
> Yeah, that would work, and might be a good way to prepare for group
> clamping if it ever becomes a thing.
>

Sadly it's not as easy as I hoped. The problem is that we need to fetch
task uclamp values here so we need to get p anyway. Also, even if one
day we implement group uclamp, we need to fetch the cfs_rq this se is on
instead of the whole rq, so the function signature needs to change
anyway. Keeping it the current way might be the better thing to do here.

> [...]