Re: freezer problems

[Gautham R Shenoy]

On Fri, Feb 23, 2007 at 04:32:01PM +0530, Srivatsa Vaddagiri wrote:
> On Fri, Feb 23, 2007 at 04:17:23PM +0530, Srivatsa Vaddagiri wrote:
> > Ok that was my point of concern. For hotplug we would ideally like
> > everyone to be frozen. If we are not freezing some (like vfork parents),
> > (rather if we dont -wait- for them to get frozen) before offlining a
> > cpu, then it may expose some hotplug unsafe code in the caller of vfork
> > in kernel - hopefully that is not a issue practically speaking.
> 
> I notice that __call_usermodehelper() work function calls kernel_thread with
> CLONE_VFORK set. __call_usermodehelper() is usualled called in the
> context of a worker thread (kevent).

But I see __call_usermodehelper being called from the context of
khelper_wq which is a singlethreaded workqueue.

I thought we were not planning to freeze singlethreaded workqueue's for
hotplug, since we are not kthread_stopping them anywhere.

So this kthread_stop waiting for parent(khelper_wq) which is blocked on
wait_for_complete(child->vfork_done) shouldn't occur. No?

Thanks and Regards
gautham.

-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Gautham R Shenoy]

On Thu, Feb 22, 2007 at 06:03:37PM +0100, Rafael J. Wysocki wrote:
> 
> Why don't we just drop the warning?  try_to_freeze_tasks() should give us a
> warning if there's anything wrong anyway.

The patches look good. I will add my hotplug changes on the top of
these. 

And yeah, removing the warnings from thaw_processes
sounds like a good thing to me. I was constantly getting warnings like
"Strange, but ksoftirqd/1 was not frozen" when ksoftirqd was created in 
the CPU_UP path from the frozen context! 
I was wondering if freezing the processes created from the
frozen context is a good thing to silence these warnings, but I guess that
would open up some new races.


> 
> Greetings,
> Rafael

regards
gautham.
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Gautham R Shenoy]

On Thu, Feb 22, 2007 at 06:03:37PM +0100, Rafael J. Wysocki wrote:
 
 Why don't we just drop the warning?  try_to_freeze_tasks() should give us a
 warning if there's anything wrong anyway.

The patches look good. I will add my hotplug changes on the top of
these. 

And yeah, removing the warnings from thaw_processes
sounds like a good thing to me. I was constantly getting warnings like
Strange, but ksoftirqd/1 was not frozen when ksoftirqd was created in 
the CPU_UP path from the frozen context! 
I was wondering if freezing the processes created from the
frozen context is a good thing to silence these warnings, but I guess that
would open up some new races.


 
 Greetings,
 Rafael

regards
gautham.
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Gautham R Shenoy]

On Fri, Feb 23, 2007 at 04:32:01PM +0530, Srivatsa Vaddagiri wrote:
 On Fri, Feb 23, 2007 at 04:17:23PM +0530, Srivatsa Vaddagiri wrote:
  Ok that was my point of concern. For hotplug we would ideally like
  everyone to be frozen. If we are not freezing some (like vfork parents),
  (rather if we dont -wait- for them to get frozen) before offlining a
  cpu, then it may expose some hotplug unsafe code in the caller of vfork
  in kernel - hopefully that is not a issue practically speaking.
 
 I notice that __call_usermodehelper() work function calls kernel_thread with
 CLONE_VFORK set. __call_usermodehelper() is usualled called in the
 context of a worker thread (kevent).

But I see __call_usermodehelper being called from the context of
khelper_wq which is a singlethreaded workqueue.

I thought we were not planning to freeze singlethreaded workqueue's for
hotplug, since we are not kthread_stopping them anywhere.

So this kthread_stop waiting for parent(khelper_wq) which is blocked on
wait_for_complete(child-vfork_done) shouldn't occur. No?

Thanks and Regards
gautham.

-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Oleg Nesterov]

On 02/22, Rafael J. Wysocki wrote:
>
> On Thursday, 22 February 2007 18:44, Oleg Nesterov wrote:
> > On 02/22, Rafael J. Wysocki wrote:
> > > 
> > > Okay, attached.  The first one closes the race between thaw_tasks() and 
> > > the
> > > refrigerator that can occurs if the freezing fails.  The second one fixes 
> > > the
> > > vfork problem (should go on top of the first one).
> > 
> > Looks good to me.
> > 
> > > > Any other ideas? In any case we should imho avoid a separate loop for
> > > > PF_FREEZER_SKIP tasks to just fix debug messages. In fact it can't help
> > > > anyway.
> > > 
> > > Why don't we just drop the warning?  try_to_freeze_tasks() should give us 
> > > a
> > > warning if there's anything wrong anyway.
> > 
> > Indeed :)
> 
> Still, there is a tiny race in the error path of try_to_freeze_tasks(), where
> a vfork parent process can be preempted after clearing PF_FREEZER_SKIP and
> before entering refrigerator(), so try_to_freeze_tasks() will mistakenly 
> report
> that this process has caused a problem.

Yes, basically the same race. But unlike thaw_tasks(), this is the error path,
it is not so critical to filter out the false warnings. We can't do this anyway.
Since try_to_freeze_tasks() failed, new processes can be created, we don't 
filter
out "TASK_TRACED && frozen(p->parent)", etc.

> I think this race can be closed by (1) clearing PF_FREEZER_SKIP after calling
> try_to_freeze() in freezer_count(), (2) clearing PF_FREEZER_SKIP in
> refrigerator() before calling frozen_process() and (3) taking task_lock()
> around the warning check in the error path of try_to_freeze_tasks().

I am a bit confused by this patch. Which method it uses?

> +static inline void freezer_count(void)
> +{
> + try_to_freeze();
> + current->flags &= ~PF_FREEZER_SKIP;
> +}
>
> ...
>
> @@ -42,6 +42,7 @@ void refrigerator(void)
>  
>   task_lock(current);
>   if (freezing(current)) {
> + current->flags &= ~PF_FREEZER_SKIP;

Isn't it better to clear PF_FREEZER_SKIP unconditionally? freezer_count() will
do try_to_freeze(), nothing more.

It is not safe to clear PF_FREEZER_SKIP in freezer_count() ater try_to_freeze().
PF_FREEZER_SKIP is a promise to to try_to_freeze(). try_to_freeze_tasks() fails,
does cancel_freezing(), a CLONE_VFORK parent returns from try_to_freeze() with
PF_FREEZER_SKIP set, and now comes another try_to_freeze_tasks() ?

Surely, this can't happen in practice, but still.

Oleg.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Oleg Nesterov]

On 02/22, Rafael J. Wysocki wrote:

 On Thursday, 22 February 2007 18:44, Oleg Nesterov wrote:
  On 02/22, Rafael J. Wysocki wrote:
   
   Okay, attached.  The first one closes the race between thaw_tasks() and 
   the
   refrigerator that can occurs if the freezing fails.  The second one fixes 
   the
   vfork problem (should go on top of the first one).
  
  Looks good to me.
  
Any other ideas? In any case we should imho avoid a separate loop for
PF_FREEZER_SKIP tasks to just fix debug messages. In fact it can't help
anyway.
   
   Why don't we just drop the warning?  try_to_freeze_tasks() should give us 
   a
   warning if there's anything wrong anyway.
  
  Indeed :)
 
 Still, there is a tiny race in the error path of try_to_freeze_tasks(), where
 a vfork parent process can be preempted after clearing PF_FREEZER_SKIP and
 before entering refrigerator(), so try_to_freeze_tasks() will mistakenly 
 report
 that this process has caused a problem.

Yes, basically the same race. But unlike thaw_tasks(), this is the error path,
it is not so critical to filter out the false warnings. We can't do this anyway.
Since try_to_freeze_tasks() failed, new processes can be created, we don't 
filter
out TASK_TRACED  frozen(p-parent), etc.

 I think this race can be closed by (1) clearing PF_FREEZER_SKIP after calling
 try_to_freeze() in freezer_count(), (2) clearing PF_FREEZER_SKIP in
 refrigerator() before calling frozen_process() and (3) taking task_lock()
 around the warning check in the error path of try_to_freeze_tasks().

I am a bit confused by this patch. Which method it uses?

 +static inline void freezer_count(void)
 +{
 + try_to_freeze();
 + current-flags = ~PF_FREEZER_SKIP;
 +}

 ...

 @@ -42,6 +42,7 @@ void refrigerator(void)
  
   task_lock(current);
   if (freezing(current)) {
 + current-flags = ~PF_FREEZER_SKIP;

Isn't it better to clear PF_FREEZER_SKIP unconditionally? freezer_count() will
do try_to_freeze(), nothing more.

It is not safe to clear PF_FREEZER_SKIP in freezer_count() ater try_to_freeze().
PF_FREEZER_SKIP is a promise to to try_to_freeze(). try_to_freeze_tasks() fails,
does cancel_freezing(), a CLONE_VFORK parent returns from try_to_freeze() with
PF_FREEZER_SKIP set, and now comes another try_to_freeze_tasks() ?

Surely, this can't happen in practice, but still.

Oleg.

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Thursday, 22 February 2007 18:44, Oleg Nesterov wrote:
> On 02/22, Rafael J. Wysocki wrote:
> > 
> > Okay, attached.  The first one closes the race between thaw_tasks() and the
> > refrigerator that can occurs if the freezing fails.  The second one fixes 
> > the
> > vfork problem (should go on top of the first one).
> 
> Looks good to me.
> 
> > > Any other ideas? In any case we should imho avoid a separate loop for
> > > PF_FREEZER_SKIP tasks to just fix debug messages. In fact it can't help
> > > anyway.
> > 
> > Why don't we just drop the warning?  try_to_freeze_tasks() should give us a
> > warning if there's anything wrong anyway.
> 
> Indeed :)

Still, there is a tiny race in the error path of try_to_freeze_tasks(), where
a vfork parent process can be preempted after clearing PF_FREEZER_SKIP and
before entering refrigerator(), so try_to_freeze_tasks() will mistakenly report
that this process has caused a problem.

I think this race can be closed by (1) clearing PF_FREEZER_SKIP after calling
try_to_freeze() in freezer_count(), (2) clearing PF_FREEZER_SKIP in
refrigerator() before calling frozen_process() and (3) taking task_lock()
around the warning check in the error path of try_to_freeze_tasks().

I have modified freezer-fix-vfork-problem.patch to implement this (appended;
it assumes that freezer-fix-theoretical-race.patch has already been applied).

If this is the right thing to do, I think there's a reason to additionally move
task_lock/unlock() from cancel_freezing() to the error path in
try_to_freeze_tasks().

Greetings,
Rafael


 include/linux/freezer.h |   30 --
 include/linux/sched.h   |1 +
 kernel/fork.c   |3 +++
 kernel/power/process.c  |   32 +---
 4 files changed, 45 insertions(+), 21 deletions(-)

Index: linux-2.6.20-mm2/include/linux/sched.h
===
--- linux-2.6.20-mm2.orig/include/linux/sched.h
+++ linux-2.6.20-mm2/include/linux/sched.h
@@ -1189,6 +1189,7 @@ static inline void put_task_struct(struc
 #define PF_SPREAD_SLAB 0x0200  /* Spread some slab caches over cpuset 
*/
 #define PF_MEMPOLICY   0x1000  /* Non-default NUMA mempolicy */
 #define PF_MUTEX_TESTER0x2000  /* Thread belongs to the rt 
mutex tester */
+#define PF_FREEZER_SKIP0x4000  /* Freezer should not count it 
as freezeable */
 
 /*
  * Only the _current_ task can read/write to tsk->flags, but other
Index: linux-2.6.20-mm2/include/linux/freezer.h
===
--- linux-2.6.20-mm2.orig/include/linux/freezer.h
+++ linux-2.6.20-mm2/include/linux/freezer.h
@@ -75,7 +75,31 @@ static inline int try_to_freeze(void)
return 0;
 }
 
-extern void thaw_some_processes(int all);
+/*
+ * Tell the freezer not to count current task as freezeable
+ */
+static inline void freezer_do_not_count(void)
+{
+   current->flags |= PF_FREEZER_SKIP;
+}
+
+/*
+ * Try to freeze the current task and tell the freezer to count it as 
freezeable
+ * again
+ */
+static inline void freezer_count(void)
+{
+   try_to_freeze();
+   current->flags &= ~PF_FREEZER_SKIP;
+}
+
+/*
+ * Check if the task should be counted as freezeable by the freezer
+ */
+static inline int freezer_should_skip(struct task_struct *p)
+{
+   return !!(p->flags & PF_FREEZER_SKIP);
+}
 
 #else
 static inline int frozen(struct task_struct *p) { return 0; }
@@ -90,5 +114,7 @@ static inline void thaw_processes(void) 
 
 static inline int try_to_freeze(void) { return 0; }
 
-
+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; }
 #endif
