[PATCH] nvme: fix flush dependency in delete controller flow
Paul E. McKenney
paulmck at linux.vnet.ibm.com
Mon Apr 9 09:30:05 PDT 2018
On Mon, Apr 09, 2018 at 09:39:01AM +0300, Nitzan Carmi wrote:
> I'll be honored :-).
>
> Tested-by: Nitzan Carmi <nitzanc at mellanox.com>
Applied, and thank you very much for the testing!
Thanx, Paul
> On 08/04/2018 19:48, Paul E. McKenney wrote:
> >On Sun, Apr 08, 2018 at 10:20:10AM +0300, Nitzan Carmi wrote:
> >>Yes, The patch works very well.
> >>It seems to solve the problem (with the conversion to
> >>cleanup_srcu_struct_quiesced in nvme), and definitely better
> >>than my original WA.
> >
> >May I have your Tested-by?
> >
> > Thanx, Paul
> >
> >>Thanks!
> >>
> >>On 06/04/2018 19:30, Paul E. McKenney wrote:
> >>>On Fri, Apr 06, 2018 at 08:19:20AM +0200, Christoph Hellwig wrote:
> >>>>On Thu, Apr 05, 2018 at 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---
> >>>>
> >>>
> >>
> >
>
More information about the Linux-nvme
mailing list