Re: [PATCH v2 3/3] PM / EM: Skip inefficient OPPs

[Lukasz Luba <lukasz.luba@arm.com>]

On 5/28/21 6:04 AM, Viresh Kumar wrote:
> On 21-05-21, 17:54, Vincent Donnefort wrote:
>> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
>> +static inline
>> +struct em_perf_state *em_pd_get_efficient_state(struct em_perf_domain *pd,
>> +						unsigned long freq)
>> +{
>> +	struct em_perf_state *ps;
>> +	int i;
>> +
>> +	for (i = 0; i < pd->nr_perf_states; i++) {
>> +		ps = &pd->table[i];
>> +		if (ps->flags & EM_PERF_STATE_INEFFICIENT)
>> +			continue;
>> +		if (ps->frequency >= freq)
>> +			break;
> 
> I believe it may be more optimal if we change the sequence of these two 'if'
> blocks here. We only need to check for inefficient frequencies if it is >= freq.

Make sense (especially in context of your example below).

> 
>> +	}
>> +
>> +	return ps;
>> +}
> 
> 
>> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
>> @@ -153,6 +154,9 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
>>   
>>   	freq = map_util_freq(util, freq, max);
>>   
>> +	/* Avoid inefficient performance states */
>> +	freq = em_pd_get_efficient_freq(em_cpu_get(policy->cpu), freq);
>> +
>>   	if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
>>   		return sg_policy->next_freq;
> 
> Assume this freq-table (E=efficient, IE=inefficient): 800M (E), 1G (E), 1.2G (IE), 1.4G (IE), 1.6G (E).
> Thermal limits max to 1.4G
> 
> Freq returned by map_util_freq() is 1.01G.
> 
> Will we not end up selecting 1.4G here ? Inefficient as well as much higher than
> what we requested for ?
> 

Correct, it will be 1.4G, instead of selecting 1.2G, in this example.
The two or more consecutive inefficient OPPs in the table ruins the
proper behavior. We have to reevaluate this.

Thanks Viresh for pointing this out.

Regards,
Lukasz