LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Frederic Weisbecker <frederic@kernel.org>
To: bigeasy@linutronix.de, peterz@infradead.org, mingo@kernel.org,
	linux-kernel@vger.kernel.org, anna-maria@linutronix.de,
	tglx@linutronix.de, hpa@zytor.com
Cc: linux-tip-commits@vger.kernel.org
Subject: Re: [tip:timers/core] hrtimer: Prepare support for PREEMPT_RT
Date: Tue, 20 Aug 2019 15:26:57 +0200
Message-ID: <20190820132656.GC2093@lenoir> (raw)
In-Reply-To: <tip-f61eff83cec9cfab31fd30a2ca8856be379cdcd5@git.kernel.org>

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@linutronix.de>
> AuthorDate: Fri, 26 Jul 2019 20:30:59 +0200
> Committer:  Thomas Gleixner <tglx@linutronix.de>
> 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@kernel.org>
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@kernel.org>
---
 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




  reply index

Thread overview: 61+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-26 18:30 [patch 00/12] (hr)timers: Prepare for PREEMPT_RT support Thomas Gleixner
2019-07-26 18:30 ` [patch 01/12] hrtimer: Remove task argument from hrtimer_init_sleeper() Thomas Gleixner
2019-07-26 19:57   ` Steven Rostedt
2019-07-26 20:01     ` Thomas Gleixner
2019-07-30 22:07   ` [tip:timers/core] " tip-bot for Thomas Gleixner
2019-07-26 18:30 ` [patch 02/12] hrtimer: Consolidate hrtimer_init() + hrtimer_init_sleeper() calls Thomas Gleixner
2019-07-30 22:08   ` [tip:timers/core] " tip-bot for Sebastian Andrzej Siewior
2019-08-01 15:49   ` tip-bot for Sebastian Andrzej Siewior
2019-07-26 18:30 ` [patch 03/12] hrtimer: Introduce HARD expiry mode Thomas Gleixner
2019-07-30 22:10   ` [tip:timers/core] " tip-bot for Sebastian Andrzej Siewior
2019-08-01 15:52   ` tip-bot for Sebastian Andrzej Siewior
2019-07-26 18:30 ` [patch 04/12] sched: Mark hrtimers to expire in hard interrupt context Thomas Gleixner
2019-07-30 22:11   ` [tip:timers/core] " tip-bot for Thomas Gleixner
2019-08-01 15:53   ` tip-bot for Sebastian Andrzej Siewior
2019-08-01 18:58   ` tip-bot for Sebastian Andrzej Siewior
2019-07-26 18:30 ` [patch 05/12] perf/core: " Thomas Gleixner
2019-07-30 22:12   ` [tip:timers/core] " tip-bot for Thomas Gleixner
2019-08-01 15:54   ` tip-bot for Sebastian Andrzej Siewior
2019-08-01 18:59   ` tip-bot for Sebastian Andrzej Siewior
2019-07-26 18:30 ` [patch 06/12] watchdog: Mark watchdog_hrtimer " Thomas Gleixner
2019-07-30 22:13   ` [tip:timers/core] " tip-bot for Sebastian Andrzej Siewior
2019-08-01 15:55   ` tip-bot for Sebastian Andrzej Siewior
2019-08-01 19:00   ` tip-bot for Sebastian Andrzej Siewior
2019-07-26 18:30 ` [patch 07/12] KVM: LAPIC: Mark hrtimer " Thomas Gleixner
2019-07-26 19:41   ` Paolo Bonzini
2019-07-30 22:14   ` [tip:timers/core] " tip-bot for Sebastian Andrzej Siewior
2019-08-01 15:55   ` tip-bot for Sebastian Andrzej Siewior
2019-08-01 19:01   ` tip-bot for Sebastian Andrzej Siewior
2019-07-26 18:30 ` [patch 08/12] tick: Mark tick related hrtimers to expiry " Thomas Gleixner
2019-07-30 22:14   ` [tip:timers/core] " tip-bot for Sebastian Andrzej Siewior
2019-08-01 15:56   ` tip-bot for Sebastian Andrzej Siewior
2019-08-01 19:01   ` tip-bot for Sebastian Andrzej Siewior
2019-07-26 18:30 ` [patch 09/12] hrtimer: Move unmarked hrtimers to soft interrupt expiry on RT Thomas Gleixner
2019-07-30 22:15   ` [tip:timers/core] " tip-bot for Sebastian Andrzej Siewior
2019-08-01 15:57   ` tip-bot for Sebastian Andrzej Siewior
2019-08-01 19:02   ` tip-bot for Sebastian Andrzej Siewior
2019-07-26 18:30 ` [patch 10/12] hrtimer: Determine hard/soft expiry mode for hrtimer sleepers " Thomas Gleixner
2019-07-26 20:44   ` Steven Rostedt
2019-07-26 20:52     ` Thomas Gleixner
2019-07-26 20:56       ` Steven Rostedt
2019-07-26 21:16   ` Julia Cartwright
2019-07-26 21:30     ` Steven Rostedt
2019-07-26 21:35     ` Thomas Gleixner
2019-07-30 22:16   ` [tip:timers/core] " tip-bot for Sebastian Andrzej Siewior
2019-08-01 15:58   ` tip-bot for Sebastian Andrzej Siewior
2019-08-01 19:03   ` tip-bot for Sebastian Andrzej Siewior
2019-07-26 18:30 ` [patch 11/12] hrtimer: Prepare support for PREEMPT_RT Thomas Gleixner
2019-07-28  9:06   ` Juergen Gross
2019-07-29 15:08     ` Steven Rostedt
2019-07-29 17:30       ` Paolo Bonzini
2019-07-31  8:45         ` Juergen Gross
2019-07-30 22:17   ` [tip:timers/core] " tip-bot for Anna-Maria Gleixner
2019-08-01 15:58   ` tip-bot for Anna-Maria Gleixner
2019-08-01 19:04   ` tip-bot for Anna-Maria Gleixner
2019-08-20 13:26     ` Frederic Weisbecker [this message]
2019-08-23  2:12       ` [tip: timers/core] hrtimer: Improve comments on handling priority inversion against softirq kthread tip-bot2 for Frederic Weisbecker
2019-07-26 18:31 ` [patch 12/12] timers: Prepare support for PREEMPT_RT Thomas Gleixner
2019-07-30 22:17   ` [tip:timers/core] " tip-bot for Anna-Maria Gleixner
2019-08-01 15:59   ` tip-bot for Anna-Maria Gleixner
2019-08-01 19:04   ` tip-bot for Anna-Maria Gleixner
2019-07-29 19:45 ` [patch 00/12] (hr)timers: Prepare for PREEMPT_RT support Peter Zijlstra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190820132656.GC2093@lenoir \
    --to=frederic@kernel.org \
    --cc=anna-maria@linutronix.de \
    --cc=bigeasy@linutronix.de \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-tip-commits@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git