[PATCH 1/2] powerpc/time: Use clockevents_register_device(), fixing an issue with large decrementer

[PATCH 1/2] powerpc/time: Use clockevents_register_device(), fixing an issue with large decrementer

[Anton Blanchard]

We currently cap the decrementer clockevent at 4 seconds, even on systems
with large decrementer support. Fix this by converting the code to use
clockevents_register_device() which calculates the upper bound based on
the max_delta passed in.

Signed-off-by: Anton Blanchard <anton@samba.org>
---
 arch/powerpc/kernel/time.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 70f145e02487..6a1f0a084ca3 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -984,10 +984,10 @@ static void register_decrementer_clockevent(int cpu)
 	*dec = decrementer_clockevent;
 	dec->cpumask = cpumask_of(cpu);
 
+	clockevents_config_and_register(dec, ppc_tb_freq, 2, decrementer_max);
+
 	printk_once(KERN_DEBUG "clockevent: %s mult[%x] shift[%d] cpu[%d]\n",
 		    dec->name, dec->mult, dec->shift, cpu);
-
-	clockevents_register_device(dec);
 }
 
 static void enable_large_decrementer(void)
@@ -1035,18 +1035,7 @@ static void __init set_decrementer_max(void)
 
 static void __init init_decrementer_clockevent(void)
 {
-	int cpu = smp_processor_id();
-
-	clockevents_calc_mult_shift(&decrementer_clockevent, ppc_tb_freq, 4);
-
-	decrementer_clockevent.max_delta_ns =
-		clockevent_delta2ns(decrementer_max, &decrementer_clockevent);
-	decrementer_clockevent.max_delta_ticks = decrementer_max;
-	decrementer_clockevent.min_delta_ns =
-		clockevent_delta2ns(2, &decrementer_clockevent);
-	decrementer_clockevent.min_delta_ticks = 2;
-
-	register_decrementer_clockevent(cpu);
+	register_decrementer_clockevent(smp_processor_id());
 }
 
 void secondary_cpu_time_init(void)
-- 
2.17.1

[PATCH 2/2] powerpc/time: Only cap decrementer when watchdog is enabled

[Anton Blanchard]

If CONFIG_PPC_WATCHDOG is enabled, we always cap the decrementer to
0x7fffffff. As suggested by Nick, add a run time check of the watchdog
cpumask, so if it is disabled we use the large decrementer.

Signed-off-by: Anton Blanchard <anton@samba.org>
---
 arch/powerpc/kernel/time.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 6a1f0a084ca3..3372019f52bd 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -60,6 +60,7 @@
 #include <linux/rtc.h>
 #include <linux/sched/cputime.h>
 #include <linux/processor.h>
+#include <linux/nmi.h>
 #include <asm/trace.h>
 
 #include <asm/io.h>
@@ -575,7 +576,8 @@ void timer_interrupt(struct pt_regs *regs)
 	 * 31 bits, which is about 4 seconds on most systems, which gives
 	 * the watchdog a chance of catching timer interrupt hard lockups.
 	 */
-	if (IS_ENABLED(CONFIG_PPC_WATCHDOG))
+	if (IS_ENABLED(CONFIG_PPC_WATCHDOG) &&
+	    cpumask_test_cpu(smp_processor_id(), &watchdog_cpumask))
 		set_dec(0x7fffffff);
 	else
 		set_dec(decrementer_max);
-- 
2.17.1

Re: [PATCH 2/2] powerpc/time: Only cap decrementer when watchdog is enabled

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3257 bytes --]

Hi Anton,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.19-rc5 next-20180928]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Anton-Blanchard/powerpc-time-Use-clockevents_register_device-fixing-an-issue-with-large-decrementer/20180929-093322
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-allnoconfig (attached as .config)
compiler: powerpc-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   arch/powerpc/kernel/time.c: In function 'timer_interrupt':
>> arch/powerpc/kernel/time.c:580:44: error: 'watchdog_cpumask' undeclared (first use in this function); did you mean 'proc_watchdog_cpumask'?
         cpumask_test_cpu(smp_processor_id(), &watchdog_cpumask))
                                               ^~~~~~~~~~~~~~~~
                                               proc_watchdog_cpumask
   arch/powerpc/kernel/time.c:580:44: note: each undeclared identifier is reported only once for each function it appears in

