LinuxLists.cc - [PATCH RT 1/3] alarmtimer: Prevent live lock in alarm

2018-03-28 10:10:07

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

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: [email protected]
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
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

2018-03-28 10:09:31

by Sebastian Andrzej Siewior

[permalink] [raw]

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

2018-03-28 10:29:36

by Sebastian Andrzej Siewior

[permalink] [raw]

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

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: [email protected]
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
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

2018-03-28 10:31:59

by Sebastian Andrzej Siewior

[permalink] [raw]

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

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: [email protected]
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
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

2018-03-28 10:56:05

by Sebastian Andrzej Siewior

[permalink] [raw]

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

2018-04-03 18:34:09

by Daniel Wagner

[permalink] [raw]

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

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: [email protected]

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

2018-04-04 07:38:44

by Sebastian Andrzej Siewior

[permalink] [raw]

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

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: [email protected]
>
> 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