tick: Remove unreasonable detached state set in tick_shutdown()

[PATCH] tick: Remove unreasonable detached state set in tick_shutdown()

Posted by Bibo Mao 1 month, 4 weeks ago

Function clockevents_switch_state() will check whether it has already
switched to specified state, do nothing if it has.

In function tick_shutdown(), it will set detached state at first and
call clockevents_switch_state() in clockevents_exchange_device(). The
function clockevents_switch_state() will do nothing since it is already
detached state. So the tick timer device will not be shutdown when CPU
is offline. In guest VM system, timer interrupt will prevent vCPU to sleep
if vCPU is hot removed.

Here remove state set before calling clockevents_exchange_device(),
its state will be set in function clockevents_switch_state() if it
succeeds to do so.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 kernel/time/tick-common.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index 9a3859443c04..eb9b777f5492 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -424,11 +424,6 @@ void tick_shutdown(unsigned int cpu)
 
 	td->mode = TICKDEV_MODE_PERIODIC;
 	if (dev) {
-		/*
-		 * Prevent that the clock events layer tries to call
-		 * the set mode function!
-		 */
-		clockevent_set_state(dev, CLOCK_EVT_STATE_DETACHED);
 		clockevents_exchange_device(dev, NULL);
 		dev->event_handler = clockevents_handle_noop;
 		td->evtdev = NULL;

base-commit: 7e161a991ea71e6ec526abc8f40c6852ebe3d946
-- 
2.39.3

Re: [PATCH] tick: Remove unreasonable detached state set in tick_shutdown()

Posted by Frederic Weisbecker 1 month ago

Le Thu, Aug 07, 2025 at 11:59:54AM +0800, Bibo Mao a écrit :
> Function clockevents_switch_state() will check whether it has already
> switched to specified state, do nothing if it has.
> 
> In function tick_shutdown(), it will set detached state at first and
> call clockevents_switch_state() in clockevents_exchange_device(). The
> function clockevents_switch_state() will do nothing since it is already
> detached state. So the tick timer device will not be shutdown when CPU
> is offline. In guest VM system, timer interrupt will prevent vCPU to sleep
> if vCPU is hot removed.
> 
> Here remove state set before calling clockevents_exchange_device(),
> its state will be set in function clockevents_switch_state() if it
> succeeds to do so.
> 
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>

On a second thought, it may need the following tags:

Fixes: bf9a001fb8e4 ("clocksource/drivers/timer-tegra: Remove clockevents shutdown call on offlining")
Fixes: cd165ce8314f ("clocksource/drivers/qcom: Remove clockevents shutdown call on offlining")
Fixes: 30f8c70a85bc ("clocksource/drivers/armada-370-xp: Remove clockevents shutdown call on offlining")
Fixes: ba23b6c7f974 ("clocksource/drivers/exynos_mct: Remove clockevents shutdown call on offlining")
Fixes: 15b810e0496e ("clocksource/drivers/arm_global_timer: Remove clockevents shutdown call on offlining")
Fixes: 78b5c2ca5f27 ("clocksource/drivers/arm_arch_timer: Remove clockevents shutdown call on offlining")
Fixes: 900053d9eedf ("ARM: smp_twd: Remove clockevents shutdown call on offlining")

Because those commits removed shutdown calls made from drivers that assumed the
core would do on their behalf. But it was not the case before your fix.

Thanks!

-- 
Frederic Weisbecker
SUSE Labs

Re: [PATCH] tick: Remove unreasonable detached state set in tick_shutdown()

Posted by Bibo Mao 1 month ago


On 2025/9/2 下午10:07, Frederic Weisbecker wrote:
> Le Thu, Aug 07, 2025 at 11:59:54AM +0800, Bibo Mao a écrit :
>> Function clockevents_switch_state() will check whether it has already
>> switched to specified state, do nothing if it has.
>>
>> In function tick_shutdown(), it will set detached state at first and
>> call clockevents_switch_state() in clockevents_exchange_device(). The
>> function clockevents_switch_state() will do nothing since it is already
>> detached state. So the tick timer device will not be shutdown when CPU
>> is offline. In guest VM system, timer interrupt will prevent vCPU to sleep
>> if vCPU is hot removed.
>>
>> Here remove state set before calling clockevents_exchange_device(),
>> its state will be set in function clockevents_switch_state() if it
>> succeeds to do so.
>>
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> 
> On a second thought, it may need the following tags:
> 
> Fixes: bf9a001fb8e4 ("clocksource/drivers/timer-tegra: Remove clockevents shutdown call on offlining")
> Fixes: cd165ce8314f ("clocksource/drivers/qcom: Remove clockevents shutdown call on offlining")
> Fixes: 30f8c70a85bc ("clocksource/drivers/armada-370-xp: Remove clockevents shutdown call on offlining")
> Fixes: ba23b6c7f974 ("clocksource/drivers/exynos_mct: Remove clockevents shutdown call on offlining")
> Fixes: 15b810e0496e ("clocksource/drivers/arm_global_timer: Remove clockevents shutdown call on offlining")
> Fixes: 78b5c2ca5f27 ("clocksource/drivers/arm_arch_timer: Remove clockevents shutdown call on offlining")
> Fixes: 900053d9eedf ("ARM: smp_twd: Remove clockevents shutdown call on offlining")
> 
> Because those commits removed shutdown calls made from drivers that assumed the
> core would do on their behalf. But it was not the case before your fix.
Ok, will add these fixes tags in next version.

Regards
Bibo Mao
> 
> Thanks!
>

Re: [PATCH] tick: Remove unreasonable detached state set in tick_shutdown()

Posted by Frederic Weisbecker 1 month ago

Le Thu, Aug 07, 2025 at 11:59:54AM +0800, Bibo Mao a écrit :
> Function clockevents_switch_state() will check whether it has already
> switched to specified state, do nothing if it has.
> 
> In function tick_shutdown(), it will set detached state at first and
> call clockevents_switch_state() in clockevents_exchange_device(). The
> function clockevents_switch_state() will do nothing since it is already
> detached state. So the tick timer device will not be shutdown when CPU
> is offline. In guest VM system, timer interrupt will prevent vCPU to sleep
> if vCPU is hot removed.
> 
> Here remove state set before calling clockevents_exchange_device(),
> its state will be set in function clockevents_switch_state() if it
> succeeds to do so.
> 
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>

Good catch. Looking back, it has been that way since the introduction
of clockevents and tick. Therefore it's not a regression and probably
not worth a Fixes: tag.

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

-- 
Frederic Weisbecker
SUSE Labs

Re: [PATCH] tick: Remove unreasonable detached state set in tick_shutdown()

Posted by Bibo Mao 1 month, 3 weeks ago

Hi,

Previously tick_shutdown() is called in BP, percpu timer cannot be 
shutdown in BP. Now it is called in AP, percpu timer can be shutdown in 
AP itself.

And there is another method, like other architectures add hotplug event 
callbak to shutdown percpu timer such as CPUHP_AP_LOONGARCH_TIMER_STARTING.

Which one do you prefer?

Regards
Bibo Mao

On 2025/8/7 上午11:59, Bibo Mao wrote:
> Function clockevents_switch_state() will check whether it has already
> switched to specified state, do nothing if it has.
> 
> In function tick_shutdown(), it will set detached state at first and
> call clockevents_switch_state() in clockevents_exchange_device(). The
> function clockevents_switch_state() will do nothing since it is already
> detached state. So the tick timer device will not be shutdown when CPU
> is offline. In guest VM system, timer interrupt will prevent vCPU to sleep
> if vCPU is hot removed.
> 
> Here remove state set before calling clockevents_exchange_device(),
> its state will be set in function clockevents_switch_state() if it
> succeeds to do so.
> 
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>   kernel/time/tick-common.c | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
> index 9a3859443c04..eb9b777f5492 100644
> --- a/kernel/time/tick-common.c
> +++ b/kernel/time/tick-common.c
> @@ -424,11 +424,6 @@ void tick_shutdown(unsigned int cpu)
>   
>   	td->mode = TICKDEV_MODE_PERIODIC;
>   	if (dev) {
> -		/*
> -		 * Prevent that the clock events layer tries to call
> -		 * the set mode function!
> -		 */
> -		clockevent_set_state(dev, CLOCK_EVT_STATE_DETACHED);
>   		clockevents_exchange_device(dev, NULL);
>   		dev->event_handler = clockevents_handle_noop;
>   		td->evtdev = NULL;
> 
> base-commit: 7e161a991ea71e6ec526abc8f40c6852ebe3d946
>

Re: [PATCH] tick: Remove unreasonable detached state set in tick_shutdown()

Posted by Frederic Weisbecker 1 month ago

Le Mon, Aug 11, 2025 at 11:57:20AM +0800, Bibo Mao a écrit :
> Hi,
> 
> Previously tick_shutdown() is called in BP, percpu timer cannot be shutdown
> in BP. Now it is called in AP, percpu timer can be shutdown in AP itself.

Ok, one more reason for not backporting it to stable then.

> 
> And there is another method, like other architectures add hotplug event
> callbak to shutdown percpu timer such as CPUHP_AP_LOONGARCH_TIMER_STARTING.
> 
> Which one do you prefer?

I prefer your fix that works for every clockevents. We want the active clockevent
to be stopped by the time we reach CPUHP_AP_OFFLINE.

If you add a loongarch hotplug callback, it must be for architecture
specifics.

Thanks.

-- 
Frederic Weisbecker
SUSE Labs