Received-SPF: pass (google.com: domain of linux-kernel+bounces-15692-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <a57b9e19-3466-bb73-1c88-c19417ac6822@bytedance.com>
Date: Wed, 3 Jan 2024 15:18:25 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [External] [PATCH v5] x86: intel_epb: Add earlyparam option to
 keep bias at performance
To: Jack Allister <jalliste@amazon.com>
Cc: "Rafael J . Wysocki" <rafael@kernel.org>,
 Paul Durrant <pdurrant@amazon.com>, Jue Wang <juew@amazon.com>,
 Jonathan Corbet <corbet@lwn.net>, Thomas Gleixner <tglx@linutronix.de>,
 Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
 Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
 "H. Peter Anvin" <hpa@zytor.com>, "Paul E. McKenney" <paulmck@kernel.org>,
 Randy Dunlap <rdunlap@infradead.org>, Tejun Heo <tj@kernel.org>,
 Peter Zijlstra <peterz@infradead.org>, Yan-Jie Wang <yanjiewtw@gmail.com>,
 Hans de Goede <hdegoede@redhat.com>, linux-doc@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <83431857-7182-471a-9ff1-9dac37e5a02f@intel.com>
 <20240103144607.46369-1-jalliste@amazon.com>
Content-Language: en-US
From: Usama Arif <usama.arif@bytedance.com>
In-Reply-To: <20240103144607.46369-1-jalliste@amazon.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 03/01/2024 14:46, Jack Allister wrote:
> Buggy BIOSes may not set a sane boot-time Energy Performance Bias (EPB).
> A result of this may be overheating or excess power usage. The kernel
> overrides any boot-time EPB "performance" bias to "normal" to avoid this.
> 
> When used in data centers it is preferable keep the EPB at "performance"
> when performing a live-update of the host kernel via a kexec to the new
> kernel. This is due to boot-time being critical when performing the kexec
> as running guest VMs will perceieve this as latency or downtime.
> 
> On Intel Xeon Ice Lake platforms it has been observed that a combination of
> EPB being set to "normal" alongside HWP (Intel Hardware P-states) being
> enabled/configured during or close to the kexec causes an increases the
> live-update/kexec downtime by 7 times compared to when the EPB is set to
> "performance".
> 
> Introduce a command-line parameter, "intel_epb=preserve", to skip the
> "performance" -> "normal" override/workaround. This maintains prior
> functionality when no parameter is set, but adds in the ability to stay at
> performance for a speedy kexec if a user wishes.
> 
> Signed-off-by: Jack Allister <jalliste@amazon.com>
> Acked-by: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Paul Durrant <pdurrant@amazon.com>
> Cc: Jue Wang <juew@amazon.com>
> Cc: Usama Arif <usama.arif@bytedance.com>
> ---
>   .../admin-guide/kernel-parameters.txt         | 12 +++++++++++
>   arch/x86/kernel/cpu/intel_epb.c               | 21 +++++++++++++++++--
>   2 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 65731b060e3f..5602ee213115 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2148,6 +2148,18 @@
>   			0	disables intel_idle and fall back on acpi_idle.
>   			1 to 9	specify maximum depth of C-state.
>   
> +	intel_epb=	[X86]
> +			auto
> +			  Same as not passing a parameter to intel_epb. This will
> +			  ensure that the intel_epb module will restore the energy
> +			  performance bias to "normal" at boot-time. This workaround
> +			  is for buggy BIOSes which may not set this value and cause
> +			  either overheating or excess power usage.
Hi,

Thanks for the patch. Is auto needed over here? It was pointed in an 
earlier review that it could be an option, but it doesn't seem to serve 
a purpose. Its also not how the code works, i.e. intel_epb=abc would be 
the same as intel_epb=auto. Just could add a print if intel_epb=preserve 
is not encountered then an unexpected value has been passed in.

> +			preserve
> +			  At kernel boot-time if the EPB value is read as "performance"
> +			  keep it at this value. This prevents the "performance" -> "normal"
> +			  transition which is a workaround mentioned above.
> +
>   	intel_pstate=	[X86]
>   			disable
>   			  Do not enable intel_pstate as the default
> diff --git a/arch/x86/kernel/cpu/intel_epb.c b/arch/x86/kernel/cpu/intel_epb.c
> index e4c3ba91321c..419e699a43e6 100644
> --- a/arch/x86/kernel/cpu/intel_epb.c
> +++ b/arch/x86/kernel/cpu/intel_epb.c
> @@ -50,7 +50,8 @@
>    * the OS will do that anyway.  That sometimes is problematic, as it may cause
>    * the system battery to drain too fast, for example, so it is better to adjust
>    * it on CPU bring-up and if the initial EPB value for a given CPU is 0, the
> - * kernel changes it to 6 ('normal').
> + * kernel changes it to 6 ('normal'). However, if it is desirable to retain the
> + * original initial EPB value, intel_epb=preserve can be set to enforce it.
>    */
>   
>   static DEFINE_PER_CPU(u8, saved_epb);
> @@ -75,6 +76,8 @@ static u8 energ_perf_values[] = {
>   	[EPB_INDEX_POWERSAVE] = ENERGY_PERF_BIAS_POWERSAVE,
>   };
>   
> +static bool intel_epb_no_override __read_mostly;
> +
>   static int intel_epb_save(void)
>   {
>   	u64 epb;
> @@ -106,7 +109,7 @@ static void intel_epb_restore(void)
>   		 * ('normal').
>   		 */
>   		val = epb & EPB_MASK;
> -		if (val == ENERGY_PERF_BIAS_PERFORMANCE) {
> +		if (!intel_epb_no_override && val == ENERGY_PERF_BIAS_PERFORMANCE) {
>   			val = energ_perf_values[EPB_INDEX_NORMAL];
>   			pr_warn_once("ENERGY_PERF_BIAS: Set to 'normal', was 'performance'\n");
>   		}
> @@ -213,6 +216,20 @@ static const struct x86_cpu_id intel_epb_normal[] = {
>   	{}
>   };
>   
> +static __init int parse_intel_epb(char *str)
> +{
> +	if (!str)
> +		return 0;
> +
> +	/* "intel_epb=preserve" prevents PERFORMANCE->NORMAL on restore. */
> +	if (!strcmp(str, "preserve"))
> +		intel_epb_no_override = true;
> +
Maybe add an print in else here to say that unexpected value has been 
encountered for intel_epb if preserve is not seen.

Thanks,
Usama
> +	return 0;
> +}
> +
> +early_param("intel_epb", parse_intel_epb);
> +
>   static __init int intel_epb_init(void)
>   {
>   	const struct x86_cpu_id *id = x86_match_cpu(intel_epb_normal);