Index: linux-2.6.20-mm2/kernel/fork.c
===
--- linux-2.6.20-mm2.orig/kernel/fork.c
+++ linux-2.6.20-mm2/kernel/fork.c
@@ -50,6 +50,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -1393,7 +1394,9 @@ long do_fork(unsigned long clone_flags,
tracehook_report_clone_complete(clone_flags, nr, p);
 
if (clone_flags & CLONE_VFORK) {
+   freezer_do_not_count();
wait_for_completion();
+   freezer_count();
tracehook_report_vfork_done(p, nr);
}
} else {
Index: linux-2.6.20-mm2/kernel/power/process.c
===
--- linux-2.6.20-mm2.orig/kernel/power/process.c
+++ linux-2.6.20-mm2/kernel/power/process.c
@@ -42,6 +42,7 @@ void refrigerator(void)
 
task_lock(current);
if (freezing(current)) {
+   current->flags &= ~PF_FREEZER_SKIP;
frozen_process(current);
task_unlock(current);
} else {
@@ -131,22 +132,12 @@ static unsigned int 

Re: freezer problems

[Oleg Nesterov]

On 02/22, Rafael J. Wysocki wrote:
> 
> Okay, attached.  The first one closes the race between thaw_tasks() and the
> refrigerator that can occurs if the freezing fails.  The second one fixes the
> vfork problem (should go on top of the first one).

Looks good to me.

> > Any other ideas? In any case we should imho avoid a separate loop for
> > PF_FREEZER_SKIP tasks to just fix debug messages. In fact it can't help
> > anyway.
> 
> Why don't we just drop the warning?  try_to_freeze_tasks() should give us a
> warning if there's anything wrong anyway.

Indeed :)

Oleg.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Thursday, 22 February 2007 11:47, Oleg Nesterov wrote:
> On 02/22, Rafael J. Wysocki wrote:
> >
> > Okay, below is what I have right now (compilation tested on x86_64):
> > 
> > This patch fixes the vfork problem by adding the PF_FREEZER_SKIP flag that
> > can be used by tasks to tell the freezer not to count them as freezeable and
> > making the vfork parents set this flag before they call 
> > wait_for_completion().
> > 
> > Secondly, it fixes the race which happens it a task with TIF_FREEZE set is
> > preempted right before calling frozen_process() in refrigerator() and stays
> > unforzen until after thaw_tasks() runs and checks its status.  For this 
> > purpose
> > task_lock() is used.
> 
> Great! But please be kind to those of us who read the source control history
> trying to understand the code. Could you make 2 separate patches?

Okay, attached.  The first one closes the race between thaw_tasks() and the
refrigerator that can occurs if the freezing fails.  The second one fixes the
vfork problem (should go on top of the first one).

> > @@ -207,7 +209,7 @@ static void thaw_tasks(int thaw_user_spa
> > if (is_user_space(p) == !thaw_user_space)
> > continue;
> >  
> > -   if (!thaw_process(p))
> > +   if (!thaw_process(p) && !freezer_should_skip(p))
> > printk(KERN_WARNING " Strange, %s not stopped\n",
> 
> This is racy, the warning could be false. We wake up the task, testing
> its ->flags is not reliable.
> 
> Damn. PF_FREEZER_SKIP task could be woken before, clear PF_FREEZER_SKIP,
> but not frozen.
> 
> We can change freezer_count() to clear PF_FREEZER_SKIP after try_to_freeze(),
> not before. Now thaw_process() can take PF_FREEZER_SKIP into account and
> return "true".
> 
> But this means the task may be PF_FREEZER_SKIP | PF_FROZEN. What if we we
> call try_to_freeze_tasks() soon after thaw_tasks()? We may hit the task which
> leaves the refrigerator, but didn't clear PF_FREEZER_SKIP yet. This means
> that thaw_process() should clear PF_FREEZER_SKIP as well. This is messy :(
> 
> Any other ideas? In any case we should imho avoid a separate loop for
> PF_FREEZER_SKIP tasks to just fix debug messages. In fact it can't help
> anyway.

Why don't we just drop the warning?  try_to_freeze_tasks() should give us a
warning if there's anything wrong anyway.

Greetings,
Rafael
 include/linux/freezer.h |4 
 kernel/power/process.c  |   14 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

Index: linux-2.6.20-mm2/include/linux/freezer.h
===
--- linux-2.6.20-mm2.orig/include/linux/freezer.h
+++ linux-2.6.20-mm2/include/linux/freezer.h
@@ -40,11 +40,15 @@ static inline void do_not_freeze(struct 
  */
 static inline int thaw_process(struct task_struct *p)
 {
+	task_lock(p);
 	if (frozen(p)) {
 		p->flags &= ~PF_FROZEN;
+		task_unlock(p);
 		wake_up_process(p);
 		return 1;
 	}
+	clear_tsk_thread_flag(p, TIF_FREEZE);
+	task_unlock(p);
 	return 0;
 }
 
Index: linux-2.6.20-mm2/kernel/power/process.c
===
--- linux-2.6.20-mm2.orig/kernel/power/process.c
+++ linux-2.6.20-mm2/kernel/power/process.c
@@ -39,10 +39,18 @@ void refrigerator(void)
 	/* Hmm, should we be allowed to suspend when there are realtime
 	   processes around? */
 	long save;
+
+	task_lock(current);
+	if (freezing(current)) {
+		frozen_process(current);
+		task_unlock(current);
+	} else {
+		task_unlock(current);
+		return;
+	}
 	save = current->state;
 	pr_debug("%s entered refrigerator\n", current->comm);
 
-	frozen_process(current);
 	spin_lock_irq(>sighand->siglock);
 	recalc_sigpending(); /* We sent fake signal, clean it up */
 	spin_unlock_irq(>sighand->siglock);
@@ -79,12 +87,16 @@ static void cancel_freezing(struct task_
 {
 	unsigned long flags;
 
+	task_lock(p);
 	if (freezing(p)) {
 		pr_debug("  clean up: %s\n", p->comm);
 		do_not_freeze(p);
+		task_unlock(p);
 		spin_lock_irqsave(>sighand->siglock, flags);
 		recalc_sigpending_tsk(p);
 		spin_unlock_irqrestore(>sighand->siglock, flags);
+	} else {
+		task_unlock(p);
 	}
 }
 
 include/linux/freezer.h |   30 --
 include/linux/sched.h   |1 +
 kernel/fork.c   |3 +++
 kernel/power/process.c  |   29 +
 4 files changed, 41 insertions(+), 22 deletions(-)

Index: linux-2.6.20-mm2/include/linux/sched.h
===
--- linux-2.6.20-mm2.orig/include/linux/sched.h
+++ linux-2.6.20-mm2/include/linux/sched.h
@@ -1189,6 +1189,7 @@ static inline void put_task_struct(struc
 #define PF_SPREAD_SLAB	0x0200	/* Spread some slab caches over cpuset */
 #define PF_MEMPOLICY	0x1000	/* Non-default NUMA mempolicy */
 #define PF_MUTEX_TESTER	0x2000	/* Thread belongs to the rt mutex tester */
+#define PF_FREEZER_SKIP	0x4000	/* Freezer should not count it as 

Re: freezer problems

[Oleg Nesterov]

On 02/22, Oleg Nesterov wrote:
>
> On 02/22, Rafael J. Wysocki wrote:
> >
> > @@ -207,7 +209,7 @@ static void thaw_tasks(int thaw_user_spa
> > if (is_user_space(p) == !thaw_user_space)
> > continue;
> >  
> > -   if (!thaw_process(p))
> > +   if (!thaw_process(p) && !freezer_should_skip(p))
> > printk(KERN_WARNING " Strange, %s not stopped\n",
> 
> This is racy, the warning could be false. We wake up the task, testing
> its ->flags is not reliable.
> 
> Damn. PF_FREEZER_SKIP task could be woken before, clear PF_FREEZER_SKIP,
> but not frozen.
> 
> We can change freezer_count() to clear PF_FREEZER_SKIP after try_to_freeze(),
> not before. Now thaw_process() can take PF_FREEZER_SKIP into account and
> return "true".
> 
> But this means the task may be PF_FREEZER_SKIP | PF_FROZEN. What if we we
> call try_to_freeze_tasks() soon after thaw_tasks()? We may hit the task which
> leaves the refrigerator, but didn't clear PF_FREEZER_SKIP yet. This means
> that thaw_process() should clear PF_FREEZER_SKIP as well. This is messy :(
> 
> Any other ideas? In any case we should imho avoid a separate loop for
> PF_FREEZER_SKIP tasks to just fix debug messages. In fact it can't help
> anyway.

Probably: current clears PF_FREEZER_SKIP along with TIF_FREEZE "atomically"
under task_lock in refrigerator(). thaw_process() takes PF_FREEZER_SKIP into
account.

Oleg.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Oleg Nesterov]

On 02/22, Rafael J. Wysocki wrote:
>
> Okay, below is what I have right now (compilation tested on x86_64):
> 
> This patch fixes the vfork problem by adding the PF_FREEZER_SKIP flag that
> can be used by tasks to tell the freezer not to count them as freezeable and
> making the vfork parents set this flag before they call wait_for_completion().
> 
> Secondly, it fixes the race which happens it a task with TIF_FREEZE set is
> preempted right before calling frozen_process() in refrigerator() and stays
> unforzen until after thaw_tasks() runs and checks its status.  For this 
> purpose
> task_lock() is used.

Great! But please be kind to those of us who read the source control history
trying to understand the code. Could you make 2 separate patches?

> @@ -207,7 +209,7 @@ static void thaw_tasks(int thaw_user_spa
>   if (is_user_space(p) == !thaw_user_space)
>   continue;
>  
> - if (!thaw_process(p))
> + if (!thaw_process(p) && !freezer_should_skip(p))
>   printk(KERN_WARNING " Strange, %s not stopped\n",

This is racy, the warning could be false. We wake up the task, testing
its ->flags is not reliable.

Damn. PF_FREEZER_SKIP task could be woken before, clear PF_FREEZER_SKIP,
but not frozen.

We can change freezer_count() to clear PF_FREEZER_SKIP after try_to_freeze(),
not before. Now thaw_process() can take PF_FREEZER_SKIP into account and
return "true".

But this means the task may be PF_FREEZER_SKIP | PF_FROZEN. What if we we
call try_to_freeze_tasks() soon after thaw_tasks()? We may hit the task which
leaves the refrigerator, but didn't clear PF_FREEZER_SKIP yet. This means
that thaw_process() should clear PF_FREEZER_SKIP as well. This is messy :(

Any other ideas? In any case we should imho avoid a separate loop for
PF_FREEZER_SKIP tasks to just fix debug messages. In fact it can't help
anyway.

Oleg.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Oleg Nesterov]

On 02/22, Rafael J. Wysocki wrote:

 Okay, below is what I have right now (compilation tested on x86_64):
 
 This patch fixes the vfork problem by adding the PF_FREEZER_SKIP flag that
 can be used by tasks to tell the freezer not to count them as freezeable and
 making the vfork parents set this flag before they call wait_for_completion().
 
 Secondly, it fixes the race which happens it a task with TIF_FREEZE set is
 preempted right before calling frozen_process() in refrigerator() and stays
 unforzen until after thaw_tasks() runs and checks its status.  For this 
 purpose
 task_lock() is used.

Great! But please be kind to those of us who read the source control history
trying to understand the code. Could you make 2 separate patches?

 @@ -207,7 +209,7 @@ static void thaw_tasks(int thaw_user_spa
   if (is_user_space(p) == !thaw_user_space)
   continue;
  
 - if (!thaw_process(p))
 + if (!thaw_process(p)  !freezer_should_skip(p))
   printk(KERN_WARNING  Strange, %s not stopped\n,

This is racy, the warning could be false. We wake up the task, testing
its -flags is not reliable.

Damn. PF_FREEZER_SKIP task could be woken before, clear PF_FREEZER_SKIP,
but not frozen.

We can change freezer_count() to clear PF_FREEZER_SKIP after try_to_freeze(),
not before. Now thaw_process() can take PF_FREEZER_SKIP into account and
return true.

But this means the task may be PF_FREEZER_SKIP | PF_FROZEN. What if we we
call try_to_freeze_tasks() soon after thaw_tasks()? We may hit the task which
leaves the refrigerator, but didn't clear PF_FREEZER_SKIP yet. This means
that thaw_process() should clear PF_FREEZER_SKIP as well. This is messy :(

Any other ideas? In any case we should imho avoid a separate loop for
PF_FREEZER_SKIP tasks to just fix debug messages. In fact it can't help
anyway.

Oleg.

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Oleg Nesterov]

On 02/22, Oleg Nesterov wrote:

 On 02/22, Rafael J. Wysocki wrote:
 
  @@ -207,7 +209,7 @@ static void thaw_tasks(int thaw_user_spa
  if (is_user_space(p) == !thaw_user_space)
  continue;
   
  -   if (!thaw_process(p))
  +   if (!thaw_process(p)  !freezer_should_skip(p))
  printk(KERN_WARNING  Strange, %s not stopped\n,
 
 This is racy, the warning could be false. We wake up the task, testing
 its -flags is not reliable.
 
 Damn. PF_FREEZER_SKIP task could be woken before, clear PF_FREEZER_SKIP,
 but not frozen.
 
 We can change freezer_count() to clear PF_FREEZER_SKIP after try_to_freeze(),
 not before. Now thaw_process() can take PF_FREEZER_SKIP into account and
 return true.
 
 But this means the task may be PF_FREEZER_SKIP | PF_FROZEN. What if we we
 call try_to_freeze_tasks() soon after thaw_tasks()? We may hit the task which
 leaves the refrigerator, but didn't clear PF_FREEZER_SKIP yet. This means
 that thaw_process() should clear PF_FREEZER_SKIP as well. This is messy :(
 
 Any other ideas? In any case we should imho avoid a separate loop for
 PF_FREEZER_SKIP tasks to just fix debug messages. In fact it can't help
 anyway.

Probably: current clears PF_FREEZER_SKIP along with TIF_FREEZE atomically
under task_lock in refrigerator(). thaw_process() takes PF_FREEZER_SKIP into
account.

Oleg.

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Thursday, 22 February 2007 11:47, Oleg Nesterov wrote:
 On 02/22, Rafael J. Wysocki wrote:
 
  Okay, below is what I have right now (compilation tested on x86_64):
  
  This patch fixes the vfork problem by adding the PF_FREEZER_SKIP flag that
  can be used by tasks to tell the freezer not to count them as freezeable and
  making the vfork parents set this flag before they call 
  wait_for_completion().
  
  Secondly, it fixes the race which happens it a task with TIF_FREEZE set is
  preempted right before calling frozen_process() in refrigerator() and stays
  unforzen until after thaw_tasks() runs and checks its status.  For this 
  purpose
  task_lock() is used.
 
 Great! But please be kind to those of us who read the source control history
 trying to understand the code. Could you make 2 separate patches?

Okay, attached.  The first one closes the race between thaw_tasks() and the
refrigerator that can occurs if the freezing fails.  The second one fixes the
vfork problem (should go on top of the first one).

  @@ -207,7 +209,7 @@ static void thaw_tasks(int thaw_user_spa
  if (is_user_space(p) == !thaw_user_space)
  continue;
   
  -   if (!thaw_process(p))
  +   if (!thaw_process(p)  !freezer_should_skip(p))
  printk(KERN_WARNING  Strange, %s not stopped\n,
 
 This is racy, the warning could be false. We wake up the task, testing
 its -flags is not reliable.
 
 Damn. PF_FREEZER_SKIP task could be woken before, clear PF_FREEZER_SKIP,
 but not frozen.
 
 We can change freezer_count() to clear PF_FREEZER_SKIP after try_to_freeze(),
 not before. Now thaw_process() can take PF_FREEZER_SKIP into account and
 return true.
 
 But this means the task may be PF_FREEZER_SKIP | PF_FROZEN. What if we we
 call try_to_freeze_tasks() soon after thaw_tasks()? We may hit the task which
 leaves the refrigerator, but didn't clear PF_FREEZER_SKIP yet. This means
 that thaw_process() should clear PF_FREEZER_SKIP as well. This is messy :(
 
 Any other ideas? In any case we should imho avoid a separate loop for
 PF_FREEZER_SKIP tasks to just fix debug messages. In fact it can't help
 anyway.

Why don't we just drop the warning?  try_to_freeze_tasks() should give us a
warning if there's anything wrong anyway.

Greetings,
Rafael
 include/linux/freezer.h |4 
 kernel/power/process.c  |   14 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

Index: linux-2.6.20-mm2/include/linux/freezer.h
===
--- linux-2.6.20-mm2.orig/include/linux/freezer.h
+++ linux-2.6.20-mm2/include/linux/freezer.h
@@ -40,11 +40,15 @@ static inline void do_not_freeze(struct 
  */
 static inline int thaw_process(struct task_struct *p)
 {
+	task_lock(p);
 	if (frozen(p)) {
 		p-flags = ~PF_FROZEN;
+		task_unlock(p);
 		wake_up_process(p);
 		return 1;
 	}
+	clear_tsk_thread_flag(p, TIF_FREEZE);
+	task_unlock(p);
 	return 0;
 }
 
Index: linux-2.6.20-mm2/kernel/power/process.c
===
--- linux-2.6.20-mm2.orig/kernel/power/process.c
+++ linux-2.6.20-mm2/kernel/power/process.c
@@ -39,10 +39,18 @@ void refrigerator(void)
 	/* Hmm, should we be allowed to suspend when there are realtime
 	   processes around? */
 	long save;
+
+	task_lock(current);
+	if (freezing(current)) {
+		frozen_process(current);
+		task_unlock(current);
+	} else {
+		task_unlock(current);
+		return;
+	}
 	save = current-state;
 	pr_debug(%s entered refrigerator\n, current-comm);
 
-	frozen_process(current);
 	spin_lock_irq(current-sighand-siglock);
 	recalc_sigpending(); /* We sent fake signal, clean it up */
 	spin_unlock_irq(current-sighand-siglock);
@@ -79,12 +87,16 @@ static void cancel_freezing(struct task_
 {
 	unsigned long flags;
 
+	task_lock(p);
 	if (freezing(p)) {
 		pr_debug(  clean up: %s\n, p-comm);
 		do_not_freeze(p);
+		task_unlock(p);
 		spin_lock_irqsave(p-sighand-siglock, flags);
 		recalc_sigpending_tsk(p);
 		spin_unlock_irqrestore(p-sighand-siglock, flags);
+	} else {
+		task_unlock(p);
 	}
 }
 
 include/linux/freezer.h |   30 --
 include/linux/sched.h   |1 +
 kernel/fork.c   |3 +++
 kernel/power/process.c  |   29 +
 4 files changed, 41 insertions(+), 22 deletions(-)

Index: linux-2.6.20-mm2/include/linux/sched.h
===
--- linux-2.6.20-mm2.orig/include/linux/sched.h
+++ linux-2.6.20-mm2/include/linux/sched.h
@@ -1189,6 +1189,7 @@ static inline void put_task_struct(struc
 #define PF_SPREAD_SLAB	0x0200	/* Spread some slab caches over cpuset */
 #define PF_MEMPOLICY	0x1000	/* Non-default NUMA mempolicy */
 #define PF_MUTEX_TESTER	0x2000	/* Thread belongs to the rt mutex tester */
+#define PF_FREEZER_SKIP	0x4000	/* Freezer should not count it as freezeable */
 
 /*
  * Only the _current_ task can read/write to 

Re: freezer problems

[Oleg Nesterov]

On 02/22, Rafael J. Wysocki wrote:
 
 Okay, attached.  The first one closes the race between thaw_tasks() and the
 refrigerator that can occurs if the freezing fails.  The second one fixes the
 vfork problem (should go on top of the first one).

Looks good to me.

  Any other ideas? In any case we should imho avoid a separate loop for
  PF_FREEZER_SKIP tasks to just fix debug messages. In fact it can't help
  anyway.
 
 Why don't we just drop the warning?  try_to_freeze_tasks() should give us a
 warning if there's anything wrong anyway.

Indeed :)

Oleg.

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Thursday, 22 February 2007 18:44, Oleg Nesterov wrote:
 On 02/22, Rafael J. Wysocki wrote:
  
  Okay, attached.  The first one closes the race between thaw_tasks() and the
  refrigerator that can occurs if the freezing fails.  The second one fixes 
  the
  vfork problem (should go on top of the first one).
 
 Looks good to me.
 
   Any other ideas? In any case we should imho avoid a separate loop for
   PF_FREEZER_SKIP tasks to just fix debug messages. In fact it can't help
   anyway.
  
  Why don't we just drop the warning?  try_to_freeze_tasks() should give us a
  warning if there's anything wrong anyway.
 
 Indeed :)

Still, there is a tiny race in the error path of try_to_freeze_tasks(), where
a vfork parent process can be preempted after clearing PF_FREEZER_SKIP and
before entering refrigerator(), so try_to_freeze_tasks() will mistakenly report
that this process has caused a problem.

I think this race can be closed by (1) clearing PF_FREEZER_SKIP after calling
try_to_freeze() in freezer_count(), (2) clearing PF_FREEZER_SKIP in
refrigerator() before calling frozen_process() and (3) taking task_lock()
around the warning check in the error path of try_to_freeze_tasks().

I have modified freezer-fix-vfork-problem.patch to implement this (appended;
it assumes that freezer-fix-theoretical-race.patch has already been applied).

If this is the right thing to do, I think there's a reason to additionally move
task_lock/unlock() from cancel_freezing() to the error path in
try_to_freeze_tasks().

Greetings,
Rafael


 include/linux/freezer.h |   30 --
 include/linux/sched.h   |1 +
 kernel/fork.c   |3 +++
 kernel/power/process.c  |   32 +---
 4 files changed, 45 insertions(+), 21 deletions(-)

Index: linux-2.6.20-mm2/include/linux/sched.h
===
--- linux-2.6.20-mm2.orig/include/linux/sched.h
+++ linux-2.6.20-mm2/include/linux/sched.h
@@ -1189,6 +1189,7 @@ static inline void put_task_struct(struc
 #define PF_SPREAD_SLAB 0x0200  /* Spread some slab caches over cpuset 
*/
 #define PF_MEMPOLICY   0x1000  /* Non-default NUMA mempolicy */
 #define PF_MUTEX_TESTER0x2000  /* Thread belongs to the rt 
mutex tester */
+#define PF_FREEZER_SKIP0x4000  /* Freezer should not count it 
as freezeable */
 
 /*
  * Only the _current_ task can read/write to tsk-flags, but other
Index: linux-2.6.20-mm2/include/linux/freezer.h
===
--- linux-2.6.20-mm2.orig/include/linux/freezer.h
+++ linux-2.6.20-mm2/include/linux/freezer.h
@@ -75,7 +75,31 @@ static inline int try_to_freeze(void)
return 0;
 }
 
-extern void thaw_some_processes(int all);
+/*
+ * Tell the freezer not to count current task as freezeable
+ */
+static inline void freezer_do_not_count(void)
+{
+   current-flags |= PF_FREEZER_SKIP;
+}
+
+/*
+ * Try to freeze the current task and tell the freezer to count it as 
freezeable
+ * again
+ */
+static inline void freezer_count(void)
+{
+   try_to_freeze();
+   current-flags = ~PF_FREEZER_SKIP;
+}
+
+/*
+ * Check if the task should be counted as freezeable by the freezer
+ */
+static inline int freezer_should_skip(struct task_struct *p)
+{
+   return !!(p-flags  PF_FREEZER_SKIP);
+}
 
 #else
 static inline int frozen(struct task_struct *p) { return 0; }
@@ -90,5 +114,7 @@ static inline void thaw_processes(void) 
 
 static inline int try_to_freeze(void) { return 0; }
 
-
+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; }
 #endif
Index: linux-2.6.20-mm2/kernel/fork.c
===
--- linux-2.6.20-mm2.orig/kernel/fork.c
+++ linux-2.6.20-mm2/kernel/fork.c
@@ -50,6 +50,7 @@
 #include linux/taskstats_kern.h
 #include linux/random.h
 #include linux/ptrace.h
+#include linux/freezer.h
 
 #include asm/pgtable.h
 #include asm/pgalloc.h
@@ -1393,7 +1394,9 @@ long do_fork(unsigned long clone_flags,
tracehook_report_clone_complete(clone_flags, nr, p);
 
if (clone_flags  CLONE_VFORK) {
+   freezer_do_not_count();
wait_for_completion(vfork);
+   freezer_count();
tracehook_report_vfork_done(p, nr);
}
} else {
Index: linux-2.6.20-mm2/kernel/power/process.c
===
--- linux-2.6.20-mm2.orig/kernel/power/process.c
+++ linux-2.6.20-mm2/kernel/power/process.c
@@ -42,6 +42,7 @@ void refrigerator(void)
 
task_lock(current);
if (freezing(current)) {
+   current-flags = ~PF_FREEZER_SKIP;
frozen_process(current);
task_unlock(current);
} else {
@@ 

Re: freezer problems

[Rafael J. Wysocki]

On Wednesday, 21 February 2007 22:06, Paul E. McKenney wrote:
> On Wed, Feb 21, 2007 at 11:03:14PM +0300, Oleg Nesterov wrote:
> > On 02/21, Rafael J. Wysocki wrote:
> > >
> > > On Wednesday, 21 February 2007 19:14, Paul E. McKenney wrote:
> > > > On Tue, Feb 20, 2007 at 07:29:01PM +0100, Rafael J. Wysocki wrote:
> > > > > On Tuesday, 20 February 2007 01:32, Rafael J. Wysocki wrote:
> > > > > > On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
> > > > > > Hm.  In the case discussed above we have a task that's right before 
> > > > > > calling
> > > > > > frozen_process(), so we can't thaw it, because it's not frozen.  It 
> > > > > > will be
> > > > > > frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() 
> > > > > > have no
> > > > > > way to check this.
> > > > > > 
> > > > > > I think to close this race the refrigerator should check TIF_FREEZE 
> > > > > > and set
> > > > > > PF_FROZEN _and_ reset TIF_FREEZE under a lock
> > 
> > I personally think this is good. Not only this allows us to close the race,
> > I think we can do more.
> > 
> > >  that would also have 
> > > to be
> > > > > > taken by try_to_freeze_tasks() in the beginning of the error path.  
> > > > > > This will
> > > > > > ensure that all tasks either freeze themselves before the error 
> > > > > > path in
> > > > > > try_to_freeze_tasks() is executed, or remain unfrozen.
> > 
> > How about take this lock in thaw_tasks() instead/too ?
> > 
> > Currently we need a separate loop in thaw_tasks() to handle 
> > PF_FREEZER_SKIP. This
> > means that PF_FREEZER_SKIP is not so generic: thaw_tasks() can't tolerate 
> > if such
> > a task was woken in between. What if we change thaw_process() to clear 
> > TIF_FREEZE ?
> > 
> > Note also that we can use task_lock() instead of global refrigerator_lock. 
> > This
> > means that thaw_process() should take it too, probably this is slowdown, 
> > but I
> > think not too much because thaw_process() is going to write to p->flags 
> > anyway.
> > In this case thaw_process() works perfectly as cancel_freezing_and_thaw() 
> > and
> > can be used to fix exec/coredump in future.
> 
> This sounds much better than a a global lock to me!  ;-)

Okay, below is what I have right now (compilation tested on x86_64):

This patch fixes the vfork problem by adding the PF_FREEZER_SKIP flag that
can be used by tasks to tell the freezer not to count them as freezeable and
making the vfork parents set this flag before they call wait_for_completion().

Secondly, it fixes the race which happens it a task with TIF_FREEZE set is
preempted right before calling frozen_process() in refrigerator() and stays
unforzen until after thaw_tasks() runs and checks its status.  For this purpose
task_lock() is used.

 include/linux/freezer.h |   34 --
 include/linux/sched.h   |1 +
 kernel/fork.c   |3 +++
 kernel/power/process.c  |   36 +++-
 4 files changed, 55 insertions(+), 19 deletions(-)

Index: linux-2.6.20-mm2/include/linux/sched.h
===
--- linux-2.6.20-mm2.orig/include/linux/sched.h
+++ linux-2.6.20-mm2/include/linux/sched.h
@@ -1189,6 +1189,7 @@ static inline void put_task_struct(struc
 #define PF_SPREAD_SLAB 0x0200  /* Spread some slab caches over cpuset 
*/
 #define PF_MEMPOLICY   0x1000  /* Non-default NUMA mempolicy */
 #define PF_MUTEX_TESTER0x2000  /* Thread belongs to the rt 
mutex tester */
+#define PF_FREEZER_SKIP0x4000  /* Freezer should not count it 
as freezeable */
 
 /*
  * Only the _current_ task can read/write to tsk->flags, but other
Index: linux-2.6.20-mm2/include/linux/freezer.h
===
--- linux-2.6.20-mm2.orig/include/linux/freezer.h
+++ linux-2.6.20-mm2/include/linux/freezer.h
@@ -40,11 +40,15 @@ static inline void do_not_freeze(struct 
  */
 static inline int thaw_process(struct task_struct *p)
 {
+   task_lock(p);
if (frozen(p)) {
p->flags &= ~PF_FROZEN;
+   task_unlock(p);
wake_up_process(p);
return 1;
}
+   clear_tsk_thread_flag(p, TIF_FREEZE);
+   task_unlock(p);
return 0;
 }
 
@@ -71,7 +75,31 @@ static inline int try_to_freeze(void)
return 0;
 }
 
-extern void thaw_some_processes(int all);
+/*
+ * Tell the freezer not to count current task as freezeable
+ */
+static inline void freezer_do_not_count(void)
+{
+   current->flags |= PF_FREEZER_SKIP;
+}
+
+/*
+ * Try to freeze the current task and tell the freezer to count it as 
freezeable
+ * again
+ */
+static inline void freezer_count(void)
+{
+   current->flags &= ~PF_FREEZER_SKIP;
+   try_to_freeze();
+}
+
+/*
+ * Check if the task should be counted as freezeable by the freezer
+ */
+static inline int 

Re: freezer problems

[Paul E. McKenney]

On Wed, Feb 21, 2007 at 11:03:14PM +0300, Oleg Nesterov wrote:
> On 02/21, Rafael J. Wysocki wrote:
> >
> > On Wednesday, 21 February 2007 19:14, Paul E. McKenney wrote:
> > > On Tue, Feb 20, 2007 at 07:29:01PM +0100, Rafael J. Wysocki wrote:
> > > > On Tuesday, 20 February 2007 01:32, Rafael J. Wysocki wrote:
> > > > > On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
> > > > > Hm.  In the case discussed above we have a task that's right before 
> > > > > calling
> > > > > frozen_process(), so we can't thaw it, because it's not frozen.  It 
> > > > > will be
> > > > > frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() 
> > > > > have no
> > > > > way to check this.
> > > > > 
> > > > > I think to close this race the refrigerator should check TIF_FREEZE 
> > > > > and set
> > > > > PF_FROZEN _and_ reset TIF_FREEZE under a lock
> 
> I personally think this is good. Not only this allows us to close the race,
> I think we can do more.
> 
> >  that would also have 
> > to be
> > > > > taken by try_to_freeze_tasks() in the beginning of the error path.  
> > > > > This will
> > > > > ensure that all tasks either freeze themselves before the error path 
> > > > > in
> > > > > try_to_freeze_tasks() is executed, or remain unfrozen.
> 
> How about take this lock in thaw_tasks() instead/too ?
> 
> Currently we need a separate loop in thaw_tasks() to handle PF_FREEZER_SKIP. 
> This
> means that PF_FREEZER_SKIP is not so generic: thaw_tasks() can't tolerate if 
> such
> a task was woken in between. What if we change thaw_process() to clear 
> TIF_FREEZE ?
> 
> Note also that we can use task_lock() instead of global refrigerator_lock. 
> This
> means that thaw_process() should take it too, probably this is slowdown, but I
> think not too much because thaw_process() is going to write to p->flags 
> anyway.
> In this case thaw_process() works perfectly as cancel_freezing_and_thaw() and
> can be used to fix exec/coredump in future.

This sounds much better than a a global lock to me!  ;-)

Thanx, Paul
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Wednesday, 21 February 2007 21:03, Oleg Nesterov wrote:
> On 02/21, Rafael J. Wysocki wrote:
> >
> > On Wednesday, 21 February 2007 19:14, Paul E. McKenney wrote:
> > > On Tue, Feb 20, 2007 at 07:29:01PM +0100, Rafael J. Wysocki wrote:
> > > > On Tuesday, 20 February 2007 01:32, Rafael J. Wysocki wrote:
> > > > > On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
> > > > > Hm.  In the case discussed above we have a task that's right before 
> > > > > calling
> > > > > frozen_process(), so we can't thaw it, because it's not frozen.  It 
> > > > > will be
> > > > > frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() 
> > > > > have no
> > > > > way to check this.
> > > > > 
> > > > > I think to close this race the refrigerator should check TIF_FREEZE 
> > > > > and set
> > > > > PF_FROZEN _and_ reset TIF_FREEZE under a lock
> 
> I personally think this is good. Not only this allows us to close the race,
> I think we can do more.
> 
> >  that would also have 
> > to be
> > > > > taken by try_to_freeze_tasks() in the beginning of the error path.  
> > > > > This will
> > > > > ensure that all tasks either freeze themselves before the error path 
> > > > > in
> > > > > try_to_freeze_tasks() is executed, or remain unfrozen.
> 
> How about take this lock in thaw_tasks() instead/too ?

Good point.  If we take it in thaw_tasks(), then the tasks that have
TIF_FREEZE set, but haven't entered the refrigerator yet, won't be able to
enter the refrigerator until thaw_tasks() releases the lock ...
> 
> Currently we need a separate loop in thaw_tasks() to handle PF_FREEZER_SKIP. 
> This
> means that PF_FREEZER_SKIP is not so generic: thaw_tasks() can't tolerate if 
> such
> a task was woken in between. What if we change thaw_process() to clear 
> TIF_FREEZE ?

... and then we can drop the PF_FREEZER_SKIP-handling loop and change
thaw_process() to clear TIF_FREEZE.

> Note also that we can use task_lock() instead of global refrigerator_lock. 
> This
> means that thaw_process() should take it too, probably this is slowdown, but I
> think not too much because thaw_process() is going to write to p->flags 
> anyway.
> In this case thaw_process() works perfectly as cancel_freezing_and_thaw() and
> can be used to fix exec/coredump in future.

Hm, I think we can try doing this too.

I'll try to prepare a patch later today.

Greetings,
Rafael
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Oleg Nesterov]

On 02/21, Rafael J. Wysocki wrote:
>
> On Wednesday, 21 February 2007 19:14, Paul E. McKenney wrote:
> > On Tue, Feb 20, 2007 at 07:29:01PM +0100, Rafael J. Wysocki wrote:
> > > On Tuesday, 20 February 2007 01:32, Rafael J. Wysocki wrote:
> > > > On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
> > > > Hm.  In the case discussed above we have a task that's right before 
> > > > calling
> > > > frozen_process(), so we can't thaw it, because it's not frozen.  It 
> > > > will be
> > > > frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() have 
> > > > no
> > > > way to check this.
> > > > 
> > > > I think to close this race the refrigerator should check TIF_FREEZE and 
> > > > set
> > > > PF_FROZEN _and_ reset TIF_FREEZE under a lock

I personally think this is good. Not only this allows us to close the race,
I think we can do more.

>  that would also have to 
> be
> > > > taken by try_to_freeze_tasks() in the beginning of the error path.  
> > > > This will
> > > > ensure that all tasks either freeze themselves before the error path in
> > > > try_to_freeze_tasks() is executed, or remain unfrozen.

How about take this lock in thaw_tasks() instead/too ?

Currently we need a separate loop in thaw_tasks() to handle PF_FREEZER_SKIP. 
This
means that PF_FREEZER_SKIP is not so generic: thaw_tasks() can't tolerate if 
such
a task was woken in between. What if we change thaw_process() to clear 
TIF_FREEZE ?

Note also that we can use task_lock() instead of global refrigerator_lock. This
means that thaw_process() should take it too, probably this is slowdown, but I
think not too much because thaw_process() is going to write to p->flags anyway.
In this case thaw_process() works perfectly as cancel_freezing_and_thaw() and
can be used to fix exec/coredump in future.

Thoughts?

Oleg.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Paul E. McKenney]

On Wed, Feb 21, 2007 at 07:13:40PM +0100, Rafael J. Wysocki wrote:
> On Wednesday, 21 February 2007 19:14, Paul E. McKenney wrote:
> > On Tue, Feb 20, 2007 at 07:29:01PM +0100, Rafael J. Wysocki wrote:
> > > On Tuesday, 20 February 2007 01:32, Rafael J. Wysocki wrote:
> > > > On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
> > > > Hm.  In the case discussed above we have a task that's right before 
> > > > calling
> > > > frozen_process(), so we can't thaw it, because it's not frozen.  It 
> > > > will be
> > > > frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() have 
> > > > no
> > > > way to check this.
> > > > 
> > > > I think to close this race the refrigerator should check TIF_FREEZE and 
> > > > set
> > > > PF_FROZEN _and_ reset TIF_FREEZE under a lock that would also have to be
> > > > taken by try_to_freeze_tasks() in the beginning of the error path.  
> > > > This will
> > > > ensure that all tasks either freeze themselves before the error path in
> > > > try_to_freeze_tasks() is executed, or remain unfrozen.
> > > > 
> > > > I'll try to prepare a patch to illustrate this, but right now I'm too 
> > > > tired to
> > > > do it. :-)
> > > 
> > > Something like this, perhaps:
> > > 
> > > ---
> > >  include/linux/freezer.h |   10 +++---
> > >  kernel/power/process.c  |   18 --
> > >  2 files changed, 19 insertions(+), 9 deletions(-)
> > > 
> > > Index: linux-2.6.20-mm2/include/linux/freezer.h
> > > ===
> > > --- linux-2.6.20-mm2.orig/include/linux/freezer.h
> > > +++ linux-2.6.20-mm2/include/linux/freezer.h
> > > @@ -58,17 +58,13 @@ static inline void frozen_process(struct
> > >   clear_tsk_thread_flag(p, TIF_FREEZE);
> > >  }
> > > 
> > > -extern void refrigerator(void);
> > > +extern int refrigerator(void);
> > >  extern int freeze_processes(void);
> > >  extern void thaw_processes(void);
> > > 
> > >  static inline int try_to_freeze(void)
> > >  {
> > > - if (freezing(current)) {
> > > - refrigerator();
> > > - return 1;
> > > - } else
> > > - return 0;
> > > + return refrigerator();
> > >  }
> > > 
> > >  /*
> > > @@ -104,7 +100,7 @@ static inline void freeze(struct task_st
> > >  static inline int thaw_process(struct task_struct *p) { return 1; }
> > >  static inline void frozen_process(struct task_struct *p) { BUG(); }
> > > 
> > > -static inline void refrigerator(void) {}
> > > +static inline int refrigerator(void) { return 0; }
> > >  static inline int freeze_processes(void) { BUG(); return 0; }
> > >  static inline void thaw_processes(void) {}
> > > 
> > > Index: linux-2.6.20-mm2/kernel/power/process.c
> > > ===
> > > --- linux-2.6.20-mm2.orig/kernel/power/process.c
> > > +++ linux-2.6.20-mm2/kernel/power/process.c
> > > @@ -24,6 +24,8 @@
> > >  #define FREEZER_KERNEL_THREADS 0
> > >  #define FREEZER_USER_SPACE 1
> > > 
> > > +spinlock_t refrigerator_lock;
> > > +
> > >  static inline int freezeable(struct task_struct * p)
> > >  {
> > >   if ((p == current) ||
> > > @@ -34,15 +36,23 @@ static inline int freezeable(struct task
> > >  }
> > > 
> > >  /* Refrigerator is place where frozen processes are stored :-). */
> > > -void refrigerator(void)
> > > +int refrigerator(void)
> > >  {
> > >   /* Hmm, should we be allowed to suspend when there are realtime
> > >  processes around? */
> > >   long save;
> > > +
> > > + spin_lock(_lock);
> > 
> > I hope we can do this without a global lock that is acquired on each
> > try_to_freeze() call!
> 
> Yes.  Here's the current version (try_to_freeze() is unchanged, so the lock
> is only taken by the tasks that are going to freeze, or so they think):

Ah, OK, that should work much better from a lock-contention viewpoint!

Thanx, Paul

> ---
>  kernel/power/process.c |   15 ++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.20-mm2/kernel/power/process.c
> ===
> --- linux-2.6.20-mm2.orig/kernel/power/process.c
> +++ linux-2.6.20-mm2/kernel/power/process.c
> @@ -24,6 +24,8 @@
>  #define FREEZER_KERNEL_THREADS 0
>  #define FREEZER_USER_SPACE 1
> 
> +static spinlock_t refrigerator_lock;
> +
>  static inline int freezeable(struct task_struct * p)
>  {
>   if ((p == current) ||
> @@ -39,10 +41,18 @@ void refrigerator(void)
>   /* Hmm, should we be allowed to suspend when there are realtime
>  processes around? */
>   long save;
> +
> + spin_lock(_lock);
> + if (freezing(current)) {
> + frozen_process(current);
> + spin_unlock(_lock);
> + } else {
> + spin_unlock(_lock);
> + return;
> + }
>   save = current->state;
>   pr_debug("%s entered refrigerator\n", current->comm);
> 
> - frozen_process(current);
>   

Re: freezer problems

[Rafael J. Wysocki]

On Wednesday, 21 February 2007 19:14, Paul E. McKenney wrote:
> On Tue, Feb 20, 2007 at 07:29:01PM +0100, Rafael J. Wysocki wrote:
> > On Tuesday, 20 February 2007 01:32, Rafael J. Wysocki wrote:
> > > On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
> > > Hm.  In the case discussed above we have a task that's right before 
> > > calling
> > > frozen_process(), so we can't thaw it, because it's not frozen.  It will 
> > > be
> > > frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() have no
> > > way to check this.
> > > 
> > > I think to close this race the refrigerator should check TIF_FREEZE and 
> > > set
> > > PF_FROZEN _and_ reset TIF_FREEZE under a lock that would also have to be
> > > taken by try_to_freeze_tasks() in the beginning of the error path.  This 
> > > will
> > > ensure that all tasks either freeze themselves before the error path in
> > > try_to_freeze_tasks() is executed, or remain unfrozen.
> > > 
> > > I'll try to prepare a patch to illustrate this, but right now I'm too 
> > > tired to
> > > do it. :-)
> > 
> > Something like this, perhaps:
> > 
> > ---
> >  include/linux/freezer.h |   10 +++---
> >  kernel/power/process.c  |   18 --
> >  2 files changed, 19 insertions(+), 9 deletions(-)
> > 
> > Index: linux-2.6.20-mm2/include/linux/freezer.h
> > ===
> > --- linux-2.6.20-mm2.orig/include/linux/freezer.h
> > +++ linux-2.6.20-mm2/include/linux/freezer.h
> > @@ -58,17 +58,13 @@ static inline void frozen_process(struct
> > clear_tsk_thread_flag(p, TIF_FREEZE);
> >  }
> > 
> > -extern void refrigerator(void);
> > +extern int refrigerator(void);
> >  extern int freeze_processes(void);
> >  extern void thaw_processes(void);
> > 
> >  static inline int try_to_freeze(void)
> >  {
> > -   if (freezing(current)) {
> > -   refrigerator();
> > -   return 1;
> > -   } else
> > -   return 0;
> > +   return refrigerator();
> >  }
> > 
> >  /*
> > @@ -104,7 +100,7 @@ static inline void freeze(struct task_st
> >  static inline int thaw_process(struct task_struct *p) { return 1; }
> >  static inline void frozen_process(struct task_struct *p) { BUG(); }
> > 
> > -static inline void refrigerator(void) {}
> > +static inline int refrigerator(void) { return 0; }
> >  static inline int freeze_processes(void) { BUG(); return 0; }
> >  static inline void thaw_processes(void) {}
> > 
> > Index: linux-2.6.20-mm2/kernel/power/process.c
> > ===
> > --- linux-2.6.20-mm2.orig/kernel/power/process.c
> > +++ linux-2.6.20-mm2/kernel/power/process.c
> > @@ -24,6 +24,8 @@
> >  #define FREEZER_KERNEL_THREADS 0
> >  #define FREEZER_USER_SPACE 1
> > 
> > +spinlock_t refrigerator_lock;
> > +
> >  static inline int freezeable(struct task_struct * p)
> >  {
> > if ((p == current) ||
> > @@ -34,15 +36,23 @@ static inline int freezeable(struct task
> >  }
> > 
> >  /* Refrigerator is place where frozen processes are stored :-). */
> > -void refrigerator(void)
> > +int refrigerator(void)
> >  {
> > /* Hmm, should we be allowed to suspend when there are realtime
> >processes around? */
> > long save;
> > +
> > +   spin_lock(_lock);
> 
> I hope we can do this without a global lock that is acquired on each
> try_to_freeze() call!

Yes.  Here's the current version (try_to_freeze() is unchanged, so the lock
is only taken by the tasks that are going to freeze, or so they think):

---
 kernel/power/process.c |   15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Index: linux-2.6.20-mm2/kernel/power/process.c
===
--- linux-2.6.20-mm2.orig/kernel/power/process.c
+++ linux-2.6.20-mm2/kernel/power/process.c
@@ -24,6 +24,8 @@
 #define FREEZER_KERNEL_THREADS 0
 #define FREEZER_USER_SPACE 1
 
+static spinlock_t refrigerator_lock;
+
 static inline int freezeable(struct task_struct * p)
 {
if ((p == current) ||
@@ -39,10 +41,18 @@ void refrigerator(void)
/* Hmm, should we be allowed to suspend when there are realtime
   processes around? */
long save;
+
+   spin_lock(_lock);
+   if (freezing(current)) {
+   frozen_process(current);
+   spin_unlock(_lock);
+   } else {
+   spin_unlock(_lock);
+   return;
+   }
save = current->state;
pr_debug("%s entered refrigerator\n", current->comm);
 
-   frozen_process(current);
spin_lock_irq(>sighand->siglock);
recalc_sigpending(); /* We sent fake signal, clean it up */
spin_unlock_irq(>sighand->siglock);
@@ -143,6 +153,7 @@ static unsigned int try_to_freeze_tasks(
"kernel threads",
TIMEOUT / HZ, todo);
read_lock(_lock);
+   spin_lock(_lock);
do_each_thread(g, 

Re: freezer problems

[Paul E. McKenney]

On Tue, Feb 20, 2007 at 07:29:01PM +0100, Rafael J. Wysocki wrote:
> On Tuesday, 20 February 2007 01:32, Rafael J. Wysocki wrote:
> > On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
> > Hm.  In the case discussed above we have a task that's right before calling
> > frozen_process(), so we can't thaw it, because it's not frozen.  It will be
> > frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() have no
> > way to check this.
> > 
> > I think to close this race the refrigerator should check TIF_FREEZE and set
> > PF_FROZEN _and_ reset TIF_FREEZE under a lock that would also have to be
> > taken by try_to_freeze_tasks() in the beginning of the error path.  This 
> > will
> > ensure that all tasks either freeze themselves before the error path in
> > try_to_freeze_tasks() is executed, or remain unfrozen.
> > 
> > I'll try to prepare a patch to illustrate this, but right now I'm too tired 
> > to
> > do it. :-)
> 
> Something like this, perhaps:
> 
> ---
>  include/linux/freezer.h |   10 +++---
>  kernel/power/process.c  |   18 --
>  2 files changed, 19 insertions(+), 9 deletions(-)
> 
> Index: linux-2.6.20-mm2/include/linux/freezer.h
> ===
> --- linux-2.6.20-mm2.orig/include/linux/freezer.h
> +++ linux-2.6.20-mm2/include/linux/freezer.h
> @@ -58,17 +58,13 @@ static inline void frozen_process(struct
>   clear_tsk_thread_flag(p, TIF_FREEZE);
>  }
> 
> -extern void refrigerator(void);
> +extern int refrigerator(void);
>  extern int freeze_processes(void);
>  extern void thaw_processes(void);
> 
>  static inline int try_to_freeze(void)
>  {
> - if (freezing(current)) {
> - refrigerator();
> - return 1;
> - } else
> - return 0;
> + return refrigerator();
>  }
> 
>  /*
> @@ -104,7 +100,7 @@ static inline void freeze(struct task_st
>  static inline int thaw_process(struct task_struct *p) { return 1; }
>  static inline void frozen_process(struct task_struct *p) { BUG(); }
> 
> -static inline void refrigerator(void) {}
> +static inline int refrigerator(void) { return 0; }
>  static inline int freeze_processes(void) { BUG(); return 0; }
>  static inline void thaw_processes(void) {}
> 
> Index: linux-2.6.20-mm2/kernel/power/process.c
> ===
> --- linux-2.6.20-mm2.orig/kernel/power/process.c
> +++ linux-2.6.20-mm2/kernel/power/process.c
> @@ -24,6 +24,8 @@
>  #define FREEZER_KERNEL_THREADS 0
>  #define FREEZER_USER_SPACE 1
> 
> +spinlock_t refrigerator_lock;
> +
>  static inline int freezeable(struct task_struct * p)
>  {
>   if ((p == current) ||
> @@ -34,15 +36,23 @@ static inline int freezeable(struct task
>  }
> 
>  /* Refrigerator is place where frozen processes are stored :-). */
> -void refrigerator(void)
> +int refrigerator(void)
>  {
>   /* Hmm, should we be allowed to suspend when there are realtime
>  processes around? */
>   long save;
> +
> + spin_lock(_lock);

I hope we can do this without a global lock that is acquired on each
try_to_freeze() call!

> + if (freezing(current)) {

Would it be possible to acquire the lock here instead, then recheck here?
Or use a per-thread lock?  (Yes, this would make the error checking path
have to acquire a very large number of threads, but...

Thanx, Paul

> + frozen_process(current);
> + spin_unlock(_lock);
> + } else {
> + spin_unlock(_lock);
> + return 0;
> + }
>   save = current->state;
>   pr_debug("%s entered refrigerator\n", current->comm);
> 
> - frozen_process(current);
>   spin_lock_irq(>sighand->siglock);
>   recalc_sigpending(); /* We sent fake signal, clean it up */
>   spin_unlock_irq(>sighand->siglock);
> @@ -53,6 +63,7 @@ void refrigerator(void)
>   }
>   pr_debug("%s left refrigerator\n", current->comm);
>   current->state = save;
> + return 1;
>  }
> 
>  static inline void freeze_process(struct task_struct *p)
> @@ -143,6 +154,7 @@ static unsigned int try_to_freeze_tasks(
>   "kernel threads",
>   TIMEOUT / HZ, todo);
>   read_lock(_lock);
> + spin_lock(_lock);
>   do_each_thread(g, p) {
>   if (is_user_space(p) == !freeze_user_space)
>   continue;
> @@ -152,6 +164,7 @@ static unsigned int try_to_freeze_tasks(
> 
>   cancel_freezing(p);
>   } while_each_thread(g, p);
> + spin_unlock(_lock);
>   read_unlock(_lock);
>   }
> 
> @@ -169,6 +182,7 @@ int freeze_processes(void)
>   unsigned int nr_unfrozen;
> 
>   printk("Stopping tasks ... ");
> + spin_lock_init(_lock);
>   nr_unfrozen = try_to_freeze_tasks(FREEZER_USER_SPACE);
>   if 

Re: freezer problems

[Paul E. McKenney]

On Tue, Feb 20, 2007 at 07:29:01PM +0100, Rafael J. Wysocki wrote:
 On Tuesday, 20 February 2007 01:32, Rafael J. Wysocki wrote:
  On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
  Hm.  In the case discussed above we have a task that's right before calling
  frozen_process(), so we can't thaw it, because it's not frozen.  It will be
  frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() have no
  way to check this.
  
  I think to close this race the refrigerator should check TIF_FREEZE and set
  PF_FROZEN _and_ reset TIF_FREEZE under a lock that would also have to be
  taken by try_to_freeze_tasks() in the beginning of the error path.  This 
  will
  ensure that all tasks either freeze themselves before the error path in
  try_to_freeze_tasks() is executed, or remain unfrozen.
  
  I'll try to prepare a patch to illustrate this, but right now I'm too tired 
  to
  do it. :-)
 
 Something like this, perhaps:
 
 ---
  include/linux/freezer.h |   10 +++---
  kernel/power/process.c  |   18 --
  2 files changed, 19 insertions(+), 9 deletions(-)
 
 Index: linux-2.6.20-mm2/include/linux/freezer.h
 ===
 --- linux-2.6.20-mm2.orig/include/linux/freezer.h
 +++ linux-2.6.20-mm2/include/linux/freezer.h
 @@ -58,17 +58,13 @@ static inline void frozen_process(struct
   clear_tsk_thread_flag(p, TIF_FREEZE);
  }
 
 -extern void refrigerator(void);
 +extern int refrigerator(void);
  extern int freeze_processes(void);
  extern void thaw_processes(void);
 
  static inline int try_to_freeze(void)
  {
 - if (freezing(current)) {
 - refrigerator();
 - return 1;
 - } else
 - return 0;
 + return refrigerator();
  }
 
  /*
 @@ -104,7 +100,7 @@ static inline void freeze(struct task_st
  static inline int thaw_process(struct task_struct *p) { return 1; }
  static inline void frozen_process(struct task_struct *p) { BUG(); }
 
 -static inline void refrigerator(void) {}
 +static inline int refrigerator(void) { return 0; }
  static inline int freeze_processes(void) { BUG(); return 0; }
  static inline void thaw_processes(void) {}
 
 Index: linux-2.6.20-mm2/kernel/power/process.c
 ===
 --- linux-2.6.20-mm2.orig/kernel/power/process.c
 +++ linux-2.6.20-mm2/kernel/power/process.c
 @@ -24,6 +24,8 @@
  #define FREEZER_KERNEL_THREADS 0
  #define FREEZER_USER_SPACE 1
 
 +spinlock_t refrigerator_lock;
 +
  static inline int freezeable(struct task_struct * p)
  {
   if ((p == current) ||
 @@ -34,15 +36,23 @@ static inline int freezeable(struct task
  }
 
  /* Refrigerator is place where frozen processes are stored :-). */
 -void refrigerator(void)
 +int refrigerator(void)
  {
   /* Hmm, should we be allowed to suspend when there are realtime
  processes around? */
   long save;
 +
 + spin_lock(refrigerator_lock);

I hope we can do this without a global lock that is acquired on each
try_to_freeze() call!

 + if (freezing(current)) {

Would it be possible to acquire the lock here instead, then recheck here?
Or use a per-thread lock?  (Yes, this would make the error checking path
have to acquire a very large number of threads, but...

Thanx, Paul

 + frozen_process(current);
 + spin_unlock(refrigerator_lock);
 + } else {
 + spin_unlock(refrigerator_lock);
 + return 0;
 + }
   save = current-state;
   pr_debug(%s entered refrigerator\n, current-comm);
 
 - frozen_process(current);
   spin_lock_irq(current-sighand-siglock);
   recalc_sigpending(); /* We sent fake signal, clean it up */
   spin_unlock_irq(current-sighand-siglock);
 @@ -53,6 +63,7 @@ void refrigerator(void)
   }
   pr_debug(%s left refrigerator\n, current-comm);
   current-state = save;
 + return 1;
  }
 
  static inline void freeze_process(struct task_struct *p)
 @@ -143,6 +154,7 @@ static unsigned int try_to_freeze_tasks(
   kernel threads,
   TIMEOUT / HZ, todo);
   read_lock(tasklist_lock);
 + spin_lock(refrigerator_lock);
   do_each_thread(g, p) {
   if (is_user_space(p) == !freeze_user_space)
   continue;
 @@ -152,6 +164,7 @@ static unsigned int try_to_freeze_tasks(
 
   cancel_freezing(p);
   } while_each_thread(g, p);
 + spin_unlock(refrigerator_lock);
   read_unlock(tasklist_lock);
   }
 
 @@ -169,6 +182,7 @@ int freeze_processes(void)
   unsigned int nr_unfrozen;
 
   printk(Stopping tasks ... );
 + spin_lock_init(refrigerator_lock);
   nr_unfrozen = try_to_freeze_tasks(FREEZER_USER_SPACE);
   if (nr_unfrozen)
   return nr_unfrozen;
-
To 

Re: freezer problems

[Rafael J. Wysocki]

On Wednesday, 21 February 2007 19:14, Paul E. McKenney wrote:
 On Tue, Feb 20, 2007 at 07:29:01PM +0100, Rafael J. Wysocki wrote:
  On Tuesday, 20 February 2007 01:32, Rafael J. Wysocki wrote:
   On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
   Hm.  In the case discussed above we have a task that's right before 
   calling
   frozen_process(), so we can't thaw it, because it's not frozen.  It will 
   be
   frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() have no
   way to check this.
   
   I think to close this race the refrigerator should check TIF_FREEZE and 
   set
   PF_FROZEN _and_ reset TIF_FREEZE under a lock that would also have to be
   taken by try_to_freeze_tasks() in the beginning of the error path.  This 
   will
   ensure that all tasks either freeze themselves before the error path in
   try_to_freeze_tasks() is executed, or remain unfrozen.
   
   I'll try to prepare a patch to illustrate this, but right now I'm too 
   tired to
   do it. :-)
  
  Something like this, perhaps:
  
  ---
   include/linux/freezer.h |   10 +++---
   kernel/power/process.c  |   18 --
   2 files changed, 19 insertions(+), 9 deletions(-)
  
  Index: linux-2.6.20-mm2/include/linux/freezer.h
  ===
  --- linux-2.6.20-mm2.orig/include/linux/freezer.h
  +++ linux-2.6.20-mm2/include/linux/freezer.h
  @@ -58,17 +58,13 @@ static inline void frozen_process(struct
  clear_tsk_thread_flag(p, TIF_FREEZE);
   }
  
  -extern void refrigerator(void);
  +extern int refrigerator(void);
   extern int freeze_processes(void);
   extern void thaw_processes(void);
  
   static inline int try_to_freeze(void)
   {
  -   if (freezing(current)) {
  -   refrigerator();
  -   return 1;
  -   } else
  -   return 0;
  +   return refrigerator();
   }
  
   /*
  @@ -104,7 +100,7 @@ static inline void freeze(struct task_st
   static inline int thaw_process(struct task_struct *p) { return 1; }
   static inline void frozen_process(struct task_struct *p) { BUG(); }
  
  -static inline void refrigerator(void) {}
  +static inline int refrigerator(void) { return 0; }
   static inline int freeze_processes(void) { BUG(); return 0; }
   static inline void thaw_processes(void) {}
  
  Index: linux-2.6.20-mm2/kernel/power/process.c
  ===
  --- linux-2.6.20-mm2.orig/kernel/power/process.c
  +++ linux-2.6.20-mm2/kernel/power/process.c
  @@ -24,6 +24,8 @@
   #define FREEZER_KERNEL_THREADS 0
   #define FREEZER_USER_SPACE 1
  
  +spinlock_t refrigerator_lock;
  +
   static inline int freezeable(struct task_struct * p)
   {
  if ((p == current) ||
  @@ -34,15 +36,23 @@ static inline int freezeable(struct task
   }
  
   /* Refrigerator is place where frozen processes are stored :-). */
  -void refrigerator(void)
  +int refrigerator(void)
   {
  /* Hmm, should we be allowed to suspend when there are realtime
 processes around? */
  long save;
  +
  +   spin_lock(refrigerator_lock);
 
 I hope we can do this without a global lock that is acquired on each
 try_to_freeze() call!

Yes.  Here's the current version (try_to_freeze() is unchanged, so the lock
is only taken by the tasks that are going to freeze, or so they think):

---
 kernel/power/process.c |   15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Index: linux-2.6.20-mm2/kernel/power/process.c
===
--- linux-2.6.20-mm2.orig/kernel/power/process.c
+++ linux-2.6.20-mm2/kernel/power/process.c
@@ -24,6 +24,8 @@
 #define FREEZER_KERNEL_THREADS 0
 #define FREEZER_USER_SPACE 1
 
+static spinlock_t refrigerator_lock;
+
 static inline int freezeable(struct task_struct * p)
 {
if ((p == current) ||
@@ -39,10 +41,18 @@ void refrigerator(void)
/* Hmm, should we be allowed to suspend when there are realtime
   processes around? */
long save;
+
+   spin_lock(refrigerator_lock);
+   if (freezing(current)) {
+   frozen_process(current);
+   spin_unlock(refrigerator_lock);
+   } else {
+   spin_unlock(refrigerator_lock);
+   return;
+   }
save = current-state;
pr_debug(%s entered refrigerator\n, current-comm);
 
-   frozen_process(current);
spin_lock_irq(current-sighand-siglock);
recalc_sigpending(); /* We sent fake signal, clean it up */
spin_unlock_irq(current-sighand-siglock);
@@ -143,6 +153,7 @@ static unsigned int try_to_freeze_tasks(
kernel threads,
TIMEOUT / HZ, todo);
read_lock(tasklist_lock);
+   spin_lock(refrigerator_lock);
do_each_thread(g, p) {
if (is_user_space(p) == !freeze_user_space)
continue;
@@ 

Re: freezer problems

[Paul E. McKenney]

On Wed, Feb 21, 2007 at 07:13:40PM +0100, Rafael J. Wysocki wrote:
 On Wednesday, 21 February 2007 19:14, Paul E. McKenney wrote:
  On Tue, Feb 20, 2007 at 07:29:01PM +0100, Rafael J. Wysocki wrote:
   On Tuesday, 20 February 2007 01:32, Rafael J. Wysocki wrote:
On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
Hm.  In the case discussed above we have a task that's right before 
calling
frozen_process(), so we can't thaw it, because it's not frozen.  It 
will be
frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() have 
no
way to check this.

I think to close this race the refrigerator should check TIF_FREEZE and 
set
PF_FROZEN _and_ reset TIF_FREEZE under a lock that would also have to be
taken by try_to_freeze_tasks() in the beginning of the error path.  
This will
ensure that all tasks either freeze themselves before the error path in
try_to_freeze_tasks() is executed, or remain unfrozen.

I'll try to prepare a patch to illustrate this, but right now I'm too 
tired to
do it. :-)
   
   Something like this, perhaps:
   
   ---
include/linux/freezer.h |   10 +++---
kernel/power/process.c  |   18 --
2 files changed, 19 insertions(+), 9 deletions(-)
   
   Index: linux-2.6.20-mm2/include/linux/freezer.h
   ===
   --- linux-2.6.20-mm2.orig/include/linux/freezer.h
   +++ linux-2.6.20-mm2/include/linux/freezer.h
   @@ -58,17 +58,13 @@ static inline void frozen_process(struct
 clear_tsk_thread_flag(p, TIF_FREEZE);
}
   
   -extern void refrigerator(void);
   +extern int refrigerator(void);
extern int freeze_processes(void);
extern void thaw_processes(void);
   
static inline int try_to_freeze(void)
{
   - if (freezing(current)) {
   - refrigerator();
   - return 1;
   - } else
   - return 0;
   + return refrigerator();
}
   
/*
   @@ -104,7 +100,7 @@ static inline void freeze(struct task_st
static inline int thaw_process(struct task_struct *p) { return 1; }
static inline void frozen_process(struct task_struct *p) { BUG(); }
   
   -static inline void refrigerator(void) {}
   +static inline int refrigerator(void) { return 0; }
static inline int freeze_processes(void) { BUG(); return 0; }
static inline void thaw_processes(void) {}
   
   Index: linux-2.6.20-mm2/kernel/power/process.c
   ===
   --- linux-2.6.20-mm2.orig/kernel/power/process.c
   +++ linux-2.6.20-mm2/kernel/power/process.c
   @@ -24,6 +24,8 @@
#define FREEZER_KERNEL_THREADS 0
#define FREEZER_USER_SPACE 1
   
   +spinlock_t refrigerator_lock;
   +
static inline int freezeable(struct task_struct * p)
{
 if ((p == current) ||
   @@ -34,15 +36,23 @@ static inline int freezeable(struct task
}
   
/* Refrigerator is place where frozen processes are stored :-). */
   -void refrigerator(void)
   +int refrigerator(void)
{
 /* Hmm, should we be allowed to suspend when there are realtime
processes around? */
 long save;
   +
   + spin_lock(refrigerator_lock);
  
  I hope we can do this without a global lock that is acquired on each
  try_to_freeze() call!
 
 Yes.  Here's the current version (try_to_freeze() is unchanged, so the lock
 is only taken by the tasks that are going to freeze, or so they think):

Ah, OK, that should work much better from a lock-contention viewpoint!

Thanx, Paul

 ---
  kernel/power/process.c |   15 ++-
  1 file changed, 14 insertions(+), 1 deletion(-)
 
 Index: linux-2.6.20-mm2/kernel/power/process.c
 ===
 --- linux-2.6.20-mm2.orig/kernel/power/process.c
 +++ linux-2.6.20-mm2/kernel/power/process.c
 @@ -24,6 +24,8 @@
  #define FREEZER_KERNEL_THREADS 0
  #define FREEZER_USER_SPACE 1
 
 +static spinlock_t refrigerator_lock;
 +
  static inline int freezeable(struct task_struct * p)
  {
   if ((p == current) ||
 @@ -39,10 +41,18 @@ void refrigerator(void)
   /* Hmm, should we be allowed to suspend when there are realtime
  processes around? */
   long save;
 +
 + spin_lock(refrigerator_lock);
 + if (freezing(current)) {
 + frozen_process(current);
 + spin_unlock(refrigerator_lock);
 + } else {
 + spin_unlock(refrigerator_lock);
 + return;
 + }
   save = current-state;
   pr_debug(%s entered refrigerator\n, current-comm);
 
 - frozen_process(current);
   spin_lock_irq(current-sighand-siglock);
   recalc_sigpending(); /* We sent fake signal, clean it up */
   spin_unlock_irq(current-sighand-siglock);
 @@ -143,6 +153,7 @@ static unsigned int try_to_freeze_tasks(
   kernel threads,
  

Re: freezer problems

[Oleg Nesterov]

On 02/21, Rafael J. Wysocki wrote:

 On Wednesday, 21 February 2007 19:14, Paul E. McKenney wrote:
  On Tue, Feb 20, 2007 at 07:29:01PM +0100, Rafael J. Wysocki wrote:
   On Tuesday, 20 February 2007 01:32, Rafael J. Wysocki wrote:
On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
Hm.  In the case discussed above we have a task that's right before 
calling
frozen_process(), so we can't thaw it, because it's not frozen.  It 
will be
frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() have 
no
way to check this.

I think to close this race the refrigerator should check TIF_FREEZE and 
set
PF_FROZEN _and_ reset TIF_FREEZE under a lock

I personally think this is good. Not only this allows us to close the race,
I think we can do more.

  that would also have to 
 be
taken by try_to_freeze_tasks() in the beginning of the error path.  
This will
ensure that all tasks either freeze themselves before the error path in
try_to_freeze_tasks() is executed, or remain unfrozen.

How about take this lock in thaw_tasks() instead/too ?

Currently we need a separate loop in thaw_tasks() to handle PF_FREEZER_SKIP. 
This
means that PF_FREEZER_SKIP is not so generic: thaw_tasks() can't tolerate if 
such
a task was woken in between. What if we change thaw_process() to clear 
TIF_FREEZE ?

Note also that we can use task_lock() instead of global refrigerator_lock. This
means that thaw_process() should take it too, probably this is slowdown, but I
think not too much because thaw_process() is going to write to p-flags anyway.
In this case thaw_process() works perfectly as cancel_freezing_and_thaw() and
can be used to fix exec/coredump in future.

Thoughts?

Oleg.

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Wednesday, 21 February 2007 21:03, Oleg Nesterov wrote:
 On 02/21, Rafael J. Wysocki wrote:
 
  On Wednesday, 21 February 2007 19:14, Paul E. McKenney wrote:
   On Tue, Feb 20, 2007 at 07:29:01PM +0100, Rafael J. Wysocki wrote:
On Tuesday, 20 February 2007 01:32, Rafael J. Wysocki wrote:
 On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
 Hm.  In the case discussed above we have a task that's right before 
 calling
 frozen_process(), so we can't thaw it, because it's not frozen.  It 
 will be
 frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() 
 have no
 way to check this.
 
 I think to close this race the refrigerator should check TIF_FREEZE 
 and set
 PF_FROZEN _and_ reset TIF_FREEZE under a lock
 
 I personally think this is good. Not only this allows us to close the race,
 I think we can do more.
 
   that would also have 
  to be
 taken by try_to_freeze_tasks() in the beginning of the error path.  
 This will
 ensure that all tasks either freeze themselves before the error path 
 in
 try_to_freeze_tasks() is executed, or remain unfrozen.
 
 How about take this lock in thaw_tasks() instead/too ?

Good point.  If we take it in thaw_tasks(), then the tasks that have
TIF_FREEZE set, but haven't entered the refrigerator yet, won't be able to
enter the refrigerator until thaw_tasks() releases the lock ...
 
 Currently we need a separate loop in thaw_tasks() to handle PF_FREEZER_SKIP. 
 This
 means that PF_FREEZER_SKIP is not so generic: thaw_tasks() can't tolerate if 
 such
 a task was woken in between. What if we change thaw_process() to clear 
 TIF_FREEZE ?

... and then we can drop the PF_FREEZER_SKIP-handling loop and change
thaw_process() to clear TIF_FREEZE.

 Note also that we can use task_lock() instead of global refrigerator_lock. 
 This
 means that thaw_process() should take it too, probably this is slowdown, but I
 think not too much because thaw_process() is going to write to p-flags 
 anyway.
 In this case thaw_process() works perfectly as cancel_freezing_and_thaw() and
 can be used to fix exec/coredump in future.

Hm, I think we can try doing this too.

I'll try to prepare a patch later today.

Greetings,
Rafael
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Paul E. McKenney]

On Wed, Feb 21, 2007 at 11:03:14PM +0300, Oleg Nesterov wrote:
 On 02/21, Rafael J. Wysocki wrote:
 
  On Wednesday, 21 February 2007 19:14, Paul E. McKenney wrote:
   On Tue, Feb 20, 2007 at 07:29:01PM +0100, Rafael J. Wysocki wrote:
On Tuesday, 20 February 2007 01:32, Rafael J. Wysocki wrote:
 On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
 Hm.  In the case discussed above we have a task that's right before 
 calling
 frozen_process(), so we can't thaw it, because it's not frozen.  It 
 will be
 frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() 
 have no
 way to check this.
 
 I think to close this race the refrigerator should check TIF_FREEZE 
 and set
 PF_FROZEN _and_ reset TIF_FREEZE under a lock
 
 I personally think this is good. Not only this allows us to close the race,
 I think we can do more.
 
   that would also have 
  to be
 taken by try_to_freeze_tasks() in the beginning of the error path.  
 This will
 ensure that all tasks either freeze themselves before the error path 
 in
 try_to_freeze_tasks() is executed, or remain unfrozen.
 
 How about take this lock in thaw_tasks() instead/too ?
 
 Currently we need a separate loop in thaw_tasks() to handle PF_FREEZER_SKIP. 
 This
 means that PF_FREEZER_SKIP is not so generic: thaw_tasks() can't tolerate if 
 such
 a task was woken in between. What if we change thaw_process() to clear 
 TIF_FREEZE ?
 
 Note also that we can use task_lock() instead of global refrigerator_lock. 
 This
 means that thaw_process() should take it too, probably this is slowdown, but I
 think not too much because thaw_process() is going to write to p-flags 
 anyway.
 In this case thaw_process() works perfectly as cancel_freezing_and_thaw() and
 can be used to fix exec/coredump in future.

This sounds much better than a a global lock to me!  ;-)

Thanx, Paul
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Wednesday, 21 February 2007 22:06, Paul E. McKenney wrote:
 On Wed, Feb 21, 2007 at 11:03:14PM +0300, Oleg Nesterov wrote:
  On 02/21, Rafael J. Wysocki wrote:
  
   On Wednesday, 21 February 2007 19:14, Paul E. McKenney wrote:
On Tue, Feb 20, 2007 at 07:29:01PM +0100, Rafael J. Wysocki wrote:
 On Tuesday, 20 February 2007 01:32, Rafael J. Wysocki wrote:
  On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
  Hm.  In the case discussed above we have a task that's right before 
  calling
  frozen_process(), so we can't thaw it, because it's not frozen.  It 
  will be
  frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() 
  have no
  way to check this.
  
  I think to close this race the refrigerator should check TIF_FREEZE 
  and set
  PF_FROZEN _and_ reset TIF_FREEZE under a lock
  
  I personally think this is good. Not only this allows us to close the race,
  I think we can do more.
  
that would also have 
   to be
  taken by try_to_freeze_tasks() in the beginning of the error path.  
  This will
  ensure that all tasks either freeze themselves before the error 
  path in
  try_to_freeze_tasks() is executed, or remain unfrozen.
  
  How about take this lock in thaw_tasks() instead/too ?
  
  Currently we need a separate loop in thaw_tasks() to handle 
  PF_FREEZER_SKIP. This
  means that PF_FREEZER_SKIP is not so generic: thaw_tasks() can't tolerate 
  if such
  a task was woken in between. What if we change thaw_process() to clear 
  TIF_FREEZE ?
  
  Note also that we can use task_lock() instead of global refrigerator_lock. 
  This
  means that thaw_process() should take it too, probably this is slowdown, 
  but I
  think not too much because thaw_process() is going to write to p-flags 
  anyway.
  In this case thaw_process() works perfectly as cancel_freezing_and_thaw() 
  and
  can be used to fix exec/coredump in future.
 
 This sounds much better than a a global lock to me!  ;-)

Okay, below is what I have right now (compilation tested on x86_64):

This patch fixes the vfork problem by adding the PF_FREEZER_SKIP flag that
can be used by tasks to tell the freezer not to count them as freezeable and
making the vfork parents set this flag before they call wait_for_completion().

Secondly, it fixes the race which happens it a task with TIF_FREEZE set is
preempted right before calling frozen_process() in refrigerator() and stays
unforzen until after thaw_tasks() runs and checks its status.  For this purpose
task_lock() is used.

 include/linux/freezer.h |   34 --
 include/linux/sched.h   |1 +
 kernel/fork.c   |3 +++
 kernel/power/process.c  |   36 +++-
 4 files changed, 55 insertions(+), 19 deletions(-)

Index: linux-2.6.20-mm2/include/linux/sched.h
===
--- linux-2.6.20-mm2.orig/include/linux/sched.h
+++ linux-2.6.20-mm2/include/linux/sched.h
@@ -1189,6 +1189,7 @@ static inline void put_task_struct(struc
 #define PF_SPREAD_SLAB 0x0200  /* Spread some slab caches over cpuset 
*/
 #define PF_MEMPOLICY   0x1000  /* Non-default NUMA mempolicy */
 #define PF_MUTEX_TESTER0x2000  /* Thread belongs to the rt 
mutex tester */
+#define PF_FREEZER_SKIP0x4000  /* Freezer should not count it 
as freezeable */
 
 /*
  * Only the _current_ task can read/write to tsk-flags, but other
Index: linux-2.6.20-mm2/include/linux/freezer.h
===
--- linux-2.6.20-mm2.orig/include/linux/freezer.h
+++ linux-2.6.20-mm2/include/linux/freezer.h
@@ -40,11 +40,15 @@ static inline void do_not_freeze(struct 
  */
 static inline int thaw_process(struct task_struct *p)
 {
+   task_lock(p);
if (frozen(p)) {
p-flags = ~PF_FROZEN;
+   task_unlock(p);
wake_up_process(p);
return 1;
}
+   clear_tsk_thread_flag(p, TIF_FREEZE);
+   task_unlock(p);
return 0;
 }
 
@@ -71,7 +75,31 @@ static inline int try_to_freeze(void)
return 0;
 }
 
-extern void thaw_some_processes(int all);
+/*
+ * Tell the freezer not to count current task as freezeable
+ */
+static inline void freezer_do_not_count(void)
+{
+   current-flags |= PF_FREEZER_SKIP;
+}
+
+/*
+ * Try to freeze the current task and tell the freezer to count it as 
freezeable
+ * again
+ */
+static inline void freezer_count(void)
+{
+   current-flags = ~PF_FREEZER_SKIP;
+   try_to_freeze();
+}
+
+/*
+ * Check if the task should be counted as freezeable by the freezer
+ */
+static inline int freezer_should_skip(struct task_struct *p)
+{
+   return !!(p-flags  PF_FREEZER_SKIP);
+}
 
 #else
 static inline int frozen(struct task_struct *p) { return 0; }
@@ -86,5 +114,7 @@ static 

Re: freezer problems

[Rafael J. Wysocki]

On Tuesday, 20 February 2007 01:32, Rafael J. Wysocki wrote:
> On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
> > On 02/20, Rafael J. Wysocki wrote:
> > >
> > > On Monday, 19 February 2007 23:41, Oleg Nesterov wrote:
> > > > On 02/19, Rafael J. Wysocki wrote:
> > > > >
> > > > > On Monday, 19 February 2007 21:23, Oleg Nesterov wrote:
> > > > > 
> > > > > > > @@ -199,6 +189,10 @@ static void thaw_tasks(int thaw_user_spa
> > > > > > >
> > > > > > > do_each_thread(g, p) {
> > > > > > > +   if (freezer_should_skip(p))
> > > > > > > +   cancel_freezing(p);
> > > > > > > +   } while_each_thread(g, p);
> > > > > > > +   do_each_thread(g, p) {
> > > > > > > if (!freezeable(p))
> > > > > > > continue;
> > > > > > 
> > > > > > Any reason for 2 separate do_each_thread() loops ?
> > > > > 
> > > > > Yes.  If there is a "freeze" request pending for the vfork parent 
> > > > > (TIF_FREEZE
> > > > > set), we have to cancel it before the child is unfrozen, since 
> > > > > otherwise the
> > > > > parent may go freezing after we try to reset PF_FROZEN for it.
> > > > 
> > > > I see, thanks... thaw_process() doesn't take TIF_FREEZE into account.
> > > > 
> > > > But doesn't this mean we have a race?
> > > > 
> > > > Suppose that try_to_freeze_tasks() failed. It does cancel_freezing() 
> > > > for each
> > > > process before return, but what if some thread already checked 
> > > > TIF_FREEZE and
> > > > (for simplicity) it is preempted before frozen_process() in 
> > > > refrigerator().
> > > > 
> > > > thaw_tasks() runs, ignores this task (P), returns. P gets CPU, and 
> > > > becomes
> > > > frozen, but nobody will thaw it.
> > > > 
> > > > No?
> > > 
> > > Well, I think this is highly theoretical.  Namely, try_to_freeze_tasks() 
> > > only
> > > fails after the timeout that's currently set to 20 sec., and it yields 
> > > the CPU
> > > in each iteration of the main loop.  The task in question would have to 
> > > refuse
> > > being frozen for 20 sec. and then suddenly decide to freeze itself right 
> > > before
> > > try_to_freeze_tasks() checks the timeout for the very last time.  Then, it
> > > would have to get preempted at this very moment and stay unfrozen at least
> > > until thaw_tasks() starts running and in fact even longer.
> > 
> > Yes, yes, it is pure theroretical,
> > 
> > > I think we may avoid this by making try_to_freeze_tasks() sleep for some 
> > > time
> > > after it has reset TIF_FREEZE for all tasks in the error path, if anyone 
> > > is
> > > ever able to trigger it.
> > 
> > This makes this race  (pure theroretical) ** 2  :)
> > 
> > Still. May be it make sense to introduce cancel_freezing_and_thaw() function
> > (not right now) which stops the task from sleeping in refrigirator reliably.
> 
> Hm.  In the case discussed above we have a task that's right before calling
> frozen_process(), so we can't thaw it, because it's not frozen.  It will be
> frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() have no
> way to check this.
> 
> I think to close this race the refrigerator should check TIF_FREEZE and set
> PF_FROZEN _and_ reset TIF_FREEZE under a lock that would also have to be
> taken by try_to_freeze_tasks() in the beginning of the error path.  This will
> ensure that all tasks either freeze themselves before the error path in
> try_to_freeze_tasks() is executed, or remain unfrozen.
> 
> I'll try to prepare a patch to illustrate this, but right now I'm too tired to
> do it. :-)

Something like this, perhaps:

---
 include/linux/freezer.h |   10 +++---
 kernel/power/process.c  |   18 --
 2 files changed, 19 insertions(+), 9 deletions(-)

Index: linux-2.6.20-mm2/include/linux/freezer.h
===
--- linux-2.6.20-mm2.orig/include/linux/freezer.h
+++ linux-2.6.20-mm2/include/linux/freezer.h
@@ -58,17 +58,13 @@ static inline void frozen_process(struct
clear_tsk_thread_flag(p, TIF_FREEZE);
 }
 
-extern void refrigerator(void);
+extern int refrigerator(void);
 extern int freeze_processes(void);
 extern void thaw_processes(void);
 
 static inline int try_to_freeze(void)
 {
-   if (freezing(current)) {
-   refrigerator();
-   return 1;
-   } else
-   return 0;
+   return refrigerator();
 }
 
 /*
@@ -104,7 +100,7 @@ static inline void freeze(struct task_st
 static inline int thaw_process(struct task_struct *p) { return 1; }
 static inline void frozen_process(struct task_struct *p) { BUG(); }
 
-static inline void refrigerator(void) {}
+static inline int refrigerator(void) { return 0; }
 static inline int freeze_processes(void) { BUG(); return 0; }
 static inline void thaw_processes(void) {}
 
Index: linux-2.6.20-mm2/kernel/power/process.c
===
--- linux-2.6.20-mm2.orig/kernel/power/process.c
+++ 

Re: freezer problems

[Rafael J. Wysocki]

On Tuesday, 20 February 2007 01:50, Oleg Nesterov wrote:
> On 02/20, Rafael J. Wysocki wrote:
> >
> > BTW, what do you think of the updated patch I sent two messages ago?
> 
> Ah, sorry, I just forgot... I think it is nice.

Thanks. :-)

I've started to collect the refrigerator-related patches posted recently
and I'm going to post them again soon as a series.

Greetings,
Rafael
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Tuesday, 20 February 2007 01:50, Oleg Nesterov wrote:
 On 02/20, Rafael J. Wysocki wrote:
 
  BTW, what do you think of the updated patch I sent two messages ago?
 
 Ah, sorry, I just forgot... I think it is nice.

Thanks. :-)

I've started to collect the refrigerator-related patches posted recently
and I'm going to post them again soon as a series.

Greetings,
Rafael
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Tuesday, 20 February 2007 01:32, Rafael J. Wysocki wrote:
 On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
  On 02/20, Rafael J. Wysocki wrote:
  
   On Monday, 19 February 2007 23:41, Oleg Nesterov wrote:
On 02/19, Rafael J. Wysocki wrote:

 On Monday, 19 February 2007 21:23, Oleg Nesterov wrote:
 
   @@ -199,6 +189,10 @@ static void thaw_tasks(int thaw_user_spa
  
   do_each_thread(g, p) {
   +   if (freezer_should_skip(p))
   +   cancel_freezing(p);
   +   } while_each_thread(g, p);
   +   do_each_thread(g, p) {
   if (!freezeable(p))
   continue;
  
  Any reason for 2 separate do_each_thread() loops ?
 
 Yes.  If there is a freeze request pending for the vfork parent 
 (TIF_FREEZE
 set), we have to cancel it before the child is unfrozen, since 
 otherwise the
 parent may go freezing after we try to reset PF_FROZEN for it.

I see, thanks... thaw_process() doesn't take TIF_FREEZE into account.

But doesn't this mean we have a race?

Suppose that try_to_freeze_tasks() failed. It does cancel_freezing() 
for each
process before return, but what if some thread already checked 
TIF_FREEZE and
(for simplicity) it is preempted before frozen_process() in 
refrigerator().

thaw_tasks() runs, ignores this task (P), returns. P gets CPU, and 
becomes
frozen, but nobody will thaw it.

No?
   
   Well, I think this is highly theoretical.  Namely, try_to_freeze_tasks() 
   only
   fails after the timeout that's currently set to 20 sec., and it yields 
   the CPU
   in each iteration of the main loop.  The task in question would have to 
   refuse
   being frozen for 20 sec. and then suddenly decide to freeze itself right 
   before
   try_to_freeze_tasks() checks the timeout for the very last time.  Then, it
   would have to get preempted at this very moment and stay unfrozen at least
   until thaw_tasks() starts running and in fact even longer.
  
  Yes, yes, it is pure theroretical,
  
   I think we may avoid this by making try_to_freeze_tasks() sleep for some 
   time
   after it has reset TIF_FREEZE for all tasks in the error path, if anyone 
   is
   ever able to trigger it.
  
  This makes this race  (pure theroretical) ** 2  :)
  
  Still. May be it make sense to introduce cancel_freezing_and_thaw() function
  (not right now) which stops the task from sleeping in refrigirator reliably.
 
 Hm.  In the case discussed above we have a task that's right before calling
 frozen_process(), so we can't thaw it, because it's not frozen.  It will be
 frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() have no
 way to check this.
 
 I think to close this race the refrigerator should check TIF_FREEZE and set
 PF_FROZEN _and_ reset TIF_FREEZE under a lock that would also have to be
 taken by try_to_freeze_tasks() in the beginning of the error path.  This will
 ensure that all tasks either freeze themselves before the error path in
 try_to_freeze_tasks() is executed, or remain unfrozen.
 
 I'll try to prepare a patch to illustrate this, but right now I'm too tired to
 do it. :-)

Something like this, perhaps:

---
 include/linux/freezer.h |   10 +++---
 kernel/power/process.c  |   18 --
 2 files changed, 19 insertions(+), 9 deletions(-)

Index: linux-2.6.20-mm2/include/linux/freezer.h
===
--- linux-2.6.20-mm2.orig/include/linux/freezer.h
+++ linux-2.6.20-mm2/include/linux/freezer.h
@@ -58,17 +58,13 @@ static inline void frozen_process(struct
clear_tsk_thread_flag(p, TIF_FREEZE);
 }
 
-extern void refrigerator(void);
+extern int refrigerator(void);
 extern int freeze_processes(void);
 extern void thaw_processes(void);
 
 static inline int try_to_freeze(void)
 {
-   if (freezing(current)) {
-   refrigerator();
-   return 1;
-   } else
-   return 0;
+   return refrigerator();
 }
 
 /*
@@ -104,7 +100,7 @@ static inline void freeze(struct task_st
 static inline int thaw_process(struct task_struct *p) { return 1; }
 static inline void frozen_process(struct task_struct *p) { BUG(); }
 
-static inline void refrigerator(void) {}
+static inline int refrigerator(void) { return 0; }
 static inline int freeze_processes(void) { BUG(); return 0; }
 static inline void thaw_processes(void) {}
 
Index: linux-2.6.20-mm2/kernel/power/process.c
===
--- linux-2.6.20-mm2.orig/kernel/power/process.c
+++ linux-2.6.20-mm2/kernel/power/process.c
@@ -24,6 +24,8 @@
 #define FREEZER_KERNEL_THREADS 0
 #define FREEZER_USER_SPACE 1
 
+spinlock_t refrigerator_lock;
+
 static inline int freezeable(struct task_struct * p)
 {
if ((p == current) ||
@@ -34,15 +36,23 @@ static inline int 

Re: freezer problems

[Oleg Nesterov]

On 02/20, Rafael J. Wysocki wrote:
>
> BTW, what do you think of the updated patch I sent two messages ago?

Ah, sorry, I just forgot... I think it is nice.

Oleg.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
> On 02/20, Rafael J. Wysocki wrote:
> >
> > On Monday, 19 February 2007 23:41, Oleg Nesterov wrote:
> > > On 02/19, Rafael J. Wysocki wrote:
> > > >
> > > > On Monday, 19 February 2007 21:23, Oleg Nesterov wrote:
> > > > 
> > > > > > @@ -199,6 +189,10 @@ static void thaw_tasks(int thaw_user_spa
> > > > > >
> > > > > > do_each_thread(g, p) {
> > > > > > +   if (freezer_should_skip(p))
> > > > > > +   cancel_freezing(p);
> > > > > > +   } while_each_thread(g, p);
> > > > > > +   do_each_thread(g, p) {
> > > > > > if (!freezeable(p))
> > > > > > continue;
> > > > > 
> > > > > Any reason for 2 separate do_each_thread() loops ?
> > > > 
> > > > Yes.  If there is a "freeze" request pending for the vfork parent 
> > > > (TIF_FREEZE
> > > > set), we have to cancel it before the child is unfrozen, since 
> > > > otherwise the
> > > > parent may go freezing after we try to reset PF_FROZEN for it.
> > > 
> > > I see, thanks... thaw_process() doesn't take TIF_FREEZE into account.
> > > 
> > > But doesn't this mean we have a race?
> > > 
> > > Suppose that try_to_freeze_tasks() failed. It does cancel_freezing() for 
> > > each
> > > process before return, but what if some thread already checked TIF_FREEZE 
> > > and
> > > (for simplicity) it is preempted before frozen_process() in 
> > > refrigerator().
> > > 
> > > thaw_tasks() runs, ignores this task (P), returns. P gets CPU, and becomes
> > > frozen, but nobody will thaw it.
> > > 
> > > No?
> > 
> > Well, I think this is highly theoretical.  Namely, try_to_freeze_tasks() 
> > only
> > fails after the timeout that's currently set to 20 sec., and it yields the 
> > CPU
> > in each iteration of the main loop.  The task in question would have to 
> > refuse
> > being frozen for 20 sec. and then suddenly decide to freeze itself right 
> > before
> > try_to_freeze_tasks() checks the timeout for the very last time.  Then, it
> > would have to get preempted at this very moment and stay unfrozen at least
> > until thaw_tasks() starts running and in fact even longer.
> 
> Yes, yes, it is pure theroretical,
> 
> > I think we may avoid this by making try_to_freeze_tasks() sleep for some 
> > time
> > after it has reset TIF_FREEZE for all tasks in the error path, if anyone is
> > ever able to trigger it.
> 
> This makes this race  (pure theroretical) ** 2  :)
> 
> Still. May be it make sense to introduce cancel_freezing_and_thaw() function
> (not right now) which stops the task from sleeping in refrigirator reliably.

Hm.  In the case discussed above we have a task that's right before calling
frozen_process(), so we can't thaw it, because it's not frozen.  It will be
frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() have no
way to check this.

I think to close this race the refrigerator should check TIF_FREEZE and set
PF_FROZEN _and_ reset TIF_FREEZE under a lock that would also have to be
taken by try_to_freeze_tasks() in the beginning of the error path.  This will
ensure that all tasks either freeze themselves before the error path in
try_to_freeze_tasks() is executed, or remain unfrozen.

I'll try to prepare a patch to illustrate this, but right now I'm too tired to
do it. :-)

> I didn't think much about this, but it looks like we can fix coredump/exec
> problems. Of course, this is not so important, we can ignore them at least
> for now (->vfork_done is different, should be imho solved, because any user
> can block freezer forever).
> 
> The fix:
> 
>   refrigerator:
> 
>   +   // we are going to call do_exit() really soon,
>   +   // we have a pending SIGKILL
>   +   if (current->signal->flags & SIGNAL_GROUP_EXIT)
>   +   return;
> 
>   frozen_process(current);
>   ...
> 
> 
>   zap_other_threads:
> 
>   for_each_subthread() {
>   ...
> 
>   +   //  SIGNAL_GROUP_EXIT is set --
>   +   // we can check sig->group_exit_task to detect 
> de_thread,
>   +   // but perhaps it doesn't hurt if the caller is 
> do_group_exit
>   +   cancel_freezing_and_thaw(p);
>   sigaddset(>pending.signal, SIGKILL);
>   signal_wake_up(t, 1);
>   }
> 
> This way execer reliably kills all sub-threads and proceeds without blocking
> try_to_freeze_tasks(). The same change could be done for zap_process() to fix
> coredump.

Yes, at first sight it looks good.

BTW, what do you think of the updated patch I sent two messages ago?

Rafael
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Oleg Nesterov]

On 02/20, Rafael J. Wysocki wrote:
>
> On Monday, 19 February 2007 23:41, Oleg Nesterov wrote:
> > On 02/19, Rafael J. Wysocki wrote:
> > >
> > > On Monday, 19 February 2007 21:23, Oleg Nesterov wrote:
> > > 
> > > > > @@ -199,6 +189,10 @@ static void thaw_tasks(int thaw_user_spa
> > > > >
> > > > > do_each_thread(g, p) {
> > > > > +   if (freezer_should_skip(p))
> > > > > +   cancel_freezing(p);
> > > > > +   } while_each_thread(g, p);
> > > > > +   do_each_thread(g, p) {
> > > > > if (!freezeable(p))
> > > > > continue;
> > > > 
> > > > Any reason for 2 separate do_each_thread() loops ?
> > > 
> > > Yes.  If there is a "freeze" request pending for the vfork parent 
> > > (TIF_FREEZE
> > > set), we have to cancel it before the child is unfrozen, since otherwise 
> > > the
> > > parent may go freezing after we try to reset PF_FROZEN for it.
> > 
> > I see, thanks... thaw_process() doesn't take TIF_FREEZE into account.
> > 
> > But doesn't this mean we have a race?
> > 
> > Suppose that try_to_freeze_tasks() failed. It does cancel_freezing() for 
> > each
> > process before return, but what if some thread already checked TIF_FREEZE 
> > and
> > (for simplicity) it is preempted before frozen_process() in refrigerator().
> > 
> > thaw_tasks() runs, ignores this task (P), returns. P gets CPU, and becomes
> > frozen, but nobody will thaw it.
> > 
> > No?
> 
> Well, I think this is highly theoretical.  Namely, try_to_freeze_tasks() only
> fails after the timeout that's currently set to 20 sec., and it yields the CPU
> in each iteration of the main loop.  The task in question would have to refuse
> being frozen for 20 sec. and then suddenly decide to freeze itself right 
> before
> try_to_freeze_tasks() checks the timeout for the very last time.  Then, it
> would have to get preempted at this very moment and stay unfrozen at least
> until thaw_tasks() starts running and in fact even longer.

Yes, yes, it is pure theroretical,

> I think we may avoid this by making try_to_freeze_tasks() sleep for some time
> after it has reset TIF_FREEZE for all tasks in the error path, if anyone is
> ever able to trigger it.

This makes this race  (pure theroretical) ** 2  :)

Still. May be it make sense to introduce cancel_freezing_and_thaw() function
(not right now) which stops the task from sleeping in refrigirator reliably.
I didn't think much about this, but it looks like we can fix coredump/exec
problems. Of course, this is not so important, we can ignore them at least
for now (->vfork_done is different, should be imho solved, because any user
can block freezer forever).

The fix:

refrigerator:

+   // we are going to call do_exit() really soon,
+   // we have a pending SIGKILL
+   if (current->signal->flags & SIGNAL_GROUP_EXIT)
+   return;

frozen_process(current);
...


zap_other_threads:

for_each_subthread() {
...

+   //  SIGNAL_GROUP_EXIT is set --
+   // we can check sig->group_exit_task to detect 
de_thread,
+   // but perhaps it doesn't hurt if the caller is 
do_group_exit
+   cancel_freezing_and_thaw(p);
sigaddset(>pending.signal, SIGKILL);
signal_wake_up(t, 1);
}

This way execer reliably kills all sub-threads and proceeds without blocking
try_to_freeze_tasks(). The same change could be done for zap_process() to fix
coredump.

Oleg.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Monday, 19 February 2007 23:41, Oleg Nesterov wrote:
> On 02/19, Rafael J. Wysocki wrote:
> >
> > On Monday, 19 February 2007 21:23, Oleg Nesterov wrote:
> > 
> > > > @@ -199,6 +189,10 @@ static void thaw_tasks(int thaw_user_spa
> > > >
> > > > do_each_thread(g, p) {
> > > > +   if (freezer_should_skip(p))
> > > > +   cancel_freezing(p);
> > > > +   } while_each_thread(g, p);
> > > > +   do_each_thread(g, p) {
> > > > if (!freezeable(p))
> > > > continue;
> > > 
> > > Any reason for 2 separate do_each_thread() loops ?
> > 
> > Yes.  If there is a "freeze" request pending for the vfork parent 
> > (TIF_FREEZE
> > set), we have to cancel it before the child is unfrozen, since otherwise the
> > parent may go freezing after we try to reset PF_FROZEN for it.
> 
> I see, thanks... thaw_process() doesn't take TIF_FREEZE into account.
> 
> But doesn't this mean we have a race?
> 
> Suppose that try_to_freeze_tasks() failed. It does cancel_freezing() for each
> process before return, but what if some thread already checked TIF_FREEZE and
> (for simplicity) it is preempted before frozen_process() in refrigerator().
> 
> thaw_tasks() runs, ignores this task (P), returns. P gets CPU, and becomes
> frozen, but nobody will thaw it.
> 
> No?

Well, I think this is highly theoretical.  Namely, try_to_freeze_tasks() only
fails after the timeout that's currently set to 20 sec., and it yields the CPU
in each iteration of the main loop.  The task in question would have to refuse
being frozen for 20 sec. and then suddenly decide to freeze itself right before
try_to_freeze_tasks() checks the timeout for the very last time.  Then, it
would have to get preempted at this very moment and stay unfrozen at least
until thaw_tasks() starts running and in fact even longer.

I think we may avoid this by making try_to_freeze_tasks() sleep for some time
after it has reset TIF_FREEZE for all tasks in the error path, if anyone is
ever able to trigger it.

Greetings,
Rafael
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Oleg Nesterov]

On 02/19, Rafael J. Wysocki wrote:
>
> On Monday, 19 February 2007 21:23, Oleg Nesterov wrote:
> 
> > > @@ -199,6 +189,10 @@ static void thaw_tasks(int thaw_user_spa
> > >
> > > do_each_thread(g, p) {
> > > +   if (freezer_should_skip(p))
> > > +   cancel_freezing(p);
> > > +   } while_each_thread(g, p);
> > > +   do_each_thread(g, p) {
> > > if (!freezeable(p))
> > > continue;
> > 
> > Any reason for 2 separate do_each_thread() loops ?
> 
> Yes.  If there is a "freeze" request pending for the vfork parent (TIF_FREEZE
> set), we have to cancel it before the child is unfrozen, since otherwise the
> parent may go freezing after we try to reset PF_FROZEN for it.

I see, thanks... thaw_process() doesn't take TIF_FREEZE into account.

But doesn't this mean we have a race?

Suppose that try_to_freeze_tasks() failed. It does cancel_freezing() for each
process before return, but what if some thread already checked TIF_FREEZE and
(for simplicity) it is preempted before frozen_process() in refrigerator().

thaw_tasks() runs, ignores this task (P), returns. P gets CPU, and becomes
frozen, but nobody will thaw it.

No?

Oleg.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Monday, 19 February 2007 21:23, Oleg Nesterov wrote:
> On 02/19, Rafael J. Wysocki wrote:
> >
> > On Sunday, 18 February 2007 23:01, Oleg Nesterov wrote:
> > > > --- linux-2.6.20-mm2.orig/include/asm-i386/thread_info.h
> > > > 2007-02-18 19:49:34.0 +0100
> > > > +++ linux-2.6.20-mm2/include/asm-i386/thread_info.h 2007-02-18 
> > > > 19:50:37.0 +0100
> > > > @@ -135,6 +135,7 @@ static inline struct thread_info *curren
> > > >  #define TIF_IO_BITMAP  18  /* uses I/O bitmap */
> > > >  #define TIF_FREEZE 19  /* is freezing for suspend */
> > > >  #define TIF_FORCED_TF  20  /* true if TF in eflags 
> > > > artificially */
> > > > +#define TIF_FREEZER_SKIP   21  /* task freezer should not 
> > > > count us */
> > > 
> > > Do we need to put this flag into thread_info? It is always modified by
> > > "current", so it could live in task_struct->flags instead.
> > 
> > I thought we were running low on the task_struct->flags bits. :-)
> 
> Didn't think about that :)

Seriously, I'm not sure.  There are 23 PF_* flags already defined, while
for example on x86_64 there are 17 TIF_* flags defined which is not that
much better.

> > Apart from this, we may need to set it from somewhere else in the future.
> 
> I doubt. In any case, since you provided the nice helpers, it would be very
> easy to convert from thread to process flags. My main concern is that we
> have 24 include/asm-*/thread_info.h files, but only 1 include/linux/sched.h.
> It seems more easy to start with PF_ flags at first.

OK

> > @@ -1393,7 +1394,10 @@ long do_fork(unsigned long clone_flags,
> >
> > if (clone_flags & CLONE_VFORK) {
> > +   freezer_do_not_count(current);
> >   wait_for_completion();
> > +   try_to_freeze();
> > +   freezer_count(current);
> 
> freezer_do_not_count() implies that we must do try_to_freeze() later, I'd
> suggest to shift try_to_freeze() into freezer_count(). Actually, I think that
> freezer_do_not_count/freezer_count should be "(void)", like try_to_freeze().
> IOW,
> 
>   freezer_do_not_count()
>   ... sleep in 'D' state ...
>   freezer_count()
> 
> means that current doesn't hold any "important" locks, may be considered as
> frozen, it can do nothing except enter refrigerator if it gets CPU.
> 
> (Please feel free to ignore, this is a matter of taste of course).

Well, if we use a PF_* flag for that, it's also a matter of correctness (only
current should be able to set its flags).

> > @@ -199,6 +189,10 @@ static void thaw_tasks(int thaw_user_spa
> >
> > read_lock(_lock);
> > do_each_thread(g, p) {
> > +   if (freezer_should_skip(p))
> > +   cancel_freezing(p);
> > +   } while_each_thread(g, p);
> > +   do_each_thread(g, p) {
> > if (!freezeable(p))
> > continue;
> 
> Any reason for 2 separate do_each_thread() loops ?

Yes.  If there is a "freeze" request pending for the vfork parent (TIF_FREEZE
set), we have to cancel it before the child is unfrozen, since otherwise the
parent may go freezing after we try to reset PF_FROZEN for it.

> I think this patch is correct, but I still can't convince myself I really
> understand freezer :)

Oh, that takes time.  It took me a year or so. ;-)

Here's the updated patch.  It hasn't been tested yet, but at least it compiles
(on x86_64).

 include/linux/sched.h   |1 +
 include/linux/freezer.h |   30 --
 kernel/fork.c   |3 +++
 kernel/power/process.c  |   27 ---
 4 files changed, 44 insertions(+), 17 deletions(-)

Index: linux-2.6.20-mm2/include/linux/sched.h
===
--- linux-2.6.20-mm2.orig/include/linux/sched.h
+++ linux-2.6.20-mm2/include/linux/sched.h
@@ -1189,6 +1189,7 @@ static inline void put_task_struct(struc
 #define PF_SPREAD_SLAB 0x0200  /* Spread some slab caches over cpuset 
*/
 #define PF_MEMPOLICY   0x1000  /* Non-default NUMA mempolicy */
 #define PF_MUTEX_TESTER0x2000  /* Thread belongs to the rt 
mutex tester */
+#define PF_FREEZER_SKIP0x4000  /* Freezer should not count it 
as freezeable */
 
 /*
  * Only the _current_ task can read/write to tsk->flags, but other
Index: linux-2.6.20-mm2/include/linux/freezer.h
===
--- linux-2.6.20-mm2.orig/include/linux/freezer.h
+++ linux-2.6.20-mm2/include/linux/freezer.h
@@ -71,7 +71,31 @@ static inline int try_to_freeze(void)
return 0;
 }
 
-extern void thaw_some_processes(int all);
+/*
+ * Tell the freezer not to count current task as freezeable
+ */
+static inline void freezer_do_not_count(void)
+{
+   current->flags |= PF_FREEZER_SKIP;
+}
+
+/*
+ * Try to freeze the current task and tell the 

Re: freezer problems

[Oleg Nesterov]

On 02/19, Rafael J. Wysocki wrote:
>
> On Sunday, 18 February 2007 23:01, Oleg Nesterov wrote:
> > > --- linux-2.6.20-mm2.orig/include/asm-i386/thread_info.h  2007-02-18 
> > > 19:49:34.0 +0100
> > > +++ linux-2.6.20-mm2/include/asm-i386/thread_info.h   2007-02-18 
> > > 19:50:37.0 +0100
> > > @@ -135,6 +135,7 @@ static inline struct thread_info *curren
> > >  #define TIF_IO_BITMAP18  /* uses I/O bitmap */
> > >  #define TIF_FREEZE   19  /* is freezing for suspend */
> > >  #define TIF_FORCED_TF20  /* true if TF in eflags 
> > > artificially */
> > > +#define TIF_FREEZER_SKIP 21  /* task freezer should not count us */
> > 
> > Do we need to put this flag into thread_info? It is always modified by
> > "current", so it could live in task_struct->flags instead.
> 
> I thought we were running low on the task_struct->flags bits. :-)

Didn't think about that :)

> Apart from this, we may need to set it from somewhere else in the future.

I doubt. In any case, since you provided the nice helpers, it would be very
easy to convert from thread to process flags. My main concern is that we
have 24 include/asm-*/thread_info.h files, but only 1 include/linux/sched.h.
It seems more easy to start with PF_ flags at first.

> @@ -1393,7 +1394,10 @@ long do_fork(unsigned long clone_flags,
>
>   if (clone_flags & CLONE_VFORK) {
> +   freezer_do_not_count(current);
> wait_for_completion();
> +   try_to_freeze();
> +   freezer_count(current);

freezer_do_not_count() implies that we must do try_to_freeze() later, I'd
suggest to shift try_to_freeze() into freezer_count(). Actually, I think that
freezer_do_not_count/freezer_count should be "(void)", like try_to_freeze().
IOW,

freezer_do_not_count()
... sleep in 'D' state ...
freezer_count()

means that current doesn't hold any "important" locks, may be considered as
frozen, it can do nothing except enter refrigerator if it gets CPU.

(Please feel free to ignore, this is a matter of taste of course).

> @@ -199,6 +189,10 @@ static void thaw_tasks(int thaw_user_spa
>
> read_lock(_lock);
> do_each_thread(g, p) {
> +   if (freezer_should_skip(p))
> +   cancel_freezing(p);
> +   } while_each_thread(g, p);
> +   do_each_thread(g, p) {
> if (!freezeable(p))
> continue;

Any reason for 2 separate do_each_thread() loops ?

I think this patch is correct, but I still can't convince myself I really
understand freezer :)

Btw,

On 02/18, Some Idiot wrote:
>
> On 02/18, Rafael J. Wysocki wrote:
> >
> > +   /* The parent is uninterruptible and will stay so until this task exits,
> > +* so we can mark it as frozen.
> > +*/
> > +   if (current->vfork_done)
> > +   frozen_process(current->parent);
>
> This is not safe. task->flags is not atomic, we can change ->flags only
> if we know the task won't touch it itself (ptrace, thaw_process).
> The parent could be interrupted, irq may play with current->flags (slab,
> for example).

Irq only does atomic allocations, so the slab won't play with task->flags.
Still I believe the concern was valid in general and I personally think
the new patch is better (and more generic).

Oleg.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Oleg Nesterov]

On 02/19, Rafael J. Wysocki wrote:

 On Sunday, 18 February 2007 23:01, Oleg Nesterov wrote:
   --- linux-2.6.20-mm2.orig/include/asm-i386/thread_info.h  2007-02-18 
   19:49:34.0 +0100
   +++ linux-2.6.20-mm2/include/asm-i386/thread_info.h   2007-02-18 
   19:50:37.0 +0100
   @@ -135,6 +135,7 @@ static inline struct thread_info *curren
#define TIF_IO_BITMAP18  /* uses I/O bitmap */
#define TIF_FREEZE   19  /* is freezing for suspend */
#define TIF_FORCED_TF20  /* true if TF in eflags 
   artificially */
   +#define TIF_FREEZER_SKIP 21  /* task freezer should not count us */
  
  Do we need to put this flag into thread_info? It is always modified by
  current, so it could live in task_struct-flags instead.
 
 I thought we were running low on the task_struct-flags bits. :-)

Didn't think about that :)

 Apart from this, we may need to set it from somewhere else in the future.

I doubt. In any case, since you provided the nice helpers, it would be very
easy to convert from thread to process flags. My main concern is that we
have 24 include/asm-*/thread_info.h files, but only 1 include/linux/sched.h.
It seems more easy to start with PF_ flags at first.

 @@ -1393,7 +1394,10 @@ long do_fork(unsigned long clone_flags,

   if (clone_flags  CLONE_VFORK) {
 +   freezer_do_not_count(current);
 wait_for_completion(vfork);
 +   try_to_freeze();
 +   freezer_count(current);

freezer_do_not_count() implies that we must do try_to_freeze() later, I'd
suggest to shift try_to_freeze() into freezer_count(). Actually, I think that
freezer_do_not_count/freezer_count should be (void), like try_to_freeze().
IOW,

freezer_do_not_count()
... sleep in 'D' state ...
freezer_count()

means that current doesn't hold any important locks, may be considered as
frozen, it can do nothing except enter refrigerator if it gets CPU.

(Please feel free to ignore, this is a matter of taste of course).

 @@ -199,6 +189,10 @@ static void thaw_tasks(int thaw_user_spa

 read_lock(tasklist_lock);
 do_each_thread(g, p) {
 +   if (freezer_should_skip(p))
 +   cancel_freezing(p);
 +   } while_each_thread(g, p);
 +   do_each_thread(g, p) {
 if (!freezeable(p))
 continue;

Any reason for 2 separate do_each_thread() loops ?

I think this patch is correct, but I still can't convince myself I really
understand freezer :)

Btw,

On 02/18, Some Idiot wrote:

 On 02/18, Rafael J. Wysocki wrote:
 
  +   /* The parent is uninterruptible and will stay so until this task exits,
  +* so we can mark it as frozen.
  +*/
  +   if (current-vfork_done)
  +   frozen_process(current-parent);

 This is not safe. task-flags is not atomic, we can change -flags only
 if we know the task won't touch it itself (ptrace, thaw_process).
 The parent could be interrupted, irq may play with current-flags (slab,
 for example).

Irq only does atomic allocations, so the slab won't play with task-flags.
Still I believe the concern was valid in general and I personally think
the new patch is better (and more generic).

Oleg.

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Monday, 19 February 2007 21:23, Oleg Nesterov wrote:
 On 02/19, Rafael J. Wysocki wrote:
 
  On Sunday, 18 February 2007 23:01, Oleg Nesterov wrote:
--- linux-2.6.20-mm2.orig/include/asm-i386/thread_info.h
2007-02-18 19:49:34.0 +0100
+++ linux-2.6.20-mm2/include/asm-i386/thread_info.h 2007-02-18 
19:50:37.0 +0100
@@ -135,6 +135,7 @@ static inline struct thread_info *curren
 #define TIF_IO_BITMAP  18  /* uses I/O bitmap */
 #define TIF_FREEZE 19  /* is freezing for suspend */
 #define TIF_FORCED_TF  20  /* true if TF in eflags 
artificially */
+#define TIF_FREEZER_SKIP   21  /* task freezer should not 
count us */
   
   Do we need to put this flag into thread_info? It is always modified by
   current, so it could live in task_struct-flags instead.
  
  I thought we were running low on the task_struct-flags bits. :-)
 
 Didn't think about that :)

Seriously, I'm not sure.  There are 23 PF_* flags already defined, while
for example on x86_64 there are 17 TIF_* flags defined which is not that
much better.

  Apart from this, we may need to set it from somewhere else in the future.
 
 I doubt. In any case, since you provided the nice helpers, it would be very
 easy to convert from thread to process flags. My main concern is that we
 have 24 include/asm-*/thread_info.h files, but only 1 include/linux/sched.h.
 It seems more easy to start with PF_ flags at first.

OK

  @@ -1393,7 +1394,10 @@ long do_fork(unsigned long clone_flags,
 
  if (clone_flags  CLONE_VFORK) {
  +   freezer_do_not_count(current);
wait_for_completion(vfork);
  +   try_to_freeze();
  +   freezer_count(current);
 
 freezer_do_not_count() implies that we must do try_to_freeze() later, I'd
 suggest to shift try_to_freeze() into freezer_count(). Actually, I think that
 freezer_do_not_count/freezer_count should be (void), like try_to_freeze().
 IOW,
 
   freezer_do_not_count()
   ... sleep in 'D' state ...
   freezer_count()
 
 means that current doesn't hold any important locks, may be considered as
 frozen, it can do nothing except enter refrigerator if it gets CPU.
 
 (Please feel free to ignore, this is a matter of taste of course).

Well, if we use a PF_* flag for that, it's also a matter of correctness (only
current should be able to set its flags).

  @@ -199,6 +189,10 @@ static void thaw_tasks(int thaw_user_spa
 
  read_lock(tasklist_lock);
  do_each_thread(g, p) {
  +   if (freezer_should_skip(p))
  +   cancel_freezing(p);
  +   } while_each_thread(g, p);
  +   do_each_thread(g, p) {
  if (!freezeable(p))
  continue;
 
 Any reason for 2 separate do_each_thread() loops ?

Yes.  If there is a freeze request pending for the vfork parent (TIF_FREEZE
set), we have to cancel it before the child is unfrozen, since otherwise the
parent may go freezing after we try to reset PF_FROZEN for it.

 I think this patch is correct, but I still can't convince myself I really
 understand freezer :)

Oh, that takes time.  It took me a year or so. ;-)

Here's the updated patch.  It hasn't been tested yet, but at least it compiles
(on x86_64).

 include/linux/sched.h   |1 +
 include/linux/freezer.h |   30 --
 kernel/fork.c   |3 +++
 kernel/power/process.c  |   27 ---
 4 files changed, 44 insertions(+), 17 deletions(-)

Index: linux-2.6.20-mm2/include/linux/sched.h
===
--- linux-2.6.20-mm2.orig/include/linux/sched.h
+++ linux-2.6.20-mm2/include/linux/sched.h
@@ -1189,6 +1189,7 @@ static inline void put_task_struct(struc
 #define PF_SPREAD_SLAB 0x0200  /* Spread some slab caches over cpuset 
*/
 #define PF_MEMPOLICY   0x1000  /* Non-default NUMA mempolicy */
 #define PF_MUTEX_TESTER0x2000  /* Thread belongs to the rt 
mutex tester */
+#define PF_FREEZER_SKIP0x4000  /* Freezer should not count it 
as freezeable */
 
 /*
  * Only the _current_ task can read/write to tsk-flags, but other
Index: linux-2.6.20-mm2/include/linux/freezer.h
===
--- linux-2.6.20-mm2.orig/include/linux/freezer.h
+++ linux-2.6.20-mm2/include/linux/freezer.h
@@ -71,7 +71,31 @@ static inline int try_to_freeze(void)
return 0;
 }
 
-extern void thaw_some_processes(int all);
+/*
+ * Tell the freezer not to count current task as freezeable
+ */
+static inline void freezer_do_not_count(void)
+{
+   current-flags |= PF_FREEZER_SKIP;
+}
+
+/*
+ * Try to freeze the current task and tell the freezer to count it as 
freezeable
+ * again
+ */
+static inline void freezer_count(void)
+{
+   try_to_freeze();
+   

Re: freezer problems

[Oleg Nesterov]

On 02/19, Rafael J. Wysocki wrote:

 On Monday, 19 February 2007 21:23, Oleg Nesterov wrote:
 
   @@ -199,6 +189,10 @@ static void thaw_tasks(int thaw_user_spa
  
   do_each_thread(g, p) {
   +   if (freezer_should_skip(p))
   +   cancel_freezing(p);
   +   } while_each_thread(g, p);
   +   do_each_thread(g, p) {
   if (!freezeable(p))
   continue;
  
  Any reason for 2 separate do_each_thread() loops ?
 
 Yes.  If there is a freeze request pending for the vfork parent (TIF_FREEZE
 set), we have to cancel it before the child is unfrozen, since otherwise the
 parent may go freezing after we try to reset PF_FROZEN for it.

I see, thanks... thaw_process() doesn't take TIF_FREEZE into account.

But doesn't this mean we have a race?

Suppose that try_to_freeze_tasks() failed. It does cancel_freezing() for each
process before return, but what if some thread already checked TIF_FREEZE and
(for simplicity) it is preempted before frozen_process() in refrigerator().

thaw_tasks() runs, ignores this task (P), returns. P gets CPU, and becomes
frozen, but nobody will thaw it.

No?

Oleg.

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Monday, 19 February 2007 23:41, Oleg Nesterov wrote:
 On 02/19, Rafael J. Wysocki wrote:
 
  On Monday, 19 February 2007 21:23, Oleg Nesterov wrote:
  
@@ -199,6 +189,10 @@ static void thaw_tasks(int thaw_user_spa
   
do_each_thread(g, p) {
+   if (freezer_should_skip(p))
+   cancel_freezing(p);
+   } while_each_thread(g, p);
+   do_each_thread(g, p) {
if (!freezeable(p))
continue;
   
   Any reason for 2 separate do_each_thread() loops ?
  
  Yes.  If there is a freeze request pending for the vfork parent 
  (TIF_FREEZE
  set), we have to cancel it before the child is unfrozen, since otherwise the
  parent may go freezing after we try to reset PF_FROZEN for it.
 
 I see, thanks... thaw_process() doesn't take TIF_FREEZE into account.
 
 But doesn't this mean we have a race?
 
 Suppose that try_to_freeze_tasks() failed. It does cancel_freezing() for each
 process before return, but what if some thread already checked TIF_FREEZE and
 (for simplicity) it is preempted before frozen_process() in refrigerator().
 
 thaw_tasks() runs, ignores this task (P), returns. P gets CPU, and becomes
 frozen, but nobody will thaw it.
 
 No?

Well, I think this is highly theoretical.  Namely, try_to_freeze_tasks() only
fails after the timeout that's currently set to 20 sec., and it yields the CPU
in each iteration of the main loop.  The task in question would have to refuse
being frozen for 20 sec. and then suddenly decide to freeze itself right before
try_to_freeze_tasks() checks the timeout for the very last time.  Then, it
would have to get preempted at this very moment and stay unfrozen at least
until thaw_tasks() starts running and in fact even longer.

I think we may avoid this by making try_to_freeze_tasks() sleep for some time
after it has reset TIF_FREEZE for all tasks in the error path, if anyone is
ever able to trigger it.

Greetings,
Rafael
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Oleg Nesterov]

On 02/20, Rafael J. Wysocki wrote:

 On Monday, 19 February 2007 23:41, Oleg Nesterov wrote:
  On 02/19, Rafael J. Wysocki wrote:
  
   On Monday, 19 February 2007 21:23, Oleg Nesterov wrote:
   
 @@ -199,6 +189,10 @@ static void thaw_tasks(int thaw_user_spa

 do_each_thread(g, p) {
 +   if (freezer_should_skip(p))
 +   cancel_freezing(p);
 +   } while_each_thread(g, p);
 +   do_each_thread(g, p) {
 if (!freezeable(p))
 continue;

Any reason for 2 separate do_each_thread() loops ?
   
   Yes.  If there is a freeze request pending for the vfork parent 
   (TIF_FREEZE
   set), we have to cancel it before the child is unfrozen, since otherwise 
   the
   parent may go freezing after we try to reset PF_FROZEN for it.
  
  I see, thanks... thaw_process() doesn't take TIF_FREEZE into account.
  
  But doesn't this mean we have a race?
  
  Suppose that try_to_freeze_tasks() failed. It does cancel_freezing() for 
  each
  process before return, but what if some thread already checked TIF_FREEZE 
  and
  (for simplicity) it is preempted before frozen_process() in refrigerator().
  
  thaw_tasks() runs, ignores this task (P), returns. P gets CPU, and becomes
  frozen, but nobody will thaw it.
  
  No?
 
 Well, I think this is highly theoretical.  Namely, try_to_freeze_tasks() only
 fails after the timeout that's currently set to 20 sec., and it yields the CPU
 in each iteration of the main loop.  The task in question would have to refuse
 being frozen for 20 sec. and then suddenly decide to freeze itself right 
 before
 try_to_freeze_tasks() checks the timeout for the very last time.  Then, it
 would have to get preempted at this very moment and stay unfrozen at least
 until thaw_tasks() starts running and in fact even longer.

Yes, yes, it is pure theroretical,

 I think we may avoid this by making try_to_freeze_tasks() sleep for some time
 after it has reset TIF_FREEZE for all tasks in the error path, if anyone is
 ever able to trigger it.

This makes this race  (pure theroretical) ** 2  :)

Still. May be it make sense to introduce cancel_freezing_and_thaw() function
(not right now) which stops the task from sleeping in refrigirator reliably.
I didn't think much about this, but it looks like we can fix coredump/exec
problems. Of course, this is not so important, we can ignore them at least
for now (-vfork_done is different, should be imho solved, because any user
can block freezer forever).

The fix:

refrigerator:

+   // we are going to call do_exit() really soon,
+   // we have a pending SIGKILL
+   if (current-signal-flags  SIGNAL_GROUP_EXIT)
+   return;

frozen_process(current);
...


zap_other_threads:

for_each_subthread() {
...

+   //  SIGNAL_GROUP_EXIT is set --
+   // we can check sig-group_exit_task to detect 
de_thread,
+   // but perhaps it doesn't hurt if the caller is 
do_group_exit
+   cancel_freezing_and_thaw(p);
sigaddset(t-pending.signal, SIGKILL);
signal_wake_up(t, 1);
}

This way execer reliably kills all sub-threads and proceeds without blocking
try_to_freeze_tasks(). The same change could be done for zap_process() to fix
coredump.

Oleg.

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Tuesday, 20 February 2007 01:12, Oleg Nesterov wrote:
 On 02/20, Rafael J. Wysocki wrote:
 
  On Monday, 19 February 2007 23:41, Oleg Nesterov wrote:
   On 02/19, Rafael J. Wysocki wrote:
   
On Monday, 19 February 2007 21:23, Oleg Nesterov wrote:

  @@ -199,6 +189,10 @@ static void thaw_tasks(int thaw_user_spa
 
  do_each_thread(g, p) {
  +   if (freezer_should_skip(p))
  +   cancel_freezing(p);
  +   } while_each_thread(g, p);
  +   do_each_thread(g, p) {
  if (!freezeable(p))
  continue;
 
 Any reason for 2 separate do_each_thread() loops ?

Yes.  If there is a freeze request pending for the vfork parent 
(TIF_FREEZE
set), we have to cancel it before the child is unfrozen, since 
otherwise the
parent may go freezing after we try to reset PF_FROZEN for it.
   
   I see, thanks... thaw_process() doesn't take TIF_FREEZE into account.
   
   But doesn't this mean we have a race?
   
   Suppose that try_to_freeze_tasks() failed. It does cancel_freezing() for 
   each
   process before return, but what if some thread already checked TIF_FREEZE 
   and
   (for simplicity) it is preempted before frozen_process() in 
   refrigerator().
   
   thaw_tasks() runs, ignores this task (P), returns. P gets CPU, and becomes
   frozen, but nobody will thaw it.
   
   No?
  
  Well, I think this is highly theoretical.  Namely, try_to_freeze_tasks() 
  only
  fails after the timeout that's currently set to 20 sec., and it yields the 
  CPU
  in each iteration of the main loop.  The task in question would have to 
  refuse
  being frozen for 20 sec. and then suddenly decide to freeze itself right 
  before
  try_to_freeze_tasks() checks the timeout for the very last time.  Then, it
  would have to get preempted at this very moment and stay unfrozen at least
  until thaw_tasks() starts running and in fact even longer.
 
 Yes, yes, it is pure theroretical,
 
  I think we may avoid this by making try_to_freeze_tasks() sleep for some 
  time
  after it has reset TIF_FREEZE for all tasks in the error path, if anyone is
  ever able to trigger it.
 
 This makes this race  (pure theroretical) ** 2  :)
 
 Still. May be it make sense to introduce cancel_freezing_and_thaw() function
 (not right now) which stops the task from sleeping in refrigirator reliably.

Hm.  In the case discussed above we have a task that's right before calling
frozen_process(), so we can't thaw it, because it's not frozen.  It will be
frozen just in a while, but try_to_freeze_tasks() and thaw_tasks() have no
way to check this.

I think to close this race the refrigerator should check TIF_FREEZE and set
PF_FROZEN _and_ reset TIF_FREEZE under a lock that would also have to be
taken by try_to_freeze_tasks() in the beginning of the error path.  This will
ensure that all tasks either freeze themselves before the error path in
try_to_freeze_tasks() is executed, or remain unfrozen.

I'll try to prepare a patch to illustrate this, but right now I'm too tired to
do it. :-)

 I didn't think much about this, but it looks like we can fix coredump/exec
 problems. Of course, this is not so important, we can ignore them at least
 for now (-vfork_done is different, should be imho solved, because any user
 can block freezer forever).
 
 The fix:
 
   refrigerator:
 
   +   // we are going to call do_exit() really soon,
   +   // we have a pending SIGKILL
   +   if (current-signal-flags  SIGNAL_GROUP_EXIT)
   +   return;
 
   frozen_process(current);
   ...
 
 
   zap_other_threads:
 
   for_each_subthread() {
   ...
 
   +   //  SIGNAL_GROUP_EXIT is set --
   +   // we can check sig-group_exit_task to detect 
 de_thread,
   +   // but perhaps it doesn't hurt if the caller is 
 do_group_exit
   +   cancel_freezing_and_thaw(p);
   sigaddset(t-pending.signal, SIGKILL);
   signal_wake_up(t, 1);
   }
 
 This way execer reliably kills all sub-threads and proceeds without blocking
 try_to_freeze_tasks(). The same change could be done for zap_process() to fix
 coredump.

Yes, at first sight it looks good.

BTW, what do you think of the updated patch I sent two messages ago?

Rafael
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Oleg Nesterov]

On 02/20, Rafael J. Wysocki wrote:

 BTW, what do you think of the updated patch I sent two messages ago?

Ah, sorry, I just forgot... I think it is nice.

Oleg.

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Sunday, 18 February 2007 23:01, Oleg Nesterov wrote:
> On 02/18, Rafael J. Wysocki wrote:
> > 
> > Appended is a patch that does something along these lines.  The necessary
> > thread_info flags are defined for i386 and x86_64, for now.
> 
> I'll try to look at this patch when I am not so sleepy ...
> 
> just one small nit right now,
> 
> > --- linux-2.6.20-mm2.orig/include/asm-i386/thread_info.h2007-02-18 
> > 19:49:34.0 +0100
> > +++ linux-2.6.20-mm2/include/asm-i386/thread_info.h 2007-02-18 
> > 19:50:37.0 +0100
> > @@ -135,6 +135,7 @@ static inline struct thread_info *curren
> >  #define TIF_IO_BITMAP  18  /* uses I/O bitmap */
> >  #define TIF_FREEZE 19  /* is freezing for suspend */
> >  #define TIF_FORCED_TF  20  /* true if TF in eflags 
> > artificially */
> > +#define TIF_FREEZER_SKIP   21  /* task freezer should not count us */
> 
> Do we need to put this flag into thread_info? It is always modified by
> "current", so it could live in task_struct->flags instead.

I thought we were running low on the task_struct->flags bits. :-)

Apart from this, we may need to set it from somewhere else in the future.

Greetings,
Rafael
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Oleg Nesterov]

On 02/18, Rafael J. Wysocki wrote:
> 
> Appended is a patch that does something along these lines.  The necessary
> thread_info flags are defined for i386 and x86_64, for now.

I'll try to look at this patch when I am not so sleepy ...

just one small nit right now,

> --- linux-2.6.20-mm2.orig/include/asm-i386/thread_info.h  2007-02-18 
> 19:49:34.0 +0100
> +++ linux-2.6.20-mm2/include/asm-i386/thread_info.h   2007-02-18 
> 19:50:37.0 +0100
> @@ -135,6 +135,7 @@ static inline struct thread_info *curren
>  #define TIF_IO_BITMAP18  /* uses I/O bitmap */
>  #define TIF_FREEZE   19  /* is freezing for suspend */
>  #define TIF_FORCED_TF20  /* true if TF in eflags 
> artificially */
> +#define TIF_FREEZER_SKIP 21  /* task freezer should not count us */

Do we need to put this flag into thread_info? It is always modified by
"current", so it could live in task_struct->flags instead.

Oleg.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Sunday, 18 February 2007 15:52, Oleg Nesterov wrote:
> On 02/18, Rafael J. Wysocki wrote:
> >
> > On Sunday, 18 February 2007 12:31, Oleg Nesterov wrote:
> > > > > 
> > > > > However, this means that sys_vfork() makes impossible to freeze 
> > > > > processes
> > > > > until child exits/execs. Not good.
> > > > 
> > > I forgot to say that we have another problem: coredumping.
> > > 
> > > A thread which does do_coredump() send SIGKILL to ->mm users, and sleeps
> > > on ->mm->core_startup_done. Now it can't be frozen if sub-thread goes to
> > > refrigerator. I think this could be solved easily if we add a check to
> > > refrigerator() as you suggested for ->vfork_donw.
> > > 
> > > > I think the real solution would be to use an interruptible completion 
> > > > in the
> > > > vfork code.  It was discussed some time ago and, IIRC, Ingo had an 
> > > > experimental
> > > > patch that implemented it.  Still, for the suspend this really is not 
> > > > an issue
> > > > in practice, so it wasn't merged.
> > > 
> > > It is not (afaics) so trivial to do rightly, and with this change the 
> > > parent
> > > will be seen as TASK_INTERRUPTIBLE even without freezer in progress.
> > > 
> > > A very vague idea: what if parent will do
> > > 
> > >   current->flags |= PF_PLEASE_CONSIDER_ME_AS_FROZEN_BUT_SET_TIF_FREEZE
> > >   wait_for_completion();
> > >   try_to_freeze();
> > > 
> > > ?
> > 
> > This should work,
> 
> Good. So try_to_freeze_tasks() can forget about "if (!p->vfork_done)" check.
> This needs more thinking, of course. For example, thaw_process() should be
> carefull to clear TIF_FREEZE if we have the new flag set, but not PF_FROZEN.
> frozen() should be changed to return true if PF_NEW_FLAG && TIF_FREEZE, but
> it also called by refrigerator.
> 
> But IF we really can do this, it will be a general solution.

Appended is a patch that does something along these lines.  The necessary
thread_info flags are defined for i386 and x86_64, for now.

Greetings,
Rafael


 include/asm-i386/thread_info.h   |2 ++
 include/asm-x86_64/thread_info.h |2 ++
 include/linux/freezer.h  |   24 
 kernel/fork.c|4 
 kernel/power/process.c   |   24 +---
 5 files changed, 41 insertions(+), 15 deletions(-)

Index: linux-2.6.20-mm2/include/asm-i386/thread_info.h
===
--- linux-2.6.20-mm2.orig/include/asm-i386/thread_info.h2007-02-18 
19:49:34.0 +0100
+++ linux-2.6.20-mm2/include/asm-i386/thread_info.h 2007-02-18 
19:50:37.0 +0100
@@ -135,6 +135,7 @@ static inline struct thread_info *curren
 #define TIF_IO_BITMAP  18  /* uses I/O bitmap */
 #define TIF_FREEZE 19  /* is freezing for suspend */
 #define TIF_FORCED_TF  20  /* true if TF in eflags artificially */
+#define TIF_FREEZER_SKIP   21  /* task freezer should not count us */
 
 #define _TIF_SYSCALL_TRACE (1<
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -1393,7 +1394,10 @@ long do_fork(unsigned long clone_flags,
tracehook_report_clone_complete(clone_flags, nr, p);
 
if (clone_flags & CLONE_VFORK) {
+   freezer_do_not_count(current);
wait_for_completion();
+   try_to_freeze();
+   freezer_count(current);
tracehook_report_vfork_done(p, nr);
}
} else {
Index: linux-2.6.20-mm2/kernel/power/process.c
===
--- linux-2.6.20-mm2.orig/kernel/power/process.c2007-02-18 
19:49:34.0 +0100
+++ linux-2.6.20-mm2/kernel/power/process.c 2007-02-18 19:50:37.0 
+0100
@@ -117,22 +117,12 @@ static unsigned int try_to_freeze_tasks(
cancel_freezing(p);
continue;
}
-   if (is_user_space(p)) {
-   if (!freeze_user_space)
-   continue;
-
-   /* Freeze the task unless there is a vfork
-* completion pending
-*/
-   if (!p->vfork_done)
-   freeze_process(p);
-   } else {
-   if (freeze_user_space)
-   continue;
+   if (is_user_space(p) == !freeze_user_space)
+   continue;
 
-   freeze_process(p);
-   }
-   todo++;
+   freeze_process(p);
+   if (!freezer_should_skip(p))
+   todo++;
} while_each_thread(g, p);

Re: freezer problems

[Rafael J. Wysocki]

On Sunday, 18 February 2007 17:19, Oleg Nesterov wrote:
> On 02/18, Rafael J. Wysocki wrote:
> >
> > On Sunday, 18 February 2007 15:52, Oleg Nesterov wrote:
> > > 
> > > And now another problem: exec. de_thread() sleeps in TASK_UNINTERRUPTIBLE
> > > waiting for all sub-threads to die, and we have the same "deadlock" if
> > > one of them is frozen. This is nasty. Probably we can change the ->state
> > > to TASK_INTERRUPTIBLE and add try_to_freeze(), or play with the new PF_
> > > flag, but I am not sure it is safe to freeze() the task which is deep
> > > in the exec() path.
> > 
> > Hm, I haven't been aware of this case.
> > 
> > Well, probably we can do something like in the patch that I've just sent: 
> > the
> > child that enters the refrigerator should know that the parent is
> > uninterruptible and will wait for it to exit.  Thus it can either mark the
> > parent as frozen or just exit the refrigerator without freezing itself.
> 
> Sub-thread could already sleep in refrigerator when another thread does exec.
> So we have no choice but somehow freeze the execer. But again, I don't know
> if it is safe to freeze it here, at de_thread() stage. It is called from
> load_xxx_binary(), we may hold some important locks...

So it probably isn't safe.

Rafael

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Oleg Nesterov]

On 02/18, Rafael J. Wysocki wrote:
>
> On Sunday, 18 February 2007 15:52, Oleg Nesterov wrote:
> > 
> > And now another problem: exec. de_thread() sleeps in TASK_UNINTERRUPTIBLE
> > waiting for all sub-threads to die, and we have the same "deadlock" if
> > one of them is frozen. This is nasty. Probably we can change the ->state
> > to TASK_INTERRUPTIBLE and add try_to_freeze(), or play with the new PF_
> > flag, but I am not sure it is safe to freeze() the task which is deep
> > in the exec() path.
> 
> Hm, I haven't been aware of this case.
> 
> Well, probably we can do something like in the patch that I've just sent: the
> child that enters the refrigerator should know that the parent is
> uninterruptible and will wait for it to exit.  Thus it can either mark the
> parent as frozen or just exit the refrigerator without freezing itself.

Sub-thread could already sleep in refrigerator when another thread does exec.
So we have no choice but somehow freeze the execer. But again, I don't know
if it is safe to freeze it here, at de_thread() stage. It is called from
load_xxx_binary(), we may hold some important locks...

Oleg.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Sunday, 18 February 2007 15:52, Oleg Nesterov wrote:
> On 02/18, Rafael J. Wysocki wrote:
> >
> > On Sunday, 18 February 2007 12:31, Oleg Nesterov wrote:
> > > > > 
> > > > > However, this means that sys_vfork() makes impossible to freeze 
> > > > > processes
> > > > > until child exits/execs. Not good.
> > > > 
> > > I forgot to say that we have another problem: coredumping.
> > > 
> > > A thread which does do_coredump() send SIGKILL to ->mm users, and sleeps
> > > on ->mm->core_startup_done. Now it can't be frozen if sub-thread goes to
> > > refrigerator. I think this could be solved easily if we add a check to
> > > refrigerator() as you suggested for ->vfork_donw.
> > > 
> > > > I think the real solution would be to use an interruptible completion 
> > > > in the
> > > > vfork code.  It was discussed some time ago and, IIRC, Ingo had an 
> > > > experimental
> > > > patch that implemented it.  Still, for the suspend this really is not 
> > > > an issue
> > > > in practice, so it wasn't merged.
> > > 
> > > It is not (afaics) so trivial to do rightly, and with this change the 
> > > parent
> > > will be seen as TASK_INTERRUPTIBLE even without freezer in progress.
> > > 
> > > A very vague idea: what if parent will do
> > > 
> > >   current->flags |= PF_PLEASE_CONSIDER_ME_AS_FROZEN_BUT_SET_TIF_FREEZE
> > >   wait_for_completion();
> > >   try_to_freeze();
> > > 
> > > ?
> > 
> > This should work,
> 
> Good. So try_to_freeze_tasks() can forget about "if (!p->vfork_done)" check.
> This needs more thinking, of course. For example, thaw_process() should be
> carefull to clear TIF_FREEZE if we have the new flag set, but not PF_FROZEN.
> frozen() should be changed to return true if PF_NEW_FLAG && TIF_FREEZE, but
> it also called by refrigerator.
> 
> But IF we really can do this, it will be a general solution.
> 
> >but we'll need a separate process flag for it.  If that's
> > acceptable,
> 
> I can't judge. Changed the subject to have more attention from experts.
> 
> >  I'd call it PF_VFORK_PARENT
> 
> I'd suggest not to put "VFORK" into the name. Probably we will find other
> usage for this flag which in fact means: "I am sleeping TASK_UNINTERRUPTIBLE
> at the safe place to freeze, I promise to do try_to_freeze() when I have
> CPU".
> 
> And now another problem: exec. de_thread() sleeps in TASK_UNINTERRUPTIBLE
> waiting for all sub-threads to die, and we have the same "deadlock" if
> one of them is frozen. This is nasty. Probably we can change the ->state
> to TASK_INTERRUPTIBLE and add try_to_freeze(), or play with the new PF_
> flag, but I am not sure it is safe to freeze() the task which is deep
> in the exec() path.

Hm, I haven't been aware of this case.

Well, probably we can do something like in the patch that I've just sent: the
child that enters the refrigerator should know that the parent is
uninterruptible and will wait for it to exit.  Thus it can either mark the
parent as frozen or just exit the refrigerator without freezing itself.

Greetings,
Rafael
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

freezer problems

[Oleg Nesterov]

On 02/18, Rafael J. Wysocki wrote:
>
> On Sunday, 18 February 2007 12:31, Oleg Nesterov wrote:
> > > > 
> > > > However, this means that sys_vfork() makes impossible to freeze 
> > > > processes
> > > > until child exits/execs. Not good.
> > > 
> > I forgot to say that we have another problem: coredumping.
> > 
> > A thread which does do_coredump() send SIGKILL to ->mm users, and sleeps
> > on ->mm->core_startup_done. Now it can't be frozen if sub-thread goes to
> > refrigerator. I think this could be solved easily if we add a check to
> > refrigerator() as you suggested for ->vfork_donw.
> > 
> > > I think the real solution would be to use an interruptible completion in 
> > > the
> > > vfork code.  It was discussed some time ago and, IIRC, Ingo had an 
> > > experimental
> > > patch that implemented it.  Still, for the suspend this really is not an 
> > > issue
> > > in practice, so it wasn't merged.
> > 
> > It is not (afaics) so trivial to do rightly, and with this change the parent
> > will be seen as TASK_INTERRUPTIBLE even without freezer in progress.
> > 
> > A very vague idea: what if parent will do
> > 
> > current->flags |= PF_PLEASE_CONSIDER_ME_AS_FROZEN_BUT_SET_TIF_FREEZE
> > wait_for_completion();
> > try_to_freeze();
> > 
> > ?
> 
> This should work,

Good. So try_to_freeze_tasks() can forget about "if (!p->vfork_done)" check.
This needs more thinking, of course. For example, thaw_process() should be
carefull to clear TIF_FREEZE if we have the new flag set, but not PF_FROZEN.
frozen() should be changed to return true if PF_NEW_FLAG && TIF_FREEZE, but
it also called by refrigerator.

But IF we really can do this, it will be a general solution.

>but we'll need a separate process flag for it.  If that's
> acceptable,

I can't judge. Changed the subject to have more attention from experts.

>  I'd call it PF_VFORK_PARENT

I'd suggest not to put "VFORK" into the name. Probably we will find other
usage for this flag which in fact means: "I am sleeping TASK_UNINTERRUPTIBLE
at the safe place to freeze, I promise to do try_to_freeze() when I have
CPU".

And now another problem: exec. de_thread() sleeps in TASK_UNINTERRUPTIBLE
waiting for all sub-threads to die, and we have the same "deadlock" if
one of them is frozen. This is nasty. Probably we can change the ->state
to TASK_INTERRUPTIBLE and add try_to_freeze(), or play with the new PF_
flag, but I am not sure it is safe to freeze() the task which is deep
in the exec() path.

Oleg.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

freezer problems

[Oleg Nesterov]

On 02/18, Rafael J. Wysocki wrote:

 On Sunday, 18 February 2007 12:31, Oleg Nesterov wrote:

However, this means that sys_vfork() makes impossible to freeze 
processes
until child exits/execs. Not good.
   
  I forgot to say that we have another problem: coredumping.
  
  A thread which does do_coredump() send SIGKILL to -mm users, and sleeps
  on -mm-core_startup_done. Now it can't be frozen if sub-thread goes to
  refrigerator. I think this could be solved easily if we add a check to
  refrigerator() as you suggested for -vfork_donw.
  
   I think the real solution would be to use an interruptible completion in 
   the
   vfork code.  It was discussed some time ago and, IIRC, Ingo had an 
   experimental
   patch that implemented it.  Still, for the suspend this really is not an 
   issue
   in practice, so it wasn't merged.
  
  It is not (afaics) so trivial to do rightly, and with this change the parent
  will be seen as TASK_INTERRUPTIBLE even without freezer in progress.
  
  A very vague idea: what if parent will do
  
  current-flags |= PF_PLEASE_CONSIDER_ME_AS_FROZEN_BUT_SET_TIF_FREEZE
  wait_for_completion(vfork);
  try_to_freeze();
  
  ?
 
 This should work,

Good. So try_to_freeze_tasks() can forget about if (!p-vfork_done) check.
This needs more thinking, of course. For example, thaw_process() should be
carefull to clear TIF_FREEZE if we have the new flag set, but not PF_FROZEN.
frozen() should be changed to return true if PF_NEW_FLAG  TIF_FREEZE, but
it also called by refrigerator.

But IF we really can do this, it will be a general solution.

but we'll need a separate process flag for it.  If that's
 acceptable,

I can't judge. Changed the subject to have more attention from experts.

  I'd call it PF_VFORK_PARENT

I'd suggest not to put VFORK into the name. Probably we will find other
usage for this flag which in fact means: I am sleeping TASK_UNINTERRUPTIBLE
at the safe place to freeze, I promise to do try_to_freeze() when I have
CPU.

And now another problem: exec. de_thread() sleeps in TASK_UNINTERRUPTIBLE
waiting for all sub-threads to die, and we have the same deadlock if
one of them is frozen. This is nasty. Probably we can change the -state
to TASK_INTERRUPTIBLE and add try_to_freeze(), or play with the new PF_
flag, but I am not sure it is safe to freeze() the task which is deep
in the exec() path.

Oleg.

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Sunday, 18 February 2007 15:52, Oleg Nesterov wrote:
 On 02/18, Rafael J. Wysocki wrote:
 
  On Sunday, 18 February 2007 12:31, Oleg Nesterov wrote:
 
 However, this means that sys_vfork() makes impossible to freeze 
 processes
 until child exits/execs. Not good.

   I forgot to say that we have another problem: coredumping.
   
   A thread which does do_coredump() send SIGKILL to -mm users, and sleeps
   on -mm-core_startup_done. Now it can't be frozen if sub-thread goes to
   refrigerator. I think this could be solved easily if we add a check to
   refrigerator() as you suggested for -vfork_donw.
   
I think the real solution would be to use an interruptible completion 
in the
vfork code.  It was discussed some time ago and, IIRC, Ingo had an 
experimental
patch that implemented it.  Still, for the suspend this really is not 
an issue
in practice, so it wasn't merged.
   
   It is not (afaics) so trivial to do rightly, and with this change the 
   parent
   will be seen as TASK_INTERRUPTIBLE even without freezer in progress.
   
   A very vague idea: what if parent will do
   
 current-flags |= PF_PLEASE_CONSIDER_ME_AS_FROZEN_BUT_SET_TIF_FREEZE
 wait_for_completion(vfork);
 try_to_freeze();
   
   ?
  
  This should work,
 
 Good. So try_to_freeze_tasks() can forget about if (!p-vfork_done) check.
 This needs more thinking, of course. For example, thaw_process() should be
 carefull to clear TIF_FREEZE if we have the new flag set, but not PF_FROZEN.
 frozen() should be changed to return true if PF_NEW_FLAG  TIF_FREEZE, but
 it also called by refrigerator.
 
 But IF we really can do this, it will be a general solution.
 
 but we'll need a separate process flag for it.  If that's
  acceptable,
 
 I can't judge. Changed the subject to have more attention from experts.
 
   I'd call it PF_VFORK_PARENT
 
 I'd suggest not to put VFORK into the name. Probably we will find other
 usage for this flag which in fact means: I am sleeping TASK_UNINTERRUPTIBLE
 at the safe place to freeze, I promise to do try_to_freeze() when I have
 CPU.
 
 And now another problem: exec. de_thread() sleeps in TASK_UNINTERRUPTIBLE
 waiting for all sub-threads to die, and we have the same deadlock if
 one of them is frozen. This is nasty. Probably we can change the -state
 to TASK_INTERRUPTIBLE and add try_to_freeze(), or play with the new PF_
 flag, but I am not sure it is safe to freeze() the task which is deep
 in the exec() path.

Hm, I haven't been aware of this case.

Well, probably we can do something like in the patch that I've just sent: the
child that enters the refrigerator should know that the parent is
uninterruptible and will wait for it to exit.  Thus it can either mark the
parent as frozen or just exit the refrigerator without freezing itself.

Greetings,
Rafael
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Oleg Nesterov]

On 02/18, Rafael J. Wysocki wrote:

 On Sunday, 18 February 2007 15:52, Oleg Nesterov wrote:
  
  And now another problem: exec. de_thread() sleeps in TASK_UNINTERRUPTIBLE
  waiting for all sub-threads to die, and we have the same deadlock if
  one of them is frozen. This is nasty. Probably we can change the -state
  to TASK_INTERRUPTIBLE and add try_to_freeze(), or play with the new PF_
  flag, but I am not sure it is safe to freeze() the task which is deep
  in the exec() path.
 
 Hm, I haven't been aware of this case.
 
 Well, probably we can do something like in the patch that I've just sent: the
 child that enters the refrigerator should know that the parent is
 uninterruptible and will wait for it to exit.  Thus it can either mark the
 parent as frozen or just exit the refrigerator without freezing itself.

Sub-thread could already sleep in refrigerator when another thread does exec.
So we have no choice but somehow freeze the execer. But again, I don't know
if it is safe to freeze it here, at de_thread() stage. It is called from
load_xxx_binary(), we may hold some important locks...

Oleg.

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Sunday, 18 February 2007 17:19, Oleg Nesterov wrote:
 On 02/18, Rafael J. Wysocki wrote:
 
  On Sunday, 18 February 2007 15:52, Oleg Nesterov wrote:
   
   And now another problem: exec. de_thread() sleeps in TASK_UNINTERRUPTIBLE
   waiting for all sub-threads to die, and we have the same deadlock if
   one of them is frozen. This is nasty. Probably we can change the -state
   to TASK_INTERRUPTIBLE and add try_to_freeze(), or play with the new PF_
   flag, but I am not sure it is safe to freeze() the task which is deep
   in the exec() path.
  
  Hm, I haven't been aware of this case.
  
  Well, probably we can do something like in the patch that I've just sent: 
  the
  child that enters the refrigerator should know that the parent is
  uninterruptible and will wait for it to exit.  Thus it can either mark the
  parent as frozen or just exit the refrigerator without freezing itself.
 
 Sub-thread could already sleep in refrigerator when another thread does exec.
 So we have no choice but somehow freeze the execer. But again, I don't know
 if it is safe to freeze it here, at de_thread() stage. It is called from
 load_xxx_binary(), we may hold some important locks...

So it probably isn't safe.

Rafael

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Sunday, 18 February 2007 15:52, Oleg Nesterov wrote:
 On 02/18, Rafael J. Wysocki wrote:
 
  On Sunday, 18 February 2007 12:31, Oleg Nesterov wrote:
 
 However, this means that sys_vfork() makes impossible to freeze 
 processes
 until child exits/execs. Not good.

   I forgot to say that we have another problem: coredumping.
   
   A thread which does do_coredump() send SIGKILL to -mm users, and sleeps
   on -mm-core_startup_done. Now it can't be frozen if sub-thread goes to
   refrigerator. I think this could be solved easily if we add a check to
   refrigerator() as you suggested for -vfork_donw.
   
I think the real solution would be to use an interruptible completion 
in the
vfork code.  It was discussed some time ago and, IIRC, Ingo had an 
experimental
patch that implemented it.  Still, for the suspend this really is not 
an issue
in practice, so it wasn't merged.
   
   It is not (afaics) so trivial to do rightly, and with this change the 
   parent
   will be seen as TASK_INTERRUPTIBLE even without freezer in progress.
   
   A very vague idea: what if parent will do
   
 current-flags |= PF_PLEASE_CONSIDER_ME_AS_FROZEN_BUT_SET_TIF_FREEZE
 wait_for_completion(vfork);
 try_to_freeze();
   
   ?
  
  This should work,
 
 Good. So try_to_freeze_tasks() can forget about if (!p-vfork_done) check.
 This needs more thinking, of course. For example, thaw_process() should be
 carefull to clear TIF_FREEZE if we have the new flag set, but not PF_FROZEN.
 frozen() should be changed to return true if PF_NEW_FLAG  TIF_FREEZE, but
 it also called by refrigerator.
 
 But IF we really can do this, it will be a general solution.

Appended is a patch that does something along these lines.  The necessary
thread_info flags are defined for i386 and x86_64, for now.

Greetings,
Rafael


 include/asm-i386/thread_info.h   |2 ++
 include/asm-x86_64/thread_info.h |2 ++
 include/linux/freezer.h  |   24 
 kernel/fork.c|4 
 kernel/power/process.c   |   24 +---
 5 files changed, 41 insertions(+), 15 deletions(-)

Index: linux-2.6.20-mm2/include/asm-i386/thread_info.h
===
--- linux-2.6.20-mm2.orig/include/asm-i386/thread_info.h2007-02-18 
19:49:34.0 +0100
+++ linux-2.6.20-mm2/include/asm-i386/thread_info.h 2007-02-18 
19:50:37.0 +0100
@@ -135,6 +135,7 @@ static inline struct thread_info *curren
 #define TIF_IO_BITMAP  18  /* uses I/O bitmap */
 #define TIF_FREEZE 19  /* is freezing for suspend */
 #define TIF_FORCED_TF  20  /* true if TF in eflags artificially */
+#define TIF_FREEZER_SKIP   21  /* task freezer should not count us */
 
 #define _TIF_SYSCALL_TRACE (1TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME (1TIF_NOTIFY_RESUME)
@@ -149,6 +150,7 @@ static inline struct thread_info *curren
 #define _TIF_IO_BITMAP (1TIF_IO_BITMAP)
 #define _TIF_FREEZE(1TIF_FREEZE)
 #define _TIF_FORCED_TF (1TIF_FORCED_TF)
+#define _TIF_FREEZER_SKIP  (1TIF_FREEZER_SKIP)
 
 /* work to do on interrupt/exception return */
 #define _TIF_WORK_MASK \
Index: linux-2.6.20-mm2/include/asm-x86_64/thread_info.h
===
--- linux-2.6.20-mm2.orig/include/asm-x86_64/thread_info.h  2007-02-18 
19:49:34.0 +0100
+++ linux-2.6.20-mm2/include/asm-x86_64/thread_info.h   2007-02-18 
19:50:37.0 +0100
@@ -123,6 +123,7 @@ static inline struct thread_info *stack_
 #define TIF_DEBUG  21  /* uses debug registers */
 #define TIF_IO_BITMAP  22  /* uses I/O bitmap */
 #define TIF_FREEZE 23  /* is freezing for suspend */
+#define TIF_FREEZER_SKIP   24  /* task freezer should not count us */
 
 #define _TIF_SYSCALL_TRACE (1TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME (1TIF_NOTIFY_RESUME)
@@ -140,6 +141,7 @@ static inline struct thread_info *stack_
 #define _TIF_DEBUG (1TIF_DEBUG)
 #define _TIF_IO_BITMAP (1TIF_IO_BITMAP)
 #define _TIF_FREEZE(1TIF_FREEZE)
+#define _TIF_FREEZER_SKIP  (1TIF_FREEZER_SKIP)
 
 /* work to do on interrupt/exception return */
 #define _TIF_WORK_MASK \
Index: linux-2.6.20-mm2/include/linux/freezer.h
===
--- linux-2.6.20-mm2.orig/include/linux/freezer.h   2007-02-18 
19:49:34.0 +0100
+++ linux-2.6.20-mm2/include/linux/freezer.h2007-02-18 19:50:37.0 
+0100
@@ -36,6 +36,30 @@ static inline void do_not_freeze(struct 
 }
 
 /*
+ * Tell the freezer not to count this task as freezeable
+ */
+static inline void freezer_do_not_count(struct task_struct *p)
+{
+   set_tsk_thread_flag(p, TIF_FREEZER_SKIP);
+}
+
+/*
+ * Tell the freezer to count this task as freezeable
+ 

Re: freezer problems

[Oleg Nesterov]

On 02/18, Rafael J. Wysocki wrote:
 
 Appended is a patch that does something along these lines.  The necessary
 thread_info flags are defined for i386 and x86_64, for now.

I'll try to look at this patch when I am not so sleepy ...

just one small nit right now,

 --- linux-2.6.20-mm2.orig/include/asm-i386/thread_info.h  2007-02-18 
 19:49:34.0 +0100
 +++ linux-2.6.20-mm2/include/asm-i386/thread_info.h   2007-02-18 
 19:50:37.0 +0100
 @@ -135,6 +135,7 @@ static inline struct thread_info *curren
  #define TIF_IO_BITMAP18  /* uses I/O bitmap */
  #define TIF_FREEZE   19  /* is freezing for suspend */
  #define TIF_FORCED_TF20  /* true if TF in eflags 
 artificially */
 +#define TIF_FREEZER_SKIP 21  /* task freezer should not count us */

Do we need to put this flag into thread_info? It is always modified by
current, so it could live in task_struct-flags instead.

Oleg.

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: freezer problems

[Rafael J. Wysocki]

On Sunday, 18 February 2007 23:01, Oleg Nesterov wrote:
 On 02/18, Rafael J. Wysocki wrote:
  
  Appended is a patch that does something along these lines.  The necessary
  thread_info flags are defined for i386 and x86_64, for now.
 
 I'll try to look at this patch when I am not so sleepy ...
 
 just one small nit right now,
 
  --- linux-2.6.20-mm2.orig/include/asm-i386/thread_info.h2007-02-18 
  19:49:34.0 +0100
  +++ linux-2.6.20-mm2/include/asm-i386/thread_info.h 2007-02-18 
  19:50:37.0 +0100
  @@ -135,6 +135,7 @@ static inline struct thread_info *curren
   #define TIF_IO_BITMAP  18  /* uses I/O bitmap */
   #define TIF_FREEZE 19  /* is freezing for suspend */
   #define TIF_FORCED_TF  20  /* true if TF in eflags 
  artificially */
  +#define TIF_FREEZER_SKIP   21  /* task freezer should not count us */
 
 Do we need to put this flag into thread_info? It is always modified by
 current, so it could live in task_struct-flags instead.

I thought we were running low on the task_struct-flags bits. :-)

Apart from this, we may need to set it from somewhere else in the future.

Greetings,
Rafael
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/