powerpc: irq work racing with timer interrupt can result in timer interrupt hang

I am seeing an issue where a CPU running perf eventually hangs. Traces show timer interrupts happening every 4 seconds even when a userspace task is running on the CPU. /proc/timer_list also shows pending hrtimers have not run in over an hour, including the scheduler.

Looking closer, decrementers_next_tb is getting set to 0xffffffffffffffff, and at that point we will never take a timer interrupt again.

In __timer_interrupt() we set decrementers_next_tb to 0xffffffffffffffff and rely on ->event_handler to update it:

        *next_tb = ~(u64)0;
        if (evt->event_handler)
                evt->event_handler(evt);

In this case ->event_handler is hrtimer_interrupt. This will eventually call back through the clockevents code with the next event to be programmed:

static int decrementer_set_next_event(unsigned long evt,
struct clock_event_device *dev)

{
        /* Don't adjust the decrementer if some irq work is pending */
        if (test_irq_work_pending())
                return 0;
        __get_cpu_var(decrementers_next_tb) = get_tb_or_rtc() + evt;

If irq work came in between these two points, we will return before updating decrementers_next_tb and we never process a timer interrupt again.

This looks to have been introduced by 0215f7d8c53f (powerpc: Fix races with irq_work). Fix it by removing the early exit and relying on code later on in the function to force an early decrementer:

       /* We may have raced with new irq work */
       if (test_irq_work_pending())
               set_dec(1);

Signed-off-by Anton Blanchard <[email protected]>

Cc [email protected] # 3.14+

---

arch/powerpc/kernel/time.c +0 -0

index 122a580..4f0b676 100644

_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://lists.ozlabs.org/listinfo/linuxppc-dev

in __timer_interrupt() outside the _else_ loop? This will ensure that no matter what, before exiting timer interrupt handler we check for pending irq work.

Quoted 3 lines · Preeti U Murthy

in __timer_interrupt() outside the _else_ loop? This will ensure that no matter what, before exiting timer interrupt handler we check for pending irq work.

We still need to make sure that set_next_event() doesn't move the dec beyond the next tick if there is a pending timer... maybe we

can fix it like this:

static int decrementer_set_next_event(unsigned long evt,
struct clock_event_device *dev)

{
	__get_cpu_var(decrementers_next_tb) = get_tb_or_rtc() + evt;

	/* Don't adjust the decrementer if some irq work is pending */
	if (!test_irq_work_pending())
		set_dec(evt);

	return 0;
}

Along with a single occurrence of:

	if (test_irq_work_pending())
		set_dec(1);

At the end of __timer_interrupt(), outside if the current else {} case, this should work, don't you think ?

What about this completely untested patch ?

arch/powerpc/kernel/time.c +7 -10

index 122a580..ba7e83b 100644
@@ -503,12 +503,13 @@ void __timer_interrupt(void)
                now = *next_tb - now;
                if (now <= DECREMENTER_MAX)
                        set_dec((int)now);
-               /* We may have raced with new irq work */
-               if (test_irq_work_pending())
-                       set_dec(1);
                __get_cpu_var(irq_stat).timer_irqs_others++;
        }
 
+       /* We may have raced with new irq work */
+       if (test_irq_work_pending())
+               set_dec(1);
+
 #ifdef CONFIG_PPC64
        /* collect purr register values often, for accurate calculations */
        if (firmware_has_feature(FW_FEATURE_SPLPAR)) {

@@ -813,15 +814,11 @@ static void __init clocksource_init(void)
 static int decrementer_set_next_event(unsigned long evt,
                                      struct clock_event_device *dev)
 {
-       /* Don't adjust the decrementer if some irq work is pending */
-       if (test_irq_work_pending())
-               return 0;
        __get_cpu_var(decrementers_next_tb) = get_tb_or_rtc() + evt;
-       set_dec(evt);
 
-       /* We may have raced with new irq work */
-       if (test_irq_work_pending())
-               set_dec(1);
+       /* Don't adjust the decrementer if some irq work is pending */
+       if (!test_irq_work_pending())
+               set_dec(evt);
 
        return 0;
 }

Quoted 7 lines · Benjamin Herrenschmidt

Quoted 3 lines · Preeti U Murthy

in __timer_interrupt() outside the _else_ loop? This will ensure that no matter what, before exiting timer interrupt handler we check for pending irq work.

We still need to make sure that set_next_event() doesn't move the dec beyond the next tick if there is a pending timer... maybe we

Sorry, but didn't get this. s/if there is pending timer/if there is pending irq work ?

Quoted 1 line

can fix it like this:

We can call set_next_event() from events like hrtimer_cancel() or hrtimer_forward() as well. In that case we don't come to decrementer_set_next_event() from __timer_interrupt(). Then, if we race with irq work, we *do not do* a set_dec(1) ( I am referring to the patch below ), we might never set the decrementer to fire immediately right?

Or does this scenario never arise?

Regards
Preeti U Murthy

Quoted 67 lines

static int decrementer_set_next_event(unsigned long evt,
struct clock_event_device *dev)

{
	__get_cpu_var(decrementers_next_tb) = get_tb_or_rtc() + evt;

	/* Don't adjust the decrementer if some irq work is pending */
	if (!test_irq_work_pending())
		set_dec(evt);

	return 0;
}

Along with a single occurrence of:

	if (test_irq_work_pending())
		set_dec(1);

At the end of __timer_interrupt(), outside if the current else {} case, this should work, don't you think ?

What about this completely untested patch ?

arch/powerpc/kernel/time.c +7 -10

index 122a580..ba7e83b 100644
@@ -503,12 +503,13 @@ void __timer_interrupt(void)
                now = *next_tb - now;
                if (now <= DECREMENTER_MAX)
                        set_dec((int)now);
-               /* We may have raced with new irq work */
-               if (test_irq_work_pending())
-                       set_dec(1);
                __get_cpu_var(irq_stat).timer_irqs_others++;
        }
 
