ACPI: processor: Add QoS requests for all CPUs
diff mbox series

Message ID 2435090.1mJ0fSsrDY@kreacher
State Awaiting Upstream
Delegated to: Rafael Wysocki
Headers show
Series
  • ACPI: processor: Add QoS requests for all CPUs
Related show

Commit Message

Rafael J. Wysocki Oct. 25, 2019, 12:41 a.m. UTC
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(-)

Comments

Viresh Kumar Oct. 25, 2019, 2:53 a.m. UTC | #1
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 ?
Rafael J. Wysocki Oct. 25, 2019, 8:17 a.m. UTC | #2
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.
Rafael J. Wysocki Oct. 25, 2019, 8:46 a.m. UTC | #3
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. :-)
Viresh Kumar Oct. 25, 2019, 8:51 a.m. UTC | #4
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 Kumar Oct. 25, 2019, 8:51 a.m. UTC | #5
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 :)

Patch
diff mbox series

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)