dmatest: don't use set_freezable_with_signal()

PATCH In review Tejun Heo 2 participants 5 messages Nov 21, 2011 – Nov 23, 2011

Subsystems (2)

Linux-next tree dmaengine

lore

Share & tools

lore

Tejun Heo

author

2011-11-21 18:43 UTC #0 lore raw

PATCH 1/1 dmatest: don't use set_freezable_with_signal()

Commit 981ed70d8e (dmatest: make dmatest threads freezable) made dmatest kthread use set_freezable_with_signal(); however, the interface is scheduled to be removed in the next merge window.

The problem is that unlike userland tasks there's no default place which handles signal pending state and it isn't clear who owns and/or is responsible for clearing TIF_SIGPENDING. For example, in the current code, try_to_freeze() clears TIF_SIGPENDING but it isn't sure whether it actually owns the TIF_SIGPENDING nor is it race-free - ie. the task may continue to run with TIF_SIGPENDING set after the freezable section.

Unfortunately, we don't have wait_for_completion_freezable_timeout(). This patch open codes it and uses wait_event_freezable_timeout() instead and removes timeout reloading - wait_event_freezable_timeout() won't return across freezing events (currently racy but fix scheduled) and timer doesn't decrement while the task is in freezer. Although this does lose timer-reset-over-freezing, given that timeout is supposed to be long enough and failure to finish inside is considered irrecoverable, I don't think this is worth the complexity.

While at it, move completion to outer scope and explain that we're ignoring dangling pointer problem after timeout. This should give slightly better chance at avoiding oops after timeout.

Signed-off-by Tejun Heo <[email protected]>

Cc Guennadi Liakhovetski <[email protected]>

Cc Dan Williams <[email protected]>

Cc Nicolas Ferre <[email protected]>

---
Guennadi, Dan, how does this look? If it's okay, do you guys mind routing this through pm tree? I have some patches stacked on top removal of freezable_with_signal and it would be much easier to route these together.

Thank you.

 drivers/dma/dmatest.c |   44 ++++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)

drivers/dma/dmatest.c +26 ~~-18~~

index eb1d864..baa6b8d 100644
@@ -214,9 +214,18 @@ static unsigned int dmatest_verify(u8 **bufs, unsigned int start,
 	return error_count;
 }
 
-static void dmatest_callback(void *completion)
+/* poor man's completion - we want to use wait_event_freezable() on it */
+struct dmatest_done {
+	bool			done;
+	wait_queue_head_t	*wait;
+};
+
+static void dmatest_callback(void *arg)
 {
-	complete(completion);
+	struct dmatest_done *done = arg;
+
+	done->done = true;
+	wake_up_all(done->wait);
 }
 
 /*

@@ -235,7 +244,9 @@ static void dmatest_callback(void *completion)
  */
 static int dmatest_func(void *data)
 {
+	DECLARE_WAIT_QUEUE_HEAD_ONSTACK(done_wait);
 	struct dmatest_thread	*thread = data;
+	struct dmatest_done	done = { .wait = &done_wait };
 	struct dma_chan		*chan;
 	const char		*thread_name;
 	unsigned int		src_off, dst_off, len;

@@ -306,9 +317,6 @@ static int dmatest_func(void *data)
 		struct dma_async_tx_descriptor *tx = NULL;
 		dma_addr_t dma_srcs[src_cnt];
 		dma_addr_t dma_dsts[dst_cnt];
-		struct completion cmp;
-		unsigned long start, tmo, end = 0 /* compiler... */;
-		bool reload = true;
 		u8 align = 0;
 
 		total_tests++;

@@ -391,9 +399,9 @@ static int dmatest_func(void *data)
 			continue;
 		}
 
-		init_completion(&cmp);
+		done.done = false;
 		tx->callback = dmatest_callback;
-		tx->callback_param = &cmp;
+		tx->callback_param = &done;
 		cookie = tx->tx_submit(tx);
 
 		if (dma_submit_error(cookie)) {

@@ -407,20 +415,20 @@ static int dmatest_func(void *data)
 		}
 		dma_async_issue_pending(chan);
 
-		do {
-			start = jiffies;
-			if (reload)
-				end = start + msecs_to_jiffies(timeout);
-			else if (end <= start)
-				end = start + 1;
-			tmo = wait_for_completion_interruptible_timeout(&cmp,
-								end - start);
-			reload = try_to_freeze();
-		} while (tmo == -ERESTARTSYS);
+		wait_event_freezable_timeout(done_wait, done.done,
+					     msecs_to_jiffies(timeout));
 
 		status = dma_async_is_tx_complete(chan, cookie, NULL, NULL);
 
