Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <1588148110.21179.63.camel@suse.cz>
Subject: Re: [PATCH] x86: move turbo_disabled() out of
 intel_set_max_freq_ratio
From:   Giovanni Gherdovich <ggherdovich@suse.cz>
To:     Li RongQing <lirongqing@baidu.com>, tglx@linutronix.de,
        mingo@redhat.com, bp@alien8.de, hpa@zytor.com, x86@kernel.org,
        peterz@infradead.org, linux-kernel@vger.kernel.org,
        rafael.j.wysocki@intel.com
Date:   Wed, 29 Apr 2020 10:15:10 +0200
In-Reply-To: <1588125007-8799-1-git-send-email-lirongqing@baidu.com>
References: <1588125007-8799-1-git-send-email-lirongqing@baidu.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 2020-04-29 at 09:50 +0800, Li RongQing wrote:
> move the turbo_disabled before intel_set_max_freq_ratio,
> when turbo is disabled, the max frequency ratio is a const
> value, it is unnecessary to read MSR_TURBO_RATIO* msr to
> compute
> 
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  arch/x86/kernel/smpboot.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index fe3ab9632f3b..8979c459df2f 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1987,7 +1987,7 @@ static bool intel_set_max_freq_ratio(void)
>  out:
>  	arch_turbo_freq_ratio = div_u64(turbo_freq * SCHED_CAPACITY_SCALE,
>  					base_freq);
> -	arch_set_max_freq_ratio(turbo_disabled());
> +	arch_set_max_freq_ratio(false);
>  	return true;
>  }
>  
> @@ -2009,6 +2009,9 @@ static void init_freq_invariance(void)
>  	if (smp_processor_id() != 0 || !boot_cpu_has(X86_FEATURE_APERFMPERF))
>  		return;
>  
> +	if (turbo_disabled())
> +		return;
> +
>  	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
>  		ret = intel_set_max_freq_ratio();
>

Hello,

the problem is that turbo can be enabled/disabled by the firmware at runtime,
after the machine has booted.

This happens for example with the Dell XPS 13, where turbo gets disabled by the
firmware if the machine is disconnected from AC power and runs on battery. The
laptop could boot on battery (turbo disabled), then after some time the user
connects the AC power supply, turbo gets enabled, and with your patch we
wouldn't know what is the turbo_freq/base_freq ratio to do frequency
invariance (we skipped reading MSR_TURBO_RATIO_LIMIT at boot because turbo was
disabled at that timed).

This behavior was requested by reviewers in this thread:
https://lore.kernel.org/lkml/1906426.HDqaVa71mF@kreacher/
and implemented with 918229cdd5ab ("x86/intel_pstate: Handle runtime turbo
disablement/enablement in frequency invariance").


Thanks,
Giovanni Gherdovich