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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2860f381-24e8-2950-388a-b984c4eb51f2@arm.com>
Date:   Wed, 8 Jun 2022 11:53:42 +0200
From:   Dietmar Eggemann <dietmar.eggemann@....com>
To:     Vincent Donnefort <vdonnefort@...gle.com>, peterz@...radead.org,
        mingo@...hat.com, vincent.guittot@...aro.org
Cc:     linux-kernel@...r.kernel.org, morten.rasmussen@....com,
        chris.redpath@....com, qperret@...gle.com, tao.zhou@...ux.dev,
        kernel-team@...roid.com
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@....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@..._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@..._rq_idle is
> +	 *      cfs_rq->throttled_pelt_idle
                                          is cfs_rq->throttled_pelt_idle

Maybe align the `is foo` for readability?

[...]