vim +580 arch/powerpc/kernel/time.c

   549	
   550	/*
   551	 * timer_interrupt - gets called when the decrementer overflows,
   552	 * with interrupts disabled.
   553	 */
   554	void timer_interrupt(struct pt_regs *regs)
   555	{
   556		struct clock_event_device *evt = this_cpu_ptr(&decrementers);
   557		u64 *next_tb = this_cpu_ptr(&decrementers_next_tb);
   558		struct pt_regs *old_regs;
   559		u64 now;
   560	
   561		/* Some implementations of hotplug will get timer interrupts while
   562		 * offline, just ignore these and we also need to set
   563		 * decrementers_next_tb as MAX to make sure __check_irq_replay
   564		 * don't replay timer interrupt when return, otherwise we'll trap
   565		 * here infinitely :(
   566		 */
   567		if (unlikely(!cpu_online(smp_processor_id()))) {
   568			*next_tb = ~(u64)0;
   569			set_dec(decrementer_max);
   570			return;
   571		}
   572	
   573		/* Ensure a positive value is written to the decrementer, or else
   574		 * some CPUs will continue to take decrementer exceptions. When the
   575		 * PPC_WATCHDOG (decrementer based) is configured, keep this at most
   576		 * 31 bits, which is about 4 seconds on most systems, which gives
   577		 * the watchdog a chance of catching timer interrupt hard lockups.
   578		 */
   579		if (IS_ENABLED(CONFIG_PPC_WATCHDOG) &&
 > 580		    cpumask_test_cpu(smp_processor_id(), &watchdog_cpumask))
   581			set_dec(0x7fffffff);
   582		else
   583			set_dec(decrementer_max);
   584	
   585		/* Conditionally hard-enable interrupts now that the DEC has been
   586		 * bumped to its maximum value
   587		 */
   588		may_hard_irq_enable();
   589	
   590	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 5832 bytes --]

Re: [PATCH 2/2] powerpc/time: Only cap decrementer when watchdog is enabled

[Nicholas Piggin]

On Sat, 29 Sep 2018 11:26:07 +1000
Anton Blanchard <anton@ozlabs.org> wrote:

> If CONFIG_PPC_WATCHDOG is enabled, we always cap the decrementer to
> 0x7fffffff. As suggested by Nick, add a run time check of the watchdog
> cpumask, so if it is disabled we use the large decrementer.
> 
> Signed-off-by: Anton Blanchard <anton@samba.org>
> ---

Thanks for tracking this down. It's a fix for my breakage

