[PATCH RT 1/3] alarmtimer: Prevent live lock in alarm_cancel()

[PATCH RT 1/3] alarmtimer: Prevent live lock in alarm_cancel()

[Sebastian Andrzej Siewior]

If alarm_try_to_cancel() requires a retry, then depending on the
priority setting the retry loop might prevent timer callback completion
on RT. Prevent that by waiting for completion on RT, no change for a
non RT kernel.

Cc: stable-rt@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/time/alarmtimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index ec09ce9a6012..ede5ef787865 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -429,7 +429,7 @@ int alarm_cancel(struct alarm *alarm)
 		int ret = alarm_try_to_cancel(alarm);
 		if (ret >= 0)
 			return ret;
-		cpu_relax();
+		hrtimer_wait_for_timer(&alarm->timer);
 	}
 }
 EXPORT_SYMBOL_GPL(alarm_cancel);
-- 
2.16.3

[PATCH RT 2/3] posix-timers: user proper timer while waiting for alarmtimer

[Sebastian Andrzej Siewior]

On RT the timer can be preempted while running and therefore we wait
with timer_wait_for_callback() for the timer to complete (instead of
busy looping).
The POSIX timer has an hrtimer underneath and this hrtimer is used to
wait for its completion. The alartimer has also an hrtimer but at a
different location.

Instead of checking for ->timer_set which is the same for the alarmtimer
and the "posix-timers" I instead check for ->arm which is only used by
the "posix-timers". To match the alarmtimer I check for the alarm_clock
struct.

Cc: stable-rt@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/time/posix-timers.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 0b568087bd64..8a16cb8f684a 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -802,8 +802,10 @@ SYSCALL_DEFINE1(timer_getoverrun, timer_t, timer_id)
 static void timer_wait_for_callback(const struct k_clock *kc, struct k_itimer *timr)
 {
 #ifdef CONFIG_PREEMPT_RT_FULL
-	if (kc->timer_set == common_timer_set)
+	if (kc->arm == common_hrtimer_arm)
 		hrtimer_wait_for_timer(&timr->it.real.timer);
+	else if (kc == alarm_clock)
+		hrtimer_wait_for_timer(&timr->it.alarm.alarmtimer.timer);
 	else
 		/* FIXME: Whacky hack for posix-cpu-timers */
 		schedule_timeout(1);
-- 
2.16.3

[PATCH RT 2/3 v2] posix-timers: user proper timer while waiting for alarmtimer

[Sebastian Andrzej Siewior]

On RT the timer can be preempted while running and therefore we wait
with timer_wait_for_callback() for the timer to complete (instead of
busy looping).
The POSIX timer has an hrtimer underneath and this hrtimer is used to
wait for its completion. The alartimer has also an hrtimer but at a
different location.

Instead of checking for ->timer_set which is the same for the alarmtimer
and the "posix-timers" I instead check for ->arm which is only used by
the "posix-timers". To match the alarmtimer I check for the alarm_clock
struct.

Cc: stable-rt@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: move timer_wait_for_callback() so that it actually compiles.

 kernel/time/posix-timers.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 0b568087bd64..3abd449a926f 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -796,20 +796,6 @@ SYSCALL_DEFINE1(timer_getoverrun, timer_t, timer_id)
 	return overrun;
 }
 
