Messages in this thread | | | Subject | Re: [PATCH] fair/util_est: Separate util_est_dequeue() for cfs_rq_util_change | From | Dietmar Eggemann <> | Date | Fri, 11 Dec 2020 12:30:33 +0100 |
| |
Hi Yan,
On 09/12/2020 11:44, Xuewen Yan wrote: > when a task dequeued, it will update it's util, and cfs_rq_util_change > would check rq's util, if the cfs_rq->avg.util_est.enqueued is bigger > than cfs_rq->avg.util_avg, but because the cfs_rq->avg.util_est.enqueued > didn't be decreased, this would cause bigger cfs_rq_util by mistake, > as a result, cfs_rq_util_change may change freq unreasonablely. > > separate the util_est_dequeue() into util_est_dequeue() and > util_est_update(), and dequeue the _task_util_est(p) before update util.
The issue is that sugov_update_[shared\|single] -> sugov_get_util() -> cpu_util_cfs() operates on an old cfs_rq->avg.util_est.enqueued value?
cpu_util_cfs()
if (sched_feat(UTIL_EST)) util = max_t(util, READ_ONCE(rq->cfs.avg.util_est.enqueued)) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
dequeue_task_fair() (w/ your patch, moving (1) before (2))
/* (1) update cfs_rq->avg.util_est.enqueued */ util_est_dequeue()
/* (2) potential p->se.avg.util_avg update */ /* 2 for loops */ for_each_sched_entity()
/* this can only lead to a freq change for a root cfs_rq */ (dequeue_entity() ->) update_load_avg() -> cfs_rq_util_change() -> cpufreq_update_util() ->...-> sugov_update_[shared\|single]
/* (3) potential update p->se.avg.util_est */ util_est_update()
We do need (3) after (2) because of:
util_est_update() ... ue.enqueued = (task_util(p) | UTIL_AVG_UNCHANGED); task_util ... ^^^^^^^^^^^^^ p->se.avg.util_avg
Did I get this right?
[...]
| |