lkml.org 
[lkml]   [2012]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [GIT PULL] RCU changes for v3.3
On (01/24/12 15:29), Paul E. McKenney wrote:
> On Tue, Jan 24, 2012 at 01:11:37PM -0800, Paul E. McKenney wrote:
> > On Tue, Jan 24, 2012 at 08:57:49PM +0100, Eric Dumazet wrote:
> > > Le mardi 24 janvier 2012 à 11:41 -0800, Paul E. McKenney a écrit :
> > >
> > > > Ah, I see... I need to find one of trace_power_start(),
> > > > trace_power_frequency(), or trace_power_end(). I would have to guess
> > > > that this is either the trace_power_start() or the trace_power_end()
> > > > called from drivers/cpuidle/cpuidle.c lines 97 and 102. Those are
> > > > within cpuidle_idle_call(), which are called from cpu_idle() in
> > > > arch/x86/kernel/process_32.c and arch/x86/kernel/process_64.c, so this
> > > > sounds plausible.
> > > >
> > > > And they are indeed busted -- RCU believes the CPU is idle at the point
> > > > that cpuidle_idle_call() is invoked.
> > > >
> > > > A hacky patch is below. Here are some of my concerns with it:
> > > >
> > > > 1. Is there a configuration in which the scheduler clock gets
> > > > turned off, but in which cpuidle_idle_call() always returns
> > > > zero? If so, we either really need RCU to consider the entire
> > > > inner loop to be idle (thus needing to snapshot the value of
> > > > cpuidle_idle_call() in the outer loop) or we need explicit calls
> > > > to rcu_sched_qs() and friends.
> > > >
> > > > Yes, we could momentarily exit RCU idleness mode, but I would
> > > > need to think that one through...
> > > >
> > > > 2. I am not totally confident that I have the order of operations
> > > > surrounding the call to pm_idle() correct.
> > > >
> > > > 3. This only addresses x86, and it looks like a few other architectures
> > > > have this same problem.
> > > >
> > > > 4. Probably other things that I haven't thought of.
> > > >
> > > > That said, the patch does seem to compile, at least on my 32-bit
> > > > laptop...
> > > >
> > > > Thanx, Paul
> > > >
> > > > ------------------------------------------------------------------------
> > > >
> > > > idle: Avoid using RCU when RCU thinks the CPU is idle
> > > >
> > > > The x86 idle loops invoke cpuidle_idle_call() which uses tracing
> > > > which uses RCU. Unfortunately, these idle loops have already
> > > > told RCU to ignore this CPU when they call it. This patch hacks
> > > > the idle loops to avoid this problem, but probably causing several
> > > > other problems in the process.
> > > >
> > > > Not-yet-signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > ---
> > >
> > > Hi Paul
> > >
> > > Just tested it on my x86_64 machine, but warnings are still here
> > >
> > > Thanks !
> >
> > Gah!!! The mwait_idle() function itself (which is the default value of
> > the pm_idle function pointer) uses tracing and thus RCU! What part of
> > "don't use RCU from idle CPUs" was unclear, one wonders?
> >
> > Ah well, the good news is that we can now detect such abuse and fix it.
> >
> > But fixing it appears to require pushing rcu_idle_enter() and
> > rcu_idle_exit() pairs down to the bottom of each and every idle loop
> > and governor.
> >
> > So... The cpuidle_idle_call() function has an idle loop inside of itself,
> > namely the ->enter() call for the desired target state. It does tracing
> > on both sides of that call. Should the ->enter() calls actually avoid
> > use of tracing, I could push the rcu_idle_enter() and rcu_idle_exit()
> > down into cpuidle_idle_call(). We seem to have a ladder_governor and
> > a menu_governor in 3.2, and these have states, which in turn have ->enter
> > functions.
> >
> > Hmmm... Residual power dissipation is given in milliwatts. I could
> > imagine some heartburn from many of the more aggressive embedded folks,
> > given that they might prefer microwatts -- or maybe even nanowatts,
> > for all I know.
> >
> > There are a bunch of states defined in drivers/idle/intel_idle.c,
> > and these use intel_idle() as their ->enter() states. This one looks
> > to have a nice place for rcu_idle_enter() and rcu_idle_exit().
> >
> > But I also need to push rcu_idle_enter() and rcu_idle_exit() into any
> > function that can be assigned to pm_idle(): default_idle(), poll_idle(),
> > mwait_idle(), and amd_e400_idle(). OK, that is not all -that- bad,
> > though this must also be done for a number of other architectures as well.
> >
> > OK, will post a patch. I will need testing -- clearly my testing on KVM
> > is missing a few important code paths...
>
> And here is another version of the patch.
>
> Thanx, Paul
>


