Skip to contentSkip to conversation

nvme: fix flush dependency in delete controller flow

PATCH New Nitzan Carmi 4 participants 19 messages Apr 2, 2018 – Apr 10, 2018

Subsystems (4)
NVMe RCU Torture-test modules Sleepable read-copy update (SRCU)
lore
Share & tools
lore

Nitzan Carmi

author
#0 lore raw

PATCH 1/1 nvme: fix flush dependency in delete controller flow

nvme_delete_ctrl queues a work on a MEM_RECLAIM queue (nvme_delete_wq), which eventually calls cleanup_srcu_struct, which in turn flushes a delayed work from an !MEM_RECLAIM queue. This is unsafe as we might trigger deadlocks under severe memory pressure.

Fix this by moving the cleanups to a seperate work over the safe !MEM_RECLAIM system_wq.

Fixes ed754e5dee ("nvme: track shared namespaces")
Signed-off-by Nitzan Carmi <nitzanc at mellanox.com>
Reviewed-by Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/host/core.c | 12 ++++++++++--
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)
drivers/nvme/host/core.c +10 -2
index 75f3e4c..7fc5d9d 100644
@@ -362,6 +362,14 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 }
 EXPORT_SYMBOL_GPL(nvme_change_ctrl_state);
 
+static void nvme_free_ns_head_work(struct work_struct *work) {
+	struct nvme_ns_head *head =
+		container_of(work, struct nvme_ns_head, free_work);
+
+	cleanup_srcu_struct(&head->srcu);
+	kfree(head);
+}
+
 static void nvme_free_ns_head(struct kref *ref)
 {
 	struct nvme_ns_head *head =

@@ -370,8 +378,7 @@ static void nvme_free_ns_head(struct kref *ref)
 	nvme_mpath_remove_disk(head);
 	ida_simple_remove(&head->subsys->ns_ida, head->instance);
 	list_del_init(&head->entry);
-	cleanup_srcu_struct(&head->srcu);
-	kfree(head);
+	queue_work(system_wq, &head->free_work);
 }
 
 static void nvme_put_ns_head(struct nvme_ns_head *head)

@@ -3099,6 +3106,7 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 		goto out_free_head;
 	head->instance = ret;
 	INIT_LIST_HEAD(&head->list);
+	INIT_WORK(&head->free_work, nvme_free_ns_head_work);
 	init_srcu_struct(&head->srcu);
 	head->subsys = ctrl->subsys;
 	head->ns_id = nsid;
drivers/nvme/host/nvme.h +1 -0
index 178aced..f443608 100644
@@ -270,6 +270,7 @@ struct nvme_ns_head {
 	spinlock_t		requeue_lock;
 	struct work_struct	requeue_work;
 #endif
+	struct work_struct	free_work;
 	struct list_head	list;
 	struct srcu_struct      srcu;
 	struct nvme_subsystem	*subsys;
Signature · 1 line
1.8.2.3
maintainer
#1 lore raw

PATCH 1/1 nvme: fix flush dependency in delete controller flow

Quoted 8 lines · Nitzan Carmi

nvme_delete_ctrl queues a work on a MEM_RECLAIM queue (nvme_delete_wq), which eventually calls cleanup_srcu_struct, which in turn flushes a delayed work from an !MEM_RECLAIM queue. This is unsafe as we might trigger deadlocks under severe memory pressure.

Fix this by moving the cleanups to a seperate work over the safe !MEM_RECLAIM system_wq.

This seems like an odd workaround. It seems like not allowing cleanup_srcu_struct from mem reclaim wqs is a really odd API issue.

Paul, anything we can do there?

Quoted 59 lines
Fixes ed754e5dee ("nvme: track shared namespaces")
Signed-off-by Nitzan Carmi <nitzanc at mellanox.com>
Reviewed-by Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/host/core.c | 12 ++++++++++--
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)
drivers/nvme/host/core.c +10 -2
index 75f3e4c..7fc5d9d 100644
@@ -362,6 +362,14 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 }
 EXPORT_SYMBOL_GPL(nvme_change_ctrl_state);
 
+static void nvme_free_ns_head_work(struct work_struct *work) {
+	struct nvme_ns_head *head =
+		container_of(work, struct nvme_ns_head, free_work);
+
+	cleanup_srcu_struct(&head->srcu);
+	kfree(head);
+}
+
 static void nvme_free_ns_head(struct kref *ref)
 {
 	struct nvme_ns_head *head =

@@ -370,8 +378,7 @@ static void nvme_free_ns_head(struct kref *ref)
 	nvme_mpath_remove_disk(head);
 	ida_simple_remove(&head->subsys->ns_ida, head->instance);
 	list_del_init(&head->entry);
-	cleanup_srcu_struct(&head->srcu);
-	kfree(head);
+	queue_work(system_wq, &head->free_work);
 }
 
 static void nvme_put_ns_head(struct nvme_ns_head *head)

@@ -3099,6 +3106,7 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 		goto out_free_head;
 	head->instance = ret;
 	INIT_LIST_HEAD(&head->list);
+	INIT_WORK(&head->free_work, nvme_free_ns_head_work);
 	init_srcu_struct(&head->srcu);
 	head->subsys = ctrl->subsys;
 	head->ns_id = nsid;
