Skip to contentSkip to conversation

usb_storage: don't use set_freezable_with_signal()

PATCH In review Tejun Heo 4 participants 7 messages Nov 1, 2011 – Nov 15, 2011

Subsystems (6)
lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/2 usb_storage: don't use set_freezable_with_signal()

The current implementation of set_freezable_with_signal() is buggy and tricky to get right. usb-storage is the only user and its use can be avoided trivially.

All usb-storage wants is to be able to sleep with timeout and get woken up if freezing() becomes true. This can be trivially implemented by doing interruptible wait w/ freezing() included in the wait condition. There's no reason to use set_freezable_with_signal().

Perform interruptible wait on freezing() instead of using set_freezable_with_signal(), which is scheduled for removal.

Cc Oleg Nesterov <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Seth Forshee <[email protected]>
Cc Alan Stern <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>

---

These two patches are on top of "freezer: fix various bugs and simplify implementation, take#2" patchset[1] and are also available in the following git branch.

git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git pm-kill-freezable_with_signal

If usb-storage ppl are okay with it, I think routing this through pm would be the easiest. Oh, and this definitely is for the next merge window.

Thank you.

[1] http://thread.gmane.org/gmane.linux.kernel/1209247

 drivers/usb/storage/usb.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)
drivers/usb/storage/usb.c +7 -6
index c325e69..aa84b3d 100644
@@ -831,7 +831,8 @@ static int usb_stor_scan_thread(void * __us)
 
 	dev_dbg(dev, "device found\n");
 
-	set_freezable_with_signal();
+	set_freezable();
+
 	/*
 	 * Wait for the timeout to expire or for a disconnect
 	 *
@@ -839,16 +840,16 @@ static int usb_stor_scan_thread(void * __us)
 	 * fail to freeze, but we can't be non-freezable either. Nor can
 	 * khubd freeze while waiting for scanning to complete as it may
 	 * hold the device lock, causing a hang when suspending devices.
-	 * So we request a fake signal when freezing and use
-	 * interruptible sleep to kick us out of our wait early when
-	 * freezing happens.
+	 * So instead of using wait_event_freezable(), explicitly test
+	 * for (DONT_SCAN || freezing) in interruptible wait and proceed
+	 * if any of DONT_SCAN, freezing or timeout has happened.
 	 */
 	if (delay_use > 0) {
 		dev_dbg(dev, "waiting for device to settle "
 				"before scanning\n");
 		wait_event_interruptible_timeout(us->delay_wait,
-				test_bit(US_FLIDX_DONT_SCAN, &us->dflags),
-				delay_use * HZ);
+				test_bit(US_FLIDX_DONT_SCAN, &us->dflags) ||
+				freezing(current), delay_use * HZ);
 	}
 
 	/* If the device is still connected, perform the scanning */
Signature · 1 line
1.7.3.1
author
#1 lore raw

PATCH 2/2 freezer: kill unused set_freezable_with_signal()

There's no in-kernel user of set_freezable_with_signal() left. Mixing TIF_SIGPENDING with kernel threads can lead to nasty corner cases as kernel threads never travel signal delivery path on their own.

e.g. the current implementation is buggy in the cancelation path of __thaw_task(). It calls recalc_sigpending_and_wake() in an attempt to clear TIF_SIGPENDING but the function never clears it regardless of sigpending state. This means that signallable freezable kthreads may continue executing with !freezing() && stuck TIF_SIGPENDING, which can be troublesome.

This patch removes set_freezable_with_signal() along with PF_FREEZER_NOSIG and recalc_sigpending*() calls in freezer. User tasks get TIF_SIGPENDING, kernel tasks get woken up and the spurious sigpending is dealt with in the usual signal delivery path.

---
 include/linux/freezer.h |   20 +-------------------
 include/linux/sched.h   |    1 -
 kernel/freezer.c        |   27 ++++++---------------------
 kernel/kthread.c        |    2 +-
 4 files changed, 8 insertions(+), 42 deletions(-)
include/linux/freezer.h +1 -19
index 4b3dc4c..5f32321 100644
@@ -49,7 +49,7 @@ static inline bool try_to_freeze(void)
 }
 
 extern bool freeze_task(struct task_struct *p);
-extern bool __set_freezable(bool with_signal);
+extern bool set_freezable(void);
 
 #ifdef CONFIG_CGROUP_FREEZER
 extern bool cgroup_freezing(struct task_struct *task);
