Messages in this thread | | | Date | Wed, 8 Jun 2022 11:53:42 +0200 | From | Dietmar Eggemann <> | Subject | Re: [PATCH v10 2/7] sched/fair: Decay task PELT values during wakeup migration |
| |
On 07/06/2022 14:32, Vincent Donnefort wrote: > From: Vincent Donnefort <vincent.donnefort@arm.com>
[...]
> To that end, we need sched_clock_cpu() but it is a costly function. Limit > the usage to the case where the source CPU is idle as we know this is when > the clock is having the biggest risk of being outdated. In this such case,
s/In this such case/in such a case
> let's call it cfs_idle_lag the delta time between the rq_clock_pelt value > at rq idle and cfs_rq idle.
s/it cfs_idle_lag the delta time between the rq_clock_pelt value at rq idle and cfs_rq idle./the delta time between the rq_clock_pelt value > at rq idle and cfs_rq idle cfs_idle_lag?
And rq_idle_lag the delta between "now" and > the rq_clock_pelt at rq idle. >
--->
> The estimated PELT clock is then: > > last_update_time (the cfs_rq's last_update_time) > + cfs_idle_lag (delta between cfs_rq's update and rq's update> + rq_idle_lag (delta between rq's update and now) > > last_update_time = cfs_rq_clock_pelt() > = rq_clock_pelt() - cfs->throttled_clock_pelt_time > > cfs_idle_lag = rq_clock_pelt()@rq_idle - > rq_clock_pelt()@cfs_rq_idle > > rq_idle_lag = sched_clock_cpu() - rq_clock()@rq_idle > > The rq_clock_pelt() from last_update_time being the same as > rq_clock_pelt()@cfs_rq_idle, we can write: > > estimation = rq_clock_pelt()@rq_idle - cfs->throttled_clock_pelt_time + > sched_clock_cpu() - rq_clock()@rq_idle > > The clocks being not accessible without the rq lock taken, some timestamps > are created: > > rq_clock_pelt()@rq_idle is rq->clock_pelt_idle > rq_clock()@rq_idle is rq->enter_idle > cfs->throttled_clock_pelt_time is cfs_rq->throttled_pelt_idle >
<--- ^^^
This whole block seems to be the same information as the comment block in migrate_se_pelt_lag(). But you haven't updated it in v10. Maybe you can get rid of this here and point to the comment block in migrate_se_pelt_lag() from here instead to guarantee consistency? Otherwise they should be in sync.
[...]
> + /* > + * Estimated "now" is: last_update_time + cfs_idle_lag + rq_idle_lag, where: > + * > + * last_update_time (the cfs_rq's last_update_time) > + * = cfs_rq_clock_pelt()@cfs_rq_idle > + * = rq_clock_pelt()@cfs_rq_idle > + * - cfs->throttled_clock_pelt_time@cfs_rq_idle > + * > + * cfs_idle_lag (delta between cfs_rq's update and rq's update)
Isn't this: (delta between rq's update and cfs_rq's update) ?
> + * = rq_clock_pelt()@rq_idle - rq_clock_pelt()@cfs_rq_idle > + * > + * rq_idle_lag (delta between rq's update and now)
Isn't this: (delta between now and rq's update) ?
> + * = sched_clock_cpu() - rq_clock()@rq_idle > + * > + * We can then write: > + * > + * now = rq_clock_pelt()@rq_idle - cfs->throttled_clock_pelt_time + > + * sched_clock_cpu() - rq_clock()@rq_idle > + * Where: > + * rq_clock_pelt()@rq_idle is rq->clock_pelt_idle > + * rq_clock()@rq_idle is rq->clock_idle is rq->clock_pelt_idle is rq->clock_idle
> + * cfs->throttled_clock_pelt_time@cfs_rq_idle is > + * cfs_rq->throttled_pelt_idle is cfs_rq->throttled_pelt_idle
Maybe align the `is foo` for readability?
[...]
| |