Re: [PATCH v2] tracing: Don't account for cpu idle time with irqsoff tracers

[Steven Rostedt]

On Tue, 27 May 2014 15:21:39 -0700
Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote:


> > Arnd brings up a good point. 
> 
> Hrm.. still not getting Arnd's mails.

Strange. What mail service do you have. Could they be blocking him?

> 
> > If we disable irqs off tracing completely,
> > we may be missing places in the idle path that disable interrupts for
> > long periods of time. We may want to move the stop down further.
> >
> > The way it works (IIRC), and why tracing can start again is that it can
> > nest. Perhaps we need to stop it further down if we can't move it
> > completely.
> >
> 
> I'm not sure how much deeper it can go and I'm afraid it will become a
> game of whack-a-mole. I already see two places that disable and reenable
> irqs after stop_critical_timings() is called (first in rcu_idle_enter()
> and second in clockevents_notify()). Should rcu_idle_enter() move to
> raw_local_irq_save()? It looks like that path calls rcu_sched_qs() and
> on tiny RCU that again needs the raw_ treatement. We can probably call
> stop_critical_timings() after rcu_idle_enter() to fix this.

I don't think we need to whack-a-mole. The start stop should be around
where it goes to sleep.

> 
> What about clockevents_notify? __raw_spin_lock_irqsave() should probably
> use raw_local_irqsave().

No that solution is even worse. We need lockdep working here.

> 
> If we go the raw route, do we even need stop/start_critical_timings()?
> Can't we just use raw accessors in the idle paths
> (tick_nohz_idle_{enter,exit}(), cpuidle_enter(), etc.) and get rid of
> the stop/start stuff completely? I admit this patch is pretty much a big
> sledge hammer that tries to make things simple, but if there is some
> benefit to the raw accessors I'm willing to send patches to fix all the
> call sites.
> 

How about the following. I don't see any reason stop_critical_timings()
can't be called from within rcu_idle code, as it doesn't use any rcu.

Paul, Peter, see anything wrong with this?

-- Steve

diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 8f4390a..f5e6a64 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -88,12 +88,6 @@ static int cpuidle_idle_call(void)
 	}
 
 	/*
-	 * During the idle period, stop measuring the disabled irqs
-	 * critical sections latencies
-	 */
-	stop_critical_timings();
-
-	/*
 	 * Tell the RCU framework we are entering an idle section,
 	 * so no more rcu read side critical sections and one more
 	 * step to the grace period
@@ -144,6 +138,12 @@ static int cpuidle_idle_call(void)
 				trace_cpu_idle_rcuidle(next_state, dev->cpu);
 
 				/*
+				 * During the idle period, stop measuring the
+				 * disabled irqs critical sections latencies
+				 */
+				stop_critical_timings();
+
+				/*
 				 * Enter the idle state previously
 				 * returned by the governor
 				 * decision. This function will block
@@ -154,6 +154,8 @@ static int cpuidle_idle_call(void)
 				entered_state = cpuidle_enter(drv, dev,
 							      next_state);
 
+				start_critical_timings();
+
 				trace_cpu_idle_rcuidle(PWR_EVENT_EXIT,
 						       dev->cpu);
 
@@ -175,8 +177,11 @@ static int cpuidle_idle_call(void)
 	 * We can't use the cpuidle framework, let's use the default
 	 * idle routine
 	 */
-	if (ret)
+	if (ret) {
+		stop_critical_timings();
 		arch_cpu_idle();
+		start_critical_timings();
+	}
 
 	__current_set_polling();
 
@@ -188,7 +193,6 @@ static int cpuidle_idle_call(void)
 		local_irq_enable();
 
 	rcu_idle_exit();
-	start_critical_timings();
 
 	return 0;
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/