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@xxxxxxx>
>
> 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@xxxxxxx>
> ---
Well, s/try_to_freeze()/try_to_freeze_tasks()/ all in the changelog above.
Greetings,
Rafael
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
