Hi,
On Sunday, 26 November 2006 11:02, Rafael J. Wysocki wrote:
> On Sunday, 26 November 2006 08:47, Pavel Machek wrote:
> > Hi!
> >
> > > Currently, the PF_FREEZE process flag is used to indicate that the process
> > > should enter the refrigerator as soon as possible. Unfortunately it is set by
> > > the freezer while the process may be changing its flags for another reason
> > > and this may lead to a race between the freezer and the process itself.
> > >
> > > This problem may be solved by introducing an additional member, called (for
> > > example) 'freezing', into task_struct which will only be used to indicate that
> > > the process should enter the refrigerator. Then, if the 'freezing' member of
> > > task_struct is reset by the process itself only after it has entered the
> > > refrigerator, the modifications of it will be guaranteed to occur at different
> > > times, because the freezer can only set it before the process enters the
> > > refrigerator. Thus the code will be SMP-safe even though no explicit locking
> > > is used.
> >
> > I do not think we can go without locking here.
>
> Why exactly?
>
> > > @@ -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;
> > > + p->freezing = 0;
> > > }
> > >
> > > /*
> > > @@ -52,7 +51,8 @@ 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;
> > > + p->freezing = 0;
> > > }
> >
> > Is mb() needed between |= and freezing = 0?
>
> Hm, I'm not sure. Are there architectures on which memory writes can be
> reordered?
>
> > > 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
> > > +++ linux-2.6.19-rc6-mm1/include/linux/sched.h
> > > @@ -1065,6 +1065,9 @@ struct task_struct {
> > > #ifdef CONFIG_TASK_DELAY_ACCT
> > > struct task_delay_info *delays;
> > > #endif
> > > +#ifdef CONFIG_PM
> > > + int freezing; /* if set, we should be freezing for suspend */
> > > +#endif
> > > #ifdef CONFIG_FAULT_INJECTION
> > > int make_it_fail;
> > > #endif
> >
> > It is int, imagine machine that can't do 32-bit atomic access (only
> > does 64 bits). On such beast (alpha? something stranger?) this will
> > clobber make_it_fail field, sometimes.
> >
> > OTOH on i386 normal instructions can be used. But that's okay, we
> > should just use atomic_t here. Should be as fast on i386/x86-64, and
> > still safe.
>
> 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.
Greetings,
Rafael
Signed-off-by: Rafael J. Wysocki <rjw at sisk.pl>
---
include/linux/freezer.h | 10 +++++-----
include/linux/sched.h | 4 +++-
kernel/fork.c | 4 ++++
3 files changed, 12 insertions(+), 6 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 11:52:29.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,8 @@ 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;
+ 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;
