Fix Freezer related races.

This patch fixes the race pointed out by Oleg Nesterov.

* Freezer marks a thread as freezeable.
* The thread now marks itself PF_NOFREEZE causing it to freeze on calling try_to_freeze(). Thus the task is frozen, even though it doesn't want to.
* Subsequent thaw_processes() will also fail to thaw the task since it is marked PF_NOFREEZE.

Avoid this problem by checking the current task's PF_NOFREEZE status in the refrigerator before marking current as frozen.

Signed-off-by Gautham R Shenoy <[email protected]>

Quoted 9 lines · Gautham R Shenoy

This patch fixes the race pointed out by Oleg Nesterov.

* Freezer marks a thread as freezeable.
* The thread now marks itself PF_NOFREEZE causing it to freeze on calling try_to_freeze(). Thus the task is frozen, even though it doesn't want to.
* Subsequent thaw_processes() will also fail to thaw the task since it is marked PF_NOFREEZE.

Quoted 4 lines

Avoid this problem by checking the current task's PF_NOFREEZE status in the refrigerator before marking current as frozen.

Signed-off-by Gautham R Shenoy <[email protected]>

Looks good, although I'm not sure if we don't need to call recalc_sigpending() for tasks that turn out to be PF_NOFREEZE.

Quoted 11 lines · Rafael J. Wysocki

Quoted 9 lines · Gautham R Shenoy

This patch fixes the race pointed out by Oleg Nesterov.

* Freezer marks a thread as freezeable.
* The thread now marks itself PF_NOFREEZE causing it to freeze on calling try_to_freeze(). Thus the task is frozen, even though it doesn't want to.
* Subsequent thaw_processes() will also fail to thaw the task since it is marked PF_NOFREEZE.

Quoted 7 lines

Quoted 4 lines

Avoid this problem by checking the current task's PF_NOFREEZE status in the refrigerator before marking current as frozen.

Signed-off-by Gautham R Shenoy <[email protected]>

Looks good, although I'm not sure if we don't need to call recalc_sigpending() for tasks that turn out to be PF_NOFREEZE.

I agree, we should clear TIF_SIGPENDING. It is not so critical for user-space tasks, but for the kernel thread it may remain pending forever, causing subtle failures.

Gautham, isn't it possible to make a more simpler patch ? Just add PF_NOFREEZE check to frozen_process,

	static inline void frozen_process(struct task_struct *p)
	{
		if (!unlikely(current->flags & PF_NOFREEZE)) {
			p->flags |= PF_FROZEN;
			wmb();
		}
		clear_tsk_thread_flag(p, TIF_FREEZE);
	}

No?

Quoted 4 lines · Oleg Nesterov

Gautham, isn't it possible to make a more simpler patch ? Just add PF_NOFREEZE check to frozen_process,

Quoted 9 lines

	static inline void frozen_process(struct task_struct *p)
	{
		if (!unlikely(current->flags & PF_NOFREEZE)) {
			p->flags |= PF_FROZEN;
			wmb();
		}
		clear_tsk_thread_flag(p, TIF_FREEZE);
	}

Btw, since frozen_process is currently being called only from refrigerator, I am wondering if we still need the struct task_struct *p parameter there. It's very unlikely that some other task would mark a particular task as frozen. No?

Anyways, Andrew, Could you please replace the earlier sent patch titled "fix_pf_nofreeze_and_freezeable_race.patch" with the following one?

...

include/linux/freezer.h +0 -0

