From: Sean Christopherson <seanjc@google•com>
To: Zihuan Zhang <zhangzihuan@kylinos•cn>
Cc: "Rafael J . wysocki" <rafael@kernel•org>,
Viresh Kumar <viresh.kumar@linaro•org>,
Catalin Marinas <catalin.marinas@arm•com>,
Will Deacon <will@kernel•org>,
Paolo Bonzini <pbonzini@redhat•com>,
Thomas Gleixner <tglx@linutronix•de>,
Ingo Molnar <mingo@redhat•com>, Borislav Petkov <bp@alien8•de>,
Dave Hansen <dave.hansen@linux•intel.com>,
Markus Mayer <mmayer@broadcom•com>,
Florian Fainelli <florian.fainelli@broadcom•com>,
Srinivas Pandruvada <srinivas.pandruvada@linux•intel.com>,
Madhavan Srinivasan <maddy@linux•ibm.com>,
Michael Ellerman <mpe@ellerman•id.au>,
Krzysztof Kozlowski <krzk@kernel•org>,
Alim Akhtar <alim.akhtar@samsung•com>,
Thierry Reding <thierry.reding@gmail•com>,
Jonathan Hunter <jonathanh@nvidia•com>,
MyungJoo Ham <myungjoo.ham@samsung•com>,
Kyungmin Park <kyungmin.park@samsung•com>,
Chanwoo Choi <cw00.choi@samsung•com>,
Jani Nikula <jani.nikula@linux•intel.com>,
Joonas Lahtinen <joonas.lahtinen@linux•intel.com>,
Rodrigo Vivi <rodrigo.vivi@intel•com>,
Tvrtko Ursulin <tursulin@ursulin•net>,
David Airlie <airlied@gmail•com>,
Simona Vetter <simona@ffwll•ch>,
Daniel Lezcano <daniel.lezcano@kernel•org>,
Sascha Hauer <s.hauer@pengutronix•de>,
Shawn Guo <shawnguo@kernel•org>,
Eduardo Valentin <edubezval@gmail•com>,
Keerthy <j-keerthy@ti•com>,
Matthias Brugger <matthias.bgg@gmail•com>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora•com>,
zhenglifeng <zhenglifeng1@huawei•com>,
"H . Peter Anvin" <hpa@zytor•com>,
Zhang Rui <rui.zhang@intel•com>, Len Brown <lenb@kernel•org>,
Nicholas Piggin <npiggin@gmail•com>,
Christophe Leroy <christophe.leroy@csgroup•eu>,
Lukasz Luba <lukasz.luba@arm•com>,
Pengutronix Kernel Team <kernel@pengutronix•de>,
Beata Michalska <beata.michalska@arm•com>,
Fabio Estevam <festevam@gmail•com>,
Pavel Machek <pavel@kernel•org>, Sumit Gupta <sumitg@nvidia•com>,
Prasanna Kumar T S M <ptsm@linux•microsoft.com>,
Sudeep Holla <sudeep.holla@arm•com>,
Yicong Yang <yangyicong@hisilicon•com>,
linux-pm@vger•kernel.org, x86@kernel•org, kvm@vger•kernel.org,
linux-acpi@vger•kernel.org, linuxppc-dev@lists•ozlabs.org,
linux-samsung-soc@vger•kernel.org,
linux-arm-kernel@lists•infradead.org,
linux-tegra@vger•kernel.org, intel-gfx@lists•freedesktop.org,
dri-devel@lists•freedesktop.org, imx@lists•linux.dev,
linux-omap@vger•kernel.org, linux-mediatek@lists•infradead.org,
linux-kernel@vger•kernel.org
Subject: Re: [PATCH v2 02/18] KVM: x86: Use __free(put_cpufreq_policy) for policy reference
Date: Thu, 28 Aug 2025 10:15:17 -0700 [thread overview]
Message-ID: <aLCOpfNkcQN9P-Wa@google.com> (raw)
In-Reply-To: <874d821e-8ea3-40ac-921b-c19bb380a456@kylinos.cn>
On Thu, Aug 28, 2025, Zihuan Zhang wrote:
> > Hmm, this is technically buggy. __free() won't invoke put_cpufreq_policy() until
> > policy goes out of scope, and so using __free() means the code is effectively:
> >
> > if (IS_ENABLED(CONFIG_CPU_FREQ)) {
> > struct cpufreq_policy *policy;
> > int cpu;
> >
> > cpu = get_cpu();
> > policy = cpufreq_cpu_get(cpu);
> > if (policy && policy->cpuinfo.max_freq)
> > max_tsc_khz = policy->cpuinfo.max_freq;
> > put_cpu();
> >
> > if (policy)
> > cpufreq_cpu_put(policy);
> > }
...
> Yes, this will indeed change the execution order.
> Can you accept that?
No, because it's buggy.
> Personally, I don’t think it’s ideal either.
>
> if (IS_ENABLED(CONFIG_CPU_FREQ)) {
> int cpu;
> cpu = get_cpu();
> {
> struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
> if (policy && policy->cpuinfo.max_freq)
> max_tsc_khz = policy->cpuinfo.max_freq;
> }
> put_cpu();
>
> }
>
> Other places may also have the same issue,
>
> maybe we should consider introducing a macro to handle this properly,
> so that initialization and cleanup are well defined without changing
> the existing order unexpected.
>
> like this:
>
> #define WITH_CPUFREQ_POLICY(cpu) {\
>
> for(struct cpufreq_policy *policy __free(put_cpufreq_policy) = \
> cpufreq_cpu_get(cpu); \
> policy;)
>
> Then Use it:
>
> if (IS_ENABLED(CONFIG_CPU_FREQ)) {
> int cpu;
> cpu = get_cpu();
> WITH_CPUFREQ_POLICY(cpu){
> if (policy->cpuinfo.max_freq)
> max_tsc_khz = policy->cpuinfo.max_freq;
> }
> put_cpu();
This all feels very forced, in the sense that we have a shiny new tool and are
trying to use it everywhere without thinking critically about whether or not
doing so is actually an improvement.
At a glance, this is literally the only instance in the entire kernel where the
CPU to use is grabbed immediately before the policy.
$ git grep -B 20 cpufreq_cpu_get | grep -e get_cpu -e smp_processor_id
arch/x86/kvm/x86.c- cpu = get_cpu();
drivers/cpufreq/cppc_cpufreq.c-static int cppc_get_cpu_power(struct device *cpu_dev,
drivers/cpufreq/cppc_cpufreq.c-static int cppc_get_cpu_cost(struct device *cpu_dev, unsigned long KHz,
drivers/cpufreq/mediatek-cpufreq-hw.c-mtk_cpufreq_get_cpu_power(struct device *cpu_dev, unsigned long *uW,
Probably because KVM's usage is rather bizarre and honestly kind of dumb. But
KVM has had this behavior for 15+ years, so as weird as it is, I'm not inclined
to change it without a really, really strong reason to do so, e.g. to iterate
over all CPUs or something.
So given that this is the only intance of the problem patter, I think it makes
sense to leave KVM as-is, and not spend a bunch of time trying to figure out how
to make KVM's usage play nice with __free().
next prev parent reply other threads:[~2025-08-28 17:15 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-08-27 2:31 [PATCH v2 00/18] cpufreq: use __free() for all cpufreq_cpu_get() references Zihuan Zhang
2025-08-27 2:31 ` [PATCH v2 01/18] arm64: topology: Use __free(put_cpufreq_policy) for policy reference Zihuan Zhang
2025-08-27 8:30 ` Ben Horgan
2025-08-27 8:55 ` Zihuan Zhang
2025-08-27 9:12 ` Ben Horgan
2025-08-27 9:21 ` Zihuan Zhang
2025-08-27 9:17 ` Sudeep Holla
[not found] ` <1756341899099493.57.seg@mailgw.kylinos.cn>
2025-08-28 2:32 ` Zihuan Zhang
2025-08-27 2:31 ` [PATCH v2 02/18] KVM: x86: " Zihuan Zhang
2025-08-27 14:13 ` Sean Christopherson
2025-08-28 1:17 ` Zihuan Zhang
2025-08-28 17:15 ` Sean Christopherson [this message]
2025-08-27 2:31 ` [PATCH v2 03/18] ACPI: processor: thermal: " Zihuan Zhang
2025-08-28 9:40 ` Rafael J. Wysocki
2025-08-29 1:09 ` Zihuan Zhang
2025-08-27 2:31 ` [PATCH v2 04/18] cpufreq: brcmstb-avs-cpufreq: " Zihuan Zhang
2025-08-29 5:59 ` Viresh Kumar
2025-08-29 6:16 ` Zihuan Zhang
2025-08-29 6:26 ` Viresh Kumar
2025-08-29 6:32 ` Zihuan Zhang
2025-08-27 2:31 ` [PATCH v2 05/18] cpufreq: CPPC: " Zihuan Zhang
2025-08-29 6:04 ` Viresh Kumar
2025-08-27 2:31 ` [PATCH v2 06/18] cpufreq: intel_pstate: " Zihuan Zhang
2025-08-27 2:31 ` [PATCH v2 07/18] cpufreq: longhaul: " Zihuan Zhang
2025-08-27 2:31 ` [PATCH v2 08/18] cpufreq: mediatek: " Zihuan Zhang
2025-08-29 6:18 ` Viresh Kumar
2025-08-27 2:31 ` [PATCH v2 09/18] cpufreq: powernv: " Zihuan Zhang
2025-08-27 2:31 ` [PATCH v2 10/18] cpufreq: s5pv210: " Zihuan Zhang
2025-08-29 6:13 ` Viresh Kumar
2025-08-27 2:31 ` [PATCH v2 11/18] cpufreq: tegra186: " Zihuan Zhang
2025-08-29 6:29 ` Viresh Kumar
2025-08-27 2:31 ` [PATCH v2 12/18] PM / devfreq: " Zihuan Zhang
2025-08-27 2:31 ` [PATCH v2 13/18] drm/i915: " Zihuan Zhang
2025-08-27 2:31 ` [PATCH v2 14/18] cpufreq: powerpc: macintosh: " Zihuan Zhang
2025-08-27 2:31 ` [PATCH v2 15/18] powercap: dtpm_cpu: " Zihuan Zhang
2025-08-27 2:32 ` [PATCH v2 16/18] thermal: imx: " Zihuan Zhang
2025-08-27 2:32 ` [PATCH v2 17/18] thermal/drivers/ti-soc-thermal: " Zihuan Zhang
2025-08-27 2:32 ` [PATCH v2 18/18] PM: EM: " Zihuan Zhang
2025-08-27 3:50 ` [PATCH v2 15/18] powercap: dtpm_cpu: " Zihuan Zhang
2025-08-27 5:21 ` Zihuan Zhang
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=aLCOpfNkcQN9P-Wa@google.com \
--to=seanjc@google$(echo .)com \
--cc=airlied@gmail$(echo .)com \
--cc=alim.akhtar@samsung$(echo .)com \
--cc=angelogioacchino.delregno@collabora$(echo .)com \
--cc=beata.michalska@arm$(echo .)com \
--cc=bp@alien8$(echo .)de \
--cc=catalin.marinas@arm$(echo .)com \
--cc=christophe.leroy@csgroup$(echo .)eu \
--cc=cw00.choi@samsung$(echo .)com \
--cc=daniel.lezcano@kernel$(echo .)org \
--cc=dave.hansen@linux$(echo .)intel.com \
--cc=dri-devel@lists$(echo .)freedesktop.org \
--cc=edubezval@gmail$(echo .)com \
--cc=festevam@gmail$(echo .)com \
--cc=florian.fainelli@broadcom$(echo .)com \
--cc=hpa@zytor$(echo .)com \
--cc=imx@lists$(echo .)linux.dev \
--cc=intel-gfx@lists$(echo .)freedesktop.org \
--cc=j-keerthy@ti$(echo .)com \
--cc=jani.nikula@linux$(echo .)intel.com \
--cc=jonathanh@nvidia$(echo .)com \
--cc=joonas.lahtinen@linux$(echo .)intel.com \
--cc=kernel@pengutronix$(echo .)de \
--cc=krzk@kernel$(echo .)org \
--cc=kvm@vger$(echo .)kernel.org \
--cc=kyungmin.park@samsung$(echo .)com \
--cc=lenb@kernel$(echo .)org \
--cc=linux-acpi@vger$(echo .)kernel.org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-mediatek@lists$(echo .)infradead.org \
--cc=linux-omap@vger$(echo .)kernel.org \
--cc=linux-pm@vger$(echo .)kernel.org \
--cc=linux-samsung-soc@vger$(echo .)kernel.org \
--cc=linux-tegra@vger$(echo .)kernel.org \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=lukasz.luba@arm$(echo .)com \
--cc=maddy@linux$(echo .)ibm.com \
--cc=matthias.bgg@gmail$(echo .)com \
--cc=mingo@redhat$(echo .)com \
--cc=mmayer@broadcom$(echo .)com \
--cc=mpe@ellerman$(echo .)id.au \
--cc=myungjoo.ham@samsung$(echo .)com \
--cc=npiggin@gmail$(echo .)com \
--cc=pavel@kernel$(echo .)org \
--cc=pbonzini@redhat$(echo .)com \
--cc=ptsm@linux$(echo .)microsoft.com \
--cc=rafael@kernel$(echo .)org \
--cc=rodrigo.vivi@intel$(echo .)com \
--cc=rui.zhang@intel$(echo .)com \
--cc=s.hauer@pengutronix$(echo .)de \
--cc=shawnguo@kernel$(echo .)org \
--cc=simona@ffwll$(echo .)ch \
--cc=srinivas.pandruvada@linux$(echo .)intel.com \
--cc=sudeep.holla@arm$(echo .)com \
--cc=sumitg@nvidia$(echo .)com \
--cc=tglx@linutronix$(echo .)de \
--cc=thierry.reding@gmail$(echo .)com \
--cc=tursulin@ursulin$(echo .)net \
--cc=viresh.kumar@linaro$(echo .)org \
--cc=will@kernel$(echo .)org \
--cc=x86@kernel$(echo .)org \
--cc=yangyicong@hisilicon$(echo .)com \
--cc=zhangzihuan@kylinos$(echo .)cn \
--cc=zhenglifeng1@huawei$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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