Re: 3.11-rc regression bisected: s2disk does not work (was Re: [PATCH v3 13/16] futex: use freezable blocking call)

[Colin Cross <ccross@android.com>]

[-- Attachment #1: Type: text/plain, Size: 2505 bytes --]

On Mon, Jul 22, 2013 at 11:28 PM, Colin Cross <ccross@android.com> wrote:
> On Mon, Jul 22, 2013 at 6:41 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> On Monday, July 22, 2013 05:42:49 PM Colin Cross wrote:
>>> On Mon, Jul 22, 2013 at 5:32 PM, Linus Torvalds
>>> <torvalds@linux-foundation.org> wrote:
>>> > On Mon, Jul 22, 2013 at 4:55 PM, Colin Cross <ccross@android.com> wrote:
>>> >>
>>> >> I think the right solution is to add a flag to the freezing task that
>>> >> marks it unfreezable.  I  think PF_NOFREEZE would work, although it is
>>> >> normally used on kernel threads, can you see if the attached patch
>>> >> helps?
>>> >
>>> > Hmm. That does seem to be the right thing to do, but I wonder about
>>> > the *other* callers of freeze_processes() IOW, kexec and friends.
>>> >
>>> > So maybe we should do this in {freeze|thaw}_processes() itself, and
>>> > just make the rule be that the caller of freeze_processes() itself is
>>> > obviously not frozen, and has to be the same one that then thaws
>>> > things?
>>> >
>>> > Colin? Rafael? Comments?
>>> >
>>> >                 Linus
>>>
>>> I was worried about clearing the flag in thaw_processes().  If a
>>> kernel thread with PF_NOFREEZE set ever called thaw_processes(), which
>>> autosleep might do, it would clear the flag.  Or if a different thread
>>> called freeze_processes() and thaw_processes().
>>
>> Is that legitimate?
>
> Nothing precludes it today, but I don't see any need for it.  I'll add
> a comment when I add the flag.
>
>>> All the other callers besides the SNAPSHOT_FREEZE ioctl stay in the kernel
>>> between freeze_processes() and thaw_processes(), which makes the fanout of
>>> places that could call try_to_freeze() much more controllable.
>>>
>>> Using a new flag that operates like PF_NOFREEZE but doesn't conflict
>>> with it, or a nofreeze_depth counter, would also work.
>>
>> Well, that would be robust enough.  At least if the purpose of that new flag
>> is clearly specified, people hopefully won't be tempted to optimize it away in
>> the future.
>>
>> Thanks,
>> Rafael
>
> OK, I'll add a new flag.


Michael, can you see if this patch works and doesn't throw any
warnings during suspend or resume?

If the extra process flag is considered too precious for this
(there are only 2 left after this patch) I could get the
same functionality by having freeze_processes() reject calls
from a PF_KTHREAD|PF_NOFREEZE thread, and use PF_KTHREAD to
determine if PF_NOFREEZE should be cleared in thaw_processes().

[-- Attachment #2: 0001-power-set-PF_SUSPEND_TASK-flag-on-tasks-that-call-fr.patch --]
[-- Type: application/octet-stream, Size: 4006 bytes --]

From 188f58711ad19315a1abaa05000db9d312aaa93f Mon Sep 17 00:00:00 2001
From: Colin Cross <ccross@android.com>
Date: Mon, 22 Jul 2013 16:53:15 -0700
Subject: [PATCH] power: set PF_SUSPEND_TASK flag on tasks that call
 freeze_processes

Calling freeze_processes sets a global flag that will cause any
process that calls try_to_freeze to enter the refrigerator.  It
skips sending a signal to the current task, but if the current
task ever hits try_to_freeze all threads will be frozen and the
system will deadlock.

Set a new flag, PF_SUSPEND_TASK, on the task that calls
freeze_processes.  The flag notifies the freezer that the thread
is involved in suspend and should not be frozen.  Also add a
WARN_ON in thaw_processes if the caller does not have the
PF_SUSPEND_TASK flag set to catch if a different task calls
thaw_processes than the one that called freeze_processes, leaving
a task with PF_SUSPEND_TASK permanently set on it.

Threads that spawn off a task with PF_SUSPEND_TASK set (which
swsusp does) will also have PF_SUSPEND_TASK set, preventing them
from freezing while they are helping with suspend, but they need
to be dead by the time suspend is triggered, otherwise they may
run when userspace is expected to be frozen.  Add a WARN_ON in
thaw_processes if more than one thread has the PF_SUSPEND_TASK
flag set.

Reported-by: Michael Leun <lkml20130126@newton.leun.net>
Signed-off-by: Colin Cross <ccross@android.com>
---
 include/linux/sched.h  |  1 +
 kernel/freezer.c       |  2 +-
 kernel/power/process.c | 11 +++++++++++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 50d04b9..d722490 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1628,6 +1628,7 @@ extern void thread_group_cputime_adjusted(struct task_struct *p, cputime_t *ut,
 #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_SUSPEND_TASK 0x80000000      /* this thread called freeze_processes and should not be frozen */
 
 /*
  * Only the _current_ task can read/write to tsk->flags, but other
diff --git a/kernel/freezer.c b/kernel/freezer.c
index 8b2afc1..b462fa1 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -33,7 +33,7 @@ static DEFINE_SPINLOCK(freezer_lock);
  */
 bool freezing_slow_path(struct task_struct *p)
 {
-	if (p->flags & PF_NOFREEZE)
+	if (p->flags & (PF_NOFREEZE | PF_SUSPEND_TASK))
 		return false;
 
 	if (pm_nosig_freezing || cgroup_freezing(p))
diff --git a/kernel/power/process.c b/kernel/power/process.c
index fc0df84..06ec886 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -109,6 +109,8 @@ static int try_to_freeze_tasks(bool user_only)
 
 /**
  * freeze_processes - Signal user space processes to enter the refrigerator.
+ * The current thread will not be frozen.  The same process that calls
+ * freeze_processes must later call thaw_processes.
  *
  * On success, returns 0.  On failure, -errno and system is fully thawed.
  */
@@ -120,6 +122,9 @@ int freeze_processes(void)
 	if (error)
 		return error;
 
+	/* Make sure this task doesn't get frozen */
+	current->flags |= PF_SUSPEND_TASK;
+
 	if (!pm_freezing)
 		atomic_inc(&system_freezing_cnt);
 
@@ -168,6 +173,7 @@ int freeze_kernel_threads(void)
 void thaw_processes(void)
 {
 	struct task_struct *g, *p;
+	struct task_struct *curr = current;
 
 	if (pm_freezing)
 		atomic_dec(&system_freezing_cnt);
@@ -182,10 +188,15 @@ void thaw_processes(void)
 
 	read_lock(&tasklist_lock);
 	do_each_thread(g, p) {
+		/* No other threads should have PF_SUSPEND_TASK set */
+		WARN_ON((p != curr) && (p->flags & PF_SUSPEND_TASK));
 		__thaw_task(p);
 	} while_each_thread(g, p);
 	read_unlock(&tasklist_lock);
 
+	WARN_ON(!(curr->flags & PF_SUSPEND_TASK));
+	curr->flags &= ~PF_SUSPEND_TASK;
+
 	usermodehelper_enable();
 
 	schedule();
-- 
1.8.3