[PATCH] nvme: fix flush dependency in delete controller flow
Paul E. McKenney
paulmck at linux.vnet.ibm.com
Sun Apr 8 09:48:03 PDT 2018
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