* [PATCH] ACPI: processor: Add QoS requests for all CPUs
@ 2019-10-25 0:41 Rafael J. Wysocki
2019-10-25 2:53 ` Viresh Kumar
2019-10-25 8:51 ` Viresh Kumar
0 siblings, 2 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2019-10-25 0:41 UTC (permalink / raw)
To: Linux ACPI; +Cc: Linux PM, Viresh Kumar, Srinivas Pandruvada, LKML
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
The _PPC change notifications from the platform firmware are per-CPU,
so acpi_processor_ppc_init() needs to add a frequency QoS request
for each CPU covered by a cpufreq policy to take all of them into
account.
Even though ACPI thermal control of CPUs sets frequency limits
per processor package, it also needs a frequency QoS request for each
CPU in a cpufreq policy in case some of them are taken offline and
the frequency limit needs to be set through the remaining online
ones (this is slightly excessive, because all CPUs covered by one
cpufreq policy will set the same frequency limit through their QoS
requests, but it is not incorrect).
Modify the code in accordance with the above observations.
Fixes: d15ce412737a ("ACPI: cpufreq: Switch to QoS requests instead of cpufreq notifier")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/acpi/processor_perflib.c | 38 +++++++++++++++++++++++---------------
drivers/acpi/processor_thermal.c | 38 +++++++++++++++++++++++---------------
2 files changed, 46 insertions(+), 30 deletions(-)
Index: linux-pm/drivers/acpi/processor_thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/processor_thermal.c
+++ linux-pm/drivers/acpi/processor_thermal.c
@@ -127,26 +127,34 @@ static int cpufreq_set_cur_state(unsigne
void acpi_thermal_cpufreq_init(struct cpufreq_policy *policy)
{
- int cpu = policy->cpu;
- struct acpi_processor *pr = per_cpu(processors, cpu);
- int ret;
-
- if (!pr)
- return;
-
- ret = freq_qos_add_request(&policy->constraints, &pr->thermal_req,
- FREQ_QOS_MAX, INT_MAX);
- if (ret < 0)
- pr_err("Failed to add freq constraint for CPU%d (%d)\n", cpu,
- ret);
+ unsigned int cpu;
+
+ for_each_cpu(cpu, policy->related_cpus) {
+ struct acpi_processor *pr = per_cpu(processors, cpu);
+ int ret;
+
+ if (!pr)
+ continue;
+
+ ret = freq_qos_add_request(&policy->constraints,
+ &pr->thermal_req,
+ FREQ_QOS_MAX, INT_MAX);
+ if (ret < 0)
+ pr_err("Failed to add freq constraint for CPU%d (%d)\n",
+ cpu, ret);
+ }
}
void acpi_thermal_cpufreq_exit(struct cpufreq_policy *policy)
{
- struct acpi_processor *pr = per_cpu(processors, policy->cpu);
+ unsigned int cpu;
+
+ for_each_cpu(cpu, policy->related_cpus) {
+ struct acpi_processor *pr = per_cpu(processors, policy->cpu);
- if (pr)
- freq_qos_remove_request(&pr->thermal_req);
+ if (pr)
+ freq_qos_remove_request(&pr->thermal_req);
+ }
}
#else /* ! CONFIG_CPU_FREQ */
static int cpufreq_get_max_state(unsigned int cpu)
Index: linux-pm/drivers/acpi/processor_perflib.c
===================================================================
--- linux-pm.orig/drivers/acpi/processor_perflib.c
+++ linux-pm/drivers/acpi/processor_perflib.c
@@ -159,26 +159,34 @@ void acpi_processor_ignore_ppc_init(void
void acpi_processor_ppc_init(struct cpufreq_policy *policy)
{
- int cpu = policy->cpu;
- struct acpi_processor *pr = per_cpu(processors, cpu);
- int ret;
-
- if (!pr)
- return;
-
- ret = freq_qos_add_request(&policy->constraints, &pr->perflib_req,
- FREQ_QOS_MAX, INT_MAX);
- if (ret < 0)
- pr_err("Failed to add freq constraint for CPU%d (%d)\n", cpu,
- ret);
+ unsigned int cpu;
+
+ for_each_cpu(cpu, policy->related_cpus) {
+ struct acpi_processor *pr = per_cpu(processors, cpu);
+ int ret;
+
+ if (!pr)
+ continue;
+
+ ret = freq_qos_add_request(&policy->constraints,
+ &pr->perflib_req,
+ FREQ_QOS_MAX, INT_MAX);
+ if (ret < 0)
+ pr_err("Failed to add freq constraint for CPU%d (%d)\n",
+ cpu, ret);
+ }
}
void acpi_processor_ppc_exit(struct cpufreq_policy *policy)
{
- struct acpi_processor *pr = per_cpu(processors, policy->cpu);
+ unsigned int cpu;
+
+ for_each_cpu(cpu, policy->related_cpus) {
+ struct acpi_processor *pr = per_cpu(processors, cpu);
- if (pr)
- freq_qos_remove_request(&pr->perflib_req);
+ if (pr)
+ freq_qos_remove_request(&pr->perflib_req);
+ }
}
static int acpi_processor_get_performance_control(struct acpi_processor *pr)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ACPI: processor: Add QoS requests for all CPUs
2019-10-25 0:41 [PATCH] ACPI: processor: Add QoS requests for all CPUs Rafael J. Wysocki
@ 2019-10-25 2:53 ` Viresh Kumar
2019-10-25 8:17 ` Rafael J. Wysocki
2019-10-25 8:51 ` Viresh Kumar
1 sibling, 1 reply; 6+ messages in thread
From: Viresh Kumar @ 2019-10-25 2:53 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Linux ACPI, Linux PM, Srinivas Pandruvada, LKML
On 25-10-19, 02:41, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The _PPC change notifications from the platform firmware are per-CPU,
> so acpi_processor_ppc_init() needs to add a frequency QoS request
> for each CPU covered by a cpufreq policy to take all of them into
> account.
>
> Even though ACPI thermal control of CPUs sets frequency limits
> per processor package, it also needs a frequency QoS request for each
> CPU in a cpufreq policy in case some of them are taken offline and
> the frequency limit needs to be set through the remaining online
> ones (this is slightly excessive, because all CPUs covered by one
> cpufreq policy will set the same frequency limit through their QoS
> requests, but it is not incorrect).
>
> Modify the code in accordance with the above observations.
I am not sure if I understood everything you just said, but I don't
see how things can break with the current code we have.
Both acpi_thermal_cpufreq_init() and acpi_processor_ppc_init() are
called from acpi_processor_notifier() which is registered as a policy
notifier and is called when a policy is created or removed. Even if
some CPUs of a policy go offline, it won't matter as the request for
the policy stays and it will be dropped only when all the CPUs of a
policy go offline.
What am I missing ?
--
viresh
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ACPI: processor: Add QoS requests for all CPUs
2019-10-25 2:53 ` Viresh Kumar
@ 2019-10-25 8:17 ` Rafael J. Wysocki
2019-10-25 8:46 ` Rafael J. Wysocki
2019-10-25 8:51 ` Viresh Kumar
0 siblings, 2 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2019-10-25 8:17 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Linux ACPI, Linux PM, Srinivas Pandruvada, LKML
On Fri, Oct 25, 2019 at 4:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 25-10-19, 02:41, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The _PPC change notifications from the platform firmware are per-CPU,
> > so acpi_processor_ppc_init() needs to add a frequency QoS request
> > for each CPU covered by a cpufreq policy to take all of them into
> > account.
> >
> > Even though ACPI thermal control of CPUs sets frequency limits
> > per processor package, it also needs a frequency QoS request for each
> > CPU in a cpufreq policy in case some of them are taken offline and
> > the frequency limit needs to be set through the remaining online
> > ones (this is slightly excessive, because all CPUs covered by one
> > cpufreq policy will set the same frequency limit through their QoS
> > requests, but it is not incorrect).
> >
> > Modify the code in accordance with the above observations.
>
> I am not sure if I understood everything you just said, but I don't
> see how things can break with the current code we have.
>
> Both acpi_thermal_cpufreq_init() and acpi_processor_ppc_init() are
> called from acpi_processor_notifier() which is registered as a policy
> notifier and is called when a policy is created or removed. Even if
> some CPUs of a policy go offline, it won't matter as the request for
> the policy stays and it will be dropped only when all the CPUs of a
> policy go offline.
>
> What am I missing ?
The way the request is used.
Say there are two CPUs, A and B, in the same policy. A is
policy->cpu, so acpi_processor_ppc_init() adds a QoS request for A
only (note that the B's QoS request, B->perflib_req, remains
inactive).
Now, some time later, the platform firmware notifies the OS of a _PPC
change for B. That means acpi_processor_notify() is called and it
calls acpi_processor_ppc_has_changed(B) and that invokes
acpi_processor_get_platform_limit(B), which in turn looks at the B's
QoS request (B->perflib_req) and sees that it is inactive, so 0 is
returned without doing anything. However, *some* QoS request should
be updated then.
Would it be correct to update the A's QoS request in that case? No,
because the _PPC limit for A may be different that the _PPC limit for
B in principle.
The thermal case is not completely analogous, because
cpufreq_set_cur_state() finds online CPUs in the same package as the
target one and tries to update the QoS request for each of them, which
will include the original policy->cpu, whose QoS request has been
registered by acpi_thermal_cpufreq_init(), as long as it is online.
If it is offline, it will be skipped and there is no easy way to find
a "previous policy->cpu". It is possible to do that, but IMO it is
more straightforward to have a request for each CPU added.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ACPI: processor: Add QoS requests for all CPUs
2019-10-25 8:17 ` Rafael J. Wysocki
@ 2019-10-25 8:46 ` Rafael J. Wysocki
2019-10-25 8:51 ` Viresh Kumar
1 sibling, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2019-10-25 8:46 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Linux ACPI, Linux PM, Srinivas Pandruvada, LKML
On Fri, Oct 25, 2019 at 10:17 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Oct 25, 2019 at 4:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 25-10-19, 02:41, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > The _PPC change notifications from the platform firmware are per-CPU,
> > > so acpi_processor_ppc_init() needs to add a frequency QoS request
> > > for each CPU covered by a cpufreq policy to take all of them into
> > > account.
> > >
> > > Even though ACPI thermal control of CPUs sets frequency limits
> > > per processor package, it also needs a frequency QoS request for each
> > > CPU in a cpufreq policy in case some of them are taken offline and
> > > the frequency limit needs to be set through the remaining online
> > > ones (this is slightly excessive, because all CPUs covered by one
> > > cpufreq policy will set the same frequency limit through their QoS
> > > requests, but it is not incorrect).
> > >
> > > Modify the code in accordance with the above observations.
> >
> > I am not sure if I understood everything you just said, but I don't
> > see how things can break with the current code we have.
> >
> > Both acpi_thermal_cpufreq_init() and acpi_processor_ppc_init() are
> > called from acpi_processor_notifier() which is registered as a policy
> > notifier and is called when a policy is created or removed. Even if
> > some CPUs of a policy go offline, it won't matter as the request for
> > the policy stays and it will be dropped only when all the CPUs of a
> > policy go offline.
> >
> > What am I missing ?
>
> The way the request is used.
>
> Say there are two CPUs, A and B, in the same policy. A is
> policy->cpu, so acpi_processor_ppc_init() adds a QoS request for A
> only (note that the B's QoS request, B->perflib_req, remains
> inactive).
>
> Now, some time later, the platform firmware notifies the OS of a _PPC
> change for B. That means acpi_processor_notify() is called and it
> calls acpi_processor_ppc_has_changed(B) and that invokes
> acpi_processor_get_platform_limit(B), which in turn looks at the B's
> QoS request (B->perflib_req) and sees that it is inactive, so 0 is
> returned without doing anything. However, *some* QoS request should
> be updated then.
>
> Would it be correct to update the A's QoS request in that case? No,
> because the _PPC limit for A may be different that the _PPC limit for
> B in principle.
>
> The thermal case is not completely analogous, because
> cpufreq_set_cur_state() finds online CPUs in the same package as the
> target one and tries to update the QoS request for each of them, which
> will include the original policy->cpu, whose QoS request has been
> registered by acpi_thermal_cpufreq_init(), as long as it is online.
> If it is offline, it will be skipped and there is no easy way to find
> a "previous policy->cpu". It is possible to do that, but IMO it is
> more straightforward to have a request for each CPU added.
BTW, IMO processor_thremal can be changed to use one frequency QoS
request per policy on top of this, but I'd rather take one step at a
time. :-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ACPI: processor: Add QoS requests for all CPUs
2019-10-25 8:17 ` Rafael J. Wysocki
2019-10-25 8:46 ` Rafael J. Wysocki
@ 2019-10-25 8:51 ` Viresh Kumar
1 sibling, 0 replies; 6+ messages in thread
From: Viresh Kumar @ 2019-10-25 8:51 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Linux ACPI, Linux PM, Srinivas Pandruvada, LKML
On 25-10-19, 10:17, Rafael J. Wysocki wrote:
> On Fri, Oct 25, 2019 at 4:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 25-10-19, 02:41, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > The _PPC change notifications from the platform firmware are per-CPU,
> > > so acpi_processor_ppc_init() needs to add a frequency QoS request
> > > for each CPU covered by a cpufreq policy to take all of them into
> > > account.
> > >
> > > Even though ACPI thermal control of CPUs sets frequency limits
> > > per processor package, it also needs a frequency QoS request for each
> > > CPU in a cpufreq policy in case some of them are taken offline and
> > > the frequency limit needs to be set through the remaining online
> > > ones (this is slightly excessive, because all CPUs covered by one
> > > cpufreq policy will set the same frequency limit through their QoS
> > > requests, but it is not incorrect).
> > >
> > > Modify the code in accordance with the above observations.
> >
> > I am not sure if I understood everything you just said, but I don't
> > see how things can break with the current code we have.
> >
> > Both acpi_thermal_cpufreq_init() and acpi_processor_ppc_init() are
> > called from acpi_processor_notifier() which is registered as a policy
> > notifier and is called when a policy is created or removed. Even if
> > some CPUs of a policy go offline, it won't matter as the request for
> > the policy stays and it will be dropped only when all the CPUs of a
> > policy go offline.
> >
> > What am I missing ?
>
> The way the request is used.
Yes, I missed the point :)
--
viresh
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ACPI: processor: Add QoS requests for all CPUs
2019-10-25 0:41 [PATCH] ACPI: processor: Add QoS requests for all CPUs Rafael J. Wysocki
2019-10-25 2:53 ` Viresh Kumar
@ 2019-10-25 8:51 ` Viresh Kumar
1 sibling, 0 replies; 6+ messages in thread
From: Viresh Kumar @ 2019-10-25 8:51 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Linux ACPI, Linux PM, Srinivas Pandruvada, LKML
On 25-10-19, 02:41, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The _PPC change notifications from the platform firmware are per-CPU,
> so acpi_processor_ppc_init() needs to add a frequency QoS request
> for each CPU covered by a cpufreq policy to take all of them into
> account.
>
> Even though ACPI thermal control of CPUs sets frequency limits
> per processor package, it also needs a frequency QoS request for each
> CPU in a cpufreq policy in case some of them are taken offline and
> the frequency limit needs to be set through the remaining online
> ones (this is slightly excessive, because all CPUs covered by one
> cpufreq policy will set the same frequency limit through their QoS
> requests, but it is not incorrect).
>
> Modify the code in accordance with the above observations.
>
> Fixes: d15ce412737a ("ACPI: cpufreq: Switch to QoS requests instead of cpufreq notifier")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/acpi/processor_perflib.c | 38 +++++++++++++++++++++++---------------
> drivers/acpi/processor_thermal.c | 38 +++++++++++++++++++++++---------------
> 2 files changed, 46 insertions(+), 30 deletions(-)
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-25 8:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-25 0:41 [PATCH] ACPI: processor: Add QoS requests for all CPUs Rafael J. Wysocki
2019-10-25 2:53 ` Viresh Kumar
2019-10-25 8:17 ` Rafael J. Wysocki
2019-10-25 8:46 ` Rafael J. Wysocki
2019-10-25 8:51 ` Viresh Kumar
2019-10-25 8:51 ` Viresh Kumar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).