+       /* We may have raced with new irq work */
+       if (test_irq_work_pending())
+               set_dec(1);
+
 #ifdef CONFIG_PPC64
        /* collect purr register values often, for accurate calculations */
        if (firmware_has_feature(FW_FEATURE_SPLPAR)) {

@@ -813,15 +814,11 @@ static void __init clocksource_init(void)
 static int decrementer_set_next_event(unsigned long evt,
                                      struct clock_event_device *dev)
 {
-       /* Don't adjust the decrementer if some irq work is pending */
-       if (test_irq_work_pending())
-               return 0;
        __get_cpu_var(decrementers_next_tb) = get_tb_or_rtc() + evt;
-       set_dec(evt);
 
-       /* We may have raced with new irq work */
-       if (test_irq_work_pending())
-               set_dec(1);
+       /* Don't adjust the decrementer if some irq work is pending */
+       if (!test_irq_work_pending())
+               set_dec(evt);
 
        return 0;
 }

Quoted 11 lines · Preeti U Murthy

Quoted 7 lines · Benjamin Herrenschmidt

Quoted 3 lines · Preeti U Murthy

in __timer_interrupt() outside the _else_ loop? This will ensure that no matter what, before exiting timer interrupt handler we check for pending irq work.

We still need to make sure that set_next_event() doesn't move the dec beyond the next tick if there is a pending timer... maybe we

Sorry, but didn't get this. s/if there is pending timer/if there is pending irq work ?

Yes, sorry :-) That's what I meant.

Quoted 9 lines

Quoted 1 line

can fix it like this:

We can call set_next_event() from events like hrtimer_cancel() or hrtimer_forward() as well. In that case we don't come to decrementer_set_next_event() from __timer_interrupt(). Then, if we race with irq work, we *do not do* a set_dec(1) ( I am referring to the patch below ), we might never set the decrementer to fire immediately right?

Or does this scenario never arise?

So my proposed patch handles that no ?

With that patch, we do the set_dec(1) in two cases:

- The existing arch_irq_work_raise() which is unchanged

- At the end of __timer_interrupt() if an irq work is still pending

And the patch also makes decrementer_set_next_event() not modify the decrementer if an irq work is pending, but *still* adjust next_tb unlike what the code does now.

Thus the timer interrupt, when it happens, will re-adjust the dec properly using next_tb.

Do we still miss a case ?

Cheers,
Ben.

Quoted 69 lines

Regards
Preeti U Murthy

Quoted 67 lines

static int decrementer_set_next_event(unsigned long evt,
struct clock_event_device *dev)

{
	__get_cpu_var(decrementers_next_tb) = get_tb_or_rtc() + evt;

	/* Don't adjust the decrementer if some irq work is pending */
	if (!test_irq_work_pending())
		set_dec(evt);

	return 0;
}

Along with a single occurrence of:

	if (test_irq_work_pending())
		set_dec(1);

At the end of __timer_interrupt(), outside if the current else {} case, this should work, don't you think ?

What about this completely untested patch ?

arch/powerpc/kernel/time.c +7 -10

index 122a580..ba7e83b 100644
@@ -503,12 +503,13 @@ void __timer_interrupt(void)
                now = *next_tb - now;
                if (now <= DECREMENTER_MAX)
                        set_dec((int)now);
