Re: [RFC PATCH] cpufreq: ondemand: Increase frequency to any value proportional to load

[Stratos Karafotis <stratosk@semaphore.gr>]

On 05/30/2013 01:29 AM, Rafael J. Wysocki wrote:
> On Wednesday, May 29, 2013 06:15:56 PM Stratos Karafotis wrote:
>> On 05/28/2013 11:54 PM, Rafael J. Wysocki wrote:
>>> On Tuesday, May 28, 2013 08:03:19 PM Stratos Karafotis wrote:
>>>> I mean any value of freq table. Please let me know if you want me to rephrase
>>>> it in description.
>>>
>>> Yes, it would be nice to be more precise.
>>
>> OK sure, I will add a more precise description.
>>
>>
>>> Which is equivalent to saying that __cpufreq_driver_getavg() is not useful and
>>> may be removed (but the patch doesn't do that and I wonder why?), but surely
>>> the developer who added it wouldn't agree.
>>>
>>> So, please explain: why can we drop __cpufreq_driver_getavg()?
>>>
>>
>> With the new proposed method the next frequency is not dependent from current
>> or average frequency. The next frequency is dependent only from load.
>> So, we don't need support for freq feedback from hardware anymore.
>
> OK, but that's a more significant change than the changelog suggests.
> The changelog should tell the whole story and explain the rationale. :-)
>
> So, please explain that in fact it is not necessary to use the current or
> average frequency to compute the target and why that is the case.
>
> Also the patch should remove __cpufreq_driver_getavg() and the callback used by
> it, since that code will be dead after applying it anyway.
>
>> Even if, due to underlying hardware coordination mechanism, CPU runs in different
>> frequency than the actual, the calculation of load and of target frequency will
>> remain the unaffected, with this patch.
>>
>> With full respect to ondemand coauthor, and if the new method is acceptable,
>> I could send a patch to revert the original one about the IA32_APERF and
>> IA32_MPERF MSR support.
>
> I'm not sure what you mean by "revert", but please do as I said above.
>
>>>> Thus, in the comparison with up_threshold to increase frequency we actually
>>>> do this (in cases that getavg is not implemented):
>>>> if (load > up_theshold)
>>>> 	increase to max
>>>>
>>>> So, after the patch we keep the same comparison to decide about the max frequency.
>>>> I thought, that below up_threshold is 'fair' to decide about the next
>>>> frequency with formula that frequency is proportional to load.
>>>> For example in a CPU with min freq 100MHz and max 1000MHz with a
>>>> load 50 target frequency should be 500MHz.
>>>
>>> Well, that sounds reasonable, but the patch actually does more than that.
>>
>> I'm sorry, but I didn't understand your last point.
>
> Please see above.
>
> The changelog doesn't even mention that the code is being switched from using
> measured past frequencies to not using them, because you think that there's a
> better way of computing the target (which by the way I can agree with :-)).
>
> Thanks,
> Rafael
>
>

OK, I will send a new patch that includes your corrections and suggestions.

Thanks so much for your time and your comments!

Stratos