From: Yang Jie <yang.jie@linux.intel.com>
To: Thomas Gleixner <tglx@linutronix.de>,
x86@kernel.org, linux-kernel@vger.kernel.org
Cc: Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
Dave Hansen <dave.hansen@linux.intel.com>,
"H . Peter Anvin" <hpa@zytor.com>,
Peter Zijlstra <peterz@infradead.org>,
Yair Podemsky <ypodemsk@redhat.com>,
linux-pm@vger.kernel.org,
"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
Viresh Kumar <viresh.kumar@linaro.org>,
Doug Smythies <dsmythies@telus.net>
Subject: Re: [PATCH] x86/aperfmperf: Fix the fallback condition in arch_freq_get_on_cpu()
Date: Tue, 27 Jun 2023 17:57:36 -0700 [thread overview]
Message-ID: <dd301065-a9ec-0918-daa4-596245baae00@linux.intel.com> (raw)
In-Reply-To: <20230626193601.9169-1-yang.jie@linux.intel.com>
Sorry for top posting, I should have sent it to linux-pm and maintainers.
Doug Smythies had a good discussion with me about the related history
and issues in the bugzilla here:
https://bugzilla.kernel.org/show_bug.cgi?id=217597.
Basically, there are 2 issues here per my observation:
1. the cpu_khz shared for all CPU cores? In Intel's recent Hybrid CPUs,
what does this cpu_khz read from cpuid really mean? I am seeing
cpu_khz=3.6GHz for E-cores with Max frequecy 3GHz. We should fix that, no?
2. We don't want to wake up cores just because of the sysfs queries, so
we introduced fallback mechanism here, what is our clear design about that?
So, before discussing those issues, we should get alignment on these first:
1. What is fallback and When should we fallback. From the comment, looks
we wanted to use cpu_khz for Cores haven't executed any task during the
last 20ms, this sounds reasonable, and I patch here is to address this
issue.
2. What frequencies should we show in fallback case. This could be
controversial, 0? min_freq? base_freq? or last calculated one? Doug has
suggestion here but this is not touched in my patch here.
Thanks,
~Keyon
On 6/26/23 12:36, Keyon Jie wrote:
>>From the commit f3eca381bd49 on, the fallback condition about the 'the
> last update was too long' have been comparing ticks and milliseconds by
> mistake, which leads to that the condition is met and the fallback
> method is used frequently.
>
> The change to compare ticks here corrects that and fixes related issues
> have been seen on x86 platforms since 5.18 kernel.
>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217597
> Fixes: f3eca381bd49 ("x86/aperfmperf: Replace arch_freq_get_on_cpu()")
> CC: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
> ---
> arch/x86/kernel/cpu/aperfmperf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/aperfmperf.c b/arch/x86/kernel/cpu/aperfmperf.c
> index fdbb5f07448f..24e24e137226 100644
> --- a/arch/x86/kernel/cpu/aperfmperf.c
> +++ b/arch/x86/kernel/cpu/aperfmperf.c
> @@ -432,7 +432,7 @@ unsigned int arch_freq_get_on_cpu(int cpu)
> * Bail on invalid count and when the last update was too long ago,
> * which covers idle and NOHZ full CPUs.
> */
> - if (!mcnt || (jiffies - last) > MAX_SAMPLE_AGE)
> + if (!mcnt || (jiffies - last) > MAX_SAMPLE_AGE * cpu_khz)
> goto fallback;
>
> return div64_u64((cpu_khz * acnt), mcnt);
next prev parent reply other threads:[~2023-06-28 0:58 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-26 19:36 [PATCH] x86/aperfmperf: Fix the fallback condition in arch_freq_get_on_cpu() Keyon Jie
2023-06-28 0:57 ` Yang Jie [this message]
2023-06-30 12:35 ` Thomas Gleixner
2023-07-05 17:23 ` Keyon Jie
2023-06-30 15:13 ` Dave Hansen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dd301065-a9ec-0918-daa4-596245baae00@linux.intel.com \
--to=yang.jie@linux.intel.com \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=dsmythies@telus.net \
--cc=hpa@zytor.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=rafael.j.wysocki@intel.com \
--cc=tglx@linutronix.de \
--cc=viresh.kumar@linaro.org \
--cc=x86@kernel.org \
--cc=ypodemsk@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).