Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754677AbdDRSfS (ORCPT ); Tue, 18 Apr 2017 14:35:18 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:41675 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753855AbdDRSfA (ORCPT ); Tue, 18 Apr 2017 14:35:00 -0400 Date: Tue, 18 Apr 2017 11:34:53 -0700 From: "Paul E. McKenney" To: Josh Triplett Cc: linux-kernel@vger.kernel.org, mingo@kernel.org, jiangshanlai@gmail.com, dipankar@in.ibm.com, akpm@linux-foundation.org, mathieu.desnoyers@efficios.com, tglx@linutronix.de, peterz@infradead.org, rostedt@goodmis.org, dhowells@redhat.com, edumazet@google.com, fweisbec@gmail.com, oleg@redhat.com, bobby.prani@gmail.com Subject: Re: [PATCH v2 tip/core/rcu 04/39] srcu: Check for tardy grace-period activity in cleanup_srcu_struct() Reply-To: paulmck@linux.vnet.ibm.com References: <20170417234452.GB19013@linux.vnet.ibm.com> <1492472726-3841-4-git-send-email-paulmck@linux.vnet.ibm.com> <20170418003332.7ocpffvkr3okbhr7@x> <20170418003429.wa7qvhiy3vvccbxw@x> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170418003429.wa7qvhiy3vvccbxw@x> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-GCONF: 00 x-cbid: 17041818-0024-0000-0000-0000024C658A X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006936; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000208; SDB=6.00849244; UDB=6.00419323; IPR=6.00627884; BA=6.00005298; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00015087; XFM=3.00000013; UTC=2017-04-18 18:34:58 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17041818-0025-0000-0000-00004377CA6B Message-Id: <20170418183453.GZ3956@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-04-18_15:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=2 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1704180146 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4484 Lines: 103 On Mon, Apr 17, 2017 at 05:34:30PM -0700, Josh Triplett wrote: > On Mon, Apr 17, 2017 at 05:33:32PM -0700, Josh Triplett wrote: > > On Mon, Apr 17, 2017 at 04:44:51PM -0700, Paul E. McKenney wrote: > > > Users of SRCU are obliged to complete all grace-period activity before > > > invoking cleanup_srcu_struct(). This means that all calls to either > > > synchronize_srcu() or synchronize_srcu_expedited() must have returned, > > > and all calls to call_srcu() must have returned, and the last call to > > > call_srcu() must have been followed by a call to srcu_barrier(). > > > Furthermore, the caller must have done something to prevent any > > > further calls to synchronize_srcu(), synchronize_srcu_expedited(), > > > and call_srcu(). > > > > > > Therefore, if there has ever been an invocation of call_srcu() on > > > the srcu_struct in question, the sequence of events must be as > > > follows: > > > > > > 1. Prevent any further calls to call_srcu(). > > > 2. Wait for any pre-existing call_srcu() invocations to return. > > > 3. Invoke srcu_barrier(). > > > 4. It is now safe to invoke cleanup_srcu_struct(). > > > > > > On the other hand, if there has ever been a call to synchronize_srcu() > > > or synchronize_srcu_expedited(), the sequence of events must be as > > > follows: > > > > > > 1. Prevent any further calls to synchronize_srcu() or > > > synchronize_srcu_expedited(). > > > 2. Wait for any pre-existing synchronize_srcu() or > > > synchronize_srcu_expedited() invocations to return. > > > 3. It is now safe to invoke cleanup_srcu_struct(). > > > > > > If there have been calls to all both types of functions (call_srcu() > > > and either of synchronize_srcu() and synchronize_srcu_expedited()), then > > > the caller must do the first three steps of the call_srcu() procedure > > > above and the first two steps of the synchronize_s*() procedure above, > > > and only then invoke cleanup_srcu_struct(). > > > > This commit message clearly explains the correct sequence for the > > client, but not which aspects of this the change now enforces. Some of > > the steps above remain the responsibility of the caller, while the > > callee now checks more of them. Could you add something at the end > > explaining the change and what it enforces? > > More importantly, perhaps this explanation could find its way into the > documentation of cleanup_srcu_struct? Like this? /** * cleanup_srcu_struct - deconstruct a sleep-RCU structure * @sp: structure to clean up. * * Must invoke this only after you are finished using a given srcu_struct * that was initialized via init_srcu_struct(). This code does some * probabalistic checking, spotting late uses of srcu_read_lock(), * synchronize_srcu(), synchronize_srcu_expedited(), and call_srcu(). * If any such late uses are detected, the per-CPU memory associated with * the srcu_struct is simply leaked and WARN_ON() is invoked. If the * caller frees the srcu_struct itself, a use-after-free crash will likely * ensue, but at least there will be a warning printed. */ I added the following paragraph to the commit log: Note that cleanup_srcu_struct() does some probabilistic checks for the caller failing to follow these procedures, in which case cleanup_srcu_struct() does WARN_ON() and avoids freeing the per-CPU structures associated with the specified srcu_struct structure. And added your Reviewed-by, but please let me if more is needed. Thanx, Paul > > > Reported-by: Paolo Bonzini > > > Signed-off-by: Paul E. McKenney > > > > With the above change: > > Reviewed-by: Josh Triplett > > > > > kernel/rcu/srcu.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/kernel/rcu/srcu.c b/kernel/rcu/srcu.c > > > index ba41a5d04b49..6beeba7b0b67 100644 > > > --- a/kernel/rcu/srcu.c > > > +++ b/kernel/rcu/srcu.c > > > @@ -261,6 +261,11 @@ void cleanup_srcu_struct(struct srcu_struct *sp) > > > { > > > if (WARN_ON(srcu_readers_active(sp))) > > > return; /* Leakage unless caller handles error. */ > > > + if (WARN_ON(!rcu_all_batches_empty(sp))) > > > + return; /* Leakage unless caller handles error. */ > > > + flush_delayed_work(&sp->work); > > > + if (WARN_ON(sp->running)) > > > + return; /* Caller forgot to stop doing call_srcu()? */ > > > free_percpu(sp->per_cpu_ref); > > > sp->per_cpu_ref = NULL; > > > } > > > -- > > > 2.5.2 > > > >