diff mbox series

[v2,1/3] ACPI: processor: perflib: Use the "no limit" frequency QoS

Message ID 12124970.O9o76ZdvQC@kreacher
State New
Headers show
Series [v2,1/3] ACPI: processor: perflib: Use the "no limit" frequency QoS | expand

Commit Message

Rafael J. Wysocki Dec. 28, 2022, 9:21 p.m. UTC 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

When _PPC returns 0, it means that the CPU frequency is not limited by
the platform firmware, so make acpi_processor_get_platform_limit()
update the frequency QoS request used by it to "no limit" in that case.

This addresses a problem with limiting CPU frequency artificially on
some systems after CPU offline/online to the frequency that corresponds
to the first entry in the _PSS return package.

Reported-by: Pratyush Yadav <ptyadav@amazon.de>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2:
   * Move some changes into a separate patch
   * Update the changelog accordingly

---
 drivers/acpi/processor_perflib.c |   20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

Comments

Pratyush Yadav Dec. 29, 2022, 12:58 p.m. UTC | #1 
Hi Rafael,

On Wed, Dec 28 2022, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> When _PPC returns 0, it means that the CPU frequency is not limited by
> the platform firmware, so make acpi_processor_get_platform_limit()
> update the frequency QoS request used by it to "no limit" in that case.
>
> This addresses a problem with limiting CPU frequency artificially on
> some systems after CPU offline/online to the frequency that corresponds
> to the first entry in the _PSS return package.
>
> Reported-by: Pratyush Yadav <ptyadav@amazon.de>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> v1 -> v2:
>    * Move some changes into a separate patch
>    * Update the changelog accordingly
>
> ---
>  drivers/acpi/processor_perflib.c |   20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> Index: linux-pm/drivers/acpi/processor_perflib.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/processor_perflib.c
> +++ linux-pm/drivers/acpi/processor_perflib.c
> @@ -53,6 +53,8 @@ static int acpi_processor_get_platform_l
>  {
>         acpi_status status = 0;
>         unsigned long long ppc = 0;
> +       s32 qos_value;
> +       int index;
>         int ret;
>
>         if (!pr)
> @@ -72,17 +74,27 @@ static int acpi_processor_get_platform_l
>                 }
>         }
>
> +       index = ppc;
> +
>         pr_debug("CPU %d: _PPC is %d - frequency %s limited\n", pr->id,
> -                      (int)ppc, ppc ? "" : "not");
> +                index, index ? "is" : "is not");
>
> -       pr->performance_platform_limit = (int)ppc;
> +       pr->performance_platform_limit = index;
>
>         if (ppc >= pr->performance->state_count ||
>             unlikely(!freq_qos_request_active(&pr->perflib_req)))
>                 return 0;
>
> -       ret = freq_qos_update_request(&pr->perflib_req,
> -                       pr->performance->states[ppc].core_frequency * 1000);
> +       /*
> +        * If _PPC returns 0, it means that all of the available states can be
> +        * used ("no limit").
> +        */
> +       if (index == 0)
> +               qos_value = FREQ_QOS_MAX_DEFAULT_VALUE;

One small thing I noticed: in acpi_processor_ppc_init() "no limit" value
is set to INT_MAX and here it is set to FREQ_QOS_MAX_DEFAULT_VALUE. Both
should evaluate to the same value but I think it would be nice if the
same thing is used in both places. Perhaps you can fix that up when
applying?

Other than this,

Reviewed-by: Pratyush Yadav <ptyadav@amazon.de>
Tested-by: Pratyush Yadav <ptyadav@amazon.de>

Thanks for working on this.

