| Message ID | 20230303041411.3161780-1-srinivas.pandruvada@linux.intel.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | cpufreq: intel_pstate: Enable HWP IO boost for all servers | expand |
On Fri, Mar 3, 2023 at 5:14 AM Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> 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) { > -- Applied as 6.4 material, thanks!
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
On Mon, 2023-03-20 at 18:03 +0100, Giovanni Gherdovich wrote: > On Thu, 2023-03-02 at 20:14 -0800, Srinivas Pandruvada wrote: > ... Hi Giovanni, > 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. This feature only exists on HWP systems. There are no HWP systems without per core P-states. Here we are enabling for performance and enterprise servers only. > > Would you agree with this assessment? > Do I correctly understand the reason why per-core p-states are needed > here? This problem with IO will be more pronounced in per-core P-states systems as it will not be influenced by other cores. But even if non per-core systems if other cores are idle or lightly loaded, the same problem can occur. But I don't have data on such systems. > > 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. > CPU model updates always gets missed and also misses testing opportunity. > 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. > Thanks, Srinivas > > Giovanni >
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) {
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(-)