Re: [RFC PATCH(experimental) 2/2] Fix freezer-kthread_stop race

Rafael J. Wysocki
2007-04-20T12:46:14Z

On Friday, 20 April 2007 14:26, Gautham R Shenoy wrote:
> On Fri, Apr 20, 2007 at 01:59:29PM +0200, Rafael J. Wysocki wrote:
> >
> > 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
> > should make the freezer finish.
>
> 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
> the freezing context and reduce the probabilty of freezer failing.

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);
restore_freezable_status(current);

> > > 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.
> >
> > Well, I think it might be simpler to consider each case separately. This way
> > we may be able to avoid introducing the additional TIF_ flag.
> >
>
> Makes sense.
>
> > > 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.
> > > Unless I can spot any other valid case :)
> >
> > 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 ...
>
> I am perfectly ok with it. But I am not sure if everybody would agree to have
> another field in the task struct, though in this case it does make sense :-)

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