Re: [RFC PATCH(Experimental) 0/4] Freezer based Cpu-hotplug

[Oleg Nesterov]

On 02/17, Rafael J. Wysocki wrote:
>
> On Saturday, 17 February 2007 22:34, Oleg Nesterov wrote:
> > 
> > 	static inline int is_user_space(struct task_struct *p)
> > 	{
> > 		return p->mm && !(p->flags & PF_BORROWED_MM);
> > 	}
> > 
> > This doesn't look right. First, an exiting task has ->mm == NULL after
> > do_exit()->exit_mm(). Probably not a problem. However, PF_BORROWED_MM
> > check is racy without task_lock(), so we can have a false positive as
> > well. Is it ok? We can freeze aio_wq prematurely.
> 
> Right now aio_wq is not freezeable (PF_NOFREEZE).

Right now yes, but we are going to change this?

> > 				cancel_freezing(p);
> > 				continue;
> > 
> > Is it right? Shouldn't we increment "todo" counter?
> 
> No.  It would be wrong to do that, because TASK_TRACED tasks with frozen
> parents cannot be frozen any further.

TASK_TRACED task could be woken by SIGKILL. cancel_freezing() clears TIF_FREEZE.
The task may start do_exit() when try_to_freeze_tasks() returns "success".
Probably not a problem.

> > 				if (!p->vfork_done)
> > 					freeze_process(p);
> > 
> > 
> > Racy. do_fork(CLONE_VFORK) first does copy_process() which puts 'p' on
> > the task list and unlocks tasklist_lock. This means that 'p' is visible
> > to try_to_freeze_tasks(), and p->vfork_done == NULL. try_to_freeze_tasks()
> > sets TIF_FREEZE.
> > 
> > Now, do_fork() continues, sets ->vfork_done, p goes to user space, notices
> > the fake signal and goes to refrigerator while its parent is blocked on
> > "struct completion vfork". Freezing failed.
> 
> You are right, but this has never happened, AFAICS.
> 
> > So, shouldn't we do
> > 
> > 	if (p->vfork_done)
> > 		cancel_freezing(p);
> > 
> > instead?
> 
> I don't think so.  If p hasn't got TIF_FREEZE set yet or it has already been
> frozen, cancel_freezing(p) is a noop.

Yes, I misread cancel_freezing(), it doesn't wake up the task if it is frozen.

> Alternatively, we can move the check into refrigerator(), like this:
> 
> --- linux-2.6.20-git13.orig/kernel/power/process.c
> +++ linux-2.6.20-git13/kernel/power/process.c
> @@ -39,6 +39,11 @@ void refrigerator(void)
>  	/* Hmm, should we be allowed to suspend when there are realtime
>  	   processes around? */
>  	long save;
> +
> +	/* Freeze the task unless there is a vfork completion pending */
> +	if (current->vfork_done)
> +		return;
> +

This means that "current" returns to user space (get_signal_to_deliver
will clear TIF_SIGPENDING) and runs. While try_to_freeze_tasks() thinks
it is frozen.

Oleg.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/