Re: [PATCH V2 1/3] Calculate Thermal Pressure

From: Thara Gopinath <thara.gopinath@linaro.org>
To: Vincent Guittot <vincent.guittot@linaro.org>,
	Quentin Perret <quentin.perret@arm.com>
Cc: Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Zhang Rui <rui.zhang@intel.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Amit Kachhap <amit.kachhap@gmail.com>,
	viresh kumar <viresh.kumar@linaro.org>,
	Javi Merino <javi.merino@kernel.org>,
	Eduardo Valentin <edubezval@gmail.com>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Nicolas Dechesne <nicolas.dechesne@linaro.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>
Subject: Re: [PATCH V2 1/3] Calculate Thermal Pressure
Date: Fri, 26 Apr 2019 10:17:56 -0400	[thread overview]
Message-ID: <5CC31314.3070700@linaro.org> (raw)
In-Reply-To: <CAKfTPtBvY3xiDt6tcqF7GoZki3VihD4Tz7E5ctE8hNaNzL6NPA@mail.gmail.com>

On 04/25/2019 08:45 AM, Vincent Guittot wrote:
> On Thu, 25 Apr 2019 at 12:57, Quentin Perret <quentin.perret@arm.com> wrote:
>>
>> On Tuesday 16 Apr 2019 at 15:38:39 (-0400), Thara Gopinath wrote:
>>> +/* Per cpu structure to keep track of Thermal Pressure */
>>> +struct thermal_pressure {
>>> +     unsigned long scale; /* scale reflecting average cpu max capacity*/
>>> +     unsigned long acc_scale; /* Accumulated scale for this time window */
>>> +     unsigned long old_scale; /* Scale value for the previous window */
>>> +     unsigned long raw_scale; /* Raw max capacity */
>>> +     unsigned long age_stamp; /* Last time old_scale was updated */
>>> +     unsigned long last_update; /* Last time acc_scale was updated */
>>> +     spinlock_t lock; /* Lock for protecting from simultaneous access*/
>>> +     /* Timer for periodic update of thermal pressure */
>>> +     struct timer_list timer;
>>
>> Do you actually need the periodic update ? You only really need to
>> update the 'scale' value when updating the LB stats no ? Nobody
>> accesses that value in between two LBs.
> 
> Do you mean calling a variant of sched_update_thermal_pressure() in
> update_cpu_capacity() instead of periodic update ?
> Yes , that should be enough

Hi,

I do have some concerns in doing this.
1. Updating thermal pressure does involve some calculations for
accumulating, averaging, decaying etc which in turn could have some
finite and measurable time spent in the function. I am not sure if this
delay will be acceptable for all systems during load balancing (I have
not measured the time involved). We need to decide if this is something
we can live with.

2. More importantly, since update can happen from at least two paths (
thermal fw and periodic timer in case of this patch series)to ensure
mutual exclusion,  the update is done under a spin lock. Again calling
from update_cpu_capacity will involve holding the lock in the load
balance path which is possible not for the best.
For me, updating out of load balance minimizes the disruption to
scheduler on the whole.

But if there is an over whelming support for updating the statistics
from the LB , I can move the code.

Regards
Thara

> 
>>
>> Thanks,
>> Quentin

-- 
Regards
Thara