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

[Thara Gopinath]

On 04/25/2019 08:45 AM, Vincent Guittot wrote:
> On Thu, 25 Apr 2019 at 12:57, Quentin Perret <quentin.perret@xxxxxxx> 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