drivers/nvme/host/nvme.h +1 -0
index 178aced..f443608 100644
@@ -270,6 +270,7 @@ struct nvme_ns_head {
 	spinlock_t		requeue_lock;
 	struct work_struct	requeue_work;
 #endif
+	struct work_struct	free_work;
 	struct list_head	list;
 	struct srcu_struct      srcu;
 	struct nvme_subsystem	*subsys;
Signature · 1 line
1.8.2.3

---end quoted text---

#2 lore raw

PATCH 1/1 nvme: fix flush dependency in delete controller flow

Quoted 7 lines · Christoph Hellwig
Quoted 6 lines · Nitzan Carmi

nvme_delete_ctrl queues a work on a MEM_RECLAIM queue (nvme_delete_wq), which eventually calls cleanup_srcu_struct, which in turn flushes a delayed work from an !MEM_RECLAIM queue. This is unsafe as we might trigger deadlocks under severe memory pressure.

Quoted 8 lines
Quoted 2 lines

Fix this by moving the cleanups to a seperate work over the safe !MEM_RECLAIM system_wq.

This seems like an odd workaround. It seems like not allowing cleanup_srcu_struct from mem reclaim wqs is a really odd API issue.

Paul, anything we can do there?

Looks like the culprit is the need to do flush_delayed_work() from within cleanup_srcu_struct(). If I don't do that, then the tail end of a grace period, for example, due to a recent call_srcu(), would find itself operating on the freelist.

But if this SRCU use case doesn't ever invoke call_srcu(), I -think- that shouldn't need to do the flush_delayed_work() calls in cleanup_srcu_struct(). I think. It is a bit early out here, so I don't trust myself on this one just now, and need to take another look when fully awake.

But in the meantime, is it really the case that this SRCU use case never ever invokes call_srcu()?

Thanx, Paul

Quoted 61 lines
Quoted 59 lines
Fixes ed754e5dee ("nvme: track shared namespaces")
Signed-off-by Nitzan Carmi <nitzanc at mellanox.com>
Reviewed-by Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/host/core.c | 12 ++++++++++--
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)
drivers/nvme/host/core.c +10 -2
index 75f3e4c..7fc5d9d 100644
@@ -362,6 +362,14 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 }
 EXPORT_SYMBOL_GPL(nvme_change_ctrl_state);
 
+static void nvme_free_ns_head_work(struct work_struct *work) {
+	struct nvme_ns_head *head =
+		container_of(work, struct nvme_ns_head, free_work);
+
+	cleanup_srcu_struct(&head->srcu);
+	kfree(head);
+}
+
 static void nvme_free_ns_head(struct kref *ref)
 {
 	struct nvme_ns_head *head =

@@ -370,8 +378,7 @@ static void nvme_free_ns_head(struct kref *ref)
 	nvme_mpath_remove_disk(head);
 	ida_simple_remove(&head->subsys->ns_ida, head->instance);
 	list_del_init(&head->entry);
-	cleanup_srcu_struct(&head->srcu);
-	kfree(head);
+	queue_work(system_wq, &head->free_work);
 }
 
 static void nvme_put_ns_head(struct nvme_ns_head *head)

@@ -3099,6 +3106,7 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 		goto out_free_head;
 	head->instance = ret;
 	INIT_LIST_HEAD(&head->list);
+	INIT_WORK(&head->free_work, nvme_free_ns_head_work);
 	init_srcu_struct(&head->srcu);
 	head->subsys = ctrl->subsys;
 	head->ns_id = nsid;
drivers/nvme/host/nvme.h +1 -0
index 178aced..f443608 100644
@@ -270,6 +270,7 @@ struct nvme_ns_head {
 	spinlock_t		requeue_lock;
 	struct work_struct	requeue_work;
 #endif
+	struct work_struct	free_work;
 	struct list_head	list;
 	struct srcu_struct      srcu;
 	struct nvme_subsystem	*subsys;
Signature · 1 line
1.8.2.3

---end quoted text---

#3 lore raw

PATCH 1/1 nvme: fix flush dependency in delete controller flow

Quoted 22 lines · Paul E. McKenney
Quoted 7 lines · Christoph Hellwig
Quoted 6 lines · Nitzan Carmi

nvme_delete_ctrl queues a work on a MEM_RECLAIM queue (nvme_delete_wq), which eventually calls cleanup_srcu_struct, which in turn flushes a delayed work from an !MEM_RECLAIM queue. This is unsafe as we might trigger deadlocks under severe memory pressure.

Quoted 8 lines
Quoted 2 lines

Fix this by moving the cleanups to a seperate work over the safe !MEM_RECLAIM system_wq.

This seems like an odd workaround. It seems like not allowing cleanup_srcu_struct from mem reclaim wqs is a really odd API issue.

Paul, anything we can do there?

Looks like the culprit is the need to do flush_delayed_work() from within cleanup_srcu_struct(). If I don't do that, then the tail end of a grace period, for example, due to a recent call_srcu(), would find itself operating on the freelist.

Quoted 8 lines

But if this SRCU use case doesn't ever invoke call_srcu(), I -think- that shouldn't need to do the flush_delayed_work() calls in cleanup_srcu_struct(). I think. It is a bit early out here, so I don't trust myself on this one just now, and need to take another look when fully awake.

But in the meantime, is it really the case that this SRCU use case never ever invokes call_srcu()?

And I can do a bit better than this. I could provide something like a cleanup_srcu_struct_quiesced() that would avoid blocking if the caller had quiesced it. Here, to quiesce is to make sure that any synchronize_srcu() or synchronize_srcu_expedited() calls have returned before the call to cleanup_srcu_struct_quiesced(), and that any callbacks from call_srcu() have completed. Of course, srcu_barrier() could be used to wait for the callbacks from earlier call_srcu().

Would something like this help?

Thanx, Paul

Quoted 61 lines
Quoted 61 lines
Quoted 59 lines
Fixes ed754e5dee ("nvme: track shared namespaces")
Signed-off-by Nitzan Carmi <nitzanc at mellanox.com>
Reviewed-by Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/host/core.c | 12 ++++++++++--
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)
drivers/nvme/host/core.c +10 -2
index 75f3e4c..7fc5d9d 100644
@@ -362,6 +362,14 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 }
 EXPORT_SYMBOL_GPL(nvme_change_ctrl_state);
 
