Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <24821308109ba20d845e11caf32bede92fec5d8e.camel@linux.intel.com>
Subject: Re: [PATCH v1 1/2] ACPI: processor: perflib: Use the "no limit"
 frequency QoS
From:   srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
To:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux PM <linux-pm@vger.kernel.org>
Cc:     Pratyush Yadav <ptyadav@amazon.de>,
        LKML <linux-kernel@vger.kernel.org>,
        Linux ACPI <linux-acpi@vger.kernel.org>
Date:   Tue, 27 Dec 2022 12:40:51 -0800
In-Reply-To: <12138067.O9o76ZdvQC@kreacher>
References: <12138067.O9o76ZdvQC@kreacher>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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