From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Ingo Molnar <mingo@kernel.org>
Cc: "Andy Lutomirski" <luto@kernel.org>,
x86@kernel.org, linux-kernel@vger.kernel.org,
"Frédéric Weisbecker" <fweisbec@gmail.com>,
"Rik van Riel" <riel@redhat.com>,
"Oleg Nesterov" <oleg@redhat.com>,
"Denys Vlasenko" <vda.linux@googlemail.com>,
"Borislav Petkov" <bp@alien8.de>,
"Kees Cook" <keescook@chromium.org>,
"Brian Gerst" <brgerst@gmail.com>
Subject: Re: [RFC/INCOMPLETE 01/13] context_tracking: Add context_tracking_assert_state
Date: Tue, 30 Jun 2015 09:16:24 -0700 [thread overview]
Message-ID: <20150630161623.GB3717@linux.vnet.ibm.com> (raw)
In-Reply-To: <20150630110414.GA25988@gmail.com>
On Tue, Jun 30, 2015 at 01:04:14PM +0200, Ingo Molnar wrote:
>
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
>
> > > Yeah, and inverting the condition. Assuming the condition was assert()-style
> > > inverted to begin with! :-)
> >
> > It appears to have been. ;-)
> >
> > Please see below for an untested patch. I made this be one big patch, but could
> > have one patch add RCU_LOCKDEP_WARN(), a series convert uses from
> > rcu_lockdep_assert() to RCU_LOCKDEP_WARN(), and a final patch remove
> > rcu_lockdep_assert(). Let me know!
>
> One big patch is perfect I think - it's a rename and a relatively mechanic
> inversion of conditions, no point in splitting it up any more IMHO. (But it's your
> call really.)
>
> So I had a quick look at this patch, and IMHO the RCU_LOCKDEP_WARN() lines read a
> lot more 'naturally', because the new, inverted conditions now highlight buggy
> scenarios - which has the same logic parity as the kernel's historic
> WARN_ON()/BUG_ON() patterns:
>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>
Thank you, added!
> This one looked a bit weird:
>
> > index a0a0dd03c73a..47268fb1d27b 100644
> > --- a/kernel/rcu/update.c
> > +++ b/kernel/rcu/update.c
> > @@ -589,8 +589,8 @@ EXPORT_SYMBOL_GPL(call_rcu_tasks);
> > void synchronize_rcu_tasks(void)
> > {
> > /* Complain if the scheduler has not started. */
> > - rcu_lockdep_assert(!rcu_scheduler_active,
> > - "synchronize_rcu_tasks called too soon");
> > + RCU_LOCKDEP_WARN(rcu_scheduler_active,
> > + "synchronize_rcu_tasks called too soon");
> >
>
> So I'd assume that a flag called 'rcu_scheduler_active' would be 1 if the RCU
> scheduler is active.
>
> So why do we warn on it being active? Shouldn't the condition be:
>
> RCU_LOCKDEP_WARN(!rcu_scheduler_active,
> "synchronize_rcu_tasks called too soon");
>
> I.e. we warn when the RCU scheduler is not yet active and we called
> synchronize_rcu_tasks() too soon?
>
> So either the original assert() was wrong, or I'm missing something obvious?
You are missing nothing! But I really do test this stuff...
Ah, I see... I need the following patch in order to enable lockdep-RCU
on one of my RCU-tasks rcutorture scenarios... :-/
Good catch! There were at least three bugs hiding behind that one! ;-)
Thanx, Paul
------------------------------------------------------------------------
commit dc883f1668c83f9525a13ee1b3cd45e9e85a0fe5
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date: Tue Jun 30 09:14:01 2015 -0700
rcutorture: Enable lockdep-RCU on TASKS01
Currently none of the RCU-tasks scenarios enables lockdep-RCU, which
causes bugs to be missed. This commit therefore enables lockdep-RCU
on TASKS01.
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
diff --git a/tools/testing/selftests/rcutorture/configs/rcu/TASKS01 b/tools/testing/selftests/rcutorture/configs/rcu/TASKS01
index 2cc0e60eba6e..bafe94cbd739 100644
--- a/tools/testing/selftests/rcutorture/configs/rcu/TASKS01
+++ b/tools/testing/selftests/rcutorture/configs/rcu/TASKS01
@@ -5,6 +5,6 @@ CONFIG_PREEMPT_NONE=n
CONFIG_PREEMPT_VOLUNTARY=n
CONFIG_PREEMPT=y
CONFIG_DEBUG_LOCK_ALLOC=y
-CONFIG_PROVE_LOCKING=n
-#CHECK#CONFIG_PROVE_RCU=n
+CONFIG_PROVE_LOCKING=y
+#CHECK#CONFIG_PROVE_RCU=y
CONFIG_RCU_EXPERT=y
next prev parent reply other threads:[~2015-06-30 16:16 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-16 20:16 [RFC/INCOMPLETE 00/13] x86: Rewrite exit-to-userspace code Andy Lutomirski
2015-06-16 20:16 ` [RFC/INCOMPLETE 01/13] context_tracking: Add context_tracking_assert_state Andy Lutomirski
2015-06-17 9:41 ` Ingo Molnar
2015-06-17 14:15 ` Andy Lutomirski
2015-06-18 9:57 ` Ingo Molnar
2015-06-18 11:07 ` Andy Lutomirski
2015-06-18 15:52 ` Andy Lutomirski
2015-06-18 16:17 ` Ingo Molnar
2015-06-18 16:26 ` Frederic Weisbecker
2015-06-18 19:26 ` Andy Lutomirski
2015-06-17 15:27 ` Paul E. McKenney
2015-06-18 9:59 ` Ingo Molnar
2015-06-18 22:54 ` Paul E. McKenney
2015-06-19 2:19 ` Paul E. McKenney
2015-06-30 11:04 ` Ingo Molnar
2015-06-30 16:16 ` Paul E. McKenney [this message]
2015-06-16 20:16 ` [RFC/INCOMPLETE 02/13] notifiers: Assert that RCU is watching in notify_die Andy Lutomirski
2015-06-16 20:16 ` [RFC/INCOMPLETE 03/13] x86: Move C entry and exit code to arch/x86/entry/common.c Andy Lutomirski
2015-06-16 20:16 ` [RFC/INCOMPLETE 04/13] x86/traps: Assert that we're in CONTEXT_KERNEL in exception entries Andy Lutomirski
2015-06-16 20:16 ` [RFC/INCOMPLETE 05/13] x86/entry: Add enter_from_user_mode and use it in syscalls Andy Lutomirski
2015-06-16 20:16 ` [RFC/INCOMPLETE 06/13] x86/entry: Add new, comprehensible entry and exit hooks Andy Lutomirski
2015-06-16 20:16 ` [RFC/INCOMPLETE 07/13] x86/entry/64: Really create an error-entry-from-usermode code path Andy Lutomirski
2015-06-16 20:16 ` [RFC/INCOMPLETE 08/13] x86/entry/64: Migrate 64-bit syscalls to new exit hooks Andy Lutomirski
2015-06-17 10:00 ` Ingo Molnar
2015-06-17 10:02 ` Ingo Molnar
2015-06-17 14:12 ` Andy Lutomirski
2015-06-18 10:17 ` Ingo Molnar
2015-06-18 10:19 ` Ingo Molnar
2015-06-16 20:16 ` [RFC/INCOMPLETE 09/13] x86/entry/compat: Migrate compat " Andy Lutomirski
2015-06-16 20:16 ` [RFC/INCOMPLETE 10/13] x86/asm/entry/64: Save all regs on interrupt entry Andy Lutomirski
2015-06-16 20:16 ` [RFC/INCOMPLETE 11/13] x86/asm/entry/64: Simplify irq stack pt_regs handling Andy Lutomirski
2015-06-16 20:16 ` [RFC/INCOMPLETE 12/13] x86/asm/entry/64: Migrate error and interrupt exit work to C Andy Lutomirski
2015-06-16 20:16 ` [RFC/INCOMPLETE 13/13] x86/entry: Remove SCHEDULE_USER and asm/context-tracking.h Andy Lutomirski
2015-06-17 9:48 ` [RFC/INCOMPLETE 00/13] x86: Rewrite exit-to-userspace code Ingo Molnar
2015-06-17 10:13 ` Richard Weinberger
2015-06-17 11:04 ` Ingo Molnar
2015-06-17 14:19 ` Andy Lutomirski
2015-06-17 15:16 ` Andy Lutomirski
2015-06-18 10:14 ` Ingo Molnar
2015-06-17 10:32 ` Ingo Molnar
2015-06-17 11:14 ` Ingo Molnar
2015-06-17 14:23 ` Andy Lutomirski
2015-06-18 10:11 ` Ingo Molnar
2015-06-18 11:06 ` Andy Lutomirski
2015-06-18 16:24 ` Ingo Molnar
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=20150630161623.GB3717@linux.vnet.ibm.com \
--to=paulmck@linux.vnet.ibm.com \
--cc=bp@alien8.de \
--cc=brgerst@gmail.com \
--cc=fweisbec@gmail.com \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mingo@kernel.org \
--cc=oleg@redhat.com \
--cc=riel@redhat.com \
--cc=vda.linux@googlemail.com \
--cc=x86@kernel.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.