[PATCH] Revert "cpufreq: pcc-cpufreq: update default value of cpuinfo_transition_latency"

[PATCH] Revert "cpufreq: pcc-cpufreq: update default value of cpuinfo_transition_latency"

[Andreas Herrmann]

This reverts commit 790d849bf811a8ab5d4cd2cce0f6fda92f6aebf2.

Using a v4.7-rc7 kernel on a HP ProLiant triggered following messages

 pcc-cpufreq: (v1.10.00) driver loaded with frequency limits: 1200 MHz, 2800 MHz
 cpufreq: ondemand governor failed, too long transition latency of HW, fallback to performance governor

The last line was shown for each CPU in the system.
Testing v4.5 (where commit 790d849b was integrated) triggered
similar messages. Same behaviour on a 2nd HP Proliant system.

So commit 790d849bf (cpufreq: pcc-cpufreq: update default value of
cpuinfo_transition_latency) causes the system to use performance
governor which, I guess, was not the intention of the patch.

Enabling debug output in pcc-cpufreq provides following verbose output:

 pcc-cpufreq: (v1.10.00) driver loaded with frequency limits: 1200 MHz, 2800 MHz
 pcc_get_offset: for CPU 0: pcc_cpu_data input_offset: 0x44, pcc_cpu_data output_offset: 0x48
 init: policy->max is 2800000, policy->min is 1200000
 get: get_freq for CPU 0
 get: SUCCESS: (virtual) output_offset for cpu 0 is 0xffffc9000d7c0048, contains a value of: 0xff06. Speed is: 168000 MHz
 cpufreq: ondemand governor failed, too long transition latency of HW, fallback to performance governor
 target: CPU 0 should go to target freq: 2800000 (virtual) input_offset is 0xffffc9000d7c0044
 target: was SUCCESSFUL for cpu 0

I am asking to revert 790d849bf to re-enable usage of ondemand
governor with pcc-cpufreq.

CC: Jacob Tanenbaum <jtanenba@redhat.com>
CC: <stable@vger.kernel.org> # 4.5+
Signed-off-by: Andreas Herrmann <aherrmann@suse.com>

---
 Documentation/cpu-freq/pcc-cpufreq.txt | 4 ++--
 drivers/cpufreq/pcc-cpufreq.c          | 2 --
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/Documentation/cpu-freq/pcc-cpufreq.txt b/Documentation/cpu-freq/pcc-cpufreq.txt
index 0a94224..9e3c3b3 100644
--- a/Documentation/cpu-freq/pcc-cpufreq.txt
+++ b/Documentation/cpu-freq/pcc-cpufreq.txt
@@ -159,8 +159,8 @@ to be strictly associated with a P-state.
 
 2.2 cpuinfo_transition_latency:
 -------------------------------
-The cpuinfo_transition_latency field is CPUFREQ_ETERNAL. The PCC specification
-does not include a field to expose this value currently.
+The cpuinfo_transition_latency field is 0. The PCC specification does
+not include a field to expose this value currently.
 
 2.3 cpuinfo_cur_freq:
 ---------------------
diff --git a/drivers/cpufreq/pcc-cpufreq.c b/drivers/cpufreq/pcc-cpufreq.c
index a7ecb9a..3f0ce2a 100644
--- a/drivers/cpufreq/pcc-cpufreq.c
+++ b/drivers/cpufreq/pcc-cpufreq.c
@@ -555,8 +555,6 @@ static int pcc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	policy->min = policy->cpuinfo.min_freq =
 		ioread32(&pcch_hdr->minimum_frequency) * 1000;
 
-	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
-
 	pr_debug("init: policy->max is %d, policy->min is %d\n",
 		policy->max, policy->min);
 out:
-- 
2.1.4

Re: [PATCH] Revert "cpufreq: pcc-cpufreq: update default value of cpuinfo_transition_latency"

[Viresh Kumar]

