Re: [PATCH v10 2/7] sched/fair: Decay task PELT values during wakeup migration

[Dietmar Eggemann <dietmar.eggemann@arm.com>]

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?

[...]