a7cba02deced ("powerpc: allow soft-NMI watchdog to cover timer
interrupts with large decrementers")

Taking another look... what I had expected here is the timer subsystem
would have stopped the decrementer device after it processed the timer
and found nothing left. And we should have set DEC to max at that time.

The above patch was really intended to only cover the timer interrupt
itself locking up. I wonder if we need to add

    .set_state_oneshot_stopped = decrementer_shutdown

In our decremementer clockevent device?

Thanks,
Nick

Re: [PATCH 2/2] powerpc/time: Only cap decrementer when watchdog is enabled

[Anton Blanchard]

Hi Nick,

> Thanks for tracking this down. It's a fix for my breakage
> 
> a7cba02deced ("powerpc: allow soft-NMI watchdog to cover timer
> interrupts with large decrementers")
> 
> Taking another look... what I had expected here is the timer subsystem
> would have stopped the decrementer device after it processed the timer
> and found nothing left. And we should have set DEC to max at that
> time.
> 
> The above patch was really intended to only cover the timer interrupt
> itself locking up. I wonder if we need to add
> 
>     .set_state_oneshot_stopped = decrementer_shutdown
> 
> In our decremementer clockevent device?

Thanks Nick, that looks much nicer, and passes my tests.

Anton

[PATCH 1/2] powerpc/time: Use clockevents_register_device(), fixing an issue with large decrementer

[Anton Blanchard]

We currently cap the decrementer clockevent at 4 seconds, even on systems
with large decrementer support. Fix this by converting the code to use
clockevents_register_device() which calculates the upper bound based on
the max_delta passed in.

Signed-off-by: Anton Blanchard <anton@ozlabs.org>
---
 arch/powerpc/kernel/time.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 70f145e02487..6a1f0a084ca3 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -984,10 +984,10 @@ static void register_decrementer_clockevent(int cpu)
 	*dec = decrementer_clockevent;
 	dec->cpumask = cpumask_of(cpu);
 
+	clockevents_config_and_register(dec, ppc_tb_freq, 2, decrementer_max);
+
 	printk_once(KERN_DEBUG "clockevent: %s mult[%x] shift[%d] cpu[%d]\n",
 		    dec->name, dec->mult, dec->shift, cpu);
-
-	clockevents_register_device(dec);
 }
 
 static void enable_large_decrementer(void)
@@ -1035,18 +1035,7 @@ static void __init set_decrementer_max(void)
 
 static void __init init_decrementer_clockevent(void)
 {
-	int cpu = smp_processor_id();
-
-	clockevents_calc_mult_shift(&decrementer_clockevent, ppc_tb_freq, 4);
-
-	decrementer_clockevent.max_delta_ns =
-		clockevent_delta2ns(decrementer_max, &decrementer_clockevent);
-	decrementer_clockevent.max_delta_ticks = decrementer_max;
-	decrementer_clockevent.min_delta_ns =
-		clockevent_delta2ns(2, &decrementer_clockevent);
-	decrementer_clockevent.min_delta_ticks = 2;
-
-	register_decrementer_clockevent(cpu);
+	register_decrementer_clockevent(smp_processor_id());
 }
 
 void secondary_cpu_time_init(void)
-- 
2.17.1

Re: [PATCH 1/2] powerpc/time: Use clockevents_register_device(), fixing an issue with large decrementer

[Michael Ellerman]

Anton Blanchard <anton@ozlabs.org> writes:

> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index 70f145e02487..6a1f0a084ca3 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -1035,18 +1035,7 @@ static void __init set_decrementer_max(void)
>  
>  static void __init init_decrementer_clockevent(void)
>  {
> -	int cpu = smp_processor_id();
> -
> -	clockevents_calc_mult_shift(&decrementer_clockevent, ppc_tb_freq, 4);
> -
> -	decrementer_clockevent.max_delta_ns =
> -		clockevent_delta2ns(decrementer_max, &decrementer_clockevent);
> -	decrementer_clockevent.max_delta_ticks = decrementer_max;
> -	decrementer_clockevent.min_delta_ns =
> -		clockevent_delta2ns(2, &decrementer_clockevent);
> -	decrementer_clockevent.min_delta_ticks = 2;
> -
> -	register_decrementer_clockevent(cpu);
> +	register_decrementer_clockevent(smp_processor_id());
>  }

This broke KVM PR :)

Because we no longer set the mult/shift in decrementer_clockevent, which
is used in kvmppc_emulate_dec().

The patch below fixes it, though it'd be nice to come up with something
cleaner eventually.

cheers


diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 6a1f0a084ca3..d21b2b88c0a7 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -1036,6 +1036,9 @@ static void __init set_decrementer_max(void)
 static void __init init_decrementer_clockevent(void)
 {
 	register_decrementer_clockevent(smp_processor_id());
+
+	/* Set values for KVM, see kvm_emulate_dec() */
+	decrementer_clockevent = *dec;
 }
 
 void secondary_cpu_time_init(void)