On Thu, Aug 01, 2019 at 12:04:03PM -0700, tip-bot for Anna-Maria Gleixner wrote:
> Commit-ID: f61eff83cec9cfab31fd30a2ca8856be379cdcd5
> Gitweb: https://git.kernel.org/tip/f61eff83cec9cfab31fd30a2ca8856be379cdcd5
> Author: Anna-Maria Gleixner <anna-maria@xxxxxxxxxxxxx>
> AuthorDate: Fri, 26 Jul 2019 20:30:59 +0200
> Committer: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> CommitDate: Thu, 1 Aug 2019 20:51:22 +0200
>
> hrtimer: Prepare support for PREEMPT_RT
>
> When PREEMPT_RT is enabled, the soft interrupt thread can be preempted. If
> the soft interrupt thread is preempted in the middle of a timer callback,
> then calling hrtimer_cancel() can lead to two issues:
>
> - If the caller is on a remote CPU then it has to spin wait for the timer
> handler to complete. This can result in unbound priority inversion.
>
> - If the caller originates from the task which preempted the timer
> handler on the same CPU, then spin waiting for the timer handler to
> complete is never going to end.
[...]
> +/*
> + * This function is called on PREEMPT_RT kernels when the fast path
> + * deletion of a timer failed because the timer callback function was
> + * running.
> + *
> + * This prevents priority inversion, if the softirq thread on a remote CPU
> + * got preempted, and it prevents a life lock when the task which tries to
> + * delete a timer preempted the softirq thread running the timer callback
> + * function.
> + */
> +void hrtimer_cancel_wait_running(const struct hrtimer *timer)
> +{
> + struct hrtimer_clock_base *base = timer->base;
> +
> + if (!timer->is_soft || !base || !base->cpu_base) {
> + cpu_relax();
> + return;
> + }
> +
> + /*
> + * Mark the base as contended and grab the expiry lock, which is
> + * held by the softirq across the timer callback. Drop the lock
> + * immediately so the softirq can expire the next timer. In theory
> + * the timer could already be running again, but that's more than
> + * unlikely and just causes another wait loop.
> + */
> + atomic_inc(&base->cpu_base->timer_waiters);
> + spin_lock_bh(&base->cpu_base->softirq_expiry_lock);
> + atomic_dec(&base->cpu_base->timer_waiters);
> + spin_unlock_bh(&base->cpu_base->softirq_expiry_lock);
> +}
So, while reviewing the posix timers series, I stumbled upon timer_wait_running() which
lacked any explanation, which led me to hrtimer_cancel_wait_running() that was
a bit more helpful but still had blurry explanation.
In the end I found the approrpiate infomation in this commit changelog.
It might be helpful for future reviewers to apply this:
---
>From ef9a4d87b6e7c43899248c376c5959f4e0bcd309 Mon Sep 17 00:00:00 2001
From: Frederic Weisbecker <frederic@xxxxxxxxxx>
Date: Tue, 20 Aug 2019 15:12:23 +0200
Subject: [PATCH] hrtimer: Improve comments on handling priority inversion
against softirq kthread
The handling of a priority inversion between timer cancelling and a
a not well defined possible preemption of softirq kthread is not very
clear. Especially in the posix timers side where we don't even know why
there is a specific RT wait callback.
All the nice explanations can be found in the initial changelog of
f61eff83cec9cfab31fd30a2ca8856be379cdcd5
(hrtimer: Prepare support for PREEMPT_RT"). So lets extract the detailed
informations from there.
Signed-off-by: Frederic Weisbecker <frederic@xxxxxxxxxx>
---
kernel/time/hrtimer.c | 14 ++++++++++----
kernel/time/posix-timers.c | 5 +++++
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 499122752649..833353732554 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1201,10 +1201,16 @@ static void hrtimer_sync_wait_running(struct hrtimer_cpu_base *cpu_base,
* deletion of a timer failed because the timer callback function was
* running.
*
- * This prevents priority inversion, if the softirq thread on a remote CPU
- * got preempted, and it prevents a life lock when the task which tries to
- * delete a timer preempted the softirq thread running the timer callback
- * function.
+ * This prevents priority inversion: if the soft irq thread is preempted
+ * in the middle of a timer callback, then calling del_timer_sync() can
+ * lead to two issues:
+ *
+ * - If the caller is on a remote CPU then it has to spin wait for the timer
+ * handler to complete. This can result in unbound priority inversion.
+ *
+ * - If the caller originates from the task which preempted the timer
+ * handler on the same CPU, then spin waiting for the timer handler to
+ * complete is never going to end.
*/
void hrtimer_cancel_wait_running(const struct hrtimer *timer)
{
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index a71c1aab071c..f6713a41e4e0 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -806,6 +806,11 @@ static int common_hrtimer_try_to_cancel(struct k_itimer *timr)
}
#ifdef CONFIG_PREEMPT_RT
+/*
+ * Prevent from priority inversion against softirq kthread in case
+ * it gets preempted while executing an htimer callback. See
+ * comments in hrtimer_cancel_wait_running.
+ */
static struct k_itimer *timer_wait_running(struct k_itimer *timer,
unsigned long *flags)
{
--
2.21.0
|