+static void nvme_free_ns_head_work(struct work_struct *work) {
+	struct nvme_ns_head *head =
+		container_of(work, struct nvme_ns_head, free_work);
+
+	cleanup_srcu_struct(&head->srcu);
+	kfree(head);
+}
+
 static void nvme_free_ns_head(struct kref *ref)
 {
 	struct nvme_ns_head *head =

@@ -370,8 +378,7 @@ static void nvme_free_ns_head(struct kref *ref)
 	nvme_mpath_remove_disk(head);
 	ida_simple_remove(&head->subsys->ns_ida, head->instance);
 	list_del_init(&head->entry);
-	cleanup_srcu_struct(&head->srcu);
-	kfree(head);
+	queue_work(system_wq, &head->free_work);
 }
 
 static void nvme_put_ns_head(struct nvme_ns_head *head)

@@ -3099,6 +3106,7 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 		goto out_free_head;
 	head->instance = ret;
 	INIT_LIST_HEAD(&head->list);
+	INIT_WORK(&head->free_work, nvme_free_ns_head_work);
 	init_srcu_struct(&head->srcu);
 	head->subsys = ctrl->subsys;
 	head->ns_id = nsid;
drivers/nvme/host/nvme.h +1 -0
index 178aced..f443608 100644
@@ -270,6 +270,7 @@ struct nvme_ns_head {
 	spinlock_t		requeue_lock;
 	struct work_struct	requeue_work;
 #endif
+	struct work_struct	free_work;
 	struct list_head	list;
 	struct srcu_struct      srcu;
 	struct nvme_subsystem	*subsys;
Signature · 1 line
1.8.2.3

---end quoted text---

maintainer
#4 lore raw

PATCH 1/1 nvme: fix flush dependency in delete controller flow

Quoted 8 lines · Paul E. McKenney
Quoted 8 lines

But if this SRCU use case doesn't ever invoke call_srcu(), I -think- that shouldn't need to do the flush_delayed_work() calls in cleanup_srcu_struct(). I think. It is a bit early out here, so I don't trust myself on this one just now, and need to take another look when fully awake.

But in the meantime, is it really the case that this SRCU use case never ever invokes call_srcu()?

We never use call_srcu().

Quoted 9 lines

And I can do a bit better than this. I could provide something like a cleanup_srcu_struct_quiesced() that would avoid blocking if the caller had quiesced it. Here, to quiesce is to make sure that any synchronize_srcu() or synchronize_srcu_expedited() calls have returned before the call to cleanup_srcu_struct_quiesced(), and that any callbacks from call_srcu() have completed. Of course, srcu_barrier() could be used to wait for the callbacks from earlier call_srcu().

Would something like this help?

Yes. Note that blocking isn't even the problem, just the flush_work. But not blocking at all would be fine of course.

The pattern in nvme is that we have a struct nvme_ns_head, which has a list of struct nvme_ns structures hanging off it. We use SRCU to protect lookups in said list. We call synchronize_srcu after removing a nvme_ns from the list in struct nvme_ns_head and nowhere else. A struct nvme_ns_head is freed (and we thus call cleanup_srcu_struct) once the reference count on it hits zero, which can only happen after the last nvme_ns has been removed from the list.

I suspect this might be a common patter in other parts of the kernel as well.

#5 lore raw

PATCH 1/1 nvme: fix flush dependency in delete controller flow

Quoted 33 lines · Christoph Hellwig
Quoted 8 lines · Paul E. McKenney
Quoted 8 lines

But if this SRCU use case doesn't ever invoke call_srcu(), I -think- that shouldn't need to do the flush_delayed_work() calls in cleanup_srcu_struct(). I think. It is a bit early out here, so I don't trust myself on this one just now, and need to take another look when fully awake.

But in the meantime, is it really the case that this SRCU use case never ever invokes call_srcu()?

We never use call_srcu().

Quoted 9 lines

And I can do a bit better than this. I could provide something like a cleanup_srcu_struct_quiesced() that would avoid blocking if the caller had quiesced it. Here, to quiesce is to make sure that any synchronize_srcu() or synchronize_srcu_expedited() calls have returned before the call to cleanup_srcu_struct_quiesced(), and that any callbacks from call_srcu() have completed. Of course, srcu_barrier() could be used to wait for the callbacks from earlier call_srcu().

Would something like this help?

Yes. Note that blocking isn't even the problem, just the flush_work. But not blocking at all would be fine of course.

The pattern in nvme is that we have a struct nvme_ns_head, which has a list of struct nvme_ns structures hanging off it. We use SRCU to protect lookups in said list. We call synchronize_srcu after removing a nvme_ns from the list in struct nvme_ns_head and nowhere else. A struct nvme_ns_head is freed (and we thus call cleanup_srcu_struct) once the reference count on it hits zero, which can only happen after the last nvme_ns has been removed from the list.

Quoted 2 lines

I suspect this might be a common patter in other parts of the kernel as well.

OK, how does the following (untested) patch look?

Thanx, Paul

------------------------------------------------------------------------

include/linux/srcu.h +35 -1
index 33c1c698df09..91494d7e8e41 100644
@@ -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
 
 /**
kernel/rcu/rcutorture.c +6 -1
index 680c96d8c00f..f0e1d44459f8 100644
@@ -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. */
 }
