Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [Patch v8 4/7] sched/fair: Enable periodic update of average
 thermal pressure
To:     Peter Zijlstra <peterz@infradead.org>
References: <1579031859-18692-1-git-send-email-thara.gopinath@linaro.org>
 <1579031859-18692-5-git-send-email-thara.gopinath@linaro.org>
 <20200116151502.GQ2827@hirez.programming.kicks-ass.net>
Cc:     mingo@redhat.com, ionela.voinescu@arm.com,
        vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
        rui.zhang@intel.com, qperret@google.com, daniel.lezcano@linaro.org,
        viresh.kumar@linaro.org, linux-kernel@vger.kernel.org,
        amit.kachhap@gmail.com, javi.merino@kernel.org,
        amit.kucheria@verdurent.com
From:   Thara Gopinath <thara.gopinath@linaro.org>
Message-ID: <5E2216FD.5040903@linaro.org>
Date:   Fri, 17 Jan 2020 15:20:13 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.5.1
MIME-Version: 1.0
In-Reply-To: <20200116151502.GQ2827@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 01/16/2020 10:15 AM, Peter Zijlstra wrote:
> 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@linaro.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.
Thanks for the review Peter. I see your suggestion in Patch 1 to fix
this issue. Will send out the next version implementing it.

> 
>>  #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.

Did you intend to say here to rename  update_thermal_load_avg to
something else ?
> 
> 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?

So I guess as Vincent has replied in his email, rq_clock_pelt adjusts
clock for frequency and cpu capacity invariance. Thermal pressure signal
is already accounted for it when updated from the thermal framework.
> 
>> @@ -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.

I read all other comments from others on this. I agree. I will move this
to scheduler_tick.

> 


-- 
Warm Regards
Thara