> +       else
> +               qos_value = pr->performance->states[index].core_frequency * 1000;
> +
> +       ret = freq_qos_update_request(&pr->perflib_req, qos_value);
>         if (ret < 0) {
>                 pr_warn("Failed to update perflib freq constraint: CPU%d (%d)\n",
>                         pr->id, ret);
>
>
>
Rafael J. Wysocki Dec. 29, 2022, 7:26 p.m. UTC | #2 
On Thu, Dec 29, 2022 at 1:58 PM Pratyush Yadav <ptyadav@amazon.de> wrote:
>
> Hi Rafael,
>
> On Wed, Dec 28 2022, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > When _PPC returns 0, it means that the CPU frequency is not limited by
> > the platform firmware, so make acpi_processor_get_platform_limit()
> > update the frequency QoS request used by it to "no limit" in that case.
> >
> > This addresses a problem with limiting CPU frequency artificially on
> > some systems after CPU offline/online to the frequency that corresponds
> > to the first entry in the _PSS return package.
> >
> > Reported-by: Pratyush Yadav <ptyadav@amazon.de>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > v1 -> v2:
> >    * Move some changes into a separate patch
> >    * Update the changelog accordingly
> >
> > ---
> >  drivers/acpi/processor_perflib.c |   20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/processor_perflib.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/processor_perflib.c
> > +++ linux-pm/drivers/acpi/processor_perflib.c
> > @@ -53,6 +53,8 @@ static int acpi_processor_get_platform_l
> >  {
> >         acpi_status status = 0;
> >         unsigned long long ppc = 0;
> > +       s32 qos_value;
> > +       int index;
> >         int ret;
> >
> >         if (!pr)
> > @@ -72,17 +74,27 @@ static int acpi_processor_get_platform_l
> >                 }
> >         }
> >
> > +       index = ppc;
> > +
> >         pr_debug("CPU %d: _PPC is %d - frequency %s limited\n", pr->id,
> > -                      (int)ppc, ppc ? "" : "not");
> > +                index, index ? "is" : "is not");
> >
> > -       pr->performance_platform_limit = (int)ppc;
> > +       pr->performance_platform_limit = index;
> >
> >         if (ppc >= pr->performance->state_count ||
> >             unlikely(!freq_qos_request_active(&pr->perflib_req)))
> >                 return 0;
> >
> > -       ret = freq_qos_update_request(&pr->perflib_req,
> > -                       pr->performance->states[ppc].core_frequency * 1000);
> > +       /*
> > +        * If _PPC returns 0, it means that all of the available states can be
> > +        * used ("no limit").
> > +        */
> > +       if (index == 0)
> > +               qos_value = FREQ_QOS_MAX_DEFAULT_VALUE;
>
> One small thing I noticed: in acpi_processor_ppc_init() "no limit" value
> is set to INT_MAX and here it is set to FREQ_QOS_MAX_DEFAULT_VALUE. Both
> should evaluate to the same value but I think it would be nice if the
> same thing is used in both places. Perhaps you can fix that up when
> applying?

Yes, I'll do that.

> Other than this,
>
> Reviewed-by: Pratyush Yadav <ptyadav@amazon.de>
> Tested-by: Pratyush Yadav <ptyadav@amazon.de>

Thanks!

> Thanks for working on this.

You're welcome.
Pratyush Yadav Jan. 30, 2023, 3:23 p.m. UTC | #3 
Hi Rafael,

On Mon, Jan 30 2023, Rafael J. Wysocki wrote:
> On Mon, Jan 30, 2023 at 3:18 PM Pratyush Yadav <ptyadav@amazon.de> wrote:
>>
>> Hi Rafael,
>>
>> On Thu, Dec 29 2022, Rafael J. Wysocki wrote:
>>
>> > On Thu, Dec 29, 2022 at 1:58 PM Pratyush Yadav <ptyadav@amazon.de> wrote:
>> >>
>> >> Hi Rafael,
>> >>
>> >> On Wed, Dec 28 2022, Rafael J. Wysocki wrote:
>> >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >> >
>> >> > When _PPC returns 0, it means that the CPU frequency is not limited by
>> >> > the platform firmware, so make acpi_processor_get_platform_limit()
>> >> > update the frequency QoS request used by it to "no limit" in that case.
>> >> >
>> >> > This addresses a problem with limiting CPU frequency artificially on
>> >> > some systems after CPU offline/online to the frequency that corresponds
>> >> > to the first entry in the _PSS return package.
>> >> >
>> >> > Reported-by: Pratyush Yadav <ptyadav@amazon.de>
>> >> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >> > ---
>> [...]
>> >>
>> >> One small thing I noticed: in acpi_processor_ppc_init() "no limit" value
>> >> is set to INT_MAX and here it is set to FREQ_QOS_MAX_DEFAULT_VALUE. Both
>> >> should evaluate to the same value but I think it would be nice if the
>> >> same thing is used in both places. Perhaps you can fix that up when
>> >> applying?
>> >
>> > Yes, I'll do that.
>>
>> Following up on this series. I do not see it queued anywhere in the
>> linux-pm [0] tree. I would like to have this in the v6.3 merge window if
>> possible.
>>
>> [0] https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/
>
> It's already in the mainline:
>
> e8a0e30b742f cpufreq: intel_pstate: Drop ACPI _PSS states table patching
> 99387b016022 ACPI: processor: perflib: Avoid updating frequency QoS
> unnecessarily
> c02d5feb6e2f ACPI: processor: perflib: Use the "no limit" frequency QoS