@@ -57,8 +57,10 @@ static inline int thaw_process(struct ta
  */

Other than refrigerator, no one else calls frozen_process(). So move it from include/linux/freezer.h to kernel/power/process.c.

Quoted 3 lines · Gautham R Shenoy

Other than refrigerator, no one else calls frozen_process(). So move it from include/linux/freezer.h to kernel/power/process.c.

Could you also remove this

	static inline void frozen_process(struct task_struct *p) { BUG(); }

from freezer.h ?

Threads which wait for completion on a frozen thread might result in causing the freezer to fail, if the waiting thread is freezeable.

There are some well known cases where it's preferable to temporarily thaw the frozen process, finish the wait for completion and allow both the processes to call try_to_freeze.

kthread_stop is one such case.

flush_workqueue might be another.

This patch attempts to address such a situation with a fix for kthread_stop.

Strictly experimental. Compile tested on i386.

static inline int freezeable(struct task_struct * p) {

kernel/power/process.c +3 -1

 	if ((p == current) ||

@@ -45,7 +55,8 @@ void refrigerator(void)
 		 * *after* the freezer did the freezeable() check
 		 * on us.
 		 */
-		if (current->flags & PF_NOFREEZE) {
+		if ((current->flags & PF_NOFREEZE) ||
+		    test_tsk_thread_flag(current, TIF_FREEZER_HELD)) {
 			clear_tsk_thread_flag(current, TIF_FREEZE);
 			task_unlock(current);
 			return;

@@ -63,12 +74,16 @@ void refrigerator(void)
 	recalc_sigpending(); /* We sent fake signal, clean it up */
 	spin_unlock_irq(&current->sighand->siglock);
 
+	task_lock(current);
 	for (;;) {

include/asm-ia64/thread_info.h +4 -0

+#define TIF_FREEZER_HELD	21	/* is temporarily holding up the
+					 * process freezer
+					 */
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)

@@ -102,6 +105,7 @@ struct thread_info {
 #define _TIF_MCA_INIT		(1 << TIF_MCA_INIT)
 #define _TIF_DB_DISABLED	(1 << TIF_DB_DISABLED)
 #define _TIF_FREEZE		(1 << TIF_FREEZE)
+#define _TIF_FREEZER_HELD	(1 << TIF_FREEZER_HELD)
 
 /* "work to do on user-return" bits */
 #define TIF_ALLWORK_MASK	(_TIF_NOTIFY_RESUME|_TIF_SIGPENDING|_TIF_NEED_RESCHED|_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT)

Index: linux-2.6.21-rc6/include/asm-mips/thread_info.h
===================================================================

include/asm-mips/thread_info.h +1 -0

@@ -120,6 +120,7 @@ register struct thread_info *__current_t
 #define TIF_MEMDIE		18
 #define TIF_FREEZE		19
 #define TIF_ALLOW_FP_IN_KERNEL	20
+#define TIF_FREEZER_HELD	21
 #define TIF_SYSCALL_TRACE	31	/* syscall trace active */
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)

Quoted 8 lines · On Thu, 19 Apr 2007 17:34:19 +0530 Gautham R Shenoy <[email protected]> wrote:

Threads which wait for completion on a frozen thread might result in causing the freezer to fail, if the waiting thread is freezeable.

There are some well known cases where it's preferable to temporarily thaw the frozen process, finish the wait for completion and allow both the processes to call try_to_freeze.

kthread_stop is one such case.

hm.

Quoted 1 line

flush_workqueue might be another.

flush_workqueue() just needs to die. I think there are (almost) no legitimate users of it once cancel_work_sync() is merged.

Quoted 1 line

This patch attempts to address such a situation with a fix for kthread_stop.

Via wholly undescribed means :(

Quoted 1 line

Strictly experimental. Compile tested on i386.

Rather than doing <whatever you did>, perhaps we could make the freezing process a dual-pass thing. On pass 1, mark all the threads as "we'll be freezing you soon" and on the second pass, do the actual freezing. Then, in problematic places such as kthread_stop() we can look to see if we'll soon be asked to freeze and if so, run try_to_freeze().

Of course, running try_to_freeze() in kthread_stop() would be very wrong, so we'd actually need to do it in callers, preferably via a new kthread_stop_freezeable() wrapper.

And the two-pass-freeze thing is of course racy. It's also unnecessary:
setting a flag on every task in the machine is equivalent to setting a global variable. So perhaps just use a global variable?

int kthread_stop_freezeable(struct task_struct *k)

{
	if (freeze_state == ABOUT_TO_START) {
		wait_for(freeze_state == STARTED);
		try_to_freeze();
	}
	kthread_stop(k);
}

which is theoretically racy if another freeze_processes() starts immediately. Anyway - please have a think about it ;)

Hmm, can't we do something like this instead:

---
 kernel/kthread.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Index: linux-2.6.21-rc7/kernel/kthread.c
===================================================================

kernel/kthread.c +1 -0

@@ -13,6 +13,7 @@
 #include <linux/file.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/freezer.h>
 #include <asm/semaphore.h>
 
 /*

@@ -232,6 +233,15 @@ int kthread_stop(struct task_struct *k)
 
 	/* Now set kthread_should_stop() to true, and wake it up. */

Quoted 22 lines · Rafael J. Wysocki

Hmm, can't we do something like this instead:

---
 kernel/kthread.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Index: linux-2.6.21-rc7/kernel/kthread.c
===================================================================

kernel/kthread.c +1 -0

@@ -13,6 +13,7 @@
 #include <linux/file.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/freezer.h>
 #include <asm/semaphore.h>
 
 /*

@@ -232,6 +233,15 @@ int kthread_stop(struct task_struct *k)
 
 	/* Now set kthread_should_stop() to true, and wake it up. */

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 244

Quoted 10 lines

kernel/kthread.c +9 -0

 	kthread_stop_info.k = k;
+	if (!(current->flags & PF_NOFREEZE)) {
+		/* If we are freezable, the freezer will wait for us */
+		task_lock(k);
+		k->flags |= PF_NOFREEZE;
+		if (frozen(k))
+			k->flags &= ~PF_FROZEN;
+
+		task_unlock(k);
+	}

Yes, we can do this for now since the tasks have only two freeze states, namely Freezeable and Non Freezeable.
But with more events like cpu-hotplug and kprobes using the process freezer, the simple check

	if(!(current->flags & PF_NOFREEZE))

may not suffice and something like

	if(!(current->flags & PFE_ALL & global_freeze_mask))

/* global_freeze_mask is the mask of the events for which the system
is freezing. */

appears to be racy. Not that we cannot work it out ;-)

However, I was attempting to solve the generic problem where A waits on B and B is frozen. If A is freezeable (under one of the events) then the freezer will fail. So a solution would be for A to somehow inform B of the dependency and postpone it's freezing.

Since akpm mentioned that flush_workqueue() needs to go, I guess, I am ok with fixing only kthread_stop/kthread_should_stop for the moment. Unless I can spot any other valid case :)

Actually, I thought about it for a while. The thread that is going to stop another one may temporarily mark itself as freezable in all cases, which will have no effect on it, since it's not going to cally try_to_freeze(), but will make the freezer wait for it. Next, after returning from wait_for_completion(), it should restore its old freezability status and that should make the freezer finish.

Quoted 5 lines

However, I was attempting to solve the generic problem where A waits on B and B is frozen. If A is freezeable (under one of the events) then the freezer will fail. So a solution would be for A to somehow inform B of the dependency and postpone it's freezing.

Well, I think it might be simpler to consider each case separately. This way we may be able to avoid introducing the additional TIF_ flag.

Quoted 3 lines

Since akpm mentioned that flush_workqueue() needs to go, I guess, I am ok with fixing only kthread_stop/kthread_should_stop for the moment. Unless I can spot any other valid case :)

Sure. :-)

BTW, if it turns out that we need to introduce yet another freezer-related TIF_ flag, it may be acceptable (?) to move all of the freezer-related flags into a separate member of task_struct (eg. freezer_flags) that can only be manipulated under task_lock().

I mean, we already have four of them (PF_NOFREEZE, PF_FROZEN, PF_FREEZER_SKIP, TIF_FREEZE), and you will need to introduce two more for the freezer-based CPU hotplug, so if yet another one is needed, that will make up almost a separate u8 field ...

Greetings,
Rafael
-

Quoted 7 lines · Rafael J. Wysocki

Actually, I thought about it for a while. The thread that is going to stop another one may temporarily mark itself as freezable in all cases, which will have no effect on it, since it's not going to cally try_to_freeze(), but will make the freezer wait for it. Next, after returning from wait_for_completion(), it should restore its old freezability status and that should make the freezer finish.

But that will have no affect if the thread to be stopped is already frozen. The only affect might be that now, freezer will fail whether the thread that is going to stop another one was freezeable or not.

Concern is whether we can somehow complete these wait_for_completion()'s in the freezing context and reduce the probabilty of freezer failing.

Quoted 8 lines

Quoted 4 lines

However, I was attempting to solve the generic problem where A waits on B and B is frozen. If A is freezeable (under one of the events) then the freezer will fail. So a solution would be for A to somehow inform B of the dependency and postpone it's freezing.

Well, I think it might be simpler to consider each case separately. This way we may be able to avoid introducing the additional TIF_ flag.

Makes sense.

Quoted 11 lines

Quoted 3 lines

Since akpm mentioned that flush_workqueue() needs to go, I guess, I am ok with fixing only kthread_stop/kthread_should_stop for the moment. Unless I can spot any other valid case :)

Sure. :-)

