login
Search
Hello Everyone, Oleg had pointed out some subtle races which could lead to the failure of the process freezer in the patchset which I had posted earlier. A couple of these problems seem to be present in the latest -mm(2.6.21-rc6-mm1). This is an attempt to fix them. Awaiting you feedback, 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!" -
This patch fixes the race pointed out by Oleg Nesterov.
* Freezer marks a thread as freezeable.
* The thread now marks itself PF_NOFREEZE causing it to
freeze on calling try_to_freeze(). Thus the task is frozen, even though
it doesn't want to.
* Subsequent thaw_processes() will also fail to thaw the task since it is
marked PF_NOFREEZE.
Avoid this problem by checking the current task's PF_NOFREEZE status in the
refrigerator before marking current as frozen.
Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>
---
kernel/power/process.c | 9 +++++++++
1 file changed, 9 insertions(+)
Index: linux-2.6.21-rc6/kernel/power/process.c
===================================================================
--- linux-2.6.21-rc6.orig/kernel/power/process.c
+++ linux-2.6.21-rc6/kernel/power/process.c
@@ -41,6 +41,15 @@ void refrigerator(void)
task_lock(current);
if (freezing(current)) {
+ /* check if we had marked ourself PF_NOFREEZE
+ * *after* the freezer did the freezeable() check
+ * on us.
+ */
+ if (current->flags & PF_NOFREEZE) {
+ clear_tsk_thread_flag(current, TIF_FREEZE);
+ task_unlock(current);
+ return;
+ }
frozen_process(current);
task_unlock(current);
} else {
--
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!"
-
Hi, Looks good, although I'm not sure if we don't need to call recalc_sigpending() for tasks that turn out to be PF_NOFREEZE. Greetings, Rafael -
I agree, we should clear TIF_SIGPENDING. It is not so critical for user-space
tasks, but for the kernel thread it may remain pending forever, causing subtle
failures.
Gautham, isn't it possible to make a more simpler patch ? Just add PF_NOFREEZE
check to frozen_process,
static inline void frozen_process(struct task_struct *p)
{
if (!unlikely(current->flags & PF_NOFREEZE)) {
p->flags |= PF_FROZEN;
wmb();
}
clear_tsk_thread_flag(p, TIF_FREEZE);
}
No?
Oleg.
-
Actually yes. The idea anyway was to check one last time before declaring
ourselves as frozen. So I thought the best place was inside refrigerator since
we are already holding the task_lock there.
I wasn't too sure about calling recalc_sigpending(), but now that you
mention it, I agree, this would be a nicer way to do it.
Btw, since frozen_process is currently being called only from
refrigerator, I am wondering if we still need the struct task_struct *p
parameter there. It's very unlikely that some other task would mark a
particular task as frozen. No?
Anyways, Andrew, Could you please replace the earlier sent patch titled
"fix_pf_nofreeze_and_freezeable_race.patch" with the following one?
Thanks and Regards
gautham.
-->
This patch fixes the race pointed out by Oleg Nesterov.
* Freezer marks a thread as freezeable.
* The thread now marks itself PF_NOFREEZE causing it to
freeze on calling try_to_freeze(). Thus the task is frozen, even though
it doesn't want to.
* Subsequent thaw_processes() will also fail to thaw the task since it is
marked PF_NOFREEZE.
Avoid this problem by checking the task's PF_NOFREEZE status in
frozen_processes() before marking the task as frozen.
Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>
---
include/linux/freezer.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Index: linux-2.6.21-rc6/include/linux/freezer.h
===================================================================
--- linux-2.6.21-rc6.orig/include/linux/freezer.h
+++ linux-2.6.21-rc6/include/linux/freezer.h
@@ -57,8 +57,10 @@ static inline int thaw_process(struct ta
*/
static inline void frozen_process(struct task_struct *p)
{
- p->flags |= PF_FROZEN;
- wmb();
+ if (!unlikely(p->flags & PF_NOFREEZE)) {
+ p->flags |= PF_FROZEN;
+ wmb();
+ }
clear_tsk_thread_flag(p, TIF_FREEZE);
}
--
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because ...I agree. Only "current" can set PF_FROZEN anyway. I also think it is better to move this function into kernel/power/process.c, no need to export it in freezer.h. It is special, should be called from refrigerator() with task_lock() On the second thought, this patch doesn't help if the thread never calls try_to_freeze() exactly because it marks itself PF_NOFREEZE. Perhaps it is better to cancel freezing in soon-to-be-introduced freezer_exempt(). In any case I agree, this one is better than current fix_pf_nofreeze_and_freezeable_race.patch Oleg. -
Threads which wait for completion on a frozen thread might result in
causing the freezer to fail, if the waiting thread is freezeable.
There are some well known cases where it's preferable to temporarily thaw
the frozen process, finish the wait for completion and allow both the
processes to call try_to_freeze.
kthread_stop is one such case. flush_workqueue might be another.
This patch attempts to address such a situation with a fix for kthread_stop.
Strictly experimental. Compile tested on i386.
Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>
---
include/asm-arm/thread_info.h | 2
include/asm-blackfin/thread_info.h | 4 +
include/asm-frv/thread_info.h | 4 +
include/asm-i386/thread_info.h | 4 +
include/asm-ia64/thread_info.h | 4 +
include/asm-mips/thread_info.h | 2
include/asm-powerpc/thread_info.h | 4 +
include/asm-sh/thread_info.h | 2
include/asm-x86_64/thread_info.h | 4 +
include/linux/freezer.h | 4 +
kernel/kthread.c | 4 +
kernel/power/process.c | 81 ++++++++++++++++++++++++++++++++++++-
12 files changed, 118 insertions(+), 1 deletion(-)
Index: linux-2.6.21-rc6/kernel/power/process.c
===================================================================
--- linux-2.6.21-rc6.orig/kernel/power/process.c
+++ linux-2.6.21-rc6/kernel/power/process.c
@@ -23,6 +23,16 @@
#define FREEZER_KERNEL_THREADS 0
#define FREEZER_USER_SPACE 1
+struct freezer_status_struct {
+ spinlock_t lock;
+ int count;
+};
+
+static struct freezer_status_struct freezer_status = {
+ .lock = SPIN_LOCK_UNLOCKED,
+ .count = 0,
+ };
+
static inline int freezeable(struct task_struct * p)
{
if ((p == current) ||
@@ -45,7 +55,8 @@ void refrigerator(void)
* *after* the freezer did the freezeable() check
* on us.
*/
- if (current->flags & PF_NOFREEZE) {
+ if ((current->flags & PF_NOFREEZE) ||
+ test_tsk_thread_flag(current, ...On Thu, 19 Apr 2007 17:34:19 +0530
flush_workqueue() just needs to die. I think there are (almost) no
Rather than doing <whatever you did>, perhaps we could make the freezing
process a dual-pass thing. On pass 1, mark all the threads as "we'll be
freezing you soon" and on the second pass, do the actual freezing. Then,
in problematic places such as kthread_stop() we can look to see if we'll
soon be asked to freeze and if so, run try_to_freeze().
Of course, running try_to_freeze() in kthread_stop() would be very wrong,
so we'd actually need to do it in callers, preferably via a new
kthread_stop_freezeable() wrapper.
And the two-pass-freeze thing is of course racy. It's also unnecessary:
setting a flag on every task in the machine is equivalent to setting a
global variable. So perhaps just use a global variable?
int kthread_stop_freezeable(struct task_struct *k)
{
if (freeze_state == ABOUT_TO_START) {
wait_for(freeze_state == STARTED);
try_to_freeze();
}
kthread_stop(k);
}
which is theoretically racy if another freeze_processes() starts
hm, all this duplication is unpleasing. We could do something similar to
include/linux/buffer_head.h:BH_PrivateStart here: get all architectures to
define a TIF_COMMON_STUFF_STARTS_HERE then include asm-generic/whatever.h
which defines all the flags which every architecture must define, as
TIF_COMMON_STUFF_STARTS_HERE+0, TIF_COMMON_STUFF_STARTS_HERE+1, etc.
-
Hmm, can't we do something like this instead:
---
kernel/kthread.c | 10 ++++++++++
1 file changed, 10 insertions(+)
Index: linux-2.6.21-rc7/kernel/kthread.c
===================================================================
--- linux-2.6.21-rc7.orig/kernel/kthread.c
+++ linux-2.6.21-rc7/kernel/kthread.c
@@ -13,6 +13,7 @@
#include <linux/file.h>
#include <linux/module.h>
#include <linux/mutex.h>
+#include <linux/freezer.h>
#include <asm/semaphore.h>
/*
@@ -232,6 +233,15 @@ int kthread_stop(struct task_struct *k)
/* Now set kthread_should_stop() to true, and wake it up. */
kthread_stop_info.k = k;
+ if (!(current->flags & PF_NOFREEZE)) {
+ /* If we are freezable, the freezer will wait for us */
+ task_lock(k);
+ k->flags |= PF_NOFREEZE;
+ if (frozen(k))
+ k->flags &= ~PF_FROZEN;
+
+ task_unlock(k);
+ }
wake_up_process(k);
put_task_struct(k);
-
Yes, we can do this for now since the tasks have only two freeze states, namely Freezeable and Non Freezeable. But with more events like cpu-hotplug and kprobes using the process freezer, the simple check if(!(current->flags & PF_NOFREEZE)) may not suffice and something like if(!(current->flags & PFE_ALL & global_freeze_mask)) /* global_freeze_mask is the mask of the events for which the system is freezing. */ appears to be racy. Not that we cannot work it out ;-) However, I was attempting to solve the generic problem where A waits on B and B is frozen. If A is freezeable (under one of the events) then the freezer will fail. So a solution would be for A to somehow inform B of the dependency and postpone it's freezing. Since akpm mentioned that flush_workqueue() needs to go, I guess, I am ok with fixing only kthread_stop/kthread_should_stop for the moment. 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!" -
Actually, I thought about it for a while. The thread that is going to stop another one may temporarily mark itself as freezable in all cases, which will have no effect on it, since it's not going to cally try_to_freeze(), but will make the freezer wait for it. Next, after returning from wait_for_completion(), it should restore its old freezability status and that Well, I think it might be simpler to consider each case separately. This way Sure. :-) BTW, if it turns out that we need to introduce yet another freezer-related TIF_ flag, it may be acceptable (?) to move all of the freezer-related flags into a separate member of task_struct (eg. freezer_flags) that can only be manipulated under task_lock(). I mean, we already have four of them (PF_NOFREEZE, PF_FROZEN, PF_FREEZER_SKIP, TIF_FREEZE), and you will need to introduce two more for the freezer-based CPU hotplug, so if yet another one is needed, that will make up almost a separate u8 field ... Greetings, Rafael -
But that will have no affect if the thread to be stopped is already frozen. The only affect might be that now, freezer will fail whether the thread that is going to stop another one was freezeable or not. Concern is whether we can somehow complete these wait_for_completion()'s in I am perfectly ok with it. But I am not sure if everybody would agree to have 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 be precise, I was thinking about something like this (in pseudo-code): kthread_stop_info.k = k; save_freezable_status(current); set_always_freezable(current); /* Now, we know that the freezer will wait for us, although we're not * really going to freeze */ task_lock(k); k->flags |= PF_NOFREEZE; if (frozen(k)) k->flags &= ~PF_FROZEN; task_unlock(k); wake_up_process(k); put_task_struct(k); /* Once it dies, reset stop ptr, gather result and we're done. */ wait_for_completion(&kthread_stop_info.done); Well, I'm not sure either, but I guess the most practical way to learn it is to send a patch. ;-) Greetings, Rafael -- If you don't have the time to read, you don't have the time or the tools to write. - Stephen King -
On Fri, 20 Apr 2007 17:56:09 +0530 OK by me. You might want to consider making that fields's locking protocol be set_bit(), clear_bit(), etc rather than task_lock(). -
is OK to me too, the extra field isnt a problem. Ingo -
OK, so I'll try to prepare a patch introducing it over the weekend. :-) Greetings, Rafael -
Hi, The following three patches are related to the separation of the freezer flags from process/threadinfo flags. The first patch separates the freezer from the PM code, because it's no longer a PM-specific piece of code. This also makes the second patch look better. The second patch introduces the freezer_flags field of task_struct, present only if the freezer is compiled in. All of the freezer-related flags per-task flags are moved to this field and auxiliary functions for operating them are defined in freezer.h . This overlaps with the Gautham's work to some extent, but I think it's better to introduce these changes independently of the CPU hotplug code. The third patch is a bonus. ;-) Greetings, Rafael -
From: Rafael J. Wysocki <rjw@sisk.pl>
Fix the problem with kthread_stop() that causes the freezer to fail if a
freezable thread is attempting to stop a frozen one and that may cause the
freezer to fail if the thread being stopped is freezable and
try_to_freeze_tasks() is running concurrently with kthread_stop().
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
kernel/kthread.c | 9 +++++++++
1 file changed, 9 insertions(+)
Index: linux-2.6.21-rc6-mm1/kernel/kthread.c
===================================================================
--- linux-2.6.21-rc6-mm1.orig/kernel/kthread.c 2007-04-09 15:23:48.000000000 +0200
+++ linux-2.6.21-rc6-mm1/kernel/kthread.c 2007-04-22 19:05:29.000000000 +0200
@@ -13,6 +13,7 @@
#include <linux/file.h>
#include <linux/module.h>
#include <linux/mutex.h>
+#include <linux/freezer.h>
#include <asm/semaphore.h>
/*
@@ -232,6 +233,14 @@ int kthread_stop(struct task_struct *k)
/* Now set kthread_should_stop() to true, and wake it up. */
kthread_stop_info.k = k;
+ if (!freezer_should_exempt(current)) {
+ /* We are freezable, so we must make sure that the thread being
+ * stopped is not frozen and will not be frozen until it dies
+ */
+ freezer_exempt(k);
+ if (frozen(k))
+ clear_frozen_flag(k);
+ }
wake_up_process(k);
put_task_struct(k);
-
Do we need to take some locks to access other process' flags? Or do frozen_exempt() etc take enough locks, already? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -
Hi, After the previous patch we only use set_bit(), clear_bit() and test_bit() to access freezer_falgs, so no special locking is needed to protect them. Greetings, Rafael -
I'm trying hard to convince myself that this will work. May be I am
missing something here, but I find a potential race window (very small though)
when k is entering the refrigerator.
Here's how.
kthread_stop(k) k->refrigerator()
---------------------------------------------------------------------
task_lock(k);
1) check_if_exempted();
/* not exempted. So
* we will freeze
* ourself.
*/
2) freezer_exempt(k);
3) if(frozen(k))
/* No, we're not yet frozen. So we
* don't clear_frozen_flag(k) here
*/
4) frozen_process(k);
task_unlock(k);
5) for(;;) {
set_current_state(UNINTERRUPTIBLE);
if(!frozen_process(k))
/* k is frozen. We
* don't break :(
*/
schedule();
Thus the freezer can still fail, 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!"
-
Well, probably I missed something, but why can't we do
if (!freezer_should_exempt(current)) {
freezer_exempt(k);
thaw_process(k);
}
?
thaw_process(k) is properly serialized with refrigerator(), and it checks
frozen(k). We can make an extra wake_up, but this should not matter.
Rafael, please check the recent changes in kthread.c, kthread_stop() was
reworked, we don't have kthread_stop_info any longer.
Oleg.
-
OK, I will, thanks. Greetings, Rafael -
Yes, that's correct. We need to take task_lock() to avoid the race with refrigerator(). I'll fix the patch. BTW, I think I should rediff the entire series against -mm with your patch from http://lkml.org/lkml/2007/4/23/98 applied. Greetings, Rafael -
Even if we use thaw_task() ? Even if I am wrong, I think we should not use task_lock() for the freezing related code, except in freezer.[ch] Note also that without CONFIG_FREEZER freezer_should_exempt() == 0, so we will do unneeded task_lock/task_unlock. Oleg. -
I don't think so. As you correctly pointed out, thaw_task() is race free 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!" -
From: Rafael J. Wysocki <rjw@sisk.pl> Move all of the freezer-related flags to a separate field in task_struct and introduce functions to operate them using set_bit() etc. Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> --- Documentation/power/kernel_threads.txt | 2 - Documentation/power/swsusp.txt | 6 +-- arch/i386/kernel/apm.c | 2 - drivers/block/loop.c | 2 - drivers/char/apm-emulation.c | 6 +-- drivers/ieee1394/ieee1394_core.c | 2 - drivers/md/md.c | 2 - drivers/mmc/card/queue.c | 3 + drivers/mtd/mtd_blkdevs.c | 3 + drivers/scsi/libsas/sas_scsi_host.c | 2 - drivers/scsi/scsi_error.c | 2 - drivers/usb/storage/usb.c | 2 - include/asm-arm/thread_info.h | 2 - include/asm-blackfin/thread_info.h | 2 - include/asm-frv/thread_info.h | 2 - include/asm-i386/thread_info.h | 2 - include/asm-ia64/thread_info.h | 2 - include/asm-mips/thread_info.h | 2 - include/asm-powerpc/thread_info.h | 2 - include/asm-sh/thread_info.h | 2 - include/asm-x86_64/thread_info.h | 2 - include/linux/freezer.h | 65 ++++++++++++++++++++++++++------- include/linux/sched.h | 8 ++-- kernel/fork.c | 2 - kernel/freezer.c | 2 - kernel/rcutorture.c | 4 +- kernel/sched.c | 2 - kernel/softirq.c | 2 - kernel/softlockup.c | 2 - kernel/workqueue.c | 2 - 30 files changed, 83 insertions(+), 58 deletions(-) Index: linux-2.6.21-rc6-mm1/include/linux/sched.h =================================================================== --- linux-2.6.21-rc6-mm1.orig/include/linux/sched.h 2007-04-22 19:37:42.000000000 +0200 +++ ...
It's getting time I learned what this freezer thing is.
What would you suggest I read?
I looked in include/linux/freezer.h and didn't see any explanations.
I found one Documenation file, power/kernel_threads.txt, that explained
the interaction of freezing and kernel threads. I looked in the
comments for various 2.6.21-rc6-mm1 freezer* patches, and saw various
interesting details.
But I couldn't find any documentation telling me what a freezer was,
or what a refrigerator is.
Did I miss something?
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <pj@sgi.com> 1.925.600.0401
-
Well, unfortunately not. The freezer is not separately documented, mainly because (1) for a long time it's been a part of the suspend code and no one else used it, and (2) because it's been changing a lot recently and it's been a 'moving target' from the documentation point of view. I'll try to explain how it works. In short, we use the freezer to make tasks enter a specific function, called the refrigerator, and stay there until they are let out. This is done by calling freeze_processes() (in 2.6.21-rc7 it is located in kernel/power/process.c) that executes try_to_freeze_tasks() separately for userland processes and kernel threads (the sync() in between is needed by the suspend code). try_to_freeze_tasks() sets the FREEZE flag (TIF_FREEZE in 2.6.21-rc7) for each freezable (I'll explain what that means in a while) task and sends a fake signal to it. Userland processes then go to get_signal_to_deliver(), where they execute try_to_freeze() (defined in include/linux/freezer.h) and call refrigerator() (in kernel/power/process.c), since the FREEZE flag is set. In refrigerator() they reset the FREEZE flag and set the FROZEN flag (in 2.6.21-rc7 it is a process flag defined in sched.h). Next, they loop in refrigerator() until someone resets the FROZEN flag for them. Then we say that they are 'frozen'. Kernel threads don't call get_signal_to_deliver(), so they have to execute try_to_freeze() directly to go to the refrigerator. Moreover, kernel threads may not want to go there at all, in which case they should set the NOFREEZE flag (in 2.6.21-rc7 it is a process flag defined in sched.h), that makes try_to_freeze_tasks() ignore them. Apart from the kernel threads that have the NOFREEZE flag set, try_to_freeze_tasks() ignores the task that's running it (its current task) and the tasks that have exit_state different from 0. They all are regarded as 'not freezable'. Of course, kernel threads that declare themselves as not freezable, by setting the NOFREEZE flag, should be ...
Ok - thanks. Good explanation of how it works.
One more question - why would I want to do this?
Is this like something that would be useful on a laptop, to suspend
activity and reduce battery drain, while preserving the current state
of ones sessions and avoiding having to logout or shutdown?
Or are there other good uses for this?
Is it useful for quietting a system down before doing hot plug or
unplug of key components, such as processors and memory?
Thanks.
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <pj@sgi.com> 1.925.600.0401
-
Yes, the original purpose for the inclusion of the freezer code was to support suspend-resume (mainly for laptops, but suspend-resume could Yes, the freezer is (proposed to be, at least) moving on from being merely a suspend-resume-only thing to other usage scenarios, such as kprobes and hotlpug. -
Aren't NOFREEZE and SKIP doing the same thing? One of them appears superfluous. I'm looking at 21-rc6-mm1 and vfork(2) seems to be its only user. Seeing how vfork(2) used it, can't the call to freezer_do_not_count() be replaced with a call to freezer_exempt()? Similarly, the freezer_count() after the wait_for_completion might just as well be a clear of the NOFREEZE bit followed by a try_to_freeze(). Could you please explain the rationale behind the SKIP flag? I do see that SKIP seems to be relevant for only userspace threads and presumably only kernel threads are allowed to set NOFREEZE, but why this distinction between the two? This definitely needs to be undo_freeze() or unfreeze(). frozen_process() sounds like what frozen() is supposed to do. This could instead be mark_task_frozen(), or even mark_frozen(), because only the current task can ever mark *itself* frozen before freezing These could be called freezer_skip() and freezer_do_not_skip(). Better to stick to consistent naming / terminology. Cheers, Satyam -
Hi Satyam, The difference between the NOFREEZE and the SKIP flag is a subtle one. When a task (say p) sets it's NOFREEZE flag, it tells the freezer not to consider it for freezing. Which means freezeable(p) will return 0. So the freezer will not even mark it for freezing. However, when a task sets it SKIP flag, it tells the freezer - "I might block at a safe place. So when you are counting the processes which have been marked as freezeable, but have not frozen yet, please don't count me in. IOW, please skip me." Thus such a task can still be marked for freezing. The typical usage is freezer_do_not_count(current); /* currents goes to an uninterruptible sleep, like wait_for_completion. */ freezer_count(current); Once the task wakes up from it's uninterruptible sleep, it will call freezer_count which in turn calls try_to_freeze. If the task was marked for freezing, it will be frozen now. You may want to check the thread http://lkml.org/lkml/2007/2/18/47 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!" -
Very well explained. Thanks! Greetings, Rafael -
Hi, Well, I agree that the names are not the best ones, but I'd like to avoid changing the existing names in this particular patch. We can change them later, on top of it. Greetings, Rafael -
-- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -
We may want to rename do_not_freeze to something else. It kind of looks weird calling do_not_freeze(p) after setting the frozen flag! 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!" -
Hmm, I wanted to avoid changing the existing names in this patch, but in this particular case you are obviously right. I'll change that to clear_freeze_flag(). Greetings, Rafael -
OK, but you forgot to clear p->freezer_flags on copy_process()->dup_task_struct() path ? Oleg. -
Yes, thanks for pointing that out. Should I clear it in dup_task_struct() or is there a better place? -
That reminds me, shouldn't we set the child's TFF_FREEZE flag if the parent 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!" -
Well, usually we'll want to freeze the child too in such cases, but I'm not sure if that's always true. Greetings, Rafael -
I personally think we should do this in dup_task_struct(). In fact, I believe it is better to replace the *tsk = *orig; with some helper (like setup_thread_stack() below), and that helper clears ->freezer_flags. Say, copy_task_struct(). Oleg. -
Hmm, wouldn't that be overkill? copy_task_struct() would have to do *tsk = *orig anyway, and we only need to clear one field apart from this. Some other fields are cleared towards the end of dup_task_struct(), so perhaps we could clear freezer_flags in there too? Rafael -
Yes. And I strongly believe it is bad we don't have the helper which does some random stuf like "p->did_exec = 0". The same for thread_info. Could you answer quickly where do we clear TIF_FREEZE currently? We don't. Oleg. -
Right. That said, I think the right thing to do would be to add just one line 'freezer_flags = 0' to dup_task_struct() directly in this patch and then move all of the random stuff (including this line) to the helper in a separate patch. I mean, the introduction of the helper seems to be logically unrelated to the other things this patch does, so it should go in another patch IMHO. Greetings, Rafael -
From: Rafael J. Wysocki <rjw@sisk.pl> Now that the freezer is used by kprobes, it is no longer a PM-specific piece of code. Move the freezer code out of kernel/power and introduce the CONFIG_FREEZER option that will be chosen automatically if PM or KPROBES is set. Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> --- arch/arm/Kconfig | 5 + arch/avr32/Kconfig.debug | 5 + arch/blackfin/Kconfig | 5 + arch/frv/Kconfig | 5 + arch/i386/Kconfig | 5 + arch/ia64/Kconfig | 5 + arch/mips/Kconfig | 5 + arch/powerpc/Kconfig | 5 + arch/ppc/Kconfig | 5 + arch/s390/Kconfig | 5 + arch/sh/Kconfig | 5 + arch/sparc64/Kconfig | 5 + arch/x86_64/Kconfig | 8 + include/linux/freezer.h | 2 kernel/Makefile | 1 kernel/freezer.c | 224 +++++++++++++++++++++++++++++++++++++++++++++++ kernel/kprobes.c | 2 kernel/power/Makefile | 2 kernel/power/process.c | 224 ----------------------------------------------- 19 files changed, 296 insertions(+), 227 deletions(-) Index: linux-2.6.21-rc6-mm1/arch/x86_64/Kconfig =================================================================== --- linux-2.6.21-rc6-mm1.orig/arch/x86_64/Kconfig 2007-04-22 14:14:44.000000000 +0200 +++ linux-2.6.21-rc6-mm1/arch/x86_64/Kconfig 2007-04-22 14:16:03.000000000 +0200 @@ -695,6 +695,14 @@ config GENERIC_PENDING_IRQ depends on GENERIC_HARDIRQS && SMP default y +# +# Use the tasks freezer +# +config FREEZER + bool + default y + depends on PM || KPROBES + menu "Power management options" source kernel/power/Kconfig Index: linux-2.6.21-rc6-mm1/arch/avr32/Kconfig.debug =================================================================== --- linux-2.6.21-rc6-mm1.orig/arch/avr32/Kconfig.debug 2007-04-22 14:14:44.000000000 +0200 +++ linux-2.6.21-rc6-mm1/arch/avr32/Kconfig.debug 2007-04-22 14:16:03.000000000 +0200 @@ -17,3 +17,8 @@ config ...
ACK. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -
No, we can't change k->flags, k owns its ->flags, and it is not atomic. Rafael, may I suggest you to document task_lock() in thaw_process() ? This looks really confusing, as if task_lock() protects "p->flags &= ~PF_FROZEN". Actually, task_lock() is needed to prevent the race with refrigerator() when the freezing fails, but this is not obvious. Oleg. -
Yes, but if we move PF_FROZEN to a separate field in task_struct with Sure, I will. Greetings, Rafael -
We can do that. Just that the freezer will now have to wait for 2 sets of ack's instead of 1 set before declaring the system as frozen. But the whole point of the patch was so that a thread A can tell a thread B that it's dependent on the latter, and hence would like to postpone B's freezing for some time. So I am thinking if we can Well, even then we'll need to ensure that a thread would not call kthread_stop_freezeable() with any locks held. That would mean more Ok. 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!" -
I'll take audits over 200 new lines of code ;-). (And it should be reasonably simple to audit; person doing kthread_stop() -> ktrhread_stop_freezeable() should look after locks). Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -
I think this can work if it is used only in kthread_stop(). But what if another task wants to do hold_freezer_for_task(p) ? freezer_status.count is recursive, but TIF_FREEZER_HELD is not. IOW, I believe this is not generic enough. Also, you are planning to add different freezing states (FE_HOTPLUG_CPU, FE_SUSPEND, etc). In that case each of them needs a separate .count, because it should be negative when try_to_freeze_tasks() returns. Now consider the case when we are doing freeze_processes(FE_A | FE_B) ... Oleg. -
This can race with hold_freezer_for_task() calling thaw_process. Earlier thaw_process(p) was called only after the process 'p' was frozen. Now with hold_freezer_for_task(), we can as well call thaw_process(p) when 'p' is in the freezing stage. Hence the task_lock. Yes. If more than one tasks want another task to be temporarily thawed, this So can't we in that case find out the weight of the freeze_event variable and Thanks for the review. 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!" -
hold_freezer_for_task()->thaw_process(p) will wake up the task. Or the caller of refrigerator will notice "!frozen()". Note that refrigerator() sets PF_FROZEN under task_lock(). In fact we have the same issue when thaw_tasks()->thaw_process(p) happens Probably yes... but if we are speaking about kthrad_stop() only, this could be afaics solved in more simple way, as Rafael suggests. Oleg. -
I agree on this one. kthread_stop appears to be the only valid place where such a correction is required. So Rafael's approach makes sense 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!" -
