[PATCH v1 1/2] ACPI: processor: perflib: Use the "no limit" frequency QoS

[PATCH v1 1/2] ACPI: processor: perflib: Use the "no limit" frequency QoS

[Rafael J. Wysocki]

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
and avoid updating the QoS request when the _PPC return value has not
changed.

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.

While at it, move the _PPC return value check against the state count
earlier to avoid setting performance_platform_limit to an invalid value.

Reported-by: Pratyush Yadav <ptyadav@amazon.de>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/processor_perflib.c |   27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 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,30 @@ static int acpi_processor_get_platform_l
 		}
 	}
 
+	index = ppc;
+
+	if (pr->performance_platform_limit == index ||
+	    ppc >= pr->performance->state_count)
+		return 0;
+
 	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)))
+	if (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);

[PATCH v1 2/2] cpufreq: intel_pstate: Drop ACPI _PSS states table patching

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

After making acpi_processor_get_platform_limit() use the "no limit"
value for its frequency QoS request when _PPC returns 0, it is not
necessary to replace the frequency corresponding to the first _PSS
return package entry with the maximum turbo frequency of the given
CPU in intel_pstate_init_acpi_perf_limits() any more, so drop the
code doing that along with the comment explaining it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   14 --------------
 1 file changed, 14 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -452,20 +452,6 @@ static void intel_pstate_init_acpi_perf_
 			 (u32) cpu->acpi_perf_data.states[i].control);
 	}
 
-	/*
-	 * The _PSS table doesn't contain whole turbo frequency range.
-	 * This just contains +1 MHZ above the max non turbo frequency,
-	 * with control value corresponding to max turbo ratio. But
-	 * when cpufreq set policy is called, it will call with this
-	 * max frequency, which will cause a reduced performance as
-	 * this driver uses real max turbo frequency as the max
-	 * frequency. So correct this frequency in _PSS table to
-	 * correct max turbo frequency based on the turbo state.
-	 * Also need to convert to MHz as _PSS freq is in MHz.
-	 */
-	if (!global.turbo_disabled)
-		cpu->acpi_perf_data.states[0].core_frequency =
-					policy->cpuinfo.max_freq / 1000;
 	cpu->valid_pss_table = true;
 	pr_debug("_PPC limits will be enforced\n");
 

Re: [PATCH v1 1/2] ACPI: processor: perflib: Use the "no limit" frequency QoS

[srinivas pandruvada]

On Tue, 2022-12-27 at 20:51 +0100, 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
> and avoid updating the QoS request when the _PPC return value has not
> changed.
> 
> 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.
> 
> While at it, move the _PPC return value check against the state count
> earlier to avoid setting performance_platform_limit to an invalid
> value.
> 
> Reported-by: Pratyush Yadav <ptyadav@amazon.de>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/processor_perflib.c |   27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 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,30 @@ static int acpi_processor_get_platform_l
>                 }
>         }
>  
> +       index = ppc;
> +
> +       if (pr->performance_platform_limit == index ||
> +           ppc >= pr->performance->state_count)
> +               return 0;

Do we need to re initialize pr->performance_platform_limit to 0 in
acpi_processor_unregister_performance()?

If PPC was 1 before the offline and after online the above check will
cause it to return as the pr->performance_platform_limit is not
changed. Not sure if the PM QOS state is preserved after offline and
online. This is stored in a per CPU variable, not in dynamically
allocated memory which will be reallocated during online again.


Thanks,
Srinivas

> +
>         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)))
> +       if (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);
> 
> 
> 

Re: [PATCH v1 1/2] ACPI: processor: perflib: Use the "no limit" frequency QoS

[Rafael J. Wysocki]

On Tue, Dec 27, 2022 at 9:55 PM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Tue, 2022-12-27 at 20:51 +0100, 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
> > and avoid updating the QoS request when the _PPC return value has not
> > changed.
> >
> > 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.
> >
> > While at it, move the _PPC return value check against the state count
> > earlier to avoid setting performance_platform_limit to an invalid
> > value.
> >
> > Reported-by: Pratyush Yadav <ptyadav@amazon.de>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/processor_perflib.c |   27 +++++++++++++++++++++------
> >  1 file changed, 21 insertions(+), 6 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,30 @@ static int acpi_processor_get_platform_l
> >                 }
> >         }
> >
> > +       index = ppc;
> > +
> > +       if (pr->performance_platform_limit == index ||
> > +           ppc >= pr->performance->state_count)
> > +               return 0;
>
> Do we need to re initialize pr->performance_platform_limit to 0 in
> acpi_processor_unregister_performance()?
>
> If PPC was 1 before the offline and after online the above check will
> cause it to return as the pr->performance_platform_limit is not
> changed. Not sure if the PM QOS state is preserved after offline and
> online. This is stored in a per CPU variable, not in dynamically
> allocated memory which will be reallocated during online again.

Good point in general, but the QoS request is tied to the cpufreq
policy, so it is not freed on offline.