-               /* We may have raced with new irq work */
-               if (test_irq_work_pending())
-                       set_dec(1);
                __get_cpu_var(irq_stat).timer_irqs_others++;
        }
 
+       /* We may have raced with new irq work */
+       if (test_irq_work_pending())
+               set_dec(1);
+
 #ifdef CONFIG_PPC64
        /* collect purr register values often, for accurate calculations */
        if (firmware_has_feature(FW_FEATURE_SPLPAR)) {

@@ -813,15 +814,11 @@ static void __init clocksource_init(void)
 static int decrementer_set_next_event(unsigned long evt,
                                      struct clock_event_device *dev)
 {
-       /* Don't adjust the decrementer if some irq work is pending */
-       if (test_irq_work_pending())
-               return 0;
        __get_cpu_var(decrementers_next_tb) = get_tb_or_rtc() + evt;
-       set_dec(evt);
 
-       /* We may have raced with new irq work */
-       if (test_irq_work_pending())
-               set_dec(1);
+       /* Don't adjust the decrementer if some irq work is pending */
+       if (!test_irq_work_pending())
+               set_dec(evt);
 
        return 0;
 }

+ /* Don't adjust the decrementer if some irq work is pending
*/
+ if (!test_irq_work_pending())
+ set_dec(evt);
+ else
+ set_dec(1);

^^^^^ your patch currently does not have this
explicit
set_dec(1) here. Will that create a problem?

If there is any irq work pending at this point, will someone set the decrementer to fire immediately after this point? The current code in decrementer_set_next_event() sets set_dec(1) explicitly in case of pending irq work.

Quoted 7 lines · Preeti U Murthy

+ /* Don't adjust the decrementer if some irq work is pending
*/
+ if (!test_irq_work_pending())
+ set_dec(evt);
+ else
+ set_dec(1);

Quoted 8 lines

^^^^^ your patch currently does not have this
explicit
set_dec(1) here. Will that create a problem?

If there is any irq work pending at this point, will someone set the decrementer to fire immediately after this point? The current code in decrementer_set_next_event() sets set_dec(1) explicitly in case of pending irq work.

Hrm, actually this is an interesting point. The problem isn't that *someone* will do a set_dec, nobody else should that matters.

The problem is that irq_work can be triggered typically by NMIs or similar, which means that it might be queued between the test_irq_work_pending() and the set_dec(), thus causing a race.

So basically Anton's original patch is fine :-) I had missed that we did a post-set_dec() test already in decrementer_next_event() so as far as I can tell, removing the pre-test, which is what Anton does, is really all we need.

Cheers,
Ben.

Isn't this patch required too?

arch/powerpc/kernel/time.c +4 -3

@@ -503,12 +503,13 @@ void __timer_interrupt(void)
                now = *next_tb - now;
                if (now <= DECREMENTER_MAX)
                        set_dec((int)now);
-               /* We may have raced with new irq work */
-               if (test_irq_work_pending())
-                       set_dec(1);
                __get_cpu_var(irq_stat).timer_irqs_others++;
        }
 
+       /* We may have raced with new irq work */
+       if (test_irq_work_pending())
+               set_dec(1);
+
 

The event_handler cannot be relied upon to call decrementer_set_next_event() all the time. This is in the case where there are no pending timers. In that case we need to have the check on irq work pending at the end of __timer_interrupt() no?

Quoted 18 lines · Preeti U Murthy

Isn't this patch required too?

(quoted diff) +4 -3

@@ -503,12 +503,13 @@ void __timer_interrupt(void)
                now = *next_tb - now;
                if (now <= DECREMENTER_MAX)
                        set_dec((int)now);
-               /* We may have raced with new irq work */
-               if (test_irq_work_pending())
-                       set_dec(1);
                __get_cpu_var(irq_stat).timer_irqs_others++;
        }
 
+       /* We may have raced with new irq work */
+       if (test_irq_work_pending())
+               set_dec(1);
+
 

Quoted 4 lines

The event_handler cannot be relied upon to call decrementer_set_next_event() all the time. This is in the case where there are no pending timers. In that case we need to have the check on irq work pending at the end of __timer_interrupt() no?

I don't think we need to move the test no. If there's a pending irq_work, at that point, it will have done set_dec when being queued up. So we only care about cases where we might change the decrementer.

If the event handler doesn't call decrementer_set_next_event() then nothing will modify the decrementer and it will still trigger soon.

Cheers,
Ben.

I am seeing an issue where a CPU running perf eventually hangs. Traces show timer interrupts happening every 4 seconds even when a userspace task is running on the CPU.

/proc/timer_list
also shows pending hrtimers have not run in over an hour, including the scheduler.