kernel/rcu/srcutiny.c +6 -3
index 76ac5f50b2c7..622792abe41a 100644
@@ -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
kernel/rcu/srcutree.c +17 -13
index fb560fca9ef4..b4123d7a2cec 100644
@@ -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
maintainer
#6 lore raw

PATCH 1/1 nvme: fix flush dependency in delete controller flow

Quoted 1 line · Paul E. McKenney

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?

Quoted 147 lines

Thanx, Paul

------------------------------------------------------------------------

include/linux/srcu.h +35 -1
index 33c1c698df09..91494d7e8e41 100644
@@ -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
 
 /**
kernel/rcu/rcutorture.c +6 -1
index 680c96d8c00f..f0e1d44459f8 100644
@@ -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. */
 }
kernel/rcu/srcutiny.c +6 -3
index 76ac5f50b2c7..622792abe41a 100644
@@ -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
kernel/rcu/srcutree.c +16 -12
index fb560fca9ef4..b4123d7a2cec 100644
@@ -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)));

kernel/rcu/srcutree.c void cleanup_srcu_struct(struct srcu_struct *sp) · line 402

Quoted 8 lines
kernel/rcu/srcutree.c +1 -1
@@ -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---

#7 lore raw

PATCH 1/1 nvme: fix flush dependency in delete controller flow

Quoted 5 lines · Christoph Hellwig
Quoted 1 line · Paul E. McKenney

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

Quoted 154 lines
Quoted 144 lines

------------------------------------------------------------------------

include/linux/srcu.h +35 -1
index 33c1c698df09..91494d7e8e41 100644
@@ -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
 
 /**
kernel/rcu/rcutorture.c +6 -1
index 680c96d8c00f..f0e1d44459f8 100644
@@ -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. */
 }
kernel/rcu/srcutiny.c +6 -3
index 76ac5f50b2c7..622792abe41a 100644
@@ -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
kernel/rcu/srcutree.c +16 -12
index fb560fca9ef4..b4123d7a2cec 100644
@@ -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)));

kernel/rcu/srcutree.c void cleanup_srcu_struct(struct srcu_struct *sp) · line 402

Quoted 8 lines
kernel/rcu/srcutree.c +1 -1
@@ -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---

author
#8 lore raw

PATCH 1/1 nvme: fix flush dependency in delete controller flow

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.

Thanks!

Quoted 166 lines · Paul E. McKenney
Quoted 5 lines · Christoph Hellwig
Quoted 1 line · Paul E. McKenney

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

Quoted 154 lines
Quoted 144 lines

------------------------------------------------------------------------

include/linux/srcu.h +35 -1
index 33c1c698df09..91494d7e8e41 100644
@@ -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
 
  /**
kernel/rcu/rcutorture.c +6 -1
index 680c96d8c00f..f0e1d44459f8 100644
@@ -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. */
  }
kernel/rcu/srcutiny.c +6 -3
index 76ac5f50b2c7..622792abe41a 100644
@@ -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
kernel/rcu/srcutree.c +16 -12
index fb560fca9ef4..b4123d7a2cec 100644
@@ -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)));

kernel/rcu/srcutree.c void cleanup_srcu_struct(struct srcu_struct *sp) · line 402

Quoted 8 lines
kernel/rcu/srcutree.c +1 -1
@@ -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---

#9 lore raw

PATCH 1/1 nvme: fix flush dependency in delete controller flow

Quoted 4 lines · Nitzan Carmi

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

Quoted 161 lines

Thanks!

Quoted 157 lines · Paul E. McKenney
Quoted 5 lines · Christoph Hellwig
Quoted 1 line · Paul E. McKenney

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

Quoted 145 lines
Quoted 136 lines

------------------------------------------------------------------------

include/linux/srcu.h +35 -1
index 33c1c698df09..91494d7e8e41 100644
@@ -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
 /**
kernel/rcu/rcutorture.c +6 -1
index 680c96d8c00f..f0e1d44459f8 100644
@@ -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. */
 }
kernel/rcu/srcutiny.c +6 -3
index 76ac5f50b2c7..622792abe41a 100644
@@ -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
kernel/rcu/srcutree.c +16 -12
index fb560fca9ef4..b4123d7a2cec 100644
@@ -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)));

kernel/rcu/srcutree.c void cleanup_srcu_struct(struct srcu_struct *sp) · line 401

Quoted 7 lines
kernel/rcu/srcutree.c +1 -1
@@ -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---

author
#10 lore raw

PATCH 1/1 nvme: fix flush dependency in delete controller flow

I'll be honored :-).

Tested-by Nitzan Carmi <nitzanc at mellanox.com>
Quoted 172 lines · Paul E. McKenney
Quoted 4 lines · Nitzan Carmi

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

Quoted 161 lines

Thanks!

