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

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Thursday, 26 July 2007 14:24, Rafael J. Wysocki wrote:
> On Wednesday, 25 July 2007 16:24, Oleg Nesterov wrote:
> > 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().
> 
> OK, I see your point.  The updated patch is appended.
> 
> > (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).
> 
> Hmm, yes, I think so.
> 
> Greetings,
> Rafael
> 
> 
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Use the observation that try_to_freeze() need not loop while waiting for the
> freezing tasks to enter the refrigerator and make it use a wait queue.
> 
> The idea is that after sending freeze requests to the tasks regarded as
> freezable try_to_freeze() can go to sleep and wait until at least one task
> enters the refrigerator.  The first task that does it wakes up try_to_freeze()
> and the procedure is repeated.  If the refrigerator is not entered by any tasks
> before TIMEOUT expires, try_to_freeze() increases the counter of expired
> timeouts and sends freeze requests to the remaining tasks.  If the number of
> expired timeouts becomes greater than MAX_WAITS, the freezing of tasks fails
> (the counter of expired timeouts is reset whenever a task enters the
> refrigerator).
> 
> This way, try_to_freeze() doesn't occupy the CPU unnecessarily when some
> freezing tasks are waiting for I/O to complete and we have more fine grained
> control over the freezing procedure.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---

Well, s/try_to_freeze()/try_to_freeze_tasks()/ all in the changelog above.

Greetings,
Rafael