Hello,
I just hit the same problem.

Is this patch scheduled for 3.3 until release or will land during 3.4
merge window?


-ss

> ------------------------------------------------------------------------
>
> x86: Avoid invoking RCU when CPU is idle
>
> The idle loop is a quiscent state for RCU, which means that RCU ignores
> CPUs that have told RCU that they are idle via rcu_idle_enter(). There
> are nevertheless quite a few places where idle CPUs use RCU, most commonly
> indirectly via tracing. This patch fixes these problems for x86.
>
> Many of these bugs have been in the kernel for quite some time, but
> Frederic's recent change now gives warnings.
>
> This patch takes the straightforward approach of pushing the
> rcu_idle_enter()/rcu_idle_exit() pair further down into the core
> of the idle loop.
>
> Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 15763af..f6978b0 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -386,17 +386,21 @@ void default_idle(void)
> */
> smp_mb();
>
> + rcu_idle_enter();
> if (!need_resched())
> safe_halt(); /* enables interrupts racelessly */
> else
> local_irq_enable();
> + rcu_idle_exit();
> current_thread_info()->status |= TS_POLLING;
> trace_power_end(smp_processor_id());
> trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
> } else {
> local_irq_enable();
> /* loop is done by the caller */
> + rcu_idle_enter();
> cpu_relax();
> + rcu_idle_exit();
> }
> }
> #ifdef CONFIG_APM_MODULE
> @@ -457,14 +461,19 @@ static void mwait_idle(void)
>
> __monitor((void *)&current_thread_info()->flags, 0, 0);
> smp_mb();
> + rcu_idle_enter();
> if (!need_resched())
> __sti_mwait(0, 0);
> else
> local_irq_enable();
> + rcu_idle_exit();
> trace_power_end(smp_processor_id());
> trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
> - } else
> + } else {
> local_irq_enable();
> + rcu_idle_enter();
> + rcu_idle_exit();
> + }
> }
>
> /*
> @@ -477,8 +486,10 @@ static void poll_idle(void)
> trace_power_start(POWER_CSTATE, 0, smp_processor_id());
> trace_cpu_idle(0, smp_processor_id());
> local_irq_enable();
> + rcu_idle_enter();
> while (!need_resched())
> cpu_relax();
> + rcu_idle_exit();
> trace_power_end(smp_processor_id());
> trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
> }
> diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
> index 485204f..6d9d4d5 100644
> --- a/arch/x86/kernel/process_32.c
> +++ b/arch/x86/kernel/process_32.c
> @@ -100,7 +100,6 @@ void cpu_idle(void)
> /* endless idle loop with no priority at all */
> while (1) {
> tick_nohz_idle_enter();
> - rcu_idle_enter();
> while (!need_resched()) {
>
> check_pgt_cache();
> @@ -117,7 +116,6 @@ void cpu_idle(void)
> pm_idle();
> start_critical_timings();
> }
> - rcu_idle_exit();
> tick_nohz_idle_exit();
> preempt_enable_no_resched();
> schedule();
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 9b9fe4a..55a1a35 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -140,13 +140,9 @@ void cpu_idle(void)
> /* Don't trace irqs off for idle */
> stop_critical_timings();
>
> - /* enter_idle() needs rcu for notifiers */
> - rcu_idle_enter();
> -
> if (cpuidle_idle_call())
> pm_idle();
>
> - rcu_idle_exit();
> start_critical_timings();
>
> /* In many cases the interrupt that ended idle
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index 20bce51..a9ddab8 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -261,6 +261,7 @@ static int intel_idle(struct cpuidle_device *dev,
> kt_before = ktime_get_real();
>
> stop_critical_timings();
> + rcu_idle_enter();
> if (!need_resched()) {
>
> __monitor((void *)&current_thread_info()->flags, 0, 0);
> @@ -268,6 +269,7 @@ static int intel_idle(struct cpuidle_device *dev,
> if (!need_resched())
> __mwait(eax, ecx);
> }
> + rcu_idle_exit();
>
> start_critical_timings();
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2012-03-07 12:47    [from the cache]
©2003-2011 Jasper Spaans