BTW, if it turns out that we need to introduce yet another freezer-related TIF_ flag, it may be acceptable (?) to move all of the freezer-related flags into a separate member of task_struct (eg. freezer_flags) that can only be manipulated under task_lock().

Quoted 4 lines

I mean, we already have four of them (PF_NOFREEZE, PF_FROZEN, PF_FREEZER_SKIP, TIF_FREEZE), and you will need to introduce two more for the freezer-based CPU hotplug, so if yet another one is needed, that will make up almost a separate u8 field ...

I am perfectly ok with it. But I am not sure if everybody would agree to have another field in the task struct, though in this case it does make sense :-)

Quoted 4 lines

I mean, we already have four of them (PF_NOFREEZE, PF_FROZEN, PF_FREEZER_SKIP, TIF_FREEZE), and you will need to introduce two more for the freezer-based CPU hotplug, so if yet another one is needed, that will make up almost a separate u8 field ...

I am perfectly ok with it. But I am not sure if everybody would agree to have another field in the task struct, though in this case it does make sense :-)

Quoted 8 lines

Quoted 4 lines

I mean, we already have four of them (PF_NOFREEZE, PF_FROZEN, PF_FREEZER_SKIP, TIF_FREEZE), and you will need to introduce two more for the freezer-based CPU hotplug, so if yet another one is needed, that will make up almost a separate u8 field ...

I am perfectly ok with it. But I am not sure if everybody would agree to have another field in the task struct, though in this case it does make sense :-)

OK by me. You might want to consider making that fields's locking protocol be set_bit(), clear_bit(), etc rather than task_lock().

Quoted 5 lines · Andrew Morton

Quoted 5 lines

Quoted 4 lines

I mean, we already have four of them (PF_NOFREEZE, PF_FROZEN, PF_FREEZER_SKIP, TIF_FREEZE), and you will need to introduce two more for the freezer-based CPU hotplug, so if yet another one is needed, that will make up almost a separate u8 field ...

Quoted 6 lines

Quoted 3 lines

I am perfectly ok with it. But I am not sure if everybody would agree to have another field in the task struct, though in this case it does make sense :-)

OK by me. You might want to consider making that fields's locking protocol be set_bit(), clear_bit(), etc rather than task_lock().

is OK to me too, the extra field isnt a problem.

Now that the freezer is used by kprobes, it is no longer a PM-specific piece of code. Move the freezer code out of kernel/power and introduce the CONFIG_FREEZER option that will be chosen automatically if PM or KPROBES is set.

Signed-off-by Rafael J. Wysocki <[email protected]>

Move all of the freezer-related flags to a separate field in task_struct and introduce functions to operate them using set_bit() etc.

Quoted 2 lines · Rafael

Move all of the freezer-related flags to a separate field in task_struct and introduce functions to operate them using set_bit() etc.

It's getting time I learned what this freezer thing is.

What would you suggest I read?

I looked in include/linux/freezer.h and didn't see any explanations. I found one Documenation file, power/kernel_threads.txt, that explained the interaction of freezing and kernel threads. I looked in the comments for various 2.6.21-rc6-mm1 freezer* patches, and saw various interesting details.

But I couldn't find any documentation telling me what a freezer was, or what a refrigerator is.

Did I miss something?

I'll try to explain how it works.

One more question - why would I want to do this?

Is this like something that would be useful on a laptop, to suspend activity and reduce battery drain, while preserving the current state of ones sessions and avoiding having to logout or shutdown?

Is it useful for quietting a system down before doing hot plug or unplug of key components, such as processors and memory?

static inline int freezing(struct task_struct *p)

/*
- * Sometimes we may need to cancel the previous 'freeze' request
+ * Cancel the previous 'freeze' request
*/
static inline void do_not_freeze(struct task_struct *p)

static inline void frozen_process(struct task_struct *p)

static inline void freezer_do_not_count(void)
static inline void freezer_count(void)

Hi Rafael,

include/linux/freezer.h line 15

Quoted 10 lines

include/linux/freezer.h +7 -0

+/*
+ *     Per task flags used by the freezer
+ *
+ *     They should not be referred to directly outside of this file.
+ */
+#define TFF_NOFREEZE   0       /* task should not be frozen */
+#define TFF_FREEZE     8       /* task should go to the refrigerator ASAP 

*/

include/linux/freezer.h +2 -0

+#define TFF_SKIP       9       /* do not count this task as freezable */
+#define TFF_FROZEN     10      /* task is frozen */

Aren't NOFREEZE and SKIP doing the same thing? One of them appears superfluous. I'm looking at 21-rc6-mm1 and vfork(2) seems to be its only user. Seeing how vfork(2) used it, can't the call to freezer_do_not_count() be replaced with a call to freezer_exempt()? Similarly, the freezer_count() after the wait_for_completion might just as well be a clear of the NOFREEZE bit followed by a try_to_freeze(). Could you please explain the rationale behind the SKIP flag?

Cheers,
Satyam

Hi Satyam,

Quoted 13 lines · Satyam Sharma

Hi Rafael,

include/linux/freezer.h line 15

Quoted 10 lines

include/linux/freezer.h +7 -0

+/*
+ *     Per task flags used by the freezer
+ *
+ *     They should not be referred to directly outside of this file.
+ */
+#define TFF_NOFREEZE   0       /* task should not be frozen */
+#define TFF_FREEZE     8       /* task should go to the refrigerator ASAP 

*/

include/linux/freezer.h +2 -0

+#define TFF_SKIP       9       /* do not count this task as freezable */
+#define TFF_FROZEN     10      /* task is frozen */

Quoted 8 lines

Aren't NOFREEZE and SKIP doing the same thing? One of them appears superfluous. I'm looking at 21-rc6-mm1 and vfork(2) seems to be its only user. Seeing how vfork(2) used it, can't the call to freezer_do_not_count() be replaced with a call to freezer_exempt()? Similarly, the freezer_count() after the wait_for_completion might just as well be a clear of the NOFREEZE bit followed by a try_to_freeze(). Could you please explain the rationale behind the SKIP flag?

The difference between the NOFREEZE and the SKIP flag is a subtle one.

When a task (say p) sets it's NOFREEZE flag, it tells the freezer not to consider it for freezing. Which means freezeable(p) will return 0. So the freezer will not even mark it for freezing.

However, when a task sets it SKIP flag, it tells the freezer - "I might block at a safe place. So when you are counting the processes which have been marked as freezeable, but have not frozen yet, please don't count me in. IOW, please skip me."
Thus such a task can still be marked for freezing.

