From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753191AbbF3QQh (ORCPT ); Tue, 30 Jun 2015 12:16:37 -0400 Received: from e37.co.us.ibm.com ([32.97.110.158]:44008 "EHLO e37.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753141AbbF3QQ3 (ORCPT ); Tue, 30 Jun 2015 12:16:29 -0400 X-Helo: d03dlp01.boulder.ibm.com X-MailFrom: paulmck@linux.vnet.ibm.com X-RcptTo: linux-kernel@vger.kernel.org Date: Tue, 30 Jun 2015 09:16:24 -0700 From: "Paul E. McKenney" To: Ingo Molnar Cc: Andy Lutomirski , x86@kernel.org, linux-kernel@vger.kernel.org, =?iso-8859-1?Q?Fr=E9d=E9ric?= Weisbecker , Rik van Riel , Oleg Nesterov , Denys Vlasenko , Borislav Petkov , Kees Cook , Brian Gerst Subject: Re: [RFC/INCOMPLETE 01/13] context_tracking: Add context_tracking_assert_state Message-ID: <20150630161623.GB3717@linux.vnet.ibm.com> Reply-To: paulmck@linux.vnet.ibm.com References: <1d95640676a92a5ff7382e9c87517c12ea23ccd9.1434485184.git.luto@kernel.org> <20150617094114.GA3940@gmail.com> <20150617152756.GA3913@linux.vnet.ibm.com> <20150618095955.GB4528@gmail.com> <20150618225420.GQ3913@linux.vnet.ibm.com> <20150630110414.GA25988@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150630110414.GA25988@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15063016-0025-0000-0000-00000FBB8A0A Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jun 30, 2015 at 01:04:14PM +0200, Ingo Molnar wrote: > > * Paul E. McKenney 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 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 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 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