However, if the policy goes away and is created again for the same CPU
(like when the intel_pstate mode is changed via its 'status' attribute
in sysfs), there may be a stale value in performance_platform_limit,
so it needs to be cleared in acpi_processor_ppc_init() when the QoS
request is first set to "no limit".

I'll update the patch accordingly.  I think I'll also split it in two
to avoid making too many changes in one go.

Re: [PATCH v1 1/2] ACPI: processor: perflib: Use the "no limit" frequency QoS

[Pratyush Yadav]

On Tue, Dec 27 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
> and avoid updating the QoS request when the _PPC return value has not
> changed.
>
> 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.
>
> While at it, move the _PPC return value check against the state count
> earlier to avoid setting performance_platform_limit to an invalid value.
>
> Reported-by: Pratyush Yadav <ptyadav@amazon.de>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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

-- 
Regards,
Pratyush Yadav



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

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

[Rafael J. Wysocki]

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;
+	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);

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

[Pratyush Yadav]

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);
>
>
>

-- 
Regards,
Pratyush Yadav



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

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

[Rafael J. Wysocki]

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.

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

[Pratyush Yadav]

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/

-- 
Regards,
Pratyush Yadav



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

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

[Rafael J. Wysocki]

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

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

[Pratyush Yadav]

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.

-- 
Regards,
Pratyush Yadav



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

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

[srinivas pandruvada]

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

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

[Rafael J. Wysocki]

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.

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

[srinivas pandruvada]

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

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

[Rafael J. Wysocki]

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.

[PATCH v2 2/3] ACPI: processor: perflib: Avoid updating frequency QoS unnecessarily

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Modify acpi_processor_get_platform_limit() to avoid updating its
frequency QoS request when the _PPC return value has not changed
by comparing that value to the previous _PPC return value stored in
the performance_platform_limit field of the struct acpi_processor
corresponding to the given CPU.

While at it, do the _PPC return value check against the state count
earlier to avoid setting performance_platform_limit to an invalid value.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2:
   * New patch.
   * Contains some changes from the previous [1/2] and a change to reset
     performance_platform_limit on QoS request creation.

---
 drivers/acpi/processor_perflib.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/acpi/processor_perflib.c
===================================================================
--- linux-pm.orig/drivers/acpi/processor_perflib.c
+++ linux-pm/drivers/acpi/processor_perflib.c
@@ -76,13 +76,16 @@ static int acpi_processor_get_platform_l
 
 	index = ppc;
 
+	if (pr->performance_platform_limit == index ||
+	    ppc >= pr->performance->state_count)
+		return 0;
+
 	pr_debug("CPU %d: _PPC is %d - frequency %s limited\n", pr->id,
 		 index, index ? "is" : "is not");
 
 	pr->performance_platform_limit = index;
 
-	if (ppc >= pr->performance->state_count ||
-	    unlikely(!freq_qos_request_active(&pr->perflib_req)))
+	if (unlikely(!freq_qos_request_active(&pr->perflib_req)))
 		return 0;
 
 	/*
@@ -178,6 +181,13 @@ void acpi_processor_ppc_init(struct cpuf
 		if (!pr)
 			continue;
 
+		/*
+		 * Reset performance_platform_limit in case there is a stale
+		 * value in it, so as to make it match the "no limit" QoS value
+		 * below.
+		 */
+		pr->performance_platform_limit = 0;
+
 		ret = freq_qos_add_request(&policy->constraints,
 					   &pr->perflib_req,
 					   FREQ_QOS_MAX, INT_MAX);

[PATCH v2 3/3] cpufreq: intel_pstate: Drop ACPI _PSS states table patching

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

After making acpi_processor_get_platform_limit() use the "no limit"
value for its frequency QoS request when _PPC returns 0, it is not
necessary to replace the frequency corresponding to the first _PSS
return package entry with the maximum turbo frequency of the given
CPU in intel_pstate_init_acpi_perf_limits() any more, so drop the
code doing that along with the comment explaining it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2:
   * Same as the previous [2/2], no changes.

---
 drivers/cpufreq/intel_pstate.c |   14 --------------
 1 file changed, 14 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -452,20 +452,6 @@ static void intel_pstate_init_acpi_perf_
 			 (u32) cpu->acpi_perf_data.states[i].control);
 	}
 
-	/*
-	 * The _PSS table doesn't contain whole turbo frequency range.
-	 * This just contains +1 MHZ above the max non turbo frequency,
-	 * with control value corresponding to max turbo ratio. But
-	 * when cpufreq set policy is called, it will call with this
-	 * max frequency, which will cause a reduced performance as
-	 * this driver uses real max turbo frequency as the max
-	 * frequency. So correct this frequency in _PSS table to
-	 * correct max turbo frequency based on the turbo state.
-	 * Also need to convert to MHz as _PSS freq is in MHz.
-	 */
-	if (!global.turbo_disabled)
-		cpu->acpi_perf_data.states[0].core_frequency =
-					policy->cpuinfo.max_freq / 1000;
 	cpu->valid_pss_table = true;
 	pr_debug("_PPC limits will be enforced\n");