Looking closer, decrementers_next_tb is getting set to 0xffffffffffffffff, and at that point we will never take a timer interrupt again.

In __timer_interrupt() we set decrementers_next_tb to 0xffffffffffffffff and rely on ->event_handler to update it:

        *next_tb = ~(u64)0;
        if (evt->event_handler)
                evt->event_handler(evt);

In this case ->event_handler is hrtimer_interrupt. This will eventually call back through the clockevents code with the next event to be programmed:

static int decrementer_set_next_event(unsigned long evt,
struct clock_event_device *dev)

{
        /* Don't adjust the decrementer if some irq work is pending */
        if (test_irq_work_pending())
                return 0;
        __get_cpu_var(decrementers_next_tb) = get_tb_or_rtc() + evt;

If irq work came in between these two points, we will return before updating decrementers_next_tb and we never process a timer interrupt again.

This looks to have been introduced by 0215f7d8c53f (powerpc: Fix races with irq_work). Fix it by removing the early exit and relying on code later on in the function to force an early decrementer:

       /* We may have raced with new irq work */
       if (test_irq_work_pending())
               set_dec(1);

Signed-off-by Anton Blanchard <[email protected]>

Cc [email protected] # 3.14+

---

arch/powerpc/kernel/time.c +0 -3

index 122a580..4f0b676 100644
@@ -813,9 +888,6 @@ static void __init clocksource_init(void)
 static int decrementer_set_next_event(unsigned long evt,
 				      struct clock_event_device *dev)
 {
-	/* Don't adjust the decrementer if some irq work is pending */
-	if (test_irq_work_pending())
-		return 0;
 	__get_cpu_var(decrementers_next_tb) = get_tb_or_rtc() + evt;
 	set_dec(evt);
 

Quoted 3 lines · Anton Blanchard

I am seeing an issue where a CPU running perf eventually hangs. Traces show timer interrupts happening every 4 seconds even when a userspace task is running on the CPU.

Is this by chance every 4.2 seconds? The reason I ask is that Paul Clarke and I are seeing an interrupt every 4.2 seconds when he runs NO_HZ_FULL, and are trying to get rid of it. ;-)

Quoted 8 lines · Paul E. McKenney

Quoted 3 lines · Anton Blanchard

I am seeing an issue where a CPU running perf eventually hangs. Traces show timer interrupts happening every 4 seconds even when a userspace task is running on the CPU.

Is this by chance every 4.2 seconds? The reason I ask is that Paul Clarke and I are seeing an interrupt every 4.2 seconds when he runs NO_HZ_FULL, and are trying to get rid of it. ;-)

Hmmm, it's close to 2^32 nanoseconds, isnt't it suspiscious?

Quoted 10 lines · Gabriel Paubert

Quoted 8 lines · Paul E. McKenney

Quoted 3 lines · Anton Blanchard

I am seeing an issue where a CPU running perf eventually hangs. Traces show timer interrupts happening every 4 seconds even when a userspace task is running on the CPU.

Is this by chance every 4.2 seconds? The reason I ask is that Paul Clarke and I are seeing an interrupt every 4.2 seconds when he runs NO_HZ_FULL, and are trying to get rid of it. ;-)

Quoted 1 line

Hmmm, it's close to 2^32 nanoseconds, isnt't it suspiscious?

Now that you mention it... ;-)

So you are telling me that we are not succeeding in completely turning off the decrementer interrupt?

Quoted 11 lines · Paul E. McKenney

Quoted 10 lines · Gabriel Paubert

Quoted 8 lines · Paul E. McKenney

Quoted 3 lines · Anton Blanchard

I am seeing an issue where a CPU running perf eventually hangs. Traces show timer interrupts happening every 4 seconds even when a userspace task is running on the CPU.

Is this by chance every 4.2 seconds? The reason I ask is that Paul Clarke and I are seeing an interrupt every 4.2 seconds when he runs NO_HZ_FULL, and are trying to get rid of it. ;-)

Quoted 6 lines

Quoted 1 line

Hmmm, it's close to 2^32 nanoseconds, isnt't it suspiscious?

Now that you mention it... ;-)

So you are telling me that we are not succeeding in completely turning off the decrementer interrupt?

There is no way to turn off the decrementer interrupt without turning off external (device) interrupts.

On IBM Power CPUs since POWER6, the decrementer runs at 512MHz. If you set the decrementer to 0x7fffffff it will interrupt in 4.194 seconds, so that would be what you're seeing. The only way to avoid the interrupt becoming pending is to keep on setting it to a large value before it gets to -1.

If an interrupt every 4.2 seconds is a problem in some applications, then we need to talk to the Power architects.