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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190809021607.j4qj3jm72gbisvqh@vireshk-i7>
Date:   Fri, 9 Aug 2019 07:46:07 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Doug Smythies <dsmythies@...us.net>
Cc:     linux-pm@...r.kernel.org,
        'Vincent Guittot' <vincent.guittot@...aro.org>,
        linux-kernel@...r.kernel.org, 'Rafael Wysocki' <rjw@...ysocki.net>,
        'Srinivas Pandruvada' <srinivas.pandruvada@...ux.intel.com>,
        'Len Brown' <lenb@...nel.org>
Subject: Re: [PATCH V4 2/2] cpufreq: intel_pstate: Implement QoS supported
 freq constraints

On 08-08-19, 09:25, Doug Smythies wrote:
> On 2019.08.07 00:06 Viresh Kumar wrote:
> Tested by: Doug Smythies <dsmythies@...us.net>
> 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