Quoted 157 lines · Paul E. McKenney
Quoted 5 lines · Christoph Hellwig
Quoted 1 line · Paul E. McKenney

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

Quoted 145 lines
Quoted 136 lines

------------------------------------------------------------------------

include/linux/srcu.h +35 -1
index 33c1c698df09..91494d7e8e41 100644
@@ -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
  /**
kernel/rcu/rcutorture.c +6 -1
index 680c96d8c00f..f0e1d44459f8 100644
@@ -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. */
  }
kernel/rcu/srcutiny.c +6 -3
index 76ac5f50b2c7..622792abe41a 100644
@@ -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
kernel/rcu/srcutree.c +16 -12
index fb560fca9ef4..b4123d7a2cec 100644
@@ -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)));

kernel/rcu/srcutree.c void cleanup_srcu_struct(struct srcu_struct *sp) · line 401

Quoted 7 lines
kernel/rcu/srcutree.c +1 -1
@@ -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---

maintainer
#11 lore raw

PATCH 1/1 nvme: fix flush dependency in delete controller flow

Quoted 3 lines · Nitzan Carmi

I'll be honored :-).

Tested-by Nitzan Carmi <nitzanc at mellanox.com>

Paul, Nitzan - how do we want to handle the new series? Might make sense to add the nvme path to the rcu series? Nitzan, can you send the patch to linux-nvme and Paul?

author
#12 lore raw

PATCH 1/1 nvme: avoid flush dependency in delete controller flow

nvme_delete_ctrl queues a work on a MEM_RECLAIM queue (nvme_delete_wq), which eventually calls cleanup_srcu_struct, which in turn flushes a delayed work from an !MEM_RECLAIM queue. This is unsafe as we might trigger deadlocks under severe memory pressure.

Since we don't ever invoke call_srcu(), it is safe to use the _quiesced() version of srcu cleanup, and avoid that flush dependency.

Signed-off-by Nitzan Carmi <nitzanc at mellanox.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/host/core.c +1 -1
index 197a6ba9700f..0ef5c495ca07 100644
@@ -349,7 +349,7 @@ static void nvme_free_ns_head(struct kref *ref)
 	nvme_mpath_remove_disk(head);
 	ida_simple_remove(&head->subsys->ns_ida, head->instance);
 	list_del_init(&head->entry);
-	cleanup_srcu_struct(&head->srcu);
+	cleanup_srcu_struct_quiesced(&head->srcu);
 	kfree(head);
 }
Signature · 1 line
2.14.1
#13 lore raw

PATCH 1/1 nvme: avoid flush dependency in delete controller flow

Quoted 11 lines · Nitzan Carmi

nvme_delete_ctrl queues a work on a MEM_RECLAIM queue (nvme_delete_wq), which eventually calls cleanup_srcu_struct, which in turn flushes a delayed work from an !MEM_RECLAIM queue. This is unsafe as we might trigger deadlocks under severe memory pressure.

Since we don't ever invoke call_srcu(), it is safe to use the _quiesced() version of srcu cleanup, and avoid that flush dependency.

Signed-off-by Nitzan Carmi <nitzanc at mellanox.com>

Very good, thank you!

I queued this with a few edits to the commit log (please see below), so please let me know if I messed anything up.

Given that it only happens under high memory pressure, I am assuming that sending this up the v4.18 merge window (not this one, but the next one) will work. Please let me know if this is more urgent than that. (If it is not more urgent, I would like to allow the extra time for people to get used to yet another addition to the SRCU API.)

------------------------------------------------------------------------

commit 6fde861519fc6e4be50c130272be556ef5656324

Author: Nitzan Carmi <nitzanc at mellanox.com>
Date:   Mon Apr 9 17:50:26 2018 +0300

nvme: Avoid flush dependency in delete controller flow

The nvme_delete_ctrl() function queues a work item on a MEM_RECLAIM queue (nvme_delete_wq), which eventually calls cleanup_srcu_struct(), which in turn flushes a delayed work from an !MEM_RECLAIM queue. This is unsafe as we might trigger deadlocks under severe memory pressure.

Since we don't ever invoke call_srcu(), it is safe to use the shiny new _quiesced() version of srcu cleanup, thus avoiding that flush dependency. This commit makes that change.

Signed-off-by Nitzan Carmi <nitzanc at mellanox.com>
Signed-off-by Paul E. McKenney <paulmck at linux.vnet.ibm.com>
drivers/nvme/host/core.c +1 -1
index 7aeca5db7916..40008d06c365 100644
@@ -351,7 +351,7 @@ static void nvme_free_ns_head(struct kref *ref)
 	nvme_mpath_remove_disk(head);
 	ida_simple_remove(&head->subsys->ns_ida, head->instance);
 	list_del_init(&head->entry);
-	cleanup_srcu_struct(&head->srcu);
+	cleanup_srcu_struct_quiesced(&head->srcu);
 	kfree(head);
 }
#14 lore raw

PATCH 1/1 nvme: avoid flush dependency in delete controller flow

Quoted 15 lines · Paul E. McKenney
Quoted 11 lines · Nitzan Carmi

nvme_delete_ctrl queues a work on a MEM_RECLAIM queue (nvme_delete_wq), which eventually calls cleanup_srcu_struct, which in turn flushes a delayed work from an !MEM_RECLAIM queue. This is unsafe as we might trigger deadlocks under severe memory pressure.

Since we don't ever invoke call_srcu(), it is safe to use the _quiesced() version of srcu cleanup, and avoid that flush dependency.