-/*
- * Protected by RCU!
- */
-static void timer_wait_for_callback(const struct k_clock *kc, struct k_itimer *timr)
-{
-#ifdef CONFIG_PREEMPT_RT_FULL
-	if (kc->timer_set == common_timer_set)
-		hrtimer_wait_for_timer(&timr->it.real.timer);
-	else
-		/* FIXME: Whacky hack for posix-cpu-timers */
-		schedule_timeout(1);
-#endif
-}
-
 static void common_hrtimer_arm(struct k_itimer *timr, ktime_t expires,
 			       bool absolute, bool sigev_none)
 {
@@ -840,6 +826,22 @@ static void common_hrtimer_arm(struct k_itimer *timr, ktime_t expires,
 		hrtimer_start_expires(timer, HRTIMER_MODE_ABS);
 }
 
+/*
+ * Protected by RCU!
+ */
+static void timer_wait_for_callback(const struct k_clock *kc, struct k_itimer *timr)
+{
+#ifdef CONFIG_PREEMPT_RT_FULL
+	if (kc->timer_arm == common_hrtimer_arm)
+		hrtimer_wait_for_timer(&timr->it.real.timer);
+	else if (kc == &alarm_clock)
+		hrtimer_wait_for_timer(&timr->it.alarm.alarmtimer.timer);
+	else
+		/* FIXME: Whacky hack for posix-cpu-timers */
+		schedule_timeout(1);
+#endif
+}
+
 static int common_hrtimer_try_to_cancel(struct k_itimer *timr)
 {
 	return hrtimer_try_to_cancel(&timr->it.real.timer);
-- 
2.16.3

[PATCH RT 3/3] posix-timers: move the rcu head out of the union

[Sebastian Andrzej Siewior]

On RT the timer can be preempted while running and therefore we wait
with timer_wait_for_callback() for the timer to complete (instead of
busy looping). The RCU-readlock is held to ensure that this posix timer
is not removed while we wait on it.
If the timer is removed then it invokes call_rcu() with a pointer that
is shared with the hrtimer because it is part of the same union.
In order to avoid any possible side effects I am moving the rcu pointer
out of the union.

Cc: stable-rt@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/posix-timers.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 672c4f32311e..4754eb4298b1 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -101,8 +101,8 @@ struct k_itimer {
 		struct {
 			struct alarm	alarmtimer;
 		} alarm;
-		struct rcu_head		rcu;
 	} it;
+	struct rcu_head		rcu;
 };
 
 void run_posix_cpu_timers(struct task_struct *task);
-- 
2.16.3

[PATCH RT 3/3 v2] posix-timers: move the rcu head out of the union

[Sebastian Andrzej Siewior]

On RT the timer can be preempted while running and therefore we wait
with timer_wait_for_callback() for the timer to complete (instead of
busy looping). The RCU-readlock is held to ensure that this posix timer
is not removed while we wait on it.
If the timer is removed then it invokes call_rcu() with a pointer that
is shared with the hrtimer because it is part of the same union.
In order to avoid any possible side effects I am moving the rcu pointer
out of the union.

Cc: stable-rt@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: add the lost hunk in posix-timers.c

 include/linux/posix-timers.h | 2 +-
 kernel/time/posix-timers.c   | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 672c4f32311e..4754eb4298b1 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -101,8 +101,8 @@ struct k_itimer {
 		struct {
 			struct alarm	alarmtimer;
 		} alarm;
-		struct rcu_head		rcu;
 	} it;
+	struct rcu_head		rcu;
 };
 
 void run_posix_cpu_timers(struct task_struct *task);
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 3abd449a926f..17d4eb49e7c3 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -470,7 +470,7 @@ static struct k_itimer * alloc_posix_timer(void)
 
 static void k_itimer_rcu_free(struct rcu_head *head)
 {
-	struct k_itimer *tmr = container_of(head, struct k_itimer, it.rcu);
+	struct k_itimer *tmr = container_of(head, struct k_itimer, rcu);
 
 	kmem_cache_free(posix_timers_cache, tmr);
 }
@@ -487,7 +487,7 @@ static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
 	}
 	put_pid(tmr->it_pid);
 	sigqueue_free(tmr->sigq);
-	call_rcu(&tmr->it.rcu, k_itimer_rcu_free);
+	call_rcu(&tmr->rcu, k_itimer_rcu_free);
 }
 
 static int common_timer_create(struct k_itimer *new_timer)
-- 
2.16.3

Re: [PATCH RT 1/3] alarmtimer: Prevent live lock in alarm_cancel()

[Daniel Wagner]

Hi Sebastian,

On 03/28/2018 12:07 PM, Sebastian Andrzej Siewior wrote:
> If alarm_try_to_cancel() requires a retry, then depending on the
> priority setting the retry loop might prevent timer callback completion
> on RT. Prevent that by waiting for completion on RT, no change for a
> non RT kernel.
> 
> Cc: stable-rt@vger.kernel.org

How relevant is this serie for trees before eae1c4ae275f ("posix-timers: 
Make use of cancel/arm callbacks")? Patch #2 seems to depend on that 
change which was added in v4.12.

Thanks,
Daniel

Re: [PATCH RT 1/3] alarmtimer: Prevent live lock in alarm_cancel()

[Sebastian Andrzej Siewior]

On 2018-04-03 20:32:36 [+0200], Daniel Wagner wrote:
> Hi Sebastian,
> 
> On 03/28/2018 12:07 PM, Sebastian Andrzej Siewior wrote:
> > If alarm_try_to_cancel() requires a retry, then depending on the
> > priority setting the retry loop might prevent timer callback completion
> > on RT. Prevent that by waiting for completion on RT, no change for a
> > non RT kernel.
> > 
> > Cc: stable-rt@vger.kernel.org
> 
> How relevant is this serie for trees before eae1c4ae275f ("posix-timers:
> Make use of cancel/arm callbacks")? Patch #2 seems to depend on that change
> which was added in v4.12.

so #1 looks relevant since that alarmtimer was introduced. #2 since the
change you mentioned because after that patch alarmtimer would be
handled wrongly - before you should do the schedule_timout() which
should be fine.  And #3 since 3.0+ (since the RCU-readsection was added)
but it is not easy to trigger (you would have to delete the timer while
a callback is waiting for it).

> Thanks,
> Daniel

Sebastian