Hmm, I skimmed through the git log and did not spot any of these. Seems
like they were buried deeper in the logs due to merges. I can see them
now. My bad.
Srinivas Pandruvada Feb. 14, 2023, 1:40 p.m. UTC | #4 
On Mon, 2023-01-30 at 15:58 +0100, Rafael J. Wysocki wrote:
> On Mon, Jan 30, 2023 at 3:18 PM Pratyush Yadav <ptyadav@amazon.de>
> wrote:
> > 
> > 

[...]

> > [0]
> > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/
> 
> It's already in the mainline:
> 
> e8a0e30b742f cpufreq: intel_pstate: Drop ACPI _PSS states table
> patching
> 99387b016022 ACPI: processor: perflib: Avoid updating frequency QoS
> unnecessarily
> c02d5feb6e2f ACPI: processor: perflib: Use the "no limit" frequency
> QoS

I am checking 6.2-rc8.
I don't see these commits.

The last commit in mainline is
 git log --oneline drivers/acpi/processor_perflib.c
f1a70bac90ca ACPI: processor: perflib: Adjust
acpi_processor_notify_smm() return value

Whereas linux-pm tree it is :

git log --oneline drivers/acpi/processor_perflib.c
99387b016022 ACPI: processor: perflib: Avoid updating frequency QoS
unnecessarily
c02d5feb6e2f ACPI: processor: perflib: Use the "no limit" frequency QoS
f1a70bac90ca ACPI: processor: perflib: Adjust
acpi_processor_notify_smm() return value


Thanks,
Srinivas
Rafael J. Wysocki Feb. 14, 2023, 1:57 p.m. UTC | #5 
On Tue, Feb 14, 2023 at 2:40 PM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Mon, 2023-01-30 at 15:58 +0100, Rafael J. Wysocki wrote:
> > On Mon, Jan 30, 2023 at 3:18 PM Pratyush Yadav <ptyadav@amazon.de>
> > wrote:
> > >
> > >
>
> [...]
>
> > > [0]
> > > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/
> >
> > It's already in the mainline:
> >
> > e8a0e30b742f cpufreq: intel_pstate: Drop ACPI _PSS states table
> > patching
> > 99387b016022 ACPI: processor: perflib: Avoid updating frequency QoS
> > unnecessarily
> > c02d5feb6e2f ACPI: processor: perflib: Use the "no limit" frequency
> > QoS
>
> I am checking 6.2-rc8.
> I don't see these commits.

You are right, they are in linux-next only, sorry for the confusion.

