| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAAhV-H6P3Ld_-3kf=fTyptpNGnHE8svzQWveKfZTZm8oH795VQ@mail.gmail.com>
Date: Wed, 13 Aug 2025 11:13:39 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: lixianglai <lixianglai@...ngson.cn>
Cc: WANG Xuerui <kernel@...0n.name>, Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>, Bibo Mao <maobibo@...ngson.cn>,
Song Gao <gaosong@...ngson.cn>, Tianrui Zhao <zhaotianrui@...ngson.cn>, loongarch@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] LoongArch: time: Fix the issue of high cpu usage of vcpu
threads in virtual machines
On Wed, Aug 13, 2025 at 10:27 AM lixianglai <lixianglai@...ngson.cn> wrote:
>
> Hi Huacai Chen:
> >> On Tue, Aug 12, 2025 at 8:50 PM lixianglai <lixianglai@...ngson.cn>
> >> wrote:
> >>>
> >>>
> >>> Hi Huacai Chen:
> >>>> Hi, Xianglai,
> >>>>
> >>>> There is something that can be improved.
> >>>>
> >>>> On Tue, Aug 12, 2025 at 5:24 PM Xianglai Li
> >>>> <lixianglai@...ngson.cn> wrote:
> >>>>> When the cpu is offline, the timer under loongarch is not
> >>>>> correctly closed,
> >>>>> resulting in an excessively high cpu usage rate of the offline
> >>>>> vcpu thread
> >>>>> in the virtual machine.
> >>>>>
> >>>>> To correctly close the timer, we have made the following
> >>>>> modifications:
> >>>>>
> >>>>> Register the cpu hotplug timer start event for loongarch.This
> >>>>> event will
> >>>>> be called to close the timer when the cpu is offline.
> >>>>>
> >>>>> Clear the timer interrupt when the timer is turned off
> >>>>>
> >>>>> Signed-off-by: Xianglai Li <lixianglai@...ngson.cn>
> >>>>> ---
> >>>>> arch/loongarch/kernel/time.c | 20 ++++++++++++++++++++
> >>>>> include/linux/cpuhotplug.h | 1 +
> >>>>> 2 files changed, 21 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/loongarch/kernel/time.c
> >>>>> b/arch/loongarch/kernel/time.c
> >>>>> index 367906b10f81..4daa11512eba 100644
> >>>>> --- a/arch/loongarch/kernel/time.c
> >>>>> +++ b/arch/loongarch/kernel/time.c
> >>>>> @@ -12,6 +12,7 @@
> >>>>> #include <linux/kernel.h>
> >>>>> #include <linux/sched_clock.h>
> >>>>> #include <linux/spinlock.h>
> >>>>> +#include <linux/cpu.h>
> >>>>>
> >>>>> #include <asm/cpu-features.h>
> >>>>> #include <asm/loongarch.h>
> >>>>> @@ -86,6 +87,9 @@ static int constant_set_state_shutdown(struct
> >>>>> clock_event_device *evt)
> >>>>> timer_config &= ~CSR_TCFG_EN;
> >>>>> csr_write64(timer_config, LOONGARCH_CSR_TCFG);
> >>>>>
> >>>>> + /* Clear Timer Interrupt */
> >>>>> + write_csr_tintclear(CSR_TINTCLR_TI);
> >>>>> +
> >>>>> raw_spin_unlock(&state_lock);
> >>>>>
> >>>>> return 0;
> >>>>> @@ -208,8 +212,17 @@ int __init constant_clocksource_init(void)
> >>>>> return res;
> >>>>> }
> >>>>>
> >>>>> +static int arch_timer_dying_cpu(unsigned int cpu)
> >>>> We can use arch_timer_dying() for short. And then add an
> >>>> arch_timer_starting() like this:
> >>>>
> >>>> static int arch_timer_starting(unsigned int cpu)
> >>>> {
> >>>> set_csr_ecfg(ECFGF_TIMER);
> >>>>
> >>>> return 0;
> >>>> }
> >>>>
> >>>> Though ECFGF_TIMER may be enabled in other places, for syntax we
> >>>> need it here.
> >>>>
> >>>>> +{
> >>>>> + constant_set_state_shutdown(NULL);
> >>>>> +
> >>>>> + return 0;
> >>>>> +}
> >>>>> +
> >>>>> void __init time_init(void)
> >>>>> {
> >>>>> + int err;
> >>>>> +
> >>>>> if (!cpu_has_cpucfg)
> >>>>> const_clock_freq = cpu_clock_freq;
> >>>>> else
> >>>>> @@ -220,4 +233,11 @@ void __init time_init(void)
> >>>>> constant_clockevent_init();
> >>>>> constant_clocksource_init();
> >>>>> pv_time_init();
> >>>>> +
> >>>>> + err =
> >>>>> cpuhp_setup_state_nocalls(CPUHP_AP_LOONGARCH_ARCH_TIMER_STARTING,
> >>>>> + "loongarch/timer:starting",
> >>>>> + NULL, arch_timer_dying_cpu);
> >>>> Then we need use cpuhp_setup_state() here, because we have a startup
> >>>> function now.
> >>>>
> >>>> And "loongarch/timer:starting" should be
> >>>> "clockevents/loongarch/timer:starting" like others.
> >>>>
> >>>> And the whole should be moved to the last of
> >>>> constant_clockevent_init() because it is clockevent specific.
> >>> like this?
> >>>
> >>> @@ -164,6 +182,10 @@ int constant_clockevent_init(void)
> >>>
> >>> timer_irq_installed = 1;
> >>>
> >>> + cpuhp_setup_state(CPUHP_AP_LOONGARCH_ARCH_TIMER_STARTING,
> >>> + "clockevents/loongarch/timer:starting",
> >>> + arch_timer_starting, arch_timer_dying_cpu);
> >>> +
> >>> sync_counter();
> >>>
> >>>
> >>> I was wondering whether it should be placed before or after the
> >>> "timer_irq_installed" judgment
> >> Should be after "timer_irq_installed" because we only need to run once.
> >>
> >> The best place is after pr_info("Constant clock event device
> >> register\n");
> >
> > Got it!
> >> And there is another question:
> >> Should we move "write_csr_tintclear(CSR_TINTCLR_TI)" from
> >> constant_set_state_shutdown() to arch_timer_dying()?
> >
> > We may not need to do this. If the context in which the timer is
> > turned off is to disable interrupts, and the last timing period just
> > arrives during this process, when the interrupt is re-enabled,
> > an additional interrupt will be triggered and the next timing expiration
> > time will be filled. This is not in line with expectations.
> > Isn't it more reasonable to clean up the interrupt flag after turning off
> > the timer instead of just doing so cpu offline context cleaning.
> >
>
> This is my personal opinion, but I just trace the process of cpu offline
> and need to clear the timer interrupt.
> Because is not particularly proficient in of the overall system
> architecture, I might have a one-sided view.
> Do you have a better suggestion?
Without this patch constant_set_state_shutdown() seems work well, so I
think clearing interrupt may only needed by cpuhotplug. The reason is
that the timer interrupt handler doesn't work when the cpu is offline,
but not in other cases.
However, whether the clearing is in constant_set_state_shutdown() or
not are both OK for me.
Huacai
>
> Thanks!
> Xianglai.
>
> > Thanks!
> > Xianglai.
> >
> >>
> >> Huacai
> >>
> >>>
> >>>>> + if (err)
> >>>>> + pr_info("cpu hotplug event register failed");
> >>>> This is not so useful, because the error isn't fatal.
> >>>>
> >>>>
> >>>> Huacai
> >>>>
> >>>>> +
> >>>>> }
> >>>>> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> >>>>> index edfa61d80702..6606c1546afc 100644
> >>>>> --- a/include/linux/cpuhotplug.h
> >>>>> +++ b/include/linux/cpuhotplug.h
> >>>>> @@ -159,6 +159,7 @@ enum cpuhp_state {
> >>>>> CPUHP_AP_PERF_ARM_STARTING,
> >>>>> CPUHP_AP_PERF_RISCV_STARTING,
> >>>>> CPUHP_AP_ARM_L2X0_STARTING,
> >>>>> + CPUHP_AP_LOONGARCH_ARCH_TIMER_STARTING,
> >>>>> CPUHP_AP_EXYNOS4_MCT_TIMER_STARTING,
> >>>>> CPUHP_AP_ARM_ARCH_TIMER_STARTING,
> >>>>> CPUHP_AP_ARM_ARCH_TIMER_EVTSTRM_STARTING,
> >>>>>
> >>>>> base-commit: 53e760d8949895390e256e723e7ee46618310361
> >>>>> --
> >>>>> 2.39.1
> >>>>>
> >>>
> >
>
Powered by blists - more mailing lists