The typical usage is
freezer_do_not_count(current);
/* currents goes to an uninterruptible sleep, like wait_for_completion. */ freezer_count(current);

Once the task wakes up from it's uninterruptible sleep, it will call freezer_count which in turn calls try_to_freeze. If the task was marked for freezing, it will be frozen now.

You may want to check the thread http://lkml.org/lkml/2007/2/18/47
on how it came into existance.

Hi Rafael,

Also, I do have several gripes against the naming of some of these functions:

Quoted 1 line

static inline int freezing(struct task_struct *p)

This could be called task_should_freeze().

Quoted 5 lines

/*
- * Sometimes we may need to cancel the previous 'freeze' request
+ * Cancel the previous 'freeze' request
*/
static inline void do_not_freeze(struct task_struct *p)

This definitely needs to be undo_freeze() or unfreeze(). do_not_freeze() sounds like what freeze_exempt() does.

Quoted 1 line

static inline void frozen_process(struct task_struct *p)

frozen_process() sounds like what frozen() is supposed to do. This could instead be mark_task_frozen(), or even mark_frozen(), because only the current task can ever mark *itself* frozen before freezing itself.

Quoted 2 lines

static inline void freezer_do_not_count(void)
static inline void freezer_count(void)

These could be called freezer_skip() and freezer_do_not_skip(). Better to stick to consistent naming / terminology.

From: Rafael J. Wysocki <[email protected]>

Move all of the freezer-related flags to a separate field in task_struct and introduce functions to operate them using set_bit() etc.

Signed-off-by Rafael J. Wysocki <[email protected]>