-		if (tmo == 0) {
+		if (!done.done) {
+			/*
+			 * We're leaving the timed out dma operation with
+			 * dangling pointer to done_wait.  To make this
+			 * correct, we'll need to allocate wait_done for
+			 * each test iteration and perform "who's gonna
+			 * free it this time?" dancing.  For now, just
+			 * leave it dangling.
+			 */
 			pr_warning("%s: #%u: test timed out\n",
 				   thread_name, total_tests - 1);
 			failed_tests++;

Tejun Heo

author

2011-11-21 18:49 UTC #1 lore raw

Quoted 29 lines · Tejun Heo

Commit 981ed70d8e (dmatest: make dmatest threads freezable) made dmatest kthread use set_freezable_with_signal(); however, the interface is scheduled to be removed in the next merge window.
The problem is that unlike userland tasks there's no default place which handles signal pending state and it isn't clear who owns and/or is responsible for clearing TIF_SIGPENDING. For example, in the current code, try_to_freeze() clears TIF_SIGPENDING but it isn't sure whether it actually owns the TIF_SIGPENDING nor is it race-free - ie. the task may continue to run with TIF_SIGPENDING set after the freezable section.
Unfortunately, we don't have wait_for_completion_freezable_timeout(). This patch open codes it and uses wait_event_freezable_timeout() instead and removes timeout reloading - wait_event_freezable_timeout() won't return across freezing events (currently racy but fix scheduled) and timer doesn't decrement while the task is in freezer. Although this does lose timer-reset-over-freezing, given that timeout is supposed to be long enough and failure to finish inside is considered irrecoverable, I don't think this is worth the complexity.
While at it, move completion to outer scope and explain that we're ignoring dangling pointer problem after timeout. This should give slightly better chance at avoiding oops after timeout.
Signed-off-by Tejun Heo <[email protected]>
Cc Guennadi Liakhovetski <[email protected]>
Cc Dan Williams <[email protected]>
Cc Nicolas Ferre <[email protected]>

Quoted 5 lines

---
Guennadi, Dan, how does this look? If it's okay, do you guys mind routing this through pm tree? I have some patches stacked on top removal of freezable_with_signal and it would be much easier to route these together.

Ooh, forgot to mention that it's only compile tested.

Thanks.

Signature · 1 line

tejun

Tejun Heo

author

2011-11-21 19:20 UTC #2 lore raw

PATCH 1/1 dmatest: don't use set_freezable_with_signal()

In reply to Tejun Heo

Commit 981ed70d8e (dmatest: make dmatest threads freezable) made dmatest kthread use set_freezable_with_signal(); however, the interface is scheduled to be removed in the next merge window.

While at it, move completion to outer scope and explain that we're ignoring dangling pointer problem after timeout. This should give slightly better chance at avoiding oops after timeout.

Signed-off-by Tejun Heo <[email protected]>

Cc Guennadi Liakhovetski <[email protected]>

Cc Dan Williams <[email protected]>

Cc Nicolas Ferre <[email protected]>

---
Oops, forgot to replace set_freezable_with_signal() with set_freezable(). Updated.

Thank you.

 drivers/dma/dmatest.c |   46 +++++++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 19 deletions(-)

drivers/dma/dmatest.c +27 ~~-19~~

Index: work/drivers/dma/dmatest.c
===================================================================
@@ -214,9 +214,18 @@ static unsigned int dmatest_verify(u8 **
 	return error_count;
 }
 
-static void dmatest_callback(void *completion)
+/* poor man's completion - we want to use wait_event_freezable() on it */
+struct dmatest_done {
+	bool			done;
+	wait_queue_head_t	*wait;
+};
+
+static void dmatest_callback(void *arg)
 {
-	complete(completion);
+	struct dmatest_done *done = arg;
+
+	done->done = true;
+	wake_up_all(done->wait);
 }
 
 /*

@@ -235,7 +244,9 @@ static void dmatest_callback(void *compl
  */
 static int dmatest_func(void *data)
 {
+	DECLARE_WAIT_QUEUE_HEAD_ONSTACK(done_wait);
 	struct dmatest_thread	*thread = data;
+	struct dmatest_done	done = { .wait = &done_wait };
 	struct dma_chan		*chan;
 	const char		*thread_name;
 	unsigned int		src_off, dst_off, len;

@@ -252,7 +263,7 @@ static int dmatest_func(void *data)
 	int			i;
 
 	thread_name = current->comm;
-	set_freezable_with_signal();
+	set_freezable();
 
 	ret = -ENOMEM;

@@ -306,9 +317,6 @@ static int dmatest_func(void *data)
 		struct dma_async_tx_descriptor *tx = NULL;
 		dma_addr_t dma_srcs[src_cnt];
 		dma_addr_t dma_dsts[dst_cnt];
-		struct completion cmp;
-		unsigned long start, tmo, end = 0 /* compiler... */;
-		bool reload = true;
 		u8 align = 0;
 
 		total_tests++;

@@ -391,9 +399,9 @@ static int dmatest_func(void *data)
 			continue;
 		}
 
-		init_completion(&cmp);
+		done.done = false;
 		tx->callback = dmatest_callback;
-		tx->callback_param = &cmp;
+		tx->callback_param = &done;
 		cookie = tx->tx_submit(tx);
 
 		if (dma_submit_error(cookie)) {

@@ -407,20 +415,20 @@ static int dmatest_func(void *data)
 		}
 		dma_async_issue_pending(chan);
 
-		do {
-			start = jiffies;
-			if (reload)
-				end = start + msecs_to_jiffies(timeout);
-			else if (end <= start)
-				end = start + 1;
-			tmo = wait_for_completion_interruptible_timeout(&cmp,
-								end - start);
-			reload = try_to_freeze();
-		} while (tmo == -ERESTARTSYS);
+		wait_event_freezable_timeout(done_wait, done.done,
+					     msecs_to_jiffies(timeout));
 
 		status = dma_async_is_tx_complete(chan, cookie, NULL, NULL);
 
-		if (tmo == 0) {
+		if (!done.done) {
+			/*
+			 * We're leaving the timed out dma operation with
+			 * dangling pointer to done_wait.  To make this
+			 * correct, we'll need to allocate wait_done for
+			 * each test iteration and perform "who's gonna
+			 * free it this time?" dancing.  For now, just
+			 * leave it dangling.
+			 */
 			pr_warning("%s: #%u: test timed out\n",
 				   thread_name, total_tests - 1);
 			failed_tests++;

Williams, Dan J

Acked-by on v1 1/1

2011-11-23 07:48 UTC #3 lore raw

In reply to Tejun Heo

Quoted 29 lines · Tejun Heo

Commit 981ed70d8e (dmatest: make dmatest threads freezable) made dmatest kthread use set_freezable_with_signal(); however, the interface is scheduled to be removed in the next merge window.
The problem is that unlike userland tasks there's no default place which handles signal pending state and it isn't clear who owns and/or is responsible for clearing TIF_SIGPENDING. For example, in the current code, try_to_freeze() clears TIF_SIGPENDING but it isn't sure whether it actually owns the TIF_SIGPENDING nor is it race-free - ie. the task may continue to run with TIF_SIGPENDING set after the freezable section.
Unfortunately, we don't have wait_for_completion_freezable_timeout(). This patch open codes it and uses wait_event_freezable_timeout() instead and removes timeout reloading - wait_event_freezable_timeout() won't return across freezing events (currently racy but fix scheduled) and timer doesn't decrement while the task is in freezer. Although this does lose timer-reset-over-freezing, given that timeout is supposed to be long enough and failure to finish inside is considered irrecoverable, I don't think this is worth the complexity.
While at it, move completion to outer scope and explain that we're ignoring dangling pointer problem after timeout. This should give slightly better chance at avoiding oops after timeout.
Signed-off-by Tejun Heo <[email protected]>
Cc Guennadi Liakhovetski <[email protected]>
Cc Dan Williams <[email protected]>
Cc Nicolas Ferre <[email protected]>

Quoted 7 lines

---
Guennadi, Dan, how does this look? If it's okay, do you guys mind routing this through pm tree? I have some patches stacked on top removal of freezable_with_signal and it would be much easier to route these together.
Thank you.

Acked-by Dan Williams <[email protected]>

(for the revised commit a75ca055 on
git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git pm-freezer)

Tejun Heo

author

2011-11-23 17:39 UTC #4 lore raw

Hello,

Quoted 11 lines · Dan J

Quoted 6 lines
Guennadi, Dan, how does this look? If it's okay, do you guys mind routing this through pm tree? I have some patches stacked on top removal of freezable_with_signal and it would be much easier to route these together.
Thank you.
Acked-by Dan Williams <[email protected]>
(for the revised commit a75ca055 on
git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git pm-freezer)

Awesome, will add Acked-by and push it to Rafael.

Thank you.

Signature · 1 line

tejun