From mboxrd@z Thu Jan 1 00:00:00 1970 From: paulmck@linux.vnet.ibm.com (Paul E. McKenney) Date: Fri, 6 Apr 2018 09:30:38 -0700 Subject: [PATCH] nvme: fix flush dependency in delete controller flow In-Reply-To: <20180406061920.GA20628@lst.de> References: <20180402123727.3961-1-nitzanc@mellanox.com> <20180405085438.GC2286@lst.de> <20180405131426.GL3948@linux.vnet.ibm.com> <20180405160852.GA11416@linux.vnet.ibm.com> <20180405195512.GA12678@lst.de> <20180406001811.GQ3948@linux.vnet.ibm.com> <20180406061920.GA20628@lst.de> Message-ID: <20180406163038.GU3948@linux.vnet.ibm.com> On Fri, Apr 06, 2018@08:19:20AM +0200, Christoph Hellwig wrote: > On Thu, Apr 05, 2018@05:18:11PM -0700, Paul E. McKenney wrote: > > OK, how does the following (untested) patch look? > > Looks sensible to me. Nitzan, can you test it with the obvious nvme > conversion to cleanup_srcu_struct_quiesced? And it did pass light rcutorture testing overnight, so here is hoping! ;-) Thanx, Paul > > ------------------------------------------------------------------------ > > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h > > index 33c1c698df09..91494d7e8e41 100644 > > --- a/include/linux/srcu.h > > +++ b/include/linux/srcu.h > > @@ -69,11 +69,45 @@ struct srcu_struct { }; > > > > void call_srcu(struct srcu_struct *sp, struct rcu_head *head, > > void (*func)(struct rcu_head *head)); > > -void cleanup_srcu_struct(struct srcu_struct *sp); > > +void _cleanup_srcu_struct(struct srcu_struct *sp, bool quiesced); > > int __srcu_read_lock(struct srcu_struct *sp) __acquires(sp); > > void __srcu_read_unlock(struct srcu_struct *sp, int idx) __releases(sp); > > void synchronize_srcu(struct srcu_struct *sp); > > > > +/** > > + * cleanup_srcu_struct - deconstruct a sleep-RCU structure > > + * @sp: structure to clean up. > > + * > > + * Must invoke this after you are finished using a given srcu_struct that > > + * was initialized via init_srcu_struct(), else you leak memory. > > + */ > > +static inline void cleanup_srcu_struct(struct srcu_struct *sp) > > +{ > > + _cleanup_srcu_struct(sp, false); > > +} > > + > > +/** > > + * cleanup_srcu_struct_quiesced - deconstruct a quiesced sleep-RCU structure > > + * @sp: structure to clean up. > > + * > > + * Must invoke this after you are finished using a given srcu_struct that > > + * was initialized via init_srcu_struct(), else you leak memory. Also, > > + * all grace-period processing must have completed. > > + * > > + * "Completed" means that the last synchronize_srcu() and > > + * synchronize_srcu_expedited() calls must have returned before the call > > + * to cleanup_srcu_struct_quiesced(). It also means that the callback > > + * from the last call_srcu() must have been invoked before the call to > > + * cleanup_srcu_struct_quiesced(), but you can use srcu_barrier() to help > > + * with this last. Violating these rules will get you a WARN_ON() splat > > + * (with high probability, anyway), and will also cause the srcu_struct > > + * to be leaked. > > + */ > > +static inline void cleanup_srcu_struct_quiesced(struct srcu_struct *sp) > > +{ > > + _cleanup_srcu_struct(sp, true); > > +} > > + > > #ifdef CONFIG_DEBUG_LOCK_ALLOC > > > > /** > > diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c > > index 680c96d8c00f..f0e1d44459f8 100644 > > --- a/kernel/rcu/rcutorture.c > > +++ b/kernel/rcu/rcutorture.c > > @@ -593,7 +593,12 @@ static void srcu_torture_init(void) > > > > static void srcu_torture_cleanup(void) > > { > > - cleanup_srcu_struct(&srcu_ctld); > > + static DEFINE_TORTURE_RANDOM(rand); > > + > > + if (torture_random(&rand) & 0x800) > > + cleanup_srcu_struct(&srcu_ctld); > > + else > > + cleanup_srcu_struct_quiesced(&srcu_ctld); > > srcu_ctlp = &srcu_ctl; /* In case of a later rcutorture run. */ > > } > > > > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c > > index 76ac5f50b2c7..622792abe41a 100644 > > --- a/kernel/rcu/srcutiny.c > > +++ b/kernel/rcu/srcutiny.c > > @@ -86,16 +86,19 @@ EXPORT_SYMBOL_GPL(init_srcu_struct); > > * Must invoke this after you are finished using a given srcu_struct that > > * was initialized via init_srcu_struct(), else you leak memory. > > */ > > -void cleanup_srcu_struct(struct srcu_struct *sp) > > +void _cleanup_srcu_struct(struct srcu_struct *sp, bool quiesced) > > { > > WARN_ON(sp->srcu_lock_nesting[0] || sp->srcu_lock_nesting[1]); > > - flush_work(&sp->srcu_work); > > + if (quiesced) > > + WARN_ON(work_pending(&sp->srcu_work)); > > + else > > + flush_work(&sp->srcu_work); > > WARN_ON(sp->srcu_gp_running); > > WARN_ON(sp->srcu_gp_waiting); > > WARN_ON(sp->srcu_cb_head); > > WARN_ON(&sp->srcu_cb_head != sp->srcu_cb_tail); > > } > > -EXPORT_SYMBOL_GPL(cleanup_srcu_struct); > > +EXPORT_SYMBOL_GPL(_cleanup_srcu_struct); > > > > /* > > * Removes the count for the old reader from the appropriate element of > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > index fb560fca9ef4..b4123d7a2cec 100644 > > --- a/kernel/rcu/srcutree.c > > +++ b/kernel/rcu/srcutree.c > > @@ -366,24 +366,28 @@ static unsigned long srcu_get_delay(struct srcu_struct *sp) > > return SRCU_INTERVAL; > > } > > > > -/** > > - * cleanup_srcu_struct - deconstruct a sleep-RCU structure > > - * @sp: structure to clean up. > > - * > > - * Must invoke this after you are finished using a given srcu_struct that > > - * was initialized via init_srcu_struct(), else you leak memory. > > - */ > > -void cleanup_srcu_struct(struct srcu_struct *sp) > > +/* Helper for cleanup_srcu_struct() and cleanup_srcu_struct_quiesced(). */ > > +void _cleanup_srcu_struct(struct srcu_struct *sp, bool quiesced) > > { > > int cpu; > > > > if (WARN_ON(!srcu_get_delay(sp))) > > - return; /* Leakage unless caller handles error. */ > > + return; /* Just leak it! */ > > if (WARN_ON(srcu_readers_active(sp))) > > - return; /* Leakage unless caller handles error. */ > > - flush_delayed_work(&sp->work); > > + return; /* Just leak it! */ > > + if (quiesced) { > > + if (WARN_ON(delayed_work_pending(&sp->work))) > > + return; /* Just leak it! */ > > + } else { > > + flush_delayed_work(&sp->work); > > + } > > for_each_possible_cpu(cpu) > > - flush_delayed_work(&per_cpu_ptr(sp->sda, cpu)->work); > > + if (quiesced) { > > + if (WARN_ON(delayed_work_pending(&per_cpu_ptr(sp->sda, cpu)->work))) > > + return; /* Just leak it! */ > > + } else { > > + flush_delayed_work(&per_cpu_ptr(sp->sda, cpu)->work); > > + } > > if (WARN_ON(rcu_seq_state(READ_ONCE(sp->srcu_gp_seq)) != SRCU_STATE_IDLE) || > > WARN_ON(srcu_readers_active(sp))) { > > pr_info("%s: Active srcu_struct %p state: %d\n", __func__, sp, rcu_seq_state(READ_ONCE(sp->srcu_gp_seq))); > > @@ -392,7 +396,7 @@ void cleanup_srcu_struct(struct srcu_struct *sp) > > free_percpu(sp->sda); > > sp->sda = NULL; > > } > > -EXPORT_SYMBOL_GPL(cleanup_srcu_struct); > > +EXPORT_SYMBOL_GPL(_cleanup_srcu_struct); > > > > /* > > * Counts the new reader in the appropriate per-CPU element of the > ---end quoted text--- >