Re: [PATCH v3 1/6] cpufreq: intel_p_state: Fix limiting turbo sub states

On Mon, 2015-10-05 at 17:43 -0700, Srinivas Pandruvada wrote:

On Tue, 2015-10-06 at 00:56 +0200, Rafael J. Wysocki wrote:

[..]

quoted

 struct vid_data {

@@ -132,6 +133,8 @@ struct pstate_funcs {
    int (*get_scaling)(void);
    void (*set)(struct cpudata*, int pstate);
    void (*get_vid)(struct cpudata *);
+   u64 (*get_turbo_ratio_limit)(struct cpudata *);
+   int (*set_turbo_ratio_limit)(struct cpudata *, u64, u64);

An int is always passed as the last arg to this, so why is the arg

u64?

It can be int as we care only about low byte which is max turbo ratio.

quoted

 };

[..]

quoted

 Instead of shifting def_ratio and new_ratio in each
step, I'd shift max_ratio and the mask:

	u64 mask = 0xff;
	u64 max_ratio = new_ratio & mask;

	while (mask) {
		if (def_ratio & mask) {
			u64 ratio;

			if (new_ratio & mask) {
				ratio = new_ratio & mask;
			} else {
				ratio = def_ratio & mask;
				if (ratio > max_ratio)
					ratio = max_ratio;
			}
			out_ratio |= ratio;
		}
		max_ratio <<= 8;
		mask <<= 8;
	}

I will experiment with your algorithm and check.

Your algorithm works, so will change to this.

[..]

quoted

quoted

+	if (pstate_funcs.get_turbo_ratio_limit &&
+	    !cpu->pstate.turbo_ratio_limit)
+		cpu->pstate.turbo_ratio_limit =
+			pstate_funcs.get_turbo_ratio_limit(cpu);

So we read MSR_NHM_TURBO_RATIO_LIMIT and store it into pstate.turbo_ratio_limit
unless alread populated.  This means we do it only once during initialization.

What if the value we have in there is stale after, say, system suspend-resume?

Did I mess up here? You mean data stored in a variable
cpu->pstate.turbo_ratio_limit is corrupted after suspend/resume. This is
a vzalloc memory.. 
all_cpu_data = vzalloc(sizeof(void *) * num_possible_cpus()) one time
during __init intel_pstate_init

Also 	all_cpu_data[cpunum] is allocated only once during first cpufreq
callback for init and not freed
	if (!all_cpu_data[cpunum])
		all_cpu_data[cpunum] = kzalloc(sizeof(struct cpudata),
					       GFP_KERNEL);
Did STR tests, didn't see issue. Not sure how else corruption can happen
unless slab memory is corrupted. Can you tell how it will be stale?

Thanks,
Srinivas

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html