On 22-07-16, 17:14, Andreas Herrmann wrote:
> diff --git a/drivers/cpufreq/pcc-cpufreq.c b/drivers/cpufreq/pcc-cpufreq.c
> index a7ecb9a..3f0ce2a 100644
> --- a/drivers/cpufreq/pcc-cpufreq.c
> +++ b/drivers/cpufreq/pcc-cpufreq.c
> @@ -555,8 +555,6 @@ static int pcc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>  	policy->min = policy->cpuinfo.min_freq =
>  		ioread32(&pcch_hdr->minimum_frequency) * 1000;
>  
> -	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
> -
>  	pr_debug("init: policy->max is %d, policy->min is %d\n",
>  		policy->max, policy->min);
>  out:

Hi Rafael,

I am very confused on this, can you help me understand ?

- CPUFREQ_ETERNAL = -1
- unsigned int transition_latency = CPUFREQ_ETERNAL, will set it to UINT_MAX.
- Many drivers do it today

cpufreq.c

	if (policy->governor->max_transition_latency &&
	    policy->cpuinfo.transition_latency >
	    policy->governor->max_transition_latency) {

- And this check will always fail, unless max_transition_latency is zero.

What am I missing ?

-- 
viresh

Re: [PATCH] Revert "cpufreq: pcc-cpufreq: update default value of cpuinfo_transition_latency"

[Linda Knippers]

On 7/22/2016 11:36 AM, Viresh Kumar wrote:
> On 22-07-16, 17:14, Andreas Herrmann wrote:
>> diff --git a/drivers/cpufreq/pcc-cpufreq.c b/drivers/cpufreq/pcc-cpufreq.c
>> index a7ecb9a..3f0ce2a 100644
>> --- a/drivers/cpufreq/pcc-cpufreq.c
>> +++ b/drivers/cpufreq/pcc-cpufreq.c
>> @@ -555,8 +555,6 @@ static int pcc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>>  	policy->min = policy->cpuinfo.min_freq =
>>  		ioread32(&pcch_hdr->minimum_frequency) * 1000;
>>  
>> -	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
>> -
>>  	pr_debug("init: policy->max is %d, policy->min is %d\n",
>>  		policy->max, policy->min);
>>  out:
> 
> Hi Rafael,
> 
> I am very confused on this, can you help me understand ?
> 
> - CPUFREQ_ETERNAL = -1
> - unsigned int transition_latency = CPUFREQ_ETERNAL, will set it to UINT_MAX.
> - Many drivers do it today
> 
> cpufreq.c
> 
> 	if (policy->governor->max_transition_latency &&
> 	    policy->cpuinfo.transition_latency >
> 	    policy->governor->max_transition_latency) {
> 
> - And this check will always fail, unless max_transition_latency is zero.
> 
> What am I missing ?

I don't know what's missing but I can reproduce the problem.

-- ljk

Re: [PATCH] Revert "cpufreq: pcc-cpufreq: update default value of cpuinfo_transition_latency"

[Linda Knippers]

On 07/22/2016 03:25 PM, Linda Knippers wrote:
> 
> 
> On 7/22/2016 11:36 AM, Viresh Kumar wrote:
>> On 22-07-16, 17:14, Andreas Herrmann wrote:
>>> diff --git a/drivers/cpufreq/pcc-cpufreq.c b/drivers/cpufreq/pcc-cpufreq.c
>>> index a7ecb9a..3f0ce2a 100644
>>> --- a/drivers/cpufreq/pcc-cpufreq.c
>>> +++ b/drivers/cpufreq/pcc-cpufreq.c
>>> @@ -555,8 +555,6 @@ static int pcc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>>>  	policy->min = policy->cpuinfo.min_freq =
>>>  		ioread32(&pcch_hdr->minimum_frequency) * 1000;
>>>  
>>> -	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
>>> -
>>>  	pr_debug("init: policy->max is %d, policy->min is %d\n",
>>>  		policy->max, policy->min);
>>>  out:
>>
>> Hi Rafael,
>>
>> I am very confused on this, can you help me understand ?
>>
>> - CPUFREQ_ETERNAL = -1
>> - unsigned int transition_latency = CPUFREQ_ETERNAL, will set it to UINT_MAX.
>> - Many drivers do it today
>>
>> cpufreq.c
>>
>> 	if (policy->governor->max_transition_latency &&
>> 	    policy->cpuinfo.transition_latency >
>> 	    policy->governor->max_transition_latency) {
>>
>> - And this check will always fail, unless max_transition_latency is zero.
>>
>> What am I missing ?
> 
> I don't know what's missing but I can reproduce the problem.

I added a debug message to show the transition latency values.

[   36.113829] cpufreq: ondemand governor failed, too long transition latency of HW, fallback to
performance governor
[   36.164688] cpufreq: cpufreq_governor: max_transition_latency 0x10000000, transition_latency
0x4294967295

max_transition latency for ondemand seems to come from
#define TRANSITION_LATENCY_LIMIT                (10 * 1000 * 1000)

How does this work for any driver?

-- ljk
> 
> -- ljk
> 

Re: [PATCH] Revert "cpufreq: pcc-cpufreq: update default value of cpuinfo_transition_latency"

[Viresh Kumar]

On 22-07-16, 23:31, Rafael J. Wysocki wrote:
> > cpufreq.c
> > 
> > 	if (policy->governor->max_transition_latency &&
> > 	    policy->cpuinfo.transition_latency >
> > 	    policy->governor->max_transition_latency) {
> > 
> > - And this check will always fail, unless max_transition_latency is zero.
> 
> Why would it fail?  If governor->max_transition_latency is non-zero, but less
> than UNIT_MAX, the condition checked will be true to my eyes.

Bad wording. Sorry.

I meant, this 'if' check will always succeed (as you also noted), and
so we will always get the error message reported in this patch.

cpufreq: ondemand governor failed, too long transition latency of HW,
fallback to performance governor

-- 
viresh

Re: [PATCH] Revert "cpufreq: pcc-cpufreq: update default value of cpuinfo_transition_latency"

[Rafael J. Wysocki]

On Friday, July 22, 2016 08:36:56 AM Viresh Kumar wrote:
> On 22-07-16, 17:14, Andreas Herrmann wrote:
> > diff --git a/drivers/cpufreq/pcc-cpufreq.c b/drivers/cpufreq/pcc-cpufreq.c
> > index a7ecb9a..3f0ce2a 100644
> > --- a/drivers/cpufreq/pcc-cpufreq.c
> > +++ b/drivers/cpufreq/pcc-cpufreq.c
> > @@ -555,8 +555,6 @@ static int pcc_cpufreq_cpu_init(struct cpufreq_policy *policy)
> >  	policy->min = policy->cpuinfo.min_freq =
> >  		ioread32(&pcch_hdr->minimum_frequency) * 1000;
> >  
> > -	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
> > -
> >  	pr_debug("init: policy->max is %d, policy->min is %d\n",
> >  		policy->max, policy->min);
> >  out:
> 
> Hi Rafael,
> 
> I am very confused on this, can you help me understand ?
> 
> - CPUFREQ_ETERNAL = -1
> - unsigned int transition_latency = CPUFREQ_ETERNAL, will set it to UINT_MAX.
> - Many drivers do it today
> 
> cpufreq.c
> 
> 	if (policy->governor->max_transition_latency &&
> 	    policy->cpuinfo.transition_latency >
> 	    policy->governor->max_transition_latency) {
> 
> - And this check will always fail, unless max_transition_latency is zero.

Why would it fail?  If governor->max_transition_latency is non-zero, but less
than UNIT_MAX, the condition checked will be true to my eyes.

Thanks,
Rafael

Re: [PATCH] Revert "cpufreq: pcc-cpufreq: update default value of cpuinfo_transition_latency"

[Rafael J. Wysocki]

On Friday, July 22, 2016 02:28:52 PM Viresh Kumar wrote:
> On 22-07-16, 23:31, Rafael J. Wysocki wrote:
> > > cpufreq.c
> > > 
> > > 	if (policy->governor->max_transition_latency &&
> > > 	    policy->cpuinfo.transition_latency >
> > > 	    policy->governor->max_transition_latency) {
> > > 
> > > - And this check will always fail, unless max_transition_latency is zero.
> > 
> > Why would it fail?  If governor->max_transition_latency is non-zero, but less
> > than UNIT_MAX, the condition checked will be true to my eyes.
> 
> Bad wording. Sorry.
> 
> I meant, this 'if' check will always succeed (as you also noted), and
> so we will always get the error message reported in this patch.

Not always, but for drivers setting cpuinfo.transition_latency to CPUFREQ_ETERNAL.

Thanks,
Rafael

Re: [PATCH] Revert "cpufreq: pcc-cpufreq: update default value of cpuinfo_transition_latency"

[Viresh Kumar]

On 22-07-16, 23:46, Rafael J. Wysocki wrote:
> On Friday, July 22, 2016 02:28:52 PM Viresh Kumar wrote:
> > On 22-07-16, 23:31, Rafael J. Wysocki wrote:
> > > > cpufreq.c
> > > > 
> > > > 	if (policy->governor->max_transition_latency &&
> > > > 	    policy->cpuinfo.transition_latency >
> > > > 	    policy->governor->max_transition_latency) {
> > > > 
> > > > - And this check will always fail, unless max_transition_latency is zero.
> > > 
> > > Why would it fail?  If governor->max_transition_latency is non-zero, but less
> > > than UNIT_MAX, the condition checked will be true to my eyes.
> > 
> > Bad wording. Sorry.
> > 
> > I meant, this 'if' check will always succeed (as you also noted), and
> > so we will always get the error message reported in this patch.
> 
> Not always, but for drivers setting cpuinfo.transition_latency to CPUFREQ_ETERNAL.

So the drivers which have set their transition_latency to
CPUFREQ_ETERNAL, can't use ondemand governor unless
governor->max_transition_latency is set to 0 from userspace.

What should be done about this patch then ? It broke in late 2015.

-- 
viresh

Re: [PATCH] Revert "cpufreq: pcc-cpufreq: update default value of cpuinfo_transition_latency"

[Rafael J. Wysocki]

On Sat, Jul 23, 2016 at 1:30 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 22-07-16, 23:46, Rafael J. Wysocki wrote:
>> On Friday, July 22, 2016 02:28:52 PM Viresh Kumar wrote:
>> > On 22-07-16, 23:31, Rafael J. Wysocki wrote:
>> > > > cpufreq.c
>> > > >
>> > > >         if (policy->governor->max_transition_latency &&
>> > > >             policy->cpuinfo.transition_latency >
>> > > >             policy->governor->max_transition_latency) {
>> > > >
>> > > > - And this check will always fail, unless max_transition_latency is zero.
>> > >
>> > > Why would it fail?  If governor->max_transition_latency is non-zero, but less
>> > > than UNIT_MAX, the condition checked will be true to my eyes.
>> >
>> > Bad wording. Sorry.
>> >
>> > I meant, this 'if' check will always succeed (as you also noted), and
>> > so we will always get the error message reported in this patch.
>>
>> Not always, but for drivers setting cpuinfo.transition_latency to CPUFREQ_ETERNAL.
>
> So the drivers which have set their transition_latency to
> CPUFREQ_ETERNAL, can't use ondemand governor unless
> governor->max_transition_latency is set to 0 from userspace.
>
> What should be done about this patch then ? It broke in late 2015.

I'll apply the revert with a "Cc: stable" tag.

Question is what to do about the other drivers setting
cpuinfo.transition_latency to CPUFREQ_ETERNAL.

Thanks,
Rafael

Re: [PATCH] Revert "cpufreq: pcc-cpufreq: update default value of cpuinfo_transition_latency"

[Viresh Kumar]

On 23-07-16, 01:47, Rafael J. Wysocki wrote:
> I'll apply the revert with a "Cc: stable" tag.

That will work.

> Question is what to do about the other drivers setting
> cpuinfo.transition_latency to CPUFREQ_ETERNAL.

Perhaps leave them as is unless someone comes and reports a problem, they don't
seem to have any problem right now anyway :)

-- 
viresh