Signed-off-by Nitzan Carmi <nitzanc at mellanox.com>

Very good, thank you!

Quoted 8 lines

I queued this with a few edits to the commit log (please see below), so please let me know if I messed anything up.

Given that it only happens under high memory pressure, I am assuming that sending this up the v4.18 merge window (not this one, but the next one) will work. Please let me know if this is more urgent than that. (If it is not more urgent, I would like to allow the extra time for people to get used to yet another addition to the SRCU API.)

I guess we can wait to v4.18 but not sure we actually need to wait that long, Christoph ?

Quoted 27 lines

------------------------------------------------------------------------

commit 6fde861519fc6e4be50c130272be556ef5656324

Author: Nitzan Carmi <nitzanc at mellanox.com>
Date:   Mon Apr 9 17:50:26 2018 +0300

nvme: Avoid flush dependency in delete controller flow

The nvme_delete_ctrl() function queues a work item on a MEM_RECLAIM queue (nvme_delete_wq), which eventually calls cleanup_srcu_struct(), which in turn flushes a delayed work from an !MEM_RECLAIM queue. This is unsafe as we might trigger deadlocks under severe memory pressure.

Since we don't ever invoke call_srcu(), it is safe to use the shiny new _quiesced() version of srcu cleanup, thus avoiding that flush dependency. This commit makes that change.

Signed-off-by Nitzan Carmi <nitzanc at mellanox.com>
Signed-off-by Paul E. McKenney <paulmck at linux.vnet.ibm.com>
drivers/nvme/host/core.c +0 -0
index 7aeca5db7916..40008d06c365 100644

drivers/nvme/host/core.c static void nvme_free_ns_head(struct kref *ref) · line 357

Quoted 8 lines
drivers/nvme/host/core.c +1 -1
@@ -351,7 +351,7 @@ static void nvme_free_ns_head(struct kref *ref)
  	nvme_mpath_remove_disk(head);
  	ida_simple_remove(&head->subsys->ns_ida, head->instance);
  	list_del_init(&head->entry);
-	cleanup_srcu_struct(&head->srcu);
+	cleanup_srcu_struct_quiesced(&head->srcu);
  	kfree(head);
  }

Looks good, thanks Nitzan and Paul.
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

Quoted 6 lines

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-nvme&data=02%7C01%7Cmaxg%40mellanox.com%7Cd1344fa628dc47e8a5f308d59e399a13%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636588892600812825&sdata=HTf3qkTyLUzE24yXG16UF%2BHVoA5OjykV6naG6o7M7M0%3D&reserved=0

#15 lore raw

PATCH 1/1 nvme: avoid flush dependency in delete controller flow

Quoted 21 lines · Max Gurtovoy
Quoted 18 lines · Paul E. McKenney
Quoted 11 lines · Nitzan Carmi

nvme_delete_ctrl queues a work on a MEM_RECLAIM queue (nvme_delete_wq), which eventually calls cleanup_srcu_struct, which in turn flushes a delayed work from an !MEM_RECLAIM queue. This is unsafe as we might trigger deadlocks under severe memory pressure.

Since we don't ever invoke call_srcu(), it is safe to use the _quiesced() version of srcu cleanup, and avoid that flush dependency.

Signed-off-by Nitzan Carmi <nitzanc at mellanox.com>

Very good, thank you!

I queued this with a few edits to the commit log (please see below), so please let me know if I messed anything up.

Quoted 8 lines
Quoted 5 lines

Given that it only happens under high memory pressure, I am assuming that sending this up the v4.18 merge window (not this one, but the next one) will work. Please let me know if this is more urgent than that. (If it is not more urgent, I would like to allow the extra time for people to get used to yet another addition to the SRCU API.)

I guess we can wait to v4.18 but not sure we actually need to wait that long, Christoph ?

Or to look at it another way, do these two patches need to go to -stable, and if so, how far back?

Quoted 17 lines
Quoted 17 lines

------------------------------------------------------------------------

commit 6fde861519fc6e4be50c130272be556ef5656324

Author: Nitzan Carmi <nitzanc at mellanox.com>
Date:   Mon Apr 9 17:50:26 2018 +0300

nvme: Avoid flush dependency in delete controller flow
The nvme_delete_ctrl() function queues a work item on a MEM_RECLAIM queue (nvme_delete_wq), which eventually calls cleanup_srcu_struct(), which in turn flushes a delayed work from an !MEM_RECLAIM queue. This is unsafe as we might trigger deadlocks under severe memory pressure. Since we don't ever invoke call_srcu(), it is safe to use the shiny new _quiesced() version of srcu cleanup, thus avoiding that flush dependency. This commit makes that change.
Signed-off-by: Nitzan Carmi <nitzanc at mellanox.com>
Signed-off-by: Paul E. McKenney <paulmck at linux.vnet.ibm.com>

Quoted 15 lines

drivers/nvme/host/core.c static void nvme_free_ns_head(struct kref *ref) · line 356

Quoted 12 lines
drivers/nvme/host/core.c +1 -1
index 7aeca5db7916..40008d06c365 100644
@@ -351,7 +351,7 @@ static void nvme_free_ns_head(struct kref *ref)
 	nvme_mpath_remove_disk(head);
 	ida_simple_remove(&head->subsys->ns_ida, head->instance);
 	list_del_init(&head->entry);
-	cleanup_srcu_struct(&head->srcu);
+	cleanup_srcu_struct_quiesced(&head->srcu);
 	kfree(head);
 }

