Messages in this thread Patch in this message | | | Date | Wed, 24 Aug 2011 15:34:12 -0700 | From | Matt Helsley <> | Subject | Re: [PATCH 06/16] freezer: make exiting tasks properly unfreezable |
| |
On Fri, Aug 19, 2011 at 04:16:12PM +0200, Tejun Heo wrote: > There's no point in freezing an exiting task. The current code > seemingly tries that by calling clear_freeze_flag() from exit_mm() but > it's racy as freeze might happen afterwards. > > This patch removes the racy clear_freeze_flag() makes do_exit() set > PF_NOFREEZE after PTRACE_EVENT_EXIT, after which freezing doesn't make > sense. > > Signed-off-by: Tejun Heo <tj@kernel.org> > --- > kernel/exit.c | 8 ++++++-- > kernel/power/process.c | 3 +-- > 2 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/kernel/exit.c b/kernel/exit.c > index 2913b35..ac58259 100644 > --- a/kernel/exit.c > +++ b/kernel/exit.c > @@ -679,8 +679,6 @@ static void exit_mm(struct task_struct * tsk) > tsk->mm = NULL; > up_read(&mm->mmap_sem); > enter_lazy_tlb(mm, current); > - /* We don't want this task to be frozen prematurely */ > - clear_freeze_flag(tsk);
This patch doesn't look quite right. Perhaps that's because I'm unclear why any of the freezer flags matter in the do_exit() path. How can the task enter the refrigerator in do_exit()? Isn't it true that we don't enter the syscall return path and thus can't freeze anyway (the last thing it should do is schedule())? Or is this another kthread quirk?
If we can't enter the refrigerator then why fiddle with these flags here at all? That might explain why this race never introduced problems.
> if (tsk->signal->oom_score_adj == OOM_SCORE_ADJ_MIN) > atomic_dec(&mm->oom_disable_count); > task_unlock(tsk); > @@ -915,6 +913,12 @@ NORET_TYPE void do_exit(long code) > > ptrace_event(PTRACE_EVENT_EXIT, code); > > + /* > + * With ptrace notification done, there's no point in freezing from > + * here on. Disallow freezing. > + */ > + current->flags |= PF_NOFREEZE; > + > validate_creds_for_do_exit(tsk);
OK, now I'm assuming my reasoning above is wrong; that it's possible to enter the refrigerator in do_exit().
Couldn't TIF_FREEZE already be set? Setting PF_NOFREEZE only ensures the flag won't get set "after" this point. To fix this I think we'd need something more like:
current->flags |= PF_NOFREEZE; smp_wmb(); clear_freeze_flag(tsk);
Otherwise TIF_FREEZE might not get cleared and, despite PF_NOFREEZE, we could somehow enter the refrigerator. Also, I'm not sure the smp_wmb() is sufficient since it will only affect the order the writes appear and does not affect whether other cpus will see PF_NOFREEZE before we clear the TIF_FREEZE flag. Perhaps we need task_lock() here.
> > /* > diff --git a/kernel/power/process.c b/kernel/power/process.c > index bec09c3..ddfaba4 100644 > --- a/kernel/power/process.c > +++ b/kernel/power/process.c > @@ -25,8 +25,7 @@ > static inline int freezable(struct task_struct * p) > { > if ((p == current) || > - (p->flags & PF_NOFREEZE) || > - (p->exit_state != 0)) > + (p->flags & PF_NOFREEZE))
This patch also fiddles with the timing of adjusting or checking three different pieces of task state:
exit_state flags (PF_NOFREEZE) thread flags (TIF_FREEZE)
each of which get set/cleared at different times from the new line adding PF_NOFREEZE to the flags. So I'm a little nervous that we might be missing some important details.
Cheers, -Matt Helsley
| |