[PATCH] tick: Fix hang caused by hrtimer in broadcast mode

[PATCH] tick: Fix hang caused by hrtimer in broadcast mode

[Andreas Sandberg]

The hrtimer callback in the hrtimer's tick broadcast code sometimes
incorrectly ends up scheduling events at the current tick causing the
kernel to hang servicing the same hrtimer forever. This typically
happens when a device is swapped out by
tick_install_broadcast_device(), which replaces the event handler with
clock_events_handle_noop() and sets the device mode to
CLOCK_EVT_MODE_UNUSED. If the timer is scheduled when this happens,
the next_event field will not be updated and the hrtimer ends up being
restarted at the current tick. To prevent this from happening, only
try to restart the hrtimer if the broadcast clock event device is in
one of the active modes and try to cancel the timer when entering the
CLOCK_EVT_MODE_UNUSED mode.

Signed-off-by: Andreas Sandberg <andreas.sandberg@arm.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Catalin Marinas <catalin.marinas@arm.com>
---
 kernel/time/tick-broadcast-hrtimer.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
index 6aac4be..a20c605 100644
--- a/kernel/time/tick-broadcast-hrtimer.c
+++ b/kernel/time/tick-broadcast-hrtimer.c
@@ -22,6 +22,7 @@ static void bc_set_mode(enum clock_event_mode mode,
 			struct clock_event_device *bc)
 {
 	switch (mode) {
+	case CLOCK_EVT_MODE_UNUSED:
 	case CLOCK_EVT_MODE_SHUTDOWN:
 		/*
 		 * Note, we cannot cancel the timer here as we might
@@ -99,10 +100,14 @@ static enum hrtimer_restart bc_handler(struct hrtimer *t)
 {
 	ce_broadcast_hrtimer.event_handler(&ce_broadcast_hrtimer);
 
-	if (ce_broadcast_hrtimer.next_event.tv64 == KTIME_MAX)
+	switch (ce_broadcast_hrtimer.mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+	case CLOCK_EVT_MODE_ONESHOT:
+		if (ce_broadcast_hrtimer.next_event.tv64 != KTIME_MAX)
+			return HRTIMER_RESTART;
+	default:
 		return HRTIMER_NORESTART;
-
-	return HRTIMER_RESTART;
+	}
 }
 
 void tick_setup_hrtimer_broadcast(void)
-- 
2.1.4

Re: [PATCH] tick: Fix hang caused by hrtimer in broadcast mode

[Preeti U Murthy]

On 04/24/2015 06:36 PM, Andreas Sandberg wrote:
> The hrtimer callback in the hrtimer's tick broadcast code sometimes
> incorrectly ends up scheduling events at the current tick causing the
> kernel to hang servicing the same hrtimer forever. This typically
> happens when a device is swapped out by
> tick_install_broadcast_device(), which replaces the event handler with
> clock_events_handle_noop() and sets the device mode to
> CLOCK_EVT_MODE_UNUSED. If the timer is scheduled when this happens,
> the next_event field will not be updated and the hrtimer ends up being
> restarted at the current tick. To prevent this from happening, only
> try to restart the hrtimer if the broadcast clock event device is in
> one of the active modes and try to cancel the timer when entering the
> CLOCK_EVT_MODE_UNUSED mode.
> 
> Signed-off-by: Andreas Sandberg <andreas.sandberg@arm.com>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  kernel/time/tick-broadcast-hrtimer.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
> index 6aac4be..a20c605 100644
> --- a/kernel/time/tick-broadcast-hrtimer.c
> +++ b/kernel/time/tick-broadcast-hrtimer.c
> @@ -22,6 +22,7 @@ static void bc_set_mode(enum clock_event_mode mode,
>  			struct clock_event_device *bc)
>  {
>  	switch (mode) {
> +	case CLOCK_EVT_MODE_UNUSED:
>  	case CLOCK_EVT_MODE_SHUTDOWN:
>  		/*
>  		 * Note, we cannot cancel the timer here as we might
> @@ -99,10 +100,14 @@ static enum hrtimer_restart bc_handler(struct hrtimer *t)
>  {
>  	ce_broadcast_hrtimer.event_handler(&ce_broadcast_hrtimer);
>  
> -	if (ce_broadcast_hrtimer.next_event.tv64 == KTIME_MAX)
> +	switch (ce_broadcast_hrtimer.mode) {
> +	case CLOCK_EVT_MODE_PERIODIC:
> +	case CLOCK_EVT_MODE_ONESHOT:
> +		if (ce_broadcast_hrtimer.next_event.tv64 != KTIME_MAX)
> +			return HRTIMER_RESTART;
> +	default:
>  		return HRTIMER_NORESTART;
> -
> -	return HRTIMER_RESTART;
> +	}
>  }
>  
>  void tick_setup_hrtimer_broadcast(void)
> 
Looks good.

Reviewed-by: Preeti U. Murthy <preeti@linux.vnet.ibm.com>

Re: [PATCH] tick: Fix hang caused by hrtimer in broadcast mode

[Thomas Gleixner]

On Fri, 24 Apr 2015, Andreas Sandberg wrote:

> The hrtimer callback in the hrtimer's tick broadcast code sometimes
> incorrectly ends up scheduling events at the current tick causing the
> kernel to hang servicing the same hrtimer forever. This typically
> happens when a device is swapped out by
> tick_install_broadcast_device(), which replaces the event handler with
> clock_events_handle_noop() and sets the device mode to
> CLOCK_EVT_MODE_UNUSED. If the timer is scheduled when this happens,
> the next_event field will not be updated and the hrtimer ends up being
> restarted at the current tick. To prevent this from happening, only
> try to restart the hrtimer if the broadcast clock event device is in
> one of the active modes and try to cancel the timer when entering the
> CLOCK_EVT_MODE_UNUSED mode.
> 
> Signed-off-by: Andreas Sandberg <andreas.sandberg@arm.com>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  kernel/time/tick-broadcast-hrtimer.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
> index 6aac4be..a20c605 100644
> --- a/kernel/time/tick-broadcast-hrtimer.c
> +++ b/kernel/time/tick-broadcast-hrtimer.c
> @@ -22,6 +22,7 @@ static void bc_set_mode(enum clock_event_mode mode,
>  			struct clock_event_device *bc)
>  {
>  	switch (mode) {
> +	case CLOCK_EVT_MODE_UNUSED:
>  	case CLOCK_EVT_MODE_SHUTDOWN:
>  		/*
>  		 * Note, we cannot cancel the timer here as we might
> @@ -99,10 +100,14 @@ static enum hrtimer_restart bc_handler(struct hrtimer *t)
>  {
>  	ce_broadcast_hrtimer.event_handler(&ce_broadcast_hrtimer);
>  
> -	if (ce_broadcast_hrtimer.next_event.tv64 == KTIME_MAX)
> +	switch (ce_broadcast_hrtimer.mode) {
> +	case CLOCK_EVT_MODE_PERIODIC:

That case is pointless. The mode cannot be periodic simply because
.features lacks the periodic flag.

Thanks,

	tglx