include/linux/freezer.h static inline int thaw_process(struct ta · line 105

Quoted 9 lines · Rafael J. Wysocki

include/linux/freezer.h +2 -2

@@ -63,9 +100,9 @@ static inline int thaw_process(struct ta
  */
 static inline void frozen_process(struct task_struct *p)
 {
-	p->flags |= PF_FROZEN;
+	set_frozen_flag(p);
 	wmb();
-	clear_tsk_thread_flag(p, TIF_FREEZE);
+	do_not_freeze(p);

We may want to rename do_not_freeze to something else. It kind of looks weird calling do_not_freeze(p) after setting the frozen flag!

Probably, just a matter of taste :-)

Move all of the freezer-related flags to a separate field in task_struct and introduce functions to operate them using set_bit() etc.

[...snip...]

Quoted 12 lines · Rafael J. Wysocki

Move all of the freezer-related flags to a separate field in task_struct and introduce functions to operate them using set_bit() etc.

[...snip...]

kernel/fork.c +0 -0

@@ -922,7 +922,7 @@ static inline void copy_flags(unsigned l
 {
 	unsigned long new_flags = p->flags;
 

kernel/fork.c static inline void copy_flags(unsigned l · line 928

Quoted 5 lines

kernel/fork.c +1 -1

-	new_flags &= ~(PF_SUPERPRIV | PF_NOFREEZE);
+	new_flags &= ~PF_SUPERPRIV;
 	new_flags |= PF_FORKNOEXEC;
 	new_flags |= PF_STARTING;
 	p->flags = new_flags;

OK, but you forgot to clear p->freezer_flags on copy_process()->dup_task_struct() path ?

Quoted 13 lines · Oleg Nesterov

Quoted 12 lines · Rafael J. Wysocki

Move all of the freezer-related flags to a separate field in task_struct and introduce functions to operate them using set_bit() etc.

[...snip...]

kernel/fork.c +0 -0

@@ -922,7 +922,7 @@ static inline void copy_flags(unsigned l
 {
 	unsigned long new_flags = p->flags;
 

Quoted 8 lines

kernel/fork.c static inline void copy_flags(unsigned l · line 928

Quoted 5 lines

kernel/fork.c +1 -1

-	new_flags &= ~(PF_SUPERPRIV | PF_NOFREEZE);
+	new_flags &= ~PF_SUPERPRIV;
 	new_flags |= PF_FORKNOEXEC;
 	new_flags |= PF_STARTING;
 	p->flags = new_flags;

OK, but you forgot to clear p->freezer_flags on copy_process()->dup_task_struct() path ?

Yes, thanks for pointing that out.

Should I clear it in dup_task_struct() or is there a better place?

Quoted 23 lines · Rafael J. Wysocki

Quoted 21 lines · Oleg Nesterov

Quoted 9 lines · Rafael J. Wysocki

Move all of the freezer-related flags to a separate field in task_struct and introduce functions to operate them using set_bit() etc.

[...snip...]

kernel/fork.c static inline void copy_flags(unsigned l · line 928

Quoted 8 lines

kernel/fork.c +1 -1

@@ -922,7 +922,7 @@ static inline void copy_flags(unsigned l
 {
 	unsigned long new_flags = p->flags;
 
-	new_flags &= ~(PF_SUPERPRIV | PF_NOFREEZE);
+	new_flags &= ~PF_SUPERPRIV;
 	new_flags |= PF_FORKNOEXEC;
 	new_flags |= PF_STARTING;
 	p->flags = new_flags;

OK, but you forgot to clear p->freezer_flags on copy_process()->dup_task_struct() path ?

Quoted 1 line

Yes, thanks for pointing that out.

That reminds me, shouldn't we set the child's TFF_FREEZE flag if the parent is freezing or frozen?

Should I clear it in dup_task_struct() or is there a better place?

Quoted 2 lines · Rafael J. Wysocki

Should I clear it in dup_task_struct() or is there a better place?

I personally think we should do this in dup_task_struct(). In fact, I believe it is better to replace the

	*tsk = *orig;

with some helper (like setup_thread_stack() below), and that helper clears ->freezer_flags. Say, copy_task_struct().

Quoted 9 lines · Oleg Nesterov

Quoted 2 lines · Rafael J. Wysocki

Should I clear it in dup_task_struct() or is there a better place?

I personally think we should do this in dup_task_struct(). In fact, I believe it is better to replace the

	*tsk = *orig;

Quoted 2 lines

with some helper (like setup_thread_stack() below), and that helper clears ->freezer_flags. Say, copy_task_struct().

Hmm, wouldn't that be overkill? copy_task_struct() would have to do *tsk = *orig anyway, and we only need to clear one field apart from this.

Some other fields are cleared towards the end of dup_task_struct(), so perhaps we could clear freezer_flags in there too?

Quoted 17 lines · Rafael J. Wysocki

Quoted 11 lines · Oleg Nesterov

Quoted 2 lines · Rafael J. Wysocki

Should I clear it in dup_task_struct() or is there a better place?

I personally think we should do this in dup_task_struct(). In fact, I believe it is better to replace the

	*tsk = *orig;

with some helper (like setup_thread_stack() below), and that helper clears ->freezer_flags. Say, copy_task_struct().

Hmm, wouldn't that be overkill? copy_task_struct() would have to do *tsk = *orig anyway, and we only need to clear one field apart from this.

Quoted 2 lines

Some other fields are cleared towards the end of dup_task_struct(), so perhaps we could clear freezer_flags in there too?

Yes. And I strongly believe it is bad we don't have the helper which does some random stuf like "p->did_exec = 0".

The same for thread_info. Could you answer quickly where do we clear TIF_FREEZE currently? We don't.

Fix the problem with kthread_stop() that causes the freezer to fail if a freezable thread is attempting to stop a frozen one and that may cause the freezer to fail if the thread being stopped is freezable and try_to_freeze_tasks() is running concurrently with kthread_stop().

Index: linux-2.6.21-rc6-mm1/kernel/kthread.c
===================================================================

kernel/kthread.c +1 -0

@@ -232,6 +233,14 @@ int kthread_stop(struct task_struct *k)
 
 	/* Now set kthread_should_stop() to true, and wake it up. */
 	kthread_stop_info.k = k;
+	if (!freezer_should_exempt(current)) {

Hi!

Quoted 4 lines

Fix the problem with kthread_stop() that causes the freezer to fail if a freezable thread is attempting to stop a frozen one and that may cause the freezer to fail if the thread being stopped is freezable and try_to_freeze_tasks() is running concurrently with kthread_stop().

Parse error.

Quoted 6 lines

Index: linux-2.6.21-rc6-mm1/kernel/kthread.c
===================================================================

kernel/kthread.c +0 -0

@@ -232,6 +233,14 @@ int kthread_stop(struct task_struct *k)
 

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 246

Quoted 13 lines

kernel/kthread.c +8 -0

 	/* Now set kthread_should_stop() to true, and wake it up. */
 	kthread_stop_info.k = k;
+	if (!freezer_should_exempt(current)) {
+		/* We are freezable, so we must make sure that the thread being
+		 * stopped is not frozen and will not be frozen until it dies
+		 */
+		freezer_exempt(k);
+		if (frozen(k))
+			clear_frozen_flag(k);
+	}
 	wake_up_process(k);
 	put_task_struct(k);
 

Do we need to take some locks to access other process' flags? Or do frozen_exempt() etc take enough locks, already?

From: Rafael J. Wysocki <[email protected]>

Fix the problem with kthread_stop() that causes the freezer to fail if a freezable thread is attempting to stop a frozen one and that may cause the freezer to fail if the thread being stopped is freezable and try_to_freeze_tasks() is running concurrently with kthread_stop().

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 kernel/kthread.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Index: linux-2.6.21-rc6-mm1/kernel/kthread.c
===================================================================

kernel/kthread.c +1 -0

@@ -13,6 +13,7 @@
 #include <linux/file.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/freezer.h>
 #include <asm/semaphore.h>
 
 /*

@@ -232,6 +233,14 @@ int kthread_stop(struct task_struct *k)
 

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 243

Quoted 13 lines · Rafael J. Wysocki

/*

kernel/kthread.c +8 -0

@@ -232,6 +233,14 @@ int kthread_stop(struct task_struct *k)
 
 	/* Now set kthread_should_stop() to true, and wake it up. */
 	kthread_stop_info.k = k;
+	if (!freezer_should_exempt(current)) {
+		/* We are freezable, so we must make sure that the thread being
+		 * stopped is not frozen and will not be frozen until it dies
+		 */
+		freezer_exempt(k);
+		if (frozen(k))
+			clear_frozen_flag(k);
+	}

I'm trying hard to convince myself that this will work. May be I am missing something here, but I find a potential race window (very small though) when k is entering the refrigerator.

[... snip ... ]

IMO, we need the to take the task_lock for k here. Something like

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 236

Quoted 1 line

kernel/kthread.c +1 -0

+	if (!freezer_should_exempt(current)) {

		task_lock(k);

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 242

Quoted 6 lines

kernel/kthread.c +6 -0

+		/* We are freezable, so we must make sure that the thread being
+		 * stopped is not frozen and will not be frozen until it dies
+		 */
+		freezer_exempt(k);
+		if (frozen(k))
+			clear_frozen_flag(k);

		task_unlock(k);

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 243

Quoted 1 line

kernel/kthread.c +1 -0

+	}

Quoted 26 lines · Gautham R Shenoy

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 243

Quoted 13 lines · Rafael J. Wysocki

/*

kernel/kthread.c +8 -0

@@ -232,6 +233,14 @@ int kthread_stop(struct task_struct *k)
 
 	/* Now set kthread_should_stop() to true, and wake it up. */
 	kthread_stop_info.k = k;
+	if (!freezer_should_exempt(current)) {
+		/* We are freezable, so we must make sure that the thread being
+		 * stopped is not frozen and will not be frozen until it dies
+		 */
+		freezer_exempt(k);
+		if (frozen(k))
+			clear_frozen_flag(k);
+	}

I'm trying hard to convince myself that this will work. May be I am missing something here, but I find a potential race window (very small though) when k is entering the refrigerator.

[... snip ... ]

IMO, we need the to take the task_lock for k here. Something like

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 236

Quoted 1 line

kernel/kthread.c +1 -0

+	if (!freezer_should_exempt(current)) {

		task_lock(k);

Quoted 8 lines

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 242

Quoted 6 lines

kernel/kthread.c +6 -0

+		/* We are freezable, so we must make sure that the thread being
+		 * stopped is not frozen and will not be frozen until it dies
+		 */
+		freezer_exempt(k);
+		if (frozen(k))
+			clear_frozen_flag(k);

		task_unlock(k);

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 243

Quoted 1 line

kernel/kthread.c +1 -0

+	}

Well, probably I missed something, but why can't we do

	if (!freezer_should_exempt(current)) {
		freezer_exempt(k);
		thaw_process(k);
	}

?

thaw_process(k) is properly serialized with refrigerator(), and it checks frozen(k). We can make an extra wake_up, but this should not matter.

Rafael, please check the recent changes in kthread.c, kthread_stop() was reworked, we don't have kthread_stop_info any longer.

Quoted 26 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Fix the problem with kthread_stop() that causes the freezer to fail if a freezable thread is attempting to stop a frozen one and that may cause the freezer to fail if the thread being stopped is freezable and try_to_freeze_tasks() is running concurrently with kthread_stop().

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 kernel/kthread.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Index: linux-2.6.21-rc6-mm1/kernel/kthread.c
===================================================================

kernel/kthread.c +1 -0

@@ -13,6 +13,7 @@
 #include <linux/file.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/freezer.h>
 #include <asm/semaphore.h>
 
 /*

@@ -232,6 +233,14 @@ int kthread_stop(struct task_struct *k)
 

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 243

Quoted 10 lines

kernel/kthread.c +8 -0

 	/* Now set kthread_should_stop() to true, and wake it up. */
 	kthread_stop_info.k = k;
+	if (!freezer_should_exempt(current)) {
+		/* We are freezable, so we must make sure that the thread being
+		 * stopped is not frozen and will not be frozen until it dies
+		 */
+		freezer_exempt(k);
+		if (frozen(k))
+			clear_frozen_flag(k);
+	}

I'm trying hard to convince myself that this will work. May be I am missing something here, but I find a potential race window (very small though) when k is entering the refrigerator.

Here's how.

kthread_stop(k) k->refrigerator()
---------------------------------------------------------------------

						task_lock(k);
						1) check_if_exempted();
						/* not exempted. So 
						 * we will freeze
						 * ourself.
						 */

2) freezer_exempt(k);

3) if(frozen(k))
/* No, we're not yet frozen. So we
* don't clear_frozen_flag(k) here
*/

						4) frozen_process(k);
						   task_unlock(k);
						
						5) for(;;) {
						 set_current_state(UNINTERRUPTIBLE);
						  if(!frozen_process(k))
						  /* k is frozen. We
						   * don't break :( 
						   */
						 
						  	schedule();
						}

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 246

Quoted 3 lines

kernel/kthread.c +0 -0

 	wake_up_process(k);
 	put_task_struct(k);
 

Thus the freezer can still fail, no?
IMO, we need the to take the task_lock for k here. Something like

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 236

Quoted 1 line

kernel/kthread.c +1 -0

+	if (!freezer_should_exempt(current)) {

		task_lock(k);

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 242

Quoted 6 lines

kernel/kthread.c +6 -0

+		/* We are freezable, so we must make sure that the thread being
+		 * stopped is not frozen and will not be frozen until it dies
+		 */
+		freezer_exempt(k);
+		if (frozen(k))
+			clear_frozen_flag(k);

		task_unlock(k);

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 243

Quoted 1 line

kernel/kthread.c +1 -0

+	}

Quoted 9 lines · Gautham R Shenoy

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 236

Quoted 1 line

kernel/kthread.c +1 -0

+	if (!freezer_should_exempt(current)) {

		task_lock(k);

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 242

Quoted 6 lines

kernel/kthread.c +6 -0

+		/* We are freezable, so we must make sure that the thread being
+		 * stopped is not frozen and will not be frozen until it dies
+		 */
+		freezer_exempt(k);
+		if (frozen(k))
+			clear_frozen_flag(k);

		task_unlock(k);

Quoted 1 line

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 243

Quoted 1 line

kernel/kthread.c +1 -0

+	}

Yes, that's correct. We need to take task_lock() to avoid the race with refrigerator().

Quoted 11 lines · Rafael J. Wysocki

Quoted 9 lines · Gautham R Shenoy

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 236

Quoted 1 line

kernel/kthread.c +1 -0

+	if (!freezer_should_exempt(current)) {

		task_lock(k);

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 242

Quoted 6 lines

kernel/kthread.c +6 -0

+		/* We are freezable, so we must make sure that the thread being
+		 * stopped is not frozen and will not be frozen until it dies
+		 */
+		freezer_exempt(k);
+		if (frozen(k))
+			clear_frozen_flag(k);

		task_unlock(k);

Quoted 4 lines

Quoted 1 line

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 243

Quoted 1 line

kernel/kthread.c +1 -0

+	}

Yes, that's correct. We need to take task_lock() to avoid the race with refrigerator().

Even if we use thaw_task() ?

Even if I am wrong, I think we should not use task_lock() for the freezing related code, except in freezer.[ch]

Note also that without CONFIG_FREEZER freezer_should_exempt() == 0, so we will do unneeded task_lock/task_unlock.

Oleg.

Quoted 17 lines · Oleg Nesterov

Quoted 15 lines · Rafael J. Wysocki

Quoted 10 lines · Gautham R Shenoy

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 236

Quoted 1 line

kernel/kthread.c +1 -0

+	if (!freezer_should_exempt(current)) {

		task_lock(k);

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 242

Quoted 6 lines

kernel/kthread.c +6 -0

+		/* We are freezable, so we must make sure that the thread being
+		 * stopped is not frozen and will not be frozen until it dies
+		 */
+		freezer_exempt(k);
+		if (frozen(k))
+			clear_frozen_flag(k);

		task_unlock(k);

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 243

Quoted 1 line

kernel/kthread.c +1 -0

+	}

Yes, that's correct. We need to take task_lock() to avoid the race with refrigerator().

Quoted 1 line

Even if we use thaw_task() ?

I don't think so. As you correctly pointed out, thaw_task() is race free w.r.t the refrigerator().

Quoted 21 lines · Rafael J. Wysocki

Hmm, can't we do something like this instead:

---
 kernel/kthread.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Index: linux-2.6.21-rc7/kernel/kthread.c
===================================================================

kernel/kthread.c +1 -0

@@ -13,6 +13,7 @@
 #include <linux/file.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/freezer.h>
 #include <asm/semaphore.h>
 
 /*

@@ -232,6 +233,15 @@ int kthread_stop(struct task_struct *k)
 

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 244

Quoted 11 lines

kernel/kthread.c +9 -0

 	/* Now set kthread_should_stop() to true, and wake it up. */
 	kthread_stop_info.k = k;
+	if (!(current->flags & PF_NOFREEZE)) {
+		/* If we are freezable, the freezer will wait for us */
+		task_lock(k);
+		k->flags |= PF_NOFREEZE;
+		if (frozen(k))
+			k->flags &= ~PF_FROZEN;
+
+		task_unlock(k);
+	}

Yes, we can do this for now since the tasks have only two freeze states, namely Freezeable and Non Freezeable.

Quoted 23 lines · Gautham R Shenoy

Quoted 21 lines · Rafael J. Wysocki

Hmm, can't we do something like this instead:

---
 kernel/kthread.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Index: linux-2.6.21-rc7/kernel/kthread.c
===================================================================

kernel/kthread.c +1 -0

@@ -13,6 +13,7 @@
 #include <linux/file.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/freezer.h>
 #include <asm/semaphore.h>
 
 /*

@@ -232,6 +233,15 @@ int kthread_stop(struct task_struct *k)
 

Quoted 14 lines

kernel/kthread.c int kthread_stop(struct task_struct *k) · line 244

Quoted 11 lines

kernel/kthread.c +9 -0

 	/* Now set kthread_should_stop() to true, and wake it up. */
 	kthread_stop_info.k = k;
+	if (!(current->flags & PF_NOFREEZE)) {
+		/* If we are freezable, the freezer will wait for us */
+		task_lock(k);
+		k->flags |= PF_NOFREEZE;
+		if (frozen(k))
+			k->flags &= ~PF_FROZEN;
+
+		task_unlock(k);
+	}

Yes, we can do this for now since the tasks have only two freeze states, namely Freezeable and Non Freezeable.

No, we can't change k->flags, k owns its ->flags, and it is not atomic.

Rafael, may I suggest you to document task_lock() in thaw_process() ? This looks really confusing, as if task_lock() protects "p->flags &= ~PF_FROZEN".

Actually, task_lock() is needed to prevent the race with refrigerator() when the freezing fails, but this is not obvious.

On Thu, 19 Apr 2007 17:34:19 +0530
Gautham R Shenoy <[email protected]> wrote:

flush_workqueue() just needs to die. I think there are (almost) no legitimate users of it once cancel_work_sync() is merged.

Quoted 1 line

This patch attempts to address such a situation with a fix for kthread_stop.

Via wholly undescribed means :(

Quoted 2 lines

Strictly experimental. Compile tested on i386.

Rather than doing <whatever you did>, perhaps we could make the freezing process a dual-pass thing. On pass 1, mark all the threads as "we'll be freezing you soon" and on the second pass, do the actual freezing. Then, in problematic places such as kthread_stop() we can look to see if we'll soon be asked to freeze and if so, run try_to_freeze().

Of course, running try_to_freeze() in kthread_stop() would be very wrong, so we'd actually need to do it in callers, preferably via a new kthread_stop_freezeable() wrapper.

And the two-pass-freeze thing is of course racy. It's also unnecessary:
setting a flag on every task in the machine is equivalent to setting a global variable. So perhaps just use a global variable?

int kthread_stop_freezeable(struct task_struct *k)

{
	if (freeze_state == ABOUT_TO_START) {
		wait_for(freeze_state == STARTED);
		try_to_freeze();
	}
	kthread_stop(k);
}

which is theoretically racy if another freeze_processes() starts immediately. Anyway - please have a think about it ;)

kernel/power/process.c line 34

Quoted 4 lines

kernel/power/process.c +4 -0

+static struct freezer_status_struct freezer_status = {
+				.lock = SPIN_LOCK_UNLOCKED,
+				.count = 0,
+			};

SPIN_LOCK_UNLOCKED is deprecated (it subverts lockdep)

Quoted 19 lines

static inline int freezeable(struct task_struct * p) {

kernel/power/process.c +3 -1

 	if ((p == current) ||

@@ -45,7 +55,8 @@ void refrigerator(void)
 		 * *after* the freezer did the freezeable() check
 		 * on us.
 		 */
-		if (current->flags & PF_NOFREEZE) {
+		if ((current->flags & PF_NOFREEZE) ||
+		    test_tsk_thread_flag(current, TIF_FREEZER_HELD)) {
 			clear_tsk_thread_flag(current, TIF_FREEZE);
 			task_unlock(current);
 			return;

@@ -63,12 +74,16 @@ void refrigerator(void)
 	recalc_sigpending(); /* We sent fake signal, clean it up */
 	spin_unlock_irq(&current->sighand->siglock);
 
+	task_lock(current);
 	for (;;) {

kernel/power/process.c void refrigerator(void) · line 88

Quoted 10 lines

kernel/power/process.c +3 -0

 		set_current_state(TASK_UNINTERRUPTIBLE);
 		if (!frozen(current))
 			break;
+		task_unlock(current);
 		schedule();
+		task_lock(current);
 	}
+	task_unlock(current);
 	pr_debug("%s left refrigerator\n", current->comm);
 	current->state = save;

I guess we should use set_current_state() here.

kernel/power/process.c static void thaw_tasks(int thaw_user_spa · line 301

Quoted 8 lines

kernel/power/process.c +7 -0

+
+	if (thaw_user_space) {
+		spin_lock(&freezer_status.lock);
+			if (freezer_status.count < 0)
+				freezer_status.count++;
+		spin_unlock(&freezer_status.lock);
+	}
 }

whitespace went wrong

Quoted 25 lines

include/asm-ia64/thread_info.h +4 -0

+#define TIF_FREEZER_HELD	21	/* is temporarily holding up the
+					 * process freezer
+					 */
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)

@@ -102,6 +105,7 @@ struct thread_info {
 #define _TIF_MCA_INIT		(1 << TIF_MCA_INIT)
 #define _TIF_DB_DISABLED	(1 << TIF_DB_DISABLED)
 #define _TIF_FREEZE		(1 << TIF_FREEZE)
+#define _TIF_FREEZER_HELD	(1 << TIF_FREEZER_HELD)
 
 /* "work to do on user-return" bits */
 #define TIF_ALLWORK_MASK	(_TIF_NOTIFY_RESUME|_TIF_SIGPENDING|_TIF_NEED_RESCHED|_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT)

Index: linux-2.6.21-rc6/include/asm-mips/thread_info.h
===================================================================

include/asm-mips/thread_info.h +1 -0

@@ -120,6 +120,7 @@ register struct thread_info *__current_t
 #define TIF_MEMDIE		18
 #define TIF_FREEZE		19
 #define TIF_ALLOW_FP_IN_KERNEL	20
+#define TIF_FREEZER_HELD	21
 #define TIF_SYSCALL_TRACE	31	/* syscall trace active */
 

include/asm-mips/thread_info.h register struct thread_info *__current_t · line 136

Quoted 6 lines

include/asm-mips/thread_info.h +1 -0

 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)

@@ -132,6 +133,7 @@ register struct thread_info *__current_t
 #define _TIF_USEDFPU		(1<<TIF_USEDFPU)
 #define _TIF_POLLING_NRFLAG	(1<<TIF_POLLING_NRFLAG)
 #define _TIF_FREEZE		(1<<TIF_FREEZE)
+#define _TIF_FREEZER_HELD	(1<<TIF_FREEZER_HELD)

hm, all this duplication is unpleasing. We could do something similar to include/linux/buffer_head.h:BH_PrivateStart here: get all architectures to define a TIF_COMMON_STUFF_STARTS_HERE then include asm-generic/whatever.h which defines all the flags which every architecture must define, as TIF_COMMON_STUFF_STARTS_HERE+0, TIF_COMMON_STUFF_STARTS_HERE+1, etc.

Quoted 4 lines

Of course, running try_to_freeze() in kthread_stop() would be very wrong, so we'd actually need to do it in callers, preferably via a new kthread_stop_freezeable() wrapper.

Well, even then we'll need to ensure that a thread would not call kthread_stop_freezeable() with any locks held. That would mean more audits :)

kernel/power/process.c +1 -0

@@ -63,12 +74,16 @@ void refrigerator(void)
 	recalc_sigpending(); /* We sent fake signal, clean it up */
 	spin_unlock_irq(&current->sighand->siglock);
 
+	task_lock(current);
 	for (;;) {

kernel/power/process.c +5 -0

+int hold_freezer_for_task(struct task_struct *p)
+{
+	int ret = 0;
+	spin_lock(&freezer_status.lock);
+	if (freezer_status.count >= 0)

Quoted 5 lines · Gautham R Shenoy

kernel/power/process.c +0 -0

@@ -63,12 +74,16 @@ void refrigerator(void)
 	recalc_sigpending(); /* We sent fake signal, clean it up */
 	spin_unlock_irq(&current->sighand->siglock);
 

kernel/power/process.c void refrigerator(void) · line 88

Quoted 12 lines

kernel/power/process.c +4 -0

+	task_lock(current);
 	for (;;) {
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		if (!frozen(current))
 			break;
+		task_unlock(current);
 		schedule();
+		task_lock(current);
 	}
+	task_unlock(current);
 	pr_debug("%s left refrigerator\n", current->comm);
 	current->state = save;

Just curious, why this change?

Quoted 13 lines

kernel/power/process.c +12 -0

+int hold_freezer_for_task(struct task_struct *p)
+{
+	int ret = 0;
+	spin_lock(&freezer_status.lock);
+	if (freezer_status.count >= 0)
+	{
+		set_tsk_thread_flag(p, TIF_FREEZER_HELD);
+		thaw_process(p);
+		freezer_status.count++;
+		ret = 1;
+	}
+	spin_unlock(&freezer_status.lock);

kernel/power/process.c static inline int is_user_space(struct t · line 156

Quoted 3 lines

kernel/power/process.c +3 -0

+
+	return ret;
+}

I think this can work if it is used only in kthread_stop(). But what if another task wants to do hold_freezer_for_task(p) ? freezer_status.count is recursive, but TIF_FREEZER_HELD is not. IOW, I believe this is not generic enough.

Also, you are planning to add different freezing states (FE_HOTPLUG_CPU, FE_SUSPEND, etc). In that case each of them needs a separate .count, because it should be negative when try_to_freeze_tasks() returns. Now consider the case when we are doing freeze_processes(FE_A | FE_B) ...

Oleg.

Quoted 6 lines · Oleg Nesterov

Quoted 5 lines · Gautham R Shenoy

kernel/power/process.c +0 -0

@@ -63,12 +74,16 @@ void refrigerator(void)
 	recalc_sigpending(); /* We sent fake signal, clean it up */
 	spin_unlock_irq(&current->sighand->siglock);
 

Quoted 14 lines

kernel/power/process.c void refrigerator(void) · line 88

Quoted 12 lines

kernel/power/process.c +4 -0

+	task_lock(current);
 	for (;;) {
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		if (!frozen(current))
 			break;
+		task_unlock(current);
 		schedule();
+		task_lock(current);
 	}
+	task_unlock(current);
 	pr_debug("%s left refrigerator\n", current->comm);
 	current->state = save;

Just curious, why this change?

This can race with hold_freezer_for_task() calling thaw_process. Earlier thaw_process(p) was called only after the process 'p' was frozen. Now with hold_freezer_for_task(), we can as well call thaw_process(p) when 'p' is in the freezing stage. Hence the task_lock.

Quoted 4 lines

Also, you are planning to add different freezing states (FE_HOTPLUG_CPU, FE_SUSPEND, etc). In that case each of them needs a separate .count, because it should be negative when try_to_freeze_tasks() returns. Now consider the case when we are doing freeze_processes(FE_A | FE_B) ...

So can't we in that case find out the weight of the freeze_event variable and subtract that weight from the count (if the count is <=0 ) ?

Quoted 23 lines · Gautham R Shenoy

Quoted 6 lines · Oleg Nesterov

Quoted 5 lines · Gautham R Shenoy

kernel/power/process.c +0 -0

@@ -63,12 +74,16 @@ void refrigerator(void)
 	recalc_sigpending(); /* We sent fake signal, clean it up */
 	spin_unlock_irq(&current->sighand->siglock);
 

Quoted 14 lines

kernel/power/process.c void refrigerator(void) · line 88

Quoted 12 lines

kernel/power/process.c +4 -0

+	task_lock(current);
 	for (;;) {
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		if (!frozen(current))
 			break;
+		task_unlock(current);
 		schedule();
+		task_lock(current);
 	}
+	task_unlock(current);
 	pr_debug("%s left refrigerator\n", current->comm);
 	current->state = save;

Just curious, why this change?

Quoted 4 lines

This can race with hold_freezer_for_task() calling thaw_process. Earlier thaw_process(p) was called only after the process 'p' was frozen. Now with hold_freezer_for_task(), we can as well call thaw_process(p) when 'p' is in the freezing stage. Hence the task_lock.

hold_freezer_for_task()->thaw_process(p) will wake up the task. Or the caller of refrigerator will notice "!frozen()". Note that refrigerator() sets PF_FROZEN under task_lock().

In fact we have the same issue when thaw_tasks()->thaw_process(p) happens when the freezing fails. In that case 'p' may be not frozen.

Quoted 8 lines

Quoted 5 lines

Also, you are planning to add different freezing states (FE_HOTPLUG_CPU, FE_SUSPEND, etc). In that case each of them needs a separate .count, because it should be negative when try_to_freeze_tasks() returns. Now consider the case when we are doing freeze_processes(FE_A | FE_B) ...

So can't we in that case find out the weight of the freeze_event variable and subtract that weight from the count (if the count is <=0 ) ?

Probably yes... but if we are speaking about kthrad_stop() only, this could be afaics solved in more simple way, as Rafael suggests.

Oleg.