| 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: <61c858da-8c5e-7652-e580-fc51c211ebb0@loongson.cn>
Date: Wed, 13 Aug 2025 10:26:38 +0800
From: lixianglai <lixianglai@...ngson.cn>
To: Huacai Chen <chenhuacai@...nel.org>
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
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?
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