linux-kernel - Re: [Patch v8 4/7] sched/fair: Enable periodic update of average thermal pressure

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: <20200116151502.GQ2827@hirez.programming.kicks-ass.net>
Date:   Thu, 16 Jan 2020 16:15:02 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Thara Gopinath <thara.gopinath@...aro.org>
Cc:     mingo@...hat.com, ionela.voinescu@....com,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        rui.zhang@...el.com, qperret@...gle.com, daniel.lezcano@...aro.org,
        viresh.kumar@...aro.org, linux-kernel@...r.kernel.org,
        amit.kachhap@...il.com, javi.merino@...nel.org,
        amit.kucheria@...durent.com
Subject: Re: [Patch v8 4/7] sched/fair: Enable periodic update of average
 thermal pressure

On Tue, Jan 14, 2020 at 02:57:36PM -0500, Thara Gopinath wrote:
> Introduce support in CFS periodic tick and other bookkeeping apis
> to trigger the process of computing average thermal pressure for a
> cpu. Also consider avg_thermal.load_avg in others_have_blocked
> which allows for decay of pelt signals.
> 
> Signed-off-by: Thara Gopinath <thara.gopinath@...aro.org>
> ---
>  kernel/sched/fair.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8da0222..311bb0b 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7470,6 +7470,9 @@ static inline bool others_have_blocked(struct rq *rq)
>  	if (READ_ONCE(rq->avg_dl.util_avg))
>  		return true;
>  
> +	if (READ_ONCE(rq->avg_thermal.load_avg))
> +		return true;
> +

Given that struct sched_avg is 1 cacheline, the above is a pointless
guaranteed cacheline miss if the arch doesn't
CONFIG_HAVE_SCHED_THERMAL_PRESSURE.

>  #ifdef CONFIG_HAVE_SCHED_AVG_IRQ
>  	if (READ_ONCE(rq->avg_irq.util_avg))
>  		return true;
> @@ -7495,6 +7498,7 @@ static bool __update_blocked_others(struct rq *rq, bool *done)
>  {
>  	const struct sched_class *curr_class;
>  	u64 now = rq_clock_pelt(rq);
> +	unsigned long thermal_pressure = arch_cpu_thermal_pressure(cpu_of(rq));
>  	bool decayed;
>  
>  	/*
> @@ -7505,6 +7509,8 @@ static bool __update_blocked_others(struct rq *rq, bool *done)
>  
>  	decayed = update_rt_rq_load_avg(now, rq, curr_class == &rt_sched_class) |
>  		  update_dl_rq_load_avg(now, rq, curr_class == &dl_sched_class) |
> +		  update_thermal_load_avg(rq_clock_task(rq), rq,
> +					  thermal_pressure) 			|
>  		  update_irq_load_avg(rq, 0);
>  
>  	if (others_have_blocked(rq))

That there indentation trainwreck is a reason to rename the function.

	decayed = update_rt_rq_load_avg(now, rq, curr_class == &rt_sched_class) |
		  update_dl_rq_load_avg(now, rq, curr_class == &dl_sched_class) |
		  update_thermal_load_avg(rq_clock_task(rq), rq, thermal_pressure) |
		  update_irq_load_avg(rq, 0);

Is much better.

But now that you made me look at that, I noticed it's using a different
clock -- it is _NOT_ using now/rq_clock_pelt(), which means it'll not be
in sync with the other averages.

Is there a good reason for that?

> @@ -10275,6 +10281,7 @@ static void task_tick_fair(struct rq *rq, struct task_struct *curr, int queued)
>  {
>  	struct cfs_rq *cfs_rq;
>  	struct sched_entity *se = &curr->se;
> +	unsigned long thermal_pressure = arch_cpu_thermal_pressure(cpu_of(rq));
>  
>  	for_each_sched_entity(se) {
>  		cfs_rq = cfs_rq_of(se);
> @@ -10286,6 +10293,7 @@ static void task_tick_fair(struct rq *rq, struct task_struct *curr, int queued)
>  
>  	update_misfit_status(curr, rq);
>  	update_overutilized_status(task_rq(curr));
> +	update_thermal_load_avg(rq_clock_task(rq), rq, thermal_pressure);
>  }

I'm thinking this is the wrong place; should this not be in
scheduler_tick(), right before calling sched_class::task_tick() ? Surely
any execution will affect thermals, not only fair class execution.