From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752879AbbF3LE1 (ORCPT ); Tue, 30 Jun 2015 07:04:27 -0400 Received: from mail-wi0-f179.google.com ([209.85.212.179]:33929 "EHLO mail-wi0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751496AbbF3LET (ORCPT ); Tue, 30 Jun 2015 07:04:19 -0400 Date: Tue, 30 Jun 2015 13:04:14 +0200 From: Ingo Molnar To: "Paul E. McKenney" 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: <20150630110414.GA25988@gmail.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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150618225420.GQ3913@linux.vnet.ibm.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * 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 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? Thanks, Ingo