@@ -105,23 +105,6 @@ static inline int freezer_should_skip(struct task_struct *p)
 }
 
 /*
- * Tell the freezer that the current task should be frozen by it
- */
-static inline bool set_freezable(void)
-{
-	return __set_freezable(false);
-}
-
-/*
- * Tell the freezer that the current task should be frozen by it and that it
- * should send a fake signal to the task to freeze it.
- */
-static inline bool set_freezable_with_signal(void)
-{
-	return __set_freezable(true);
-}
-
-/*
  * Freezer-friendly wrappers around wait_event_interruptible(),
  * wait_event_killable() and wait_event_interruptible_timeout(), originally
  * defined in <linux/wait.h>
@@ -181,7 +164,6 @@ static inline void freezer_do_not_count(void) {}
 static inline void freezer_count(void) {}
 static inline int freezer_should_skip(struct task_struct *p) { return 0; }
 static inline void set_freezable(void) {}
-static inline void set_freezable_with_signal(void) {}
 
 #define wait_event_freezable(wq, condition)				\
 		wait_event_interruptible(wq, condition)
include/linux/sched.h +0 -1
index 57f916d..84fd546 100644
@@ -1781,7 +1781,6 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *
 #define PF_MEMPOLICY	0x10000000	/* Non-default NUMA mempolicy */
 #define PF_MUTEX_TESTER	0x20000000	/* Thread belongs to the rt mutex tester */
 #define PF_FREEZER_SKIP	0x40000000	/* Freezer should not count it as freezable */
-#define PF_FREEZER_NOSIG 0x80000000	/* Freezer won't send signals to it */
 
 /*
  * Only the _current_ task can read/write to tsk->flags, but other
kernel/freezer.c +6 -21
index 0845321..128c5aa 100644
@@ -39,7 +39,7 @@ bool freezing_slow_path(struct task_struct *p)
 	if (pm_nosig_freezing || cgroup_freezing(p))
 		return true;
 
-	if (pm_freezing && !(p->flags & PF_FREEZER_NOSIG))
+	if (pm_freezing && !(p->flags & PF_KTHREAD))
 		return true;
 
 	return false;
@@ -72,10 +72,6 @@ bool __refrigerator(bool check_kthr_stop)
 		schedule();
 	}
 
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-
 	pr_debug("%s left refrigerator\n", current->comm);
 
 	/*
@@ -120,7 +116,7 @@ bool freeze_task(struct task_struct *p)
 		return false;
 	}
 
-	if (!(p->flags & PF_FREEZER_NOSIG)) {
+	if (!(p->flags & PF_KTHREAD)) {
 		fake_signal_wake_up(p);
 		/*
 		 * fake_signal_wake_up() goes through p's scheduler
@@ -145,28 +141,19 @@ void __thaw_task(struct task_struct *p)
 	 * be visible to @p as waking up implies wmb.  Waking up inside
 	 * freezer_lock also prevents wakeups from leaking outside
 	 * refrigerator.
-	 *
-	 * If !FROZEN, @p hasn't reached refrigerator, recalc sigpending to
-	 * avoid leaving dangling TIF_SIGPENDING behind.
 	 */
 	spin_lock_irqsave(&freezer_lock, flags);
-	if (frozen(p)) {
+	if (frozen(p))
 		wake_up_process(p);
-	} else {
-		spin_lock(&p->sighand->siglock);
-		recalc_sigpending_and_wake(p);
-		spin_unlock(&p->sighand->siglock);
-	}
 	spin_unlock_irqrestore(&freezer_lock, flags);
 }
 
 /**
- * __set_freezable - make %current freezable
- * @with_signal: do we want %TIF_SIGPENDING for notification too?
+ * set_freezable - make %current freezable
  *
  * Mark %current freezable and enter refrigerator if necessary.
  */
-bool __set_freezable(bool with_signal)
+bool set_freezable(void)
 {
 	might_sleep();
 
@@ -177,10 +164,8 @@ bool __set_freezable(bool with_signal)
 	 */
 	spin_lock_irq(&freezer_lock);
 	current->flags &= ~PF_NOFREEZE;
-	if (with_signal)
-		current->flags &= ~PF_FREEZER_NOSIG;
 	spin_unlock_irq(&freezer_lock);
 
 	return try_to_freeze();
 }
-EXPORT_SYMBOL(__set_freezable);
+EXPORT_SYMBOL(set_freezable);
kernel/kthread.c +1 -1
index a6cbeea..7a4c862 100644
@@ -282,7 +282,7 @@ int kthreadd(void *unused)
 	set_cpus_allowed_ptr(tsk, cpu_all_mask);
 	set_mems_allowed(node_states[N_HIGH_MEMORY]);
 
-	current->flags |= PF_NOFREEZE | PF_FREEZER_NOSIG;
+	current->flags |= PF_NOFREEZE;
 
 	for (;;) {
 		set_current_state(TASK_INTERRUPTIBLE);
Signature · 1 line
1.7.3.1
#2 lore raw
In reply to Tejun Heo
Quoted 63 lines · Tejun Heo

The current implementation of set_freezable_with_signal() is buggy and tricky to get right. usb-storage is the only user and its use can be avoided trivially.

All usb-storage wants is to be able to sleep with timeout and get woken up if freezing() becomes true. This can be trivially implemented by doing interruptible wait w/ freezing() included in the wait condition. There's no reason to use set_freezable_with_signal().

Perform interruptible wait on freezing() instead of using set_freezable_with_signal(), which is scheduled for removal.

Cc Oleg Nesterov <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Seth Forshee <[email protected]>
Cc Alan Stern <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>

---

These two patches are on top of "freezer: fix various bugs and simplify implementation, take#2" patchset[1] and are also available in the following git branch.

git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git pm-kill-freezable_with_signal

If usb-storage ppl are okay with it, I think routing this through pm would be the easiest. Oh, and this definitely is for the next merge window.

Thank you.

[1] http://thread.gmane.org/gmane.linux.kernel/1209247

 drivers/usb/storage/usb.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)
