From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Frederic Weisbecker <fweisbec@gmail.com>
Cc: linux-kernel@vger.kernel.org, mingo@elte.hu,
laijs@cn.fujitsu.com, dipankar@in.ibm.com,
akpm@linux-foundation.org, mathieu.desnoyers@polymtl.ca,
josh@joshtriplett.org, niv@us.ibm.com, tglx@linutronix.de,
peterz@infradead.org, rostedt@goodmis.org,
Valdis.Kletnieks@vt.edu, dhowells@redhat.com,
eric.dumazet@gmail.com, darren@dvhart.com, patches@linaro.org,
"Paul E. McKenney" <paul.mckenney@linaro.org>
Subject: Re: [PATCH tip/core/rcu 45/47] rcu: Allow nesting of rcu_idle_enter() and rcu_idle_exit()
Date: Thu, 9 Feb 2012 07:26:20 -0800 [thread overview]
Message-ID: <20120209152620.GI32560@linux.vnet.ibm.com> (raw)
In-Reply-To: <20120209040701.GF25473@somewhere.redhat.com>
On Thu, Feb 09, 2012 at 05:07:04AM +0100, Frederic Weisbecker wrote:
> On Fri, Feb 03, 2012 at 05:45:20PM -0800, Paul E. McKenney wrote:
> > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> >
> > Use of RCU in the idle loop is incorrect, quite a few instances of
> > just that have made their way into mainline, primarily event tracing.
> > The problem with RCU read-side critical sections on CPUs that RCU believes
> > to be idle is that RCU is completely ignoring the CPU, along with any
> > attempts and RCU read-side critical sections.
> >
> > The approaches of eliminating the offending uses and of pushing the
> > definition of idle down beyond the offending uses have both proved
> > impractical. The new approach is to encapsulate offending uses of RCU
> > with rcu_idle_exit() and rcu_idle_enter(), but this requires nesting
> > for code that is invoked both during idle and and during normal execution.
> > Therefore, this commit modifies rcu_idle_enter() and rcu_idle_exit() to
> > permit nesting.
> >
> > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Reviewed-by: Josh Triplett <josh@joshtriplett.org>
> > Acked-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
> > ---
> > kernel/rcu.h | 21 ++++++++++++++++++++-
> > kernel/rcutiny.c | 16 ++++++++++++----
> > kernel/rcutree.c | 21 ++++++++++++++-------
> > 3 files changed, 46 insertions(+), 12 deletions(-)
> >
> > diff --git a/kernel/rcu.h b/kernel/rcu.h
> > index 30876f4..8ba99cd 100644
> > --- a/kernel/rcu.h
> > +++ b/kernel/rcu.h
> > @@ -33,8 +33,27 @@
> > * Process-level increment to ->dynticks_nesting field. This allows for
> > * architectures that use half-interrupts and half-exceptions from
> > * process context.
> > + *
> > + * DYNTICK_TASK_NEST_MASK defines a field of width DYNTICK_TASK_NEST_WIDTH
> > + * that counts the number of process-based reasons why RCU cannot
> > + * consider the corresponding CPU to be idle, and DYNTICK_TASK_NEST_VALUE
> > + * is the value used to increment or decrement this field.
> > + *
> > + * The rest of the bits could in principle be used to count interrupts,
> > + * but this would mean that a negative-one value in the interrupt
> > + * field could incorrectly zero out the DYNTICK_TASK_NEST_MASK field.
> > + * We therefore provide a two-bit guard field defined by DYNTICK_TASK_MASK
> > + * that is set to DYNTICK_TASK_FLAG upon initial exit from idle.
> > + * The DYNTICK_TASK_EXIT_IDLE value is thus the combined value used upon
> > + * initial exit from idle.
> > */
> > -#define DYNTICK_TASK_NESTING (LLONG_MAX / 2 - 1)
> > +#define DYNTICK_TASK_NEST_WIDTH 7
> > +#define DYNTICK_TASK_NEST_VALUE ((LLONG_MAX >> DYNTICK_TASK_NEST_WIDTH) + 1)
> > +#define DYNTICK_TASK_NEST_MASK (LLONG_MAX - DYNTICK_TASK_NEST_VALUE + 1)
> > +#define DYNTICK_TASK_FLAG ((DYNTICK_TASK_NEST_VALUE / 8) * 2)
> > +#define DYNTICK_TASK_MASK ((DYNTICK_TASK_NEST_VALUE / 8) * 3)
>
> There is one unused bit between DYNTICK_TASK_NEST_MASK and DYNTICK_TASK_MASK, is
> that intentional?
Yep, it makes it easier for me to read hex dumps of the variables.
> Also do you want to allow nesting of that kind?
>
> rcu_idle_enter();
> rcu_idle_enter();
> rcu_idle_exit();
> rcu_idle_exit()
No -- only the inverse where you exit idle multiple times.
> in which case I guess that rcu_irq_enter()/rcu_irq_exit() also need to
> be updated.
>
> If we have this:
>
> rcu_idle_enter()
> rcu_idle_enter()
>
> rcu_irq_enter()
> rcu_irq_exit()
>
> rcu_idle_exit()
> rcu_idle_exit()
>
> On rcu_irq_enter(), oldval will never be 0 and we'll miss rcu_idle_exit_common().
> rcu_irq_exit() has a similar problem as it won't enter rcu_idle_enter_common().
>
> Its check on WARN_ON_ONCE(rdtp->dynticks_nesting < 0) is also wrong because after
> two calls of rcu_idle_enter(), the value of dynticks_nesting is negative : it's
> -DYNTICK_TASK_NEST_VALUE.
>
> Perhaps this change would allow that. But again that's just in case you need to
> support that kind of nesting.
Interesting. I don't know of a use case for this -- do you have any?
Thanx, Paul
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index eacc10b..0b7d946 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -430,8 +430,8 @@ void rcu_irq_exit(void)
> rdtp = &__get_cpu_var(rcu_dynticks);
> oldval = rdtp->dynticks_nesting;
> rdtp->dynticks_nesting--;
> - WARN_ON_ONCE(rdtp->dynticks_nesting < 0);
> - if (rdtp->dynticks_nesting)
> + WARN_ON_ONCE(!oldval);
> + if (rdtp->dynticks_nesting & ~DYNTICK_TASK_NEST_MASK)
> trace_rcu_dyntick("--=", oldval, rdtp->dynticks_nesting);
> else
> rcu_idle_enter_common(rdtp, oldval);
> @@ -525,8 +525,8 @@ void rcu_irq_enter(void)
> rdtp = &__get_cpu_var(rcu_dynticks);
> oldval = rdtp->dynticks_nesting;
> rdtp->dynticks_nesting++;
> - WARN_ON_ONCE(rdtp->dynticks_nesting == 0);
> - if (oldval)
> + WARN_ON_ONCE(oldval == ~DYNTICK_TASK_NEST_MASK);
> + if (oldval & ~DYNTICK_TASK_NEST_MASK)
> trace_rcu_dyntick("++=", oldval, rdtp->dynticks_nesting);
> else
> rcu_idle_exit_common(rdtp, oldval);
>
>
>
> > +#define DYNTICK_TASK_EXIT_IDLE (DYNTICK_TASK_NEST_VALUE + \
> > + DYNTICK_TASK_FLAG)
> >
> > /*
> > * debug_rcu_head_queue()/debug_rcu_head_unqueue() are used internally
> > diff --git a/kernel/rcutiny.c b/kernel/rcutiny.c
> > index 4eb34fc..c8b0e15 100644
> > --- a/kernel/rcutiny.c
> > +++ b/kernel/rcutiny.c
> > @@ -53,7 +53,7 @@ static void __call_rcu(struct rcu_head *head,
> >
> > #include "rcutiny_plugin.h"
> >
> > -static long long rcu_dynticks_nesting = DYNTICK_TASK_NESTING;
> > +static long long rcu_dynticks_nesting = DYNTICK_TASK_EXIT_IDLE;
> >
> > /* Common code for rcu_idle_enter() and rcu_irq_exit(), see kernel/rcutree.c. */
> > static void rcu_idle_enter_common(long long oldval)
> > @@ -88,7 +88,12 @@ void rcu_idle_enter(void)
> >
> > local_irq_save(flags);
> > oldval = rcu_dynticks_nesting;
> > - rcu_dynticks_nesting = 0;
> > + WARN_ON_ONCE((rcu_dynticks_nesting & DYNTICK_TASK_NEST_MASK) == 0);
> > + if ((rcu_dynticks_nesting & DYNTICK_TASK_NEST_MASK) ==
> > + DYNTICK_TASK_NEST_VALUE)
> > + rcu_dynticks_nesting = 0;
> > + else
> > + rcu_dynticks_nesting -= DYNTICK_TASK_NEST_VALUE;
> > rcu_idle_enter_common(oldval);
> > local_irq_restore(flags);
> > }
> > @@ -140,8 +145,11 @@ void rcu_idle_exit(void)
> >
> > local_irq_save(flags);
> > oldval = rcu_dynticks_nesting;
> > - WARN_ON_ONCE(oldval != 0);
> > - rcu_dynticks_nesting = DYNTICK_TASK_NESTING;
> > + WARN_ON_ONCE(rcu_dynticks_nesting < 0);
> > + if (rcu_dynticks_nesting & DYNTICK_TASK_NEST_MASK)
> > + rcu_dynticks_nesting += DYNTICK_TASK_NEST_VALUE;
> > + else
> > + rcu_dynticks_nesting = DYNTICK_TASK_EXIT_IDLE;
> > rcu_idle_exit_common(oldval);
> > local_irq_restore(flags);
> > }
> > diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> > index df0e3c1..92b4776 100644
> > --- a/kernel/rcutree.c
> > +++ b/kernel/rcutree.c
> > @@ -198,7 +198,7 @@ void rcu_note_context_switch(int cpu)
> > EXPORT_SYMBOL_GPL(rcu_note_context_switch);
> >
> > DEFINE_PER_CPU(struct rcu_dynticks, rcu_dynticks) = {
> > - .dynticks_nesting = DYNTICK_TASK_NESTING,
> > + .dynticks_nesting = DYNTICK_TASK_EXIT_IDLE,
> > .dynticks = ATOMIC_INIT(1),
> > };
> >
> > @@ -394,7 +394,11 @@ void rcu_idle_enter(void)
> > local_irq_save(flags);
> > rdtp = &__get_cpu_var(rcu_dynticks);
> > oldval = rdtp->dynticks_nesting;
> > - rdtp->dynticks_nesting = 0;
> > + WARN_ON_ONCE((oldval & DYNTICK_TASK_NEST_MASK) == 0);
> > + if ((oldval & DYNTICK_TASK_NEST_MASK) == DYNTICK_TASK_NEST_VALUE)
> > + rdtp->dynticks_nesting = 0;
> > + else
> > + rdtp->dynticks_nesting -= DYNTICK_TASK_NEST_VALUE;
> > rcu_idle_enter_common(rdtp, oldval);
> > local_irq_restore(flags);
> > }
> > @@ -467,7 +471,7 @@ static void rcu_idle_exit_common(struct rcu_dynticks *rdtp, long long oldval)
> > * Exit idle mode, in other words, -enter- the mode in which RCU
> > * read-side critical sections can occur.
> > *
> > - * We crowbar the ->dynticks_nesting field to DYNTICK_TASK_NESTING to
> > + * We crowbar the ->dynticks_nesting field to DYNTICK_TASK_NEST to
> > * allow for the possibility of usermode upcalls messing up our count
> > * of interrupt nesting level during the busy period that is just
> > * now starting.
> > @@ -481,8 +485,11 @@ void rcu_idle_exit(void)
> > local_irq_save(flags);
> > rdtp = &__get_cpu_var(rcu_dynticks);
> > oldval = rdtp->dynticks_nesting;
> > - WARN_ON_ONCE(oldval != 0);
> > - rdtp->dynticks_nesting = DYNTICK_TASK_NESTING;
> > + WARN_ON_ONCE(oldval < 0);
> > + if (oldval & DYNTICK_TASK_NEST_MASK)
> > + rdtp->dynticks_nesting += DYNTICK_TASK_NEST_VALUE;
> > + else
> > + rdtp->dynticks_nesting = DYNTICK_TASK_EXIT_IDLE;
> > rcu_idle_exit_common(rdtp, oldval);
> > local_irq_restore(flags);
> > }
> > @@ -2253,7 +2260,7 @@ rcu_boot_init_percpu_data(int cpu, struct rcu_state *rsp)
> > rdp->qlen_lazy = 0;
> > rdp->qlen = 0;
> > rdp->dynticks = &per_cpu(rcu_dynticks, cpu);
> > - WARN_ON_ONCE(rdp->dynticks->dynticks_nesting != DYNTICK_TASK_NESTING);
> > + WARN_ON_ONCE(rdp->dynticks->dynticks_nesting != DYNTICK_TASK_EXIT_IDLE);
> > WARN_ON_ONCE(atomic_read(&rdp->dynticks->dynticks) != 1);
> > rdp->cpu = cpu;
> > rdp->rsp = rsp;
> > @@ -2281,7 +2288,7 @@ rcu_init_percpu_data(int cpu, struct rcu_state *rsp, int preemptible)
> > rdp->qlen_last_fqs_check = 0;
> > rdp->n_force_qs_snap = rsp->n_force_qs;
> > rdp->blimit = blimit;
> > - rdp->dynticks->dynticks_nesting = DYNTICK_TASK_NESTING;
> > + rdp->dynticks->dynticks_nesting = DYNTICK_TASK_EXIT_IDLE;
> > atomic_set(&rdp->dynticks->dynticks,
> > (atomic_read(&rdp->dynticks->dynticks) & ~0x1) + 1);
> > rcu_prepare_for_idle_init(cpu);
> > --
> > 1.7.8
> >
> --
> 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/
>
next prev parent reply other threads:[~2012-02-09 15:28 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-02-04 1:44 [PATCH tip/core/rcu 0/N] v2 RCU commits for 3.4 Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 01/47] rcu: Bring RTFP.txt up to date Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 02/47] rcu: Improve synchronize_rcu() diagnostics Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 03/47] rcu: Add lockdep-RCU checks for simple self-deadlock Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 04/47] rcu: Add diagnostic for misaligned rcu_head structures Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 05/47] rcu: Avoid waking up CPUs having only kfree_rcu() callbacks Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 06/47] rcu: Move RCU_TRACE to lib/Kconfig.debug Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 07/47] s390: Convert call_rcu() to kfree_rcu(), drop ext_int_hash_update() Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 08/47] tcm_fc: Convert call_rcu() to kfree_rcu(), drop ft_tport_rcu_free() Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 09/47] ipv4: Convert call_rcu() to kfree_rcu(), drop opt_kfree_rcu() Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 10/47] ipv4: Convert call_rcu() to kfree_rcu(), drop opt_kfree_rcu Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 11/47] mac80211: Convert call_rcu() to kfree_rcu(), drop mesh_gate_node_reclaim() Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 12/47] rcu: Simplify offline processing Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 13/47] rcu: Make rcutorture flag online/offline failures Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 14/47] rcu: Limit lazy-callback duration Paul E. McKenney
2012-02-04 13:54 ` Frederic Weisbecker
2012-02-04 14:30 ` Paul E. McKenney
2012-02-04 14:32 ` Frederic Weisbecker
2012-02-04 1:44 ` [PATCH tip/core/rcu 15/47] rcu: Check for callback invocation from offline CPUs Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 16/47] rcu: Don't make callbacks go through second full grace period Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 17/47] rcu: Remove single-rcu_node optimization in rcu_start_gp() Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 18/47] rcu: Protect __rcu_read_unlock() against scheduler-using irq handlers Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 19/47] rcu: Streamline code produced by __rcu_read_unlock() Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 20/47] rcu: Prevent RCU callbacks from executing before scheduler initialized Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 21/47] rcu: Inform RCU of irq_exit() activity Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 22/47] rcu: Simplify unboosting checks Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 23/47] rcu: Clean up straggling rcu_preempt_needs_cpu() name Paul E. McKenney
2012-02-04 1:44 ` [PATCH tip/core/rcu 24/47] rcu: Check for idle-loop entry while in RCU read-side critical section Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 25/47] rcu: Make rcu_sleep_check() also check rcu_lock_map Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 26/47] rcu: Note that rcu_access_pointer() can be used for teardown Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 27/47] rcu: Remove #ifdef CONFIG_SMP from TREE_RCU Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 28/47] rcu: Set RCU CPU stall times via sysfs Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 29/47] rcu: Print scheduling-clock information on RCU CPU stall-warning messages Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 30/47] rcutorture: Permit holding off CPU-hotplug operations during boot Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 31/47] rcu: Make documentation give more realistic rcutorture duration Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 32/47] rcu: Add CPU-stall capability to rcutorture Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 33/47] rcu: Update stall-warning documentation Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 34/47] rcu: Make boolean rcutorture parameters be of type "bool" Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 35/47] rcu: Check for illegal use of RCU from offlined CPUs Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 36/47] rcu: Move synchronize_sched_expedited() to rcutree.c Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 37/47] rcu: No interrupt disabling for rcu_prepare_for_idle() Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 38/47] lockdep: Add CPU-idle/offline warning to lockdep-RCU splat Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 39/47] rcu: Rework detection of use of RCU by offline CPUs Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 40/47] rcu: Call out dangers of expedited RCU primitives Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 41/47] rcu: Trace only after NULL-pointer check Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 42/47] rcu: Convert WARN_ON_ONCE() in rcu_lock_acquire() to lockdep Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 43/47] PTR_ERR should be called before its argument is cleared Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 44/47] rcu: Remove redundant check for rcu_head misalignment Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 45/47] rcu: Allow nesting of rcu_idle_enter() and rcu_idle_exit() Paul E. McKenney
2012-02-09 4:07 ` Frederic Weisbecker
2012-02-09 15:26 ` Paul E. McKenney [this message]
2012-02-09 15:33 ` Frederic Weisbecker
2012-02-04 1:45 ` [PATCH tip/core/rcu 46/47] rcu: Add RCU_NONIDLE() for idle-loop RCU read-side critical sections Paul E. McKenney
2012-02-04 1:45 ` [PATCH tip/core/rcu 47/47] cpuidle: Inform RCU of " Paul E. McKenney
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=20120209152620.GI32560@linux.vnet.ibm.com \
--to=paulmck@linux.vnet.ibm.com \
--cc=Valdis.Kletnieks@vt.edu \
--cc=akpm@linux-foundation.org \
--cc=darren@dvhart.com \
--cc=dhowells@redhat.com \
--cc=dipankar@in.ibm.com \
--cc=eric.dumazet@gmail.com \
--cc=fweisbec@gmail.com \
--cc=josh@joshtriplett.org \
--cc=laijs@cn.fujitsu.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mathieu.desnoyers@polymtl.ca \
--cc=mingo@elte.hu \
--cc=niv@us.ibm.com \
--cc=patches@linaro.org \
--cc=paul.mckenney@linaro.org \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.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
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