I'm going to push them for 6.3-rc1 this week, though.
Srinivas Pandruvada Feb. 14, 2023, 2:25 p.m. UTC | #6 
On Tue, 2023-02-14 at 14:57 +0100, Rafael J. Wysocki wrote:
> On Tue, Feb 14, 2023 at 2:40 PM srinivas pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> > 
> > On Mon, 2023-01-30 at 15:58 +0100, Rafael J. Wysocki wrote:
> > > On Mon, Jan 30, 2023 at 3:18 PM Pratyush Yadav
> > > <ptyadav@amazon.de>
> > > wrote:
> > > > 
> > > > 
> > 
> > [...]
> > 
> > > > [0]
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/
> > > 
> > > It's already in the mainline:
> > > 
> > > e8a0e30b742f cpufreq: intel_pstate: Drop ACPI _PSS states table
> > > patching
> > > 99387b016022 ACPI: processor: perflib: Avoid updating frequency
> > > QoS
> > > unnecessarily
> > > c02d5feb6e2f ACPI: processor: perflib: Use the "no limit"
> > > frequency
> > > QoS
> > 
> > I am checking 6.2-rc8.
> > I don't see these commits.
> 
> You are right, they are in linux-next only, sorry for the confusion.
> 
> I'm going to push them for 6.3-rc1 this week, though.
I don't think they are marked for stable. Can we add that?

Thanks,
Srinivas
Rafael J. Wysocki Feb. 14, 2023, 2:41 p.m. UTC | #7 
On Tue, Feb 14, 2023 at 3:25 PM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Tue, 2023-02-14 at 14:57 +0100, Rafael J. Wysocki wrote:
> > On Tue, Feb 14, 2023 at 2:40 PM srinivas pandruvada
> > <srinivas.pandruvada@linux.intel.com> wrote:
> > >
> > > On Mon, 2023-01-30 at 15:58 +0100, Rafael J. Wysocki wrote:
> > > > On Mon, Jan 30, 2023 at 3:18 PM Pratyush Yadav
> > > > <ptyadav@amazon.de>
> > > > wrote:
> > > > >
> > > > >
> > >
> > > [...]
> > >
> > > > > [0]
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/
> > > >
> > > > It's already in the mainline:
> > > >
> > > > e8a0e30b742f cpufreq: intel_pstate: Drop ACPI _PSS states table
> > > > patching
> > > > 99387b016022 ACPI: processor: perflib: Avoid updating frequency
> > > > QoS
> > > > unnecessarily
> > > > c02d5feb6e2f ACPI: processor: perflib: Use the "no limit"
> > > > frequency
> > > > QoS
> > >
> > > I am checking 6.2-rc8.
> > > I don't see these commits.
> >
> > You are right, they are in linux-next only, sorry for the confusion.
> >
> > I'm going to push them for 6.3-rc1 this week, though.
> I don't think they are marked for stable. Can we add that?

I'd rather not rebase them for that.

It is still possible to send an inclusion request to -stable when then
get into the mainline.
diff mbox series

Patch

Index: linux-pm/drivers/acpi/processor_perflib.c
===================================================================
--- linux-pm.orig/drivers/acpi/processor_perflib.c
+++ linux-pm/drivers/acpi/processor_perflib.c
@@ -53,6 +53,8 @@  static int acpi_processor_get_platform_l
 {
 	acpi_status status = 0;
 	unsigned long long ppc = 0;
+	s32 qos_value;
+	int index;
 	int ret;
 
 	if (!pr)
@@ -72,17 +74,27 @@  static int acpi_processor_get_platform_l
 		}
 	}
 
+	index = ppc;
+
 	pr_debug("CPU %d: _PPC is %d - frequency %s limited\n", pr->id,
-		       (int)ppc, ppc ? "" : "not");
+		 index, index ? "is" : "is not");
 
-	pr->performance_platform_limit = (int)ppc;
+	pr->performance_platform_limit = index;
 
 	if (ppc >= pr->performance->state_count ||
 	    unlikely(!freq_qos_request_active(&pr->perflib_req)))
 		return 0;
 
-	ret = freq_qos_update_request(&pr->perflib_req,
-			pr->performance->states[ppc].core_frequency * 1000);
+	/*
+	 * If _PPC returns 0, it means that all of the available states can be
+	 * used ("no limit").
+	 */
+	if (index == 0)
+		qos_value = FREQ_QOS_MAX_DEFAULT_VALUE;
+	else
+		qos_value = pr->performance->states[index].core_frequency * 1000;
+
+	ret = freq_qos_update_request(&pr->perflib_req, qos_value);
 	if (ret < 0) {
 		pr_warn("Failed to update perflib freq constraint: CPU%d (%d)\n",
 			pr->id, ret);