Message-ID: <70119ad043502390100e66dbbe658aa069b86e45.camel@suse.cz>
Subject: Re: [PATCH] cpufreq: intel_pstate: Enable HWP IO boost for all
 servers
From:   Giovanni Gherdovich <ggherdovich@suse.cz>
To:     Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        rafael@kernel.org, lenb@kernel.org, viresh.kumar@linaro.org
Cc:     linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Mon, 20 Mar 2023 18:03:26 +0100
In-Reply-To: <20230303041411.3161780-1-srinivas.pandruvada@linux.intel.com>
References: <20230303041411.3161780-1-srinivas.pandruvada@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.34.4 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 2023-03-02 at 20:14 -0800, Srinivas Pandruvada wrote:
> The HWP IO boost results in slight improvements for IO performance on
> both Ice Lake and Sapphire Rapid servers.
> 
> Currently there is a CPU model check for Skylake desktop and server along
> with the ACPI PM profile for performance and enterprise servers to enable
> IO boost.
> 
> Remove the CPU model check, so that all current server models enable HWP
> IO boost by default.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/cpufreq/intel_pstate.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index cb4beec27555..8edbc0856892 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -2384,12 +2384,6 @@ static const struct x86_cpu_id intel_pstate_cpu_ee_disable_ids[] = {
>  	{}
>  };
>  
> -static const struct x86_cpu_id intel_pstate_hwp_boost_ids[] = {
> -	X86_MATCH(SKYLAKE_X,		core_funcs),
> -	X86_MATCH(SKYLAKE,		core_funcs),
> -	{}
> -};
> -
>  static int intel_pstate_init_cpu(unsigned int cpunum)
>  {
>  	struct cpudata *cpu;
> @@ -2408,12 +2402,9 @@ static int intel_pstate_init_cpu(unsigned int cpunum)
>  		cpu->epp_default = -EINVAL;
>  
>  		if (hwp_active) {
> -			const struct x86_cpu_id *id;
> -
>  			intel_pstate_hwp_enable(cpu);
>  
> -			id = x86_match_cpu(intel_pstate_hwp_boost_ids);
> -			if (id && intel_pstate_acpi_pm_profile_server())
> +			if (intel_pstate_acpi_pm_profile_server())
>  				hwp_boost = true;
>  		}
>  	} else if (hwp_active) {

Hello Srinivas,

Good catch. We've had HWP IO Boost in the kernel for a while now but we
weren't enabling on most of the modern CPUs... This fixes it.

One thing though: I've the impression that HWP IO Boost depends on having
per-core p-states -- otherwise you'll be boosting up and down the entire machine
instead of just the one core waking up from IO.
Enabling the feature on all machines with the ACPI PM server profile would
force it also where per-core p-states aren't available.

Would you agree with this assessment?
Do I correctly understand the reason why per-core p-states are needed here?

I remember you mentioned the the dependency on per-core p-states in the cover
letter from the HWP IO Boost submission in 2018
https://lore.kernel.org/lkml/20180605214242.62156-1-srinivas.pandruvada@linux.intel.com/

I think there's a tradeoff here. Up until this patch, we forgot to enable the
feature on four generations of Intel CPUs. That's a lot of lost performance;
thanks to this patch it won't happen ever again, because nobody will have to
update the model list at every new CPU release.

On the other side, there may be some penalty for machines that:
- have HWP
- don't have per-core p-states
I don't know how large that interesection is, or how big the penalty.


Giovanni