linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: linux-rt-users@vger.kernel.org, linux-kernel@vger.kernel.org,
	tglx@linutronix.de,
	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
	Clark Williams <williams@redhat.com>
Subject: Re: [PATCH 2/2] timer: really raise softirq if there is irq_work to do
Date: Fri, 31 Jan 2014 12:07:57 -0500	[thread overview]
Message-ID: <20140131120757.594e24d6@gandalf.local.home> (raw)
In-Reply-To: <1391178845-15837-2-git-send-email-bigeasy@linutronix.de>

On Fri, 31 Jan 2014 15:34:05 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> from looking at the code, it seems that the softirq is only raised (in
> the !base->active_timers case) if we have also an expired timer
> (time_before_eq() is true). This patch ensures that the timer softirq is
> also raised in the !base->active_timers && no timer expired.

A couple of things. If there is no active timers, we do not need to
check the expired timers. That may contain a deferred timer that does
not need to be raised if the system is idle. This will just
re-introduce the problems that other people have been seeing.

The bug that I found is that if there *are* active timers, but they
have not expired yet. Why is this a problem? Because in that case we do
not check if there is irq_work to be done. That means the irq_work will
have to wait till the timer expires, and since RCU depends on this,
that can take a while. I've had a synchronize_sched() take up to 5
seconds to complete due to this!


The real fix is the following:

timer/rt: Always raise the softirq if there's irq_work to be done

It was previously discovered that some systems would hang on boot up
with a previous version of 3.12-rt. This was due to RCU using irq_work,
and RT defers the irq_work to a softirq. But if there's no active
timers, the softirq will not be raised, and RCU work will not get done,
causing the system to hang.  The fix was to check that if there was no
active timers but irq_work to be done, then we should raise the softirq.

But this fix was not 100% correct. It left out the case that there were
active timers that were not expired yet. This would have the softirq
not get raised even if there was irq work to be done.

If there is irq_work to be done, then we must raise the timer softirq
regardless of if there is active timers or whether they are expired or
not. The softirq can handle those cases. But we can never ignore
irq_work.

As it is only PREEMPT_RT_FULL that requires irq_work to be done in the
softirq, we can pull out the check in the active_timers condition, and
make the code a bit cleaner by having the irq_work check separate, and
put the code in with the other #ifdef PREEMPT_RT. If there is irq_work
to be done, there's no need to check the active timers or if they are
expired. Just raise the time softirq and be done with it. Otherwise, we
can do the timer checks just like we do with non -rt.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/kernel/timer.c b/kernel/timer.c
index 106968f..426d114 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -1461,18 +1461,20 @@ void run_local_timers(void)
 	 * the timer softirq.
 	 */
 #ifdef CONFIG_PREEMPT_RT_FULL
+	/* On RT, irq work runs from softirq */
+	if (irq_work_needs_cpu()) {
+		raise_softirq(TIMER_SOFTIRQ);
+		return;
+	}
+
 	if (!spin_do_trylock(&base->lock)) {
 		raise_softirq(TIMER_SOFTIRQ);
 		return;
 	}
 #endif
-	if (!base->active_timers) {
-#ifdef CONFIG_PREEMPT_RT_FULL
-		/* On RT, irq work runs from softirq */
-		if (!irq_work_needs_cpu())
-#endif
-			goto out;
-	}
+
+	if (!base->active_timers)
+		goto out;
 
 	/* Check whether the next pending timer has expired */
 	if (time_before_eq(base->next_timer, jiffies))

  reply	other threads:[~2014-01-31 17:08 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-01-31 14:34 [PATCH 1/2] irq_work: allow certain work in hard irq context Sebastian Andrzej Siewior
2014-01-31 14:34 ` [PATCH 2/2] timer: really raise softirq if there is irq_work to do Sebastian Andrzej Siewior
2014-01-31 17:07   ` Steven Rostedt [this message]
2014-01-31 17:11     ` Steven Rostedt
2014-01-31 17:42     ` Paul E. McKenney
2014-01-31 17:57       ` Steven Rostedt
2014-01-31 19:03         ` Paul E. McKenney
2014-01-31 19:26         ` Sebastian Andrzej Siewior
2014-01-31 19:34           ` Steven Rostedt
2014-01-31 19:48             ` Sebastian Andrzej Siewior
2014-01-31 19:56               ` Steven Rostedt
2014-01-31 20:05               ` Peter Zijlstra
2014-01-31 20:23                 ` Sebastian Andrzej Siewior
2014-01-31 20:29                   ` Peter Zijlstra
2014-01-31 19:54             ` Peter Zijlstra
2014-01-31 19:06     ` Sebastian Andrzej Siewior
2014-02-02  4:22 ` [PATCH 1/2] irq_work: allow certain work in hard irq context Mike Galbraith
2014-02-02 20:10   ` Sebastian Andrzej Siewior
2014-02-03  2:43     ` Mike Galbraith
2014-02-03  4:00     ` Mike Galbraith
2014-02-03  8:31       ` Sebastian Andrzej Siewior
2014-02-03  9:26         ` Mike Galbraith

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=20140131120757.594e24d6@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --cc=bigeasy@linutronix.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rt-users@vger.kernel.org \
    --cc=paulmck@linux.vnet.ibm.com \
    --cc=tglx@linutronix.de \
    --cc=williams@redhat.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).