[PATCH] nvme: fix flush dependency in delete controller flow

[paulmck@linux.vnet.ibm.com (Paul E. McKenney)]

On Sun, Apr 08, 2018@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@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---
> >>
> >
>