Re: [RFC][PATCH -mm 2/2] Freezer: Use wait queue instead of busy looping

[Oleg Nesterov <oleg@tv-sign.ru>]

On 07/25, Rafael J. Wysocki wrote:
>
> On Wednesday, 25 July 2007 15:29, Oleg Nesterov wrote:
> > On 07/25, Rafael J. Wysocki wrote:
> > >
> > >  void refrigerator(void)
> > >  {
> > > @@ -50,6 +73,9 @@ void refrigerator(void)
> > >  	   processes around? */
> > >  	long save;
> > >
> > > +	refrigerator_called = 1;
> > > +	wake_up(&refrigerator_waitq);
> > > +
> > 
> > This is a bit racy. Unless I missed something, the task should not set
> > refrigerator_called == 1 until it has PF_FROZEN.
> 
> No, it's just to signal that the task has entered the refrigerator, not that
> it has actually frozen.

Yes, I see.

> > Otherwise, try_to_freeze_tasks() can set refrigerator_called == 0 after
> > refrigerator() sets it == 1, the the main loop notices this unfrozen task,
> > and goes to sleep.
> 
> refrigerator_called is only reset after try_to_freeze_tasks() has found it
> equal to one.  There is only a small window between checking it in
> wait_event_timeout() and resetting it,

Yes.

> but then we go to send freeze requests
> to the remaining tasks and we count 'todo' from the start, so that shouldn't
> be a problem.

... and we find the task which is not frozen() yet, but which has already passed
the "set condition and wakeup", increment todo, and wait for the event. If it was
the last task, we will sleep until timeout.

I agree, this is not fatal and unlikely, but still it is a race. I think it is
better to move this code down, after frozen_process().

(offtopic: strictly speaking, we don't even need the "refrigerator_called", we
 only need the wait_queue_head_t. try_to_freeze_tasks() just adds the "current"
 to wq at the very start of the main loop).

Oleg.