Bug 83151

I also noticed that because of the changes of the turbo state not expected by
intel_pstate, max_perf is sometimes wrong.

The patch should fix my problem. I don't know if there's a way to avoid
reading the register every time or if there's a better way to deal with my
problem.

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index c5eac94..b8adfec 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -218,6 +218,13 @@ static inline void intel_pstate_reset_all_pid(void)
 	}
 }
 
+static int turbo_disabled(void) {
+	u64 misc_en;
+	rdmsrl(MSR_IA32_MISC_ENABLE, misc_en);
+	return (limits.turbo_disabled ||
+	        (misc_en & MSR_IA32_MISC_ENABLE_TURBO_DISABLE));
+}
+
 /************************** debugfs begin ************************/
 static int pid_param_set(void *data, u64 val)
 {
@@ -284,10 +291,8 @@ static ssize_t store_no_turbo(struct kobject *a, struct attribute *b,
 	if (ret != 1)
 		return -EINVAL;
 	limits.no_turbo = clamp_t(int, input, 0 , 1);
-	if (limits.turbo_disabled) {
+	if (turbo_disabled())
 		pr_warn("Turbo disabled by BIOS or unavailable on processor\n");
-		limits.no_turbo = limits.turbo_disabled;
-	}
 	return count;
 }
 
@@ -323,6 +328,12 @@ static ssize_t store_min_perf_pct(struct kobject *a, struct attribute *b,
 	return count;
 }
 
+static ssize_t show_actual_no_turbo(struct kobject *kobj,
+				     struct attribute *attr, char *buf)
+{
+	return sprintf(buf, "%u\n", limits.no_turbo || turbo_disabled());
+}
+
 show_one(no_turbo, no_turbo);
 show_one(max_perf_pct, max_perf_pct);
 show_one(min_perf_pct, min_perf_pct);
@@ -330,11 +341,13 @@ show_one(min_perf_pct, min_perf_pct);
 define_one_global_rw(no_turbo);
 define_one_global_rw(max_perf_pct);
 define_one_global_rw(min_perf_pct);
+define_one_global_ro(actual_no_turbo);
 
 static struct attribute *intel_pstate_attributes[] = {
 	&no_turbo.attr,
 	&max_perf_pct.attr,
 	&min_perf_pct.attr,
+	&actual_no_turbo.attr,
 	NULL
 };
 
@@ -386,7 +399,7 @@ static void byt_set_pstate(struct cpudata *cpudata, int pstate)
 	u32 vid;
 
 	val = pstate << 8;
-	if (limits.no_turbo && !limits.turbo_disabled)
+	if (limits.no_turbo && !turbo_disabled())
 		val |= (u64)1 << 32;
 
 	vid_fp = cpudata->vid.min + mul_fp(
@@ -454,7 +467,7 @@ static void core_set_pstate(struct cpudata *cpudata, int pstate)
 	u64 val;
 
 	val = pstate << 8;
-	if (limits.no_turbo && !limits.turbo_disabled)
+	if (limits.no_turbo && !turbo_disabled())
 		val |= (u64)1 << 32;
 
 	wrmsrl_on_cpu(cpudata->cpu, MSR_IA32_PERF_CTL, val);
@@ -501,7 +514,7 @@ static void intel_pstate_get_min_max(struct cpudata *cpu, int *min, int *max)
 	int max_perf_adj;
 	int min_perf;
 
-	if (limits.no_turbo)
+	if (limits.no_turbo || turbo_disabled())
 		max_perf = cpu->pstate.max_pstate;
 
 	max_perf_adj = fp_toint(mul_fp(int_tofp(max_perf), limits.max_perf));
@@ -719,7 +732,6 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
 		limits.min_perf = int_tofp(1);
 		limits.max_perf_pct = 100;
 		limits.max_perf = int_tofp(1);
-		limits.no_turbo = limits.turbo_disabled;
 		return 0;
 	}
 	limits.min_perf_pct = (policy->min * 100) / policy->cpuinfo.max_freq;
@@ -762,7 +774,6 @@ static int intel_pstate_cpu_init(struct cpufreq_policy *policy)
 {
 	struct cpudata *cpu;
 	int rc;
-	u64 misc_en;
 
 	rc = intel_pstate_init_cpu(policy->cpu);
 	if (rc)
@@ -770,12 +781,8 @@ static int intel_pstate_cpu_init(struct cpufreq_policy *policy)
 
 	cpu = all_cpu_data[policy->cpu];
 
-	rdmsrl(MSR_IA32_MISC_ENABLE, misc_en);
-	if (misc_en & MSR_IA32_MISC_ENABLE_TURBO_DISABLE ||
-	    cpu->pstate.max_pstate == cpu->pstate.turbo_pstate) {
+	if (cpu->pstate.max_pstate == cpu->pstate.turbo_pstate)
 		limits.turbo_disabled = 1;
-		limits.no_turbo = 1;
-	}
 	if (limits.min_perf_pct == 100 && limits.max_perf_pct == 100)
 		policy->policy = CPUFREQ_POLICY_PERFORMANCE;
 	else

I am just back from vacation give me a couple of days to dig out of the inbox to review/test

Created attachment 148771 [details]
Allow modification of no_turbo sysfs file based on current state of MSR_IA32_MISC_ENABLE_TURBO_DISABLE

Can you try the attached patch? I do not have a system that dynamically sets MSR_IA32_MISC_ENABLE_TURBO_DISABLE

It doesn't work correctly. If the turbo is disabled by the BIOS and then
I try to do something with no_turbo, it will no longer be possible for
me to do any change as nothing sets turbo_disabled back to 0.

I modified it, but still doesn't work perfectly.
I think that the problem is that max_perf depends on limits.no_turbo
which is updated when the user tries to access it through the sysfs interface.

With the current patch, this is a problem when the AC is present and the
turbo is disabled. If I unplug my laptop, then the max frequency will be
slightly lower than what it should be. That's at least what I noticed, I thought
there would be more problematic situations.

I will try to see if this can be avoided, in the meantime, here what I did so far.

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index c5eac94..1a10971 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -274,20 +274,54 @@ static void __init intel_pstate_debug_expose_params(void)
 		return sprintf(buf, "%u\n", limits.object);		\
 	}
 
+static ssize_t show_no_turbo(struct kobject *kobj,
+			     struct attribute *attr, char *buf)
+{
+	u64 misc_en;
+	int turbo_disabled = limits.turbo_disabled;
+	struct cpudata *cpu;
+
+	cpu = all_cpu_data[0];
+	rdmsrl(MSR_IA32_MISC_ENABLE, misc_en);
+	if (misc_en & MSR_IA32_MISC_ENABLE_TURBO_DISABLE ||
+	    cpu->pstate.max_pstate == cpu->pstate.turbo_pstate) {
+		limits.turbo_disabled = 1;
+	else
+		limits.turbo_disabled = 0;
+
+	if (limits.turbo_disabled != turbo_disabled)
+		limits.no_turbo = limits.turbo_disabled;
+
+	return sprintf(buf, "%u\n", limits.no_turbo);
+}
+
+
 static ssize_t store_no_turbo(struct kobject *a, struct attribute *b,
 			      const char *buf, size_t count)
 {
 	unsigned int input;
 	int ret;
+	u64 misc_en;
+	struct cpudata *cpu;
 
+	cpu = all_cpu_data[0];
 	ret = sscanf(buf, "%u", &input);
 	if (ret != 1)
 		return -EINVAL;
-	limits.no_turbo = clamp_t(int, input, 0 , 1);
+
+	rdmsrl(MSR_IA32_MISC_ENABLE, misc_en);
+	if (misc_en & MSR_IA32_MISC_ENABLE_TURBO_DISABLE ||
+	    cpu->pstate.max_pstate == cpu->pstate.turbo_pstate) {
+		limits.turbo_disabled = 1;
+	else
+		limits.turbo_disabled = 0;
+
 	if (limits.turbo_disabled) {
 		pr_warn("Turbo disabled by BIOS or unavailable on processor\n");
 		limits.no_turbo = limits.turbo_disabled;
-	}
+	} else
+		limits.no_turbo = clamp_t(int, input, 0 , 1);
+
 	return count;
 }
 
@@ -323,7 +357,6 @@ static ssize_t store_min_perf_pct(struct kobject *a, struct attribute *b,
 	return count;
 }
 
-show_one(no_turbo, no_turbo);
 show_one(max_perf_pct, max_perf_pct);
 show_one(min_perf_pct, min_perf_pct);

Created attachment 148851 [details]
Don't rely on the initial state of turbo

I modified the first patch I sent as I couldn't find a better
alternative solution as reliable as it. I just made few changes, 
"actual_no_turbo" is not really necessary, but it prevents any change of
"no_turbo" and yet know whether the turbo is enabled or not.

Created attachment 149081 [details]
cpufreq: intel_pstate: Don't rely on the initial state of the  turbo

I noticed that the last time I uploaded the wrong file, the kernel doesn't
even compile with that patch, sorry.

Anyway, I did some more changes. MSR_IA32_MISC_ENABLE_TURBO_DISABLE is still
read frequently, but less now. I also got rid of actual_no_turbo, no_turbo
is changed when turbo_disabled changes as it is currently done.

The only problem I see with this patch is that the BIOS could change
MSR_IA32_MISC_ENABLE_TURBO_DISABLE after update_turbo_state(), but before 
MSR_IA32_PERF_CTL is written. It shouldn't be a problem on my system though.

I have a question. Is it normal that when the turbo is disabled and all the
cores are at the max frequency allowed, the frequency doesn't change when I
re-enable the turbo? The frequency has to decrease first and then can increase.
The opposite transition (from turbo freq to non-turbo freq) is immediate.

Created attachment 149201 [details]
cpufreq: intel_pstate: Don't rely on the initial state of the turbo (update)

Gabriele modified you patch slightly to not touch limits.no_turbo so user settings can survive limits.turbo_disable changes. I the correct value for the no_turbo sysfs file should be shown in all cases.

Please take a look. TIA

Created attachment 149221 [details]
cpufreq: intel_pstate: Don't rely on the initial state of the turbo (update 2)

I tested the patch, it's working fine. It's exactly what I initially wanted
to do.

However, I would make some changes in store_no_turbo(). If the turbo is
disabled by the BIOS and I try to enable it, I don't want to have it disabled
once the BIOS re-enable it.
We could set no_turbo to 0 (its value has no effect in case turbo_disabled is 1
now), don't allow changes or allow changes.

- If we don't allow changes, it could be annoying as the user maybe doesn't
  know the conditions under which he can change no_turbo.
- If we set no_turbo to 0, the user preference doesn't change unless the
  user tries to change it when changes are not allowed.
- If we allow changes, it could be confusing as the user can't verify the
  success of his changes.

These are the considerations that led me to introduce "actual_no_turbo" (and make no_turbo dependant on turbo_disabled once I removed it).

That said, I updated the patch in order to always allow changes of no_turbo.

Note that I brought back "no_turbo = 0" in intel_pstate_set_policy() as it was
before limits.turbo_disabled was introduced, even though I prefer when it's not
there as it continuosly override my no_turbo preference. In any case, I would
do that change in a different commit.

(In reply to Gabriele Mazzotta from comment #8)
> Created attachment 149221 [details]
> cpufreq: intel_pstate: Don't rely on the initial state of the turbo (update
> 2)
> 
> I tested the patch, it's working fine. It's exactly what I initially wanted
> to do.
> 

Good :-)

> However, I would make some changes in store_no_turbo(). If the turbo is
> disabled by the BIOS and I try to enable it, I don't want to have it disabled
> once the BIOS re-enable it.
> We could set no_turbo to 0 (its value has no effect in case turbo_disabled
> is 1
> now), don't allow changes or allow changes.
> 
> - If we don't allow changes, it could be annoying as the user maybe doesn't
>   know the conditions under which he can change no_turbo.
> - If we set no_turbo to 0, the user preference doesn't change unless the
>   user tries to change it when changes are not allowed.
> - If we allow changes, it could be confusing as the user can't verify the
>   success of his changes.
> 
> These are the considerations that led me to introduce "actual_no_turbo" (and
> make no_turbo dependant on turbo_disabled once I removed it).
> 

So i can argue both sides of this :-)  What I went for was to accurately
reflect the current state of the system while not crushing the user setting
when the BIOS changed the availability of turbo.  I would rather the user
be annoyed that the setting was not changeable and log the message to the log
than have them think that they had forced turbo on when they really hadn't.
They will flip no_trubo to 0 then go measure the system with turbostat or
other system monitoring tools and then here the "turbo doesn't work" bug report comes.

The BIOS changing the availability of turbo takes the decision out of the users
hands I think the interface should reflect this. Maybe the message logged when the user tries to enable turbo when it not available needs to be better.

  
> That said, I updated the patch in order to always allow changes of no_turbo.
> 
> Note that I brought back "no_turbo = 0" in intel_pstate_set_policy() as it
> was
> before limits.turbo_disabled was introduced, even though I prefer when it's
> not
> there as it continuosly override my no_turbo preference. In any case, I would
> do that change in a different commit.

Created attachment 149551 [details]
cpufreq: intel_pstate: Don't rely on the initial state of the turbo (update 3)

Ok then.

I did a minor change. Tying to change the state of the turbo when it's not
allowed will now result in an error. I think this makes things a bit more clear.

I don't see this patch upstream -- re-opening.
moving to intel_pstate from cpufreq category.

Hi,

the fix is upstream (Linux v3.18-rc2) and was applied to the stable trees, so I mark this bug as resolved.


commit 4521e1a0ce173daa23dfef8312d09051e057ff8e
Author: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Date:   Mon Oct 13 08:37:41 2014 -0700

    cpufreq: intel_pstate: Reflect current no_turbo state correctly

thanks Gabriele -- marking this one closed.