Re: [PATCH V4 2/2] cpufreq: intel_pstate: Implement QoS supported freq constraints

[Viresh Kumar]

On 08-08-19, 09:25, Doug Smythies wrote:
> On 2019.08.07 00:06 Viresh Kumar wrote:
> Tested by: Doug Smythies <dsmythies@xxxxxxxxx>
> Thermald seems to now be working O.K. for all the governors.

Thanks for testing Doug.

> I do note that if one sets
> /sys/devices/system/cpu/cpufreq/policy*/scaling_max_freq
> It seems to override subsequent attempts via
> /sys/devices/system/cpu/intel_pstate/max_perf_pct.
> Myself, I find this confusing.
> 
> So the question becomes which one is the "master"?

No one is master, cpufreq takes all the requests for frequency
constraints and tries to set the value based on aggregation of all. So
for max frequency, the lowest value wins and is shown up in sysfs.

So, everything looks okay to me.

> > +static void update_qos_request(enum dev_pm_qos_req_type type)
> > +{
> > +	int max_state, turbo_max, freq, i, perf_pct;
> > +	struct dev_pm_qos_request *req;
> > +	struct cpufreq_policy *policy;
> > +
> > +	for_each_possible_cpu(i) {
> > +		struct cpudata *cpu = all_cpu_data[i];
> > +
> > +		policy = cpufreq_cpu_get(i);
> > +		if (!policy)
> > +			continue;
> > +
> > +		req = policy->driver_data;
> > +		cpufreq_cpu_put(policy);
> > +
> > +		if (!req)
> > +			continue;
> > +
> > +		if (hwp_active)
> > +			intel_pstate_get_hwp_max(i, &turbo_max, &max_state);
> > +		else
> > +			turbo_max = cpu->pstate.turbo_pstate;
> > +
> > +		if (type == DEV_PM_QOS_MIN_FREQUENCY) {
> 
> Is it O.K. to assume if the passed op code is
> not DEV_PM_QOS_MIN_FREQUENCY
> then it must have been
> DEV_PM_QOS_MAX_FREQUENCY
> ?
> 
> It is within this patch, but what about in future?

Yes, because it is called locally there is no need to add another if
statement here. And reviews should catch it in future and I don't
expect it to change much anyway.

> > +			perf_pct = global.min_perf_pct;
> > +		} else {
> > +			req++;
> > +			perf_pct = global.max_perf_pct;
> > +		}
> > +
> > +		freq = DIV_ROUND_UP(turbo_max * perf_pct, 100);
> > +		freq *= cpu->pstate.scaling;
> > +
> > +		if (dev_pm_qos_update_request(req, freq))
> > +			pr_warn("Failed to update freq constraint: CPU%d\n", i);
> 
> I get many of these messages (4520 so far, always in groups of 8 (I have 8 CPUs)),
> and have yet to figure out exactly why. It seems to actually be working fine.

Because of something I missed. dev_pm_qos_update_request() can return
1, when the constraint value gets changed. Will fix this patch.

-- 
viresh