drivers/usb/storage/usb.c +5 -4
index c325e69..aa84b3d 100644
@@ -831,7 +831,8 @@ static int usb_stor_scan_thread(void * __us)
 
 	dev_dbg(dev, "device found\n");
 
-	set_freezable_with_signal();
+	set_freezable();
+
 	/*
 	 * Wait for the timeout to expire or for a disconnect
 	 *
@@ -839,16 +840,16 @@ static int usb_stor_scan_thread(void * __us)
 	 * fail to freeze, but we can't be non-freezable either. Nor can
 	 * khubd freeze while waiting for scanning to complete as it may
 	 * hold the device lock, causing a hang when suspending devices.
-	 * So we request a fake signal when freezing and use
-	 * interruptible sleep to kick us out of our wait early when
-	 * freezing happens.
+	 * So instead of using wait_event_freezable(), explicitly test
+	 * for (DONT_SCAN || freezing) in interruptible wait and proceed
+	 * if any of DONT_SCAN, freezing or timeout has happened.
 	 */
 	if (delay_use > 0) {

drivers/usb/storage/usb.c static int usb_stor_scan_thread(void * __us) · line 855

Quoted 10 lines
drivers/usb/storage/usb.c +2 -2
 		dev_dbg(dev, "waiting for device to settle "
 				"before scanning\n");
 		wait_event_interruptible_timeout(us->delay_wait,
-				test_bit(US_FLIDX_DONT_SCAN, &us->dflags),
-				delay_use * HZ);
+				test_bit(US_FLIDX_DONT_SCAN, &us->dflags) ||
+				freezing(current), delay_use * HZ);
 	}
 
 	/* If the device is still connected, perform the scanning */

That looks like it ought to work, and it's definitely less convoluted than what's there now. I'd be happy to test it next week when I'm back home and have access to the machine that prompted the original change.

Seth

author
#3 lore raw
Quoted 3 lines · Seth Forshee

That looks like it ought to work, and it's definitely less convoluted than what's there now. I'd be happy to test it next week when I'm back home and have access to the machine that prompted the original change.

Awesome. Thanks a lot.

Signature · 1 line
tejun
Acked-by
#4 lore raw
Quoted 6 lines · Tejun Heo
Quoted 3 lines · Seth Forshee

That looks like it ought to work, and it's definitely less convoluted than what's there now. I'd be happy to test it next week when I'm back home and have access to the machine that prompted the original change.

Awesome. Thanks a lot.

Your changes are working great in my testing.

Acked-by Greg Kroah-Hartman on v1 1/2
#5 lore raw
In reply to Tejun Heo
Quoted 24 lines · Tejun Heo

The current implementation of set_freezable_with_signal() is buggy and tricky to get right. usb-storage is the only user and its use can be avoided trivially.

All usb-storage wants is to be able to sleep with timeout and get woken up if freezing() becomes true. This can be trivially implemented by doing interruptible wait w/ freezing() included in the wait condition. There's no reason to use set_freezable_with_signal().

Perform interruptible wait on freezing() instead of using set_freezable_with_signal(), which is scheduled for removal.

Cc Oleg Nesterov <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Seth Forshee <[email protected]>
Cc Alan Stern <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>

---

These two patches are on top of "freezer: fix various bugs and simplify implementation, take#2" patchset[1] and are also available in the following git branch.

Quoted 5 lines

git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git pm-kill-freezable_with_signal

If usb-storage ppl are okay with it, I think routing this through pm would be the easiest. Oh, and this definitely is for the next merge window.

I'm fine with it going that way:

#6 lore raw
Quoted 25 lines · Greg KH
Quoted 24 lines · Tejun Heo

The current implementation of set_freezable_with_signal() is buggy and tricky to get right. usb-storage is the only user and its use can be avoided trivially.

All usb-storage wants is to be able to sleep with timeout and get woken up if freezing() becomes true. This can be trivially implemented by doing interruptible wait w/ freezing() included in the wait condition. There's no reason to use set_freezable_with_signal().

Perform interruptible wait on freezing() instead of using set_freezable_with_signal(), which is scheduled for removal.

Cc Oleg Nesterov <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Seth Forshee <[email protected]>
Cc Alan Stern <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>

---

These two patches are on top of "freezer: fix various bugs and simplify implementation, take#2" patchset[1] and are also available in the following git branch.

Quoted 8 lines
Quoted 5 lines

git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git pm-kill-freezable_with_signal

If usb-storage ppl are okay with it, I think routing this through pm would be the easiest. Oh, and this definitely is for the next merge window.

I'm fine with it going that way:

It's there in my tree already (through the Tejun's pm-freezer branch).

Thanks,
Rafael