Hi, On Sunday, 26 November 2006 12:15, Rafael J. Wysocki wrote: > On Sunday, 26 November 2006 11:02, Rafael J. Wysocki wrote: > > On Sunday, 26 November 2006 08:47, Pavel Machek wrote: <--snip--> > > Okay, I'll use atomic_t. > > Patch with atomic_t follows. > > The atomic_set(..., 0) are used to avoid the (theoretical) situation in which > freezing might be decreased twice in a row and I've decided to explicitly > initialize freezing in fork.c for clarity. I've just realized that there is a race in freeze_process() where we should check if the proces hasn't been frozen already (the process may have entered the refrigerator and reset 'freezing' after we checked its PF_FROZEN flag, so we have to check if PF_FROZEN is set for the process before we set 'freezing' for it). Since an explicit memory barrier is needed here, it seems reasonable to add one in frozen_process() as well. Also, it seems to me that stopped processes require special treatment. Namely, one or more of them might receive the continuation signal after we check their states in try_to_freeze_tasks() so we should make sure this hasn't happened before we break the main loop in there. Revised patch follows. Greetings, Rafael Signed-off-by: Rafael J. Wysocki <rjw at sisk.pl> --- include/linux/freezer.h | 11 ++++++----- include/linux/sched.h | 4 +++- kernel/fork.c | 4 ++++ kernel/power/process.c | 36 +++++++++++++++++++++++++++++------- 4 files changed, 42 insertions(+), 13 deletions(-) Index: linux-2.6.19-rc6-mm1/include/linux/freezer.h =================================================================== --- linux-2.6.19-rc6-mm1.orig/include/linux/freezer.h 2006-11-26 11:31:39.000000000 +0100 +++ linux-2.6.19-rc6-mm1/include/linux/freezer.h 2006-11-26 14:07:03.000000000 +0100 @@ -14,16 +14,15 @@ static inline int frozen(struct task_str */ static inline int freezing(struct task_struct *p) { - return p->flags & PF_FREEZE; + return !!atomic_read(&p->freezing); } /* * Request that a process be frozen - * FIXME: SMP problem. We may not modify other process' flags! */ static inline void freeze(struct task_struct *p) { - p->flags |= PF_FREEZE; + atomic_inc(&p->freezing); } /* @@ -31,7 +30,7 @@ static inline void freeze(struct task_st */ static inline void do_not_freeze(struct task_struct *p) { - p->flags &= ~PF_FREEZE; + atomic_set(&p->freezing, 0); } /* @@ -52,7 +51,9 @@ static inline int thaw_process(struct ta */ static inline void frozen_process(struct task_struct *p) { - p->flags = (p->flags & ~PF_FREEZE) | PF_FROZEN; + p->flags |= PF_FROZEN; + wmb(); + atomic_set(&p->freezing, 0); } extern void refrigerator(void); Index: linux-2.6.19-rc6-mm1/include/linux/sched.h =================================================================== --- linux-2.6.19-rc6-mm1.orig/include/linux/sched.h 2006-11-26 11:31:39.000000000 +0100 +++ linux-2.6.19-rc6-mm1/include/linux/sched.h 2006-11-26 11:33:29.000000000 +0100 @@ -1065,6 +1065,9 @@ struct task_struct { #ifdef CONFIG_TASK_DELAY_ACCT struct task_delay_info *delays; #endif +#ifdef CONFIG_PM + atomic_t freezing; /* if set, we should be freezing for suspend */ +#endif #ifdef CONFIG_FAULT_INJECTION int make_it_fail; #endif @@ -1161,7 +1164,6 @@ static inline void put_task_struct(struc #define PF_MEMALLOC 0x00000800 /* Allocating memory */ #define PF_FLUSHER 0x00001000 /* responsible for disk writeback */ #define PF_USED_MATH 0x00002000 /* if unset the fpu must be initialized before use */ -#define PF_FREEZE 0x00004000 /* this task is being frozen for suspend now */ #define PF_NOFREEZE 0x00008000 /* this thread should not be frozen */ #define PF_FROZEN 0x00010000 /* frozen for system suspend */ #define PF_FSTRANS 0x00020000 /* inside a filesystem transaction */ Index: linux-2.6.19-rc6-mm1/kernel/fork.c =================================================================== --- linux-2.6.19-rc6-mm1.orig/kernel/fork.c 2006-11-25 21:26:52.000000000 +0100 +++ linux-2.6.19-rc6-mm1/kernel/fork.c 2006-11-26 11:45:00.000000000 +0100 @@ -1097,6 +1097,10 @@ static struct task_struct *copy_process( p->blocked_on = NULL; /* not blocked yet */ #endif +#ifdef CONFIG_PM + atomic_set(&p->freezing, 0); +#endif + p->tgid = p->pid; if (clone_flags & CLONE_THREAD) p->tgid = current->tgid; Index: linux-2.6.19-rc6-mm1/kernel/power/process.c =================================================================== --- linux-2.6.19-rc6-mm1.orig/kernel/power/process.c 2006-11-25 21:26:52.000000000 +0100 +++ linux-2.6.19-rc6-mm1/kernel/power/process.c 2006-11-26 14:17:11.000000000 +0100 @@ -28,8 +28,7 @@ static inline int freezeable(struct task if ((p == current) || (p->flags & PF_NOFREEZE) || (p->exit_state == EXIT_ZOMBIE) || - (p->exit_state == EXIT_DEAD) || - (p->state == TASK_STOPPED)) + (p->exit_state == EXIT_DEAD)) return 0; return 1; } @@ -61,10 +60,13 @@ static inline void freeze_process(struct unsigned long flags; if (!freezing(p)) { - freeze(p); - spin_lock_irqsave(&p->sighand->siglock, flags); - signal_wake_up(p, 0); - spin_unlock_irqrestore(&p->sighand->siglock, flags); + rmb(); + if (!frozen(p)) { + freeze(p); + spin_lock_irqsave(&p->sighand->siglock, flags); + signal_wake_up(p, 0); + spin_unlock_irqrestore(&p->sighand->siglock, flags); + } } } @@ -90,11 +92,12 @@ static unsigned int try_to_freeze_tasks( { struct task_struct *g, *p; unsigned long end_time; - unsigned int todo; + unsigned int todo, nr_stopped; end_time = jiffies + TIMEOUT; do { todo = 0; + nr_stopped = 0; read_lock(&tasklist_lock); do_each_thread(g, p) { if (!freezeable(p)) @@ -103,6 +106,10 @@ static unsigned int try_to_freeze_tasks( if (frozen(p)) continue; + if (p->state == TASK_STOPPED) { + nr_stopped++; + continue; + } if (p->state == TASK_TRACED && (frozen(p->parent) || p->parent->state == TASK_STOPPED)) { @@ -128,6 +135,21 @@ static unsigned int try_to_freeze_tasks( } while_each_thread(g, p); read_unlock(&tasklist_lock); yield(); /* Yield is okay here */ + if (!todo) { + /* Make sure that none of the stopped processes has + * received the continuation signal after we checked + * last time. + */ + todo = nr_stopped; + nr_stopped = 0; + read_lock(&tasklist_lock); + do_each_thread(g, p) { + if (p->state == TASK_STOPPED) + nr_stopped++; + } while_each_thread(g, p); + read_unlock(&tasklist_lock); + todo -= nr_stopped; + } if (todo && time_after(jiffies, end_time)) break; } while (todo);