Looks good, thanks Nitzan and Paul.
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

Added your Reviewed-by, thank you!

Thanx, Paul

#16 lore raw

PATCH 1/1 nvme: avoid flush dependency in delete controller flow

Quoted 28 lines · Paul E. McKenney
Quoted 27 lines · Max Gurtovoy
Quoted 23 lines · Paul E. McKenney
Quoted 11 lines · Nitzan Carmi

nvme_delete_ctrl queues a work on a MEM_RECLAIM queue (nvme_delete_wq), which eventually calls cleanup_srcu_struct, which in turn flushes a delayed work from an !MEM_RECLAIM queue. This is unsafe as we might trigger deadlocks under severe memory pressure.

Since we don't ever invoke call_srcu(), it is safe to use the _quiesced() version of srcu cleanup, and avoid that flush dependency.

Signed-off-by Nitzan Carmi <nitzanc at mellanox.com>

Very good, thank you!

I queued this with a few edits to the commit log (please see below), so please let me know if I messed anything up.

Given that it only happens under high memory pressure, I am assuming that sending this up the v4.18 merge window (not this one, but the next one) will work. Please let me know if this is more urgent than that. (If it is not more urgent, I would like to allow the extra time for people to get used to yet another addition to the SRCU API.)

Quoted 5 lines
Quoted 2 lines

I guess we can wait to v4.18 but not sure we actually need to wait that long, Christoph ?

Or to look at it another way, do these two patches need to go to -stable, and if so, how far back?

I don't know if they need to go to -stable but if so then for nvme_core it should go to v4.15 (below commit):

commit ed754e5deeb17f4e675c84e4b6c640cc7344e498

Author: Christoph Hellwig <hch at lst.de>
Date:   Thu Nov 9 13:50:43 2017 +0100

nvme: track shared namespaces

Introduce a new struct nvme_ns_head that holds information about an
actual
namespace, unlike struct nvme_ns, which only holds the per-controller namespace information. For private namespaces there is a 1:1
relation of
the two, but for shared namespaces this lets us discover all the
paths to
it. For now only the identifiers are moved to the new structure,
but most
of the information in struct nvme_ns should eventually move over.

To allow lockless path lookup the list of nvme_ns structures per nvme_ns_head is protected by SRCU, which requires freeing the nvme_ns structure through call_srcu.

Signed-off-by Christoph Hellwig <hch at lst.de>
Reviewed-by Keith Busch <keith.busch at intel.com>
Reviewed-by Javier Gonz<C3><A1>lez <javier at cnexlabs.com>
Reviewed-by Sagi Grimberg <sagi at grimberg.me>
Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>
Reviewed-by Martin K. Petersen <martin.petersen at oracle.com>
Reviewed-by Hannes Reinecke <hare at suse.com>
Signed-off-by Jens Axboe <axboe at kernel.dk>

As for SRCU, I guess you can find it better than me :)

Quoted 38 lines
Quoted 18 lines
Quoted 18 lines

------------------------------------------------------------------------

commit 6fde861519fc6e4be50c130272be556ef5656324

Author: Nitzan Carmi <nitzanc at mellanox.com>
Date:   Mon Apr 9 17:50:26 2018 +0300

nvme: Avoid flush dependency in delete controller flow
The nvme_delete_ctrl() function queues a work item on a MEM_RECLAIM queue (nvme_delete_wq), which eventually calls cleanup_srcu_struct(), which in turn flushes a delayed work from an !MEM_RECLAIM queue. This is unsafe as we might trigger deadlocks under severe memory pressure. Since we don't ever invoke call_srcu(), it is safe to use the shiny new _quiesced() version of srcu cleanup, thus avoiding that flush dependency. This commit makes that change.
Signed-off-by: Nitzan Carmi <nitzanc at mellanox.com>
Signed-off-by: Paul E. McKenney <paulmck at linux.vnet.ibm.com>

Quoted 15 lines

drivers/nvme/host/core.c static void nvme_free_ns_head(struct kref *ref) · line 356

Quoted 12 lines
drivers/nvme/host/core.c +1 -1
index 7aeca5db7916..40008d06c365 100644
@@ -351,7 +351,7 @@ static void nvme_free_ns_head(struct kref *ref)
  	nvme_mpath_remove_disk(head);
  	ida_simple_remove(&head->subsys->ns_ida, head->instance);
  	list_del_init(&head->entry);
-	cleanup_srcu_struct(&head->srcu);
+	cleanup_srcu_struct_quiesced(&head->srcu);
  	kfree(head);
  }

Looks good, thanks Nitzan and Paul.
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

Added your Reviewed-by, thank you!

Thanx, Paul

#17 lore raw

PATCH 1/1 nvme: avoid flush dependency in delete controller flow

Quoted 70 lines · Max Gurtovoy
Quoted 28 lines · Paul E. McKenney
Quoted 27 lines · Max Gurtovoy
Quoted 23 lines · Paul E. McKenney
Quoted 11 lines · Nitzan Carmi

nvme_delete_ctrl queues a work on a MEM_RECLAIM queue (nvme_delete_wq), which eventually calls cleanup_srcu_struct, which in turn flushes a delayed work from an !MEM_RECLAIM queue. This is unsafe as we might trigger deadlocks under severe memory pressure.

Since we don't ever invoke call_srcu(), it is safe to use the _quiesced() version of srcu cleanup, and avoid that flush dependency.

