LoongArch: Implement stable timer shutdown interface

  When cpu is hotplug out, cpu is in idle state and function
arch_cpu_idle_dead is called. Timer interrupt for this processor should
be disabled, else there will be timer interrupt for the dead cpu. Also
this prevents vcpu to schedule out during halt-polling flow when system
is running in vm mode, since there is pending timer interrupt.

This patch adds detailed implementation for timer shutdown interface, so
that timer will be disabled when cpu is plug-out.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 arch/loongarch/kernel/time.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)


base-commit: 9bacdd8996c77c42ca004440be610692275ff9d0
  

Hi, Bibo,

"stable timer" is Chinglish, use "constant timer" here.

On Tue, Nov 14, 2023 at 7:49 PM Bibo Mao <maobibo@loongson.cn> wrote:
>
> When cpu is hotplug out, cpu is in idle state and function
> arch_cpu_idle_dead is called. Timer interrupt for this processor should
> be disabled, else there will be timer interrupt for the dead cpu. Also
> this prevents vcpu to schedule out during halt-polling flow when system
> is running in vm mode, since there is pending timer interrupt.
>
> This patch adds detailed implementation for timer shutdown interface, so
> that timer will be disabled when cpu is plug-out.
>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>  arch/loongarch/kernel/time.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/arch/loongarch/kernel/time.c b/arch/loongarch/kernel/time.c
> index 3064af94db9c..2920770e30a9 100644
> --- a/arch/loongarch/kernel/time.c
> +++ b/arch/loongarch/kernel/time.c
> @@ -58,7 +58,7 @@ static int constant_set_state_oneshot(struct clock_event_device *evt)
>         return 0;
>  }
>
> -static int constant_set_state_oneshot_stopped(struct clock_event_device *evt)
> +static int constant_set_state_shutdown(struct clock_event_device *evt)
>  {
>         unsigned long timer_config;
>
Please remove the whole constant_set_state_oneshot_stopped() and move
its logic to the below constant_set_state_shutdown().

And it is very strange that this "bug" hasn't caused any problems until now.

Huacai

> @@ -90,11 +90,6 @@ static int constant_set_state_periodic(struct clock_event_device *evt)
>         return 0;
>  }
>
> -static int constant_set_state_shutdown(struct clock_event_device *evt)
> -{
> -       return 0;
> -}
> -
>  static int constant_timer_next_event(unsigned long delta, struct clock_event_device *evt)
>  {
>         unsigned long timer_config;
> @@ -161,7 +156,7 @@ int constant_clockevent_init(void)
>         cd->rating = 320;
>         cd->cpumask = cpumask_of(cpu);
>         cd->set_state_oneshot = constant_set_state_oneshot;
> -       cd->set_state_oneshot_stopped = constant_set_state_oneshot_stopped;
> +       cd->set_state_oneshot_stopped = constant_set_state_shutdown;
>         cd->set_state_periodic = constant_set_state_periodic;
>         cd->set_state_shutdown = constant_set_state_shutdown;
>         cd->set_next_event = constant_timer_next_event;
>
> base-commit: 9bacdd8996c77c42ca004440be610692275ff9d0
> --
> 2.39.3
>
  

On 2023/11/16 上午10:50, Huacai Chen wrote:
> Hi, Bibo,
> 
> "stable timer" is Chinglish, use "constant timer" here.
will do if you insist on, the name is not created by me :(

> 
> On Tue, Nov 14, 2023 at 7:49 PM Bibo Mao <maobibo@loongson.cn> wrote:
>>
>> When cpu is hotplug out, cpu is in idle state and function
>> arch_cpu_idle_dead is called. Timer interrupt for this processor should
>> be disabled, else there will be timer interrupt for the dead cpu. Also
>> this prevents vcpu to schedule out during halt-polling flow when system
>> is running in vm mode, since there is pending timer interrupt.
>>
>> This patch adds detailed implementation for timer shutdown interface, so
>> that timer will be disabled when cpu is plug-out.
>>
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   arch/loongarch/kernel/time.c | 9 ++-------
>>   1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/loongarch/kernel/time.c b/arch/loongarch/kernel/time.c
>> index 3064af94db9c..2920770e30a9 100644
>> --- a/arch/loongarch/kernel/time.c
>> +++ b/arch/loongarch/kernel/time.c
>> @@ -58,7 +58,7 @@ static int constant_set_state_oneshot(struct clock_event_device *evt)
>>          return 0;
>>   }
>>
>> -static int constant_set_state_oneshot_stopped(struct clock_event_device *evt)
>> +static int constant_set_state_shutdown(struct clock_event_device *evt)
>>   {
>>          unsigned long timer_config;
>>
> Please remove the whole constant_set_state_oneshot_stopped() and move
> its logic to the below constant_set_state_shutdown().
will do.
> 
> And it is very strange that this "bug" hasn't caused any problems until now.
Since there is no other hw timer except percpu timer, there will be 
problem if hw timer is switched, also nobody watches hw behaviors when 
cpu is plug-out. LTP cpu hotplug test cases just report passed on 
physical machine.

Regards
Bibo Mao

> 
> Huacai
> 
>> @@ -90,11 +90,6 @@ static int constant_set_state_periodic(struct clock_event_device *evt)
>>          return 0;
>>   }
>>
>> -static int constant_set_state_shutdown(struct clock_event_device *evt)
>> -{
>> -       return 0;
>> -}
>> -
>>   static int constant_timer_next_event(unsigned long delta, struct clock_event_device *evt)
>>   {
>>          unsigned long timer_config;
>> @@ -161,7 +156,7 @@ int constant_clockevent_init(void)
>>          cd->rating = 320;
>>          cd->cpumask = cpumask_of(cpu);
>>          cd->set_state_oneshot = constant_set_state_oneshot;
>> -       cd->set_state_oneshot_stopped = constant_set_state_oneshot_stopped;
>> +       cd->set_state_oneshot_stopped = constant_set_state_shutdown;
>>          cd->set_state_periodic = constant_set_state_periodic;
>>          cd->set_state_shutdown = constant_set_state_shutdown;
>>          cd->set_next_event = constant_timer_next_event;
>>
>> base-commit: 9bacdd8996c77c42ca004440be610692275ff9d0
>> --
>> 2.39.3
>>
  

Hi,

Sorry for the late review but here we go:

On 11/14/23 19:46, Bibo Mao wrote:
> When cpu is hotplug out, cpu is in idle state and function
"When a CPU is hot-unplugged, it is put into idle state and the function 
... is called"
> arch_cpu_idle_dead is called. Timer interrupt for this processor should
> be disabled, else there will be timer interrupt for the dead cpu. Also
> this prevents vcpu to schedule out during halt-polling flow when system
> is running in vm mode, since there is pending timer interrupt.

The logical relationship is a bit unclear, is my paraphrasing correct in 
your opinion?

"Timer interrupt for this processor should be disabled, else a pending 
timer interrupt will prevent the vCPU from scheduling out during the 
halt-polling flow when system is running in VM mode"

(I don't immediately know what a "schedule out" is. Is that a 
translation artifact or some KVM jargon?)

>
> This patch adds detailed implementation for timer shutdown interface, so
> that timer will be disabled when cpu is plug-out.

Missing some definite articles too.

"This patch implements the timer shutdown interface so that the timer 
will be properly disabled when a CPU is hot-unplugged"

Is this version better?

>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>   arch/loongarch/kernel/time.c | 9 ++-------
>   1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/arch/loongarch/kernel/time.c b/arch/loongarch/kernel/time.c
> index 3064af94db9c..2920770e30a9 100644
> --- a/arch/loongarch/kernel/time.c
> +++ b/arch/loongarch/kernel/time.c
> @@ -58,7 +58,7 @@ static int constant_set_state_oneshot(struct clock_event_device *evt)
>   	return 0;
>   }
>   
> -static int constant_set_state_oneshot_stopped(struct clock_event_device *evt)
> +static int constant_set_state_shutdown(struct clock_event_device *evt)
>   {
>   	unsigned long timer_config;
>   
> @@ -90,11 +90,6 @@ static int constant_set_state_periodic(struct clock_event_device *evt)
>   	return 0;
>   }
>   
> -static int constant_set_state_shutdown(struct clock_event_device *evt)
> -{
> -	return 0;
> -}
> -
>   static int constant_timer_next_event(unsigned long delta, struct clock_event_device *evt)
>   {
>   	unsigned long timer_config;
> @@ -161,7 +156,7 @@ int constant_clockevent_init(void)
>   	cd->rating = 320;
>   	cd->cpumask = cpumask_of(cpu);
>   	cd->set_state_oneshot = constant_set_state_oneshot;
> -	cd->set_state_oneshot_stopped = constant_set_state_oneshot_stopped;
> +	cd->set_state_oneshot_stopped = constant_set_state_shutdown;
>   	cd->set_state_periodic = constant_set_state_periodic;
>   	cd->set_state_shutdown = constant_set_state_shutdown;
>   	cd->set_next_event = constant_timer_next_event;
>
> base-commit: 9bacdd8996c77c42ca004440be610692275ff9d0

Otherwise LGTM (regarding the renaming of 
constant_set_state_oneshot_stopped, both it and the removed 
constant_set_state_shutdown only has one usage respectively, and looking 
at the function body it's arguably more appropriate to let it take the 
"shutdown" name: it's just clearing the enable bit from the CSR.TCFG and 
nothing else).

With the nits addressed:

Reviewed-by: WANG Xuerui <git@xen0n.name>
  

Xuerui,

Thanks for reviewing my patch, and I reply inline.

On 2023/11/16 下午3:09, WANG Xuerui wrote:
> Hi,
> 
> Sorry for the late review but here we go:
> 
> On 11/14/23 19:46, Bibo Mao wrote:
>> When cpu is hotplug out, cpu is in idle state and function
> "When a CPU is hot-unplugged, it is put into idle state and the function 
> ... is called"
Will do in next patch.

>> arch_cpu_idle_dead is called. Timer interrupt for this processor should
>> be disabled, else there will be timer interrupt for the dead cpu. Also
>> this prevents vcpu to schedule out during halt-polling flow when system
>> is running in vm mode, since there is pending timer interrupt.
> 
> The logical relationship is a bit unclear, is my paraphrasing correct in 
> your opinion?
> 
> "Timer interrupt for this processor should be disabled, else a pending 
> timer interrupt will prevent the vCPU from scheduling out during the 
> halt-polling flow when system is running in VM mode"
> 
> (I don't immediately know what a "schedule out" is. Is that a 
> translation artifact or some KVM jargon?)
how about "prevent the vCPU from giving up scheduling"?

> 
>>
>> This patch adds detailed implementation for timer shutdown interface, so
>> that timer will be disabled when cpu is plug-out.
> 
> Missing some definite articles too.
> 
> "This patch implements the timer shutdown interface so that the timer 
> will be properly disabled when a CPU is hot-unplugged"
> 
> Is this version better?
sure, will do in next version.

Regards
Bibo Mao
> 
>>
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   arch/loongarch/kernel/time.c | 9 ++-------
>>   1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/loongarch/kernel/time.c b/arch/loongarch/kernel/time.c
>> index 3064af94db9c..2920770e30a9 100644
>> --- a/arch/loongarch/kernel/time.c
>> +++ b/arch/loongarch/kernel/time.c
>> @@ -58,7 +58,7 @@ static int constant_set_state_oneshot(struct 
>> clock_event_device *evt)
>>       return 0;
>>   }
>> -static int constant_set_state_oneshot_stopped(struct 
>> clock_event_device *evt)
>> +static int constant_set_state_shutdown(struct clock_event_device *evt)
>>   {
>>       unsigned long timer_config;
>> @@ -90,11 +90,6 @@ static int constant_set_state_periodic(struct 
>> clock_event_device *evt)
>>       return 0;
>>   }
>> -static int constant_set_state_shutdown(struct clock_event_device *evt)
>> -{
>> -    return 0;
>> -}
>> -
>>   static int constant_timer_next_event(unsigned long delta, struct 
>> clock_event_device *evt)
>>   {
>>       unsigned long timer_config;
>> @@ -161,7 +156,7 @@ int constant_clockevent_init(void)
>>       cd->rating = 320;
>>       cd->cpumask = cpumask_of(cpu);
>>       cd->set_state_oneshot = constant_set_state_oneshot;
>> -    cd->set_state_oneshot_stopped = constant_set_state_oneshot_stopped;
>> +    cd->set_state_oneshot_stopped = constant_set_state_shutdown;
>>       cd->set_state_periodic = constant_set_state_periodic;
>>       cd->set_state_shutdown = constant_set_state_shutdown;
>>       cd->set_next_event = constant_timer_next_event;
>>
>> base-commit: 9bacdd8996c77c42ca004440be610692275ff9d0
> 
> Otherwise LGTM (regarding the renaming of 
> constant_set_state_oneshot_stopped, both it and the removed 
> constant_set_state_shutdown only has one usage respectively, and looking 
> at the function body it's arguably more appropriate to let it take the 
> "shutdown" name: it's just clearing the enable bit from the CSR.TCFG and 
> nothing else).
> 
> With the nits addressed:
> 
> Reviewed-by: WANG Xuerui <git@xen0n.name>
>
  

On 11/16/23 16:21, maobibo wrote:
>>> arch_cpu_idle_dead is called. Timer interrupt for this processor should
>>> be disabled, else there will be timer interrupt for the dead cpu. Also
>>> this prevents vcpu to schedule out during halt-polling flow when system
>>> is running in vm mode, since there is pending timer interrupt.
>>
>> The logical relationship is a bit unclear, is my paraphrasing correct 
>> in your opinion?
>>
>> "Timer interrupt for this processor should be disabled, else a 
>> pending timer interrupt will prevent the vCPU from scheduling out 
>> during the halt-polling flow when system is running in VM mode"
>>
>> (I don't immediately know what a "schedule out" is. Is that a 
>> translation artifact or some KVM jargon?)
> how about "prevent the vCPU from giving up scheduling"?
That's very easy to understand, so no problem and thanks!