Signed-off-by Nitzan Carmi <nitzanc at mellanox.com>

Very good, thank you!

I queued this with a few edits to the commit log (please see below), so please let me know if I messed anything up.

Given that it only happens under high memory pressure, I am assuming that sending this up the v4.18 merge window (not this one, but the next one) will work. Please let me know if this is more urgent than that. (If it is not more urgent, I would like to allow the extra time for people to get used to yet another addition to the SRCU API.)

Quoted 5 lines
Quoted 2 lines

I guess we can wait to v4.18 but not sure we actually need to wait that long, Christoph ?

Or to look at it another way, do these two patches need to go to -stable, and if so, how far back?

I don't know if they need to go to -stable but if so then for nvme_core it should go to v4.15 (below commit):

commit ed754e5deeb17f4e675c84e4b6c640cc7344e498

Author: Christoph Hellwig <hch at lst.de>
Date:   Thu Nov 9 13:50:43 2017 +0100

nvme: track shared namespaces

Introduce a new struct nvme_ns_head that holds information about
an actual
namespace, unlike struct nvme_ns, which only holds the per-controller namespace information. For private namespaces there is a 1:1
relation of
the two, but for shared namespaces this lets us discover all the
paths to
it. For now only the identifiers are moved to the new
structure, but most
of the information in struct nvme_ns should eventually move over.

To allow lockless path lookup the list of nvme_ns structures per nvme_ns_head is protected by SRCU, which requires freeing the nvme_ns structure through call_srcu.

Signed-off-by Christoph Hellwig <hch at lst.de>
Reviewed-by Keith Busch <keith.busch at intel.com>
Reviewed-by Javier Gonz<C3><A1>lez <javier at cnexlabs.com>
Reviewed-by Sagi Grimberg <sagi at grimberg.me>
Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>
Reviewed-by Martin K. Petersen <martin.petersen at oracle.com>
Reviewed-by Hannes Reinecke <hare at suse.com>
Signed-off-by Jens Axboe <axboe at kernel.dk>
Quoted 1 line

As for SRCU, I guess you can find it better than me :)

Same answer, since the only reason for the SRCU change is to support your fix. ;-)

Thanx, Paul

Quoted 38 lines
Quoted 37 lines
Quoted 17 lines
Quoted 17 lines

------------------------------------------------------------------------

commit 6fde861519fc6e4be50c130272be556ef5656324

Author: Nitzan Carmi <nitzanc at mellanox.com>
Date:   Mon Apr 9 17:50:26 2018 +0300

nvme: Avoid flush dependency in delete controller flow
The nvme_delete_ctrl() function queues a work item on a MEM_RECLAIM queue (nvme_delete_wq), which eventually calls cleanup_srcu_struct(), which in turn flushes a delayed work from an !MEM_RECLAIM queue. This is unsafe as we might trigger deadlocks under severe memory pressure. Since we don't ever invoke call_srcu(), it is safe to use the shiny new _quiesced() version of srcu cleanup, thus avoiding that flush dependency. This commit makes that change.
Signed-off-by: Nitzan Carmi <nitzanc at mellanox.com>
Signed-off-by: Paul E. McKenney <paulmck at linux.vnet.ibm.com>

Quoted 15 lines

drivers/nvme/host/core.c static void nvme_free_ns_head(struct kref *ref) · line 356

Quoted 12 lines
drivers/nvme/host/core.c +1 -1
index 7aeca5db7916..40008d06c365 100644
@@ -351,7 +351,7 @@ static void nvme_free_ns_head(struct kref *ref)
 	nvme_mpath_remove_disk(head);
 	ida_simple_remove(&head->subsys->ns_ida, head->instance);
 	list_del_init(&head->entry);
-	cleanup_srcu_struct(&head->srcu);
+	cleanup_srcu_struct_quiesced(&head->srcu);
 	kfree(head);
 }

Looks good, thanks Nitzan and Paul.
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

Added your Reviewed-by, thank you!

Thanx, Paul

#18 lore raw

PATCH 1/1 nvme: fix flush dependency in delete controller flow

In reply to Nitzan Carmi
Quoted 3 lines · Nitzan Carmi

I'll be honored :-).

Tested-by Nitzan Carmi <nitzanc at mellanox.com>

Applied, and thank you very much for the testing!

Thanx, Paul

Quoted 174 lines · Paul E. McKenney
Quoted 172 lines · Nitzan Carmi
Quoted 4 lines

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

Quoted 161 lines

Thanks!

Quoted 157 lines · Paul E. McKenney
Quoted 5 lines · Christoph Hellwig
Quoted 1 line · Paul E. McKenney

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

Quoted 145 lines
Quoted 136 lines

------------------------------------------------------------------------

include/linux/srcu.h +35 -1
index 33c1c698df09..91494d7e8e41 100644
@@ -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
 /**
kernel/rcu/rcutorture.c +6 -1
index 680c96d8c00f..f0e1d44459f8 100644
@@ -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. */
 }
kernel/rcu/srcutiny.c +6 -3
index 76ac5f50b2c7..622792abe41a 100644
@@ -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
kernel/rcu/srcutree.c +16 -12
index fb560fca9ef4..b4123d7a2cec 100644
@@ -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)));

kernel/rcu/srcutree.c void cleanup_srcu_struct(struct srcu_struct *sp) · line 401

Quoted 7 lines
kernel/rcu/srcutree.c +1 -1
@@ -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---