Re: [RFC][PATCH -mm][Experimental] swsusp: freeze userspace processes first

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

Hi,

On Wednesday 01 February 2006 12:47, Pavel Machek wrote:
> > This is an experimantal patch aimed at the "unable to freeze processes under
> > load" problem.
> > 
> > On my box the 2.6.16-rc1-mm4 kernel with this patch applied survives the
> > "dd if=/dev/hda of=/dev/null" test.
> > 
> > Please have a look.
> 
> It makes it better (well, I used my own, simpler variant, but that
> should not matter; patch is attached). I now can't reproduce hangs
> with simple stress testing,

This means the direction is right ...

> but running kernel make alongside that 
> makes it hang sometimes.

... but there still is a race here.

> Example of non-frozen gcc: 
> 
> gcc           D EEE06A70     0  1750   1749  1751
> (NOTLB)
> df85df38 00000046 bf878130 eee06a70 00004111 eee06a70 eee06a70
> 003d0900
>        00000000 c0137cf5 df85c000 00000000 c058ada2 c012503e ef2c915c
> ef2c9030
>        c1c0b480 7c3b8500 003d0927 df85c000 00000a98 7c3b8500 003d0927
> c0770800
> Call Trace:
>  [<c0137cf5>] attach_pid+0x25/0xb0
>  [<c058ada2>] _write_unlock_irq+0x12/0x30
>  [<c012503e>] copy_process+0xe5e/0x11b0
>  [<c0588f74>] wait_for_completion+0x94/0xd0
>  [<c0121690>] default_wake_function+0x0/0x10
>  [<c01254d9>] do_fork+0x149/0x210
>  [<c0101218>] sys_vfork+0x28/0x30
>  [<c0103231>] syscall_call+0x7/0xb

[I'm trying to understand this trace, but with not much success, so far.
Apparently sys_vfork() calls do_fork() which calls copy_process(),
which calls attach_pid().  But what default_wake_function() and
wait_for_completion() are doing here?  And where it got stuck, actually?]

I think the problem is related to the kernel make using fork() heavily.
Namely, if the parent process uses CLONE_VFORK, it will wait for the
vfork completion.  Now if the child process is frozen _before_
it completes the vfork completion, the parent will be unfreezeable.

> ...maybe solving this would solve journalling problems, too? It is
> similar AFAICT.

There's a chance, I think. ;-)

> diff --git a/kernel/power/disk.c b/kernel/power/disk.c
> index e24446f..90d6c1a 100644
> --- a/kernel/power/disk.c
> +++ b/kernel/power/disk.c
> @@ -87,7 +87,6 @@ static int prepare_processes(void)
>  	int error;
>  
>  	pm_prepare_console();
> -	sys_sync();
>  	disable_nonboot_cpus();
>  
>  	if (freeze_processes()) {
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index 02a1b3a..bd16e44 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -16,7 +16,7 @@
>  /* 
>   * Timeout for stopping processes
>   */
> -#define TIMEOUT	(6 * HZ)
> +#define TIMEOUT	(60 * HZ)
>  
>  
>  static inline int freezeable(struct task_struct * p)
> @@ -54,32 +54,53 @@ void refrigerator(void)
>  	current->state = save;
>  }
>  
> +static void freeze_process(struct task_struct *p)
> +{
> +       unsigned long flags;
> +
> +       freeze(p);
> +       spin_lock_irqsave(&p->sighand->siglock, flags);
> +       signal_wake_up(p, 0);
> +       spin_unlock_irqrestore(&p->sighand->siglock, flags);
> +}
> +
>  /* 0 = success, else # of processes that we failed to stop */
>  int freeze_processes(void)
>  {
> -	int todo;
> +	int todo, user_frozen, nr_user;
>  	unsigned long start_time;
>  	struct task_struct *g, *p;
>  	unsigned long flags;
>  
>  	printk( "Stopping tasks: " );
>  	start_time = jiffies;
> +	user_frozen = 0;
>  	do {
> -		todo = 0;
> +		nr_user = todo = 0;
>  		read_lock(&tasklist_lock);
>  		do_each_thread(g, p) {
>  			if (!freezeable(p))
>  				continue;
>  			if (frozen(p))
>  				continue;
> -
> -			freeze(p);
> -			spin_lock_irqsave(&p->sighand->siglock, flags);
> -			signal_wake_up(p, 0);
> -			spin_unlock_irqrestore(&p->sighand->siglock, flags);
> -			todo++;
> +			if (p->mm) {

This one is too simplistic, it appears.  We should lock the task to check its mm
and there may be kernel threads with non-null mm (they have the
PF_BORROWED_MM flag set though, so we have to check this too).

> +				/* The task is a user-space one. Freeze it */
> +				freeze_process(p);
> +				todo++;

I think you can drop this ...

> +				nr_user++;
> +			} else {
> +				/* Freeze only if the user space is frozen */
> +				if (user_frozen)
> +					freeze_process(p);
> +				todo++;
> +			}
>  		} while_each_thread(g, p);
>  		read_unlock(&tasklist_lock);

... if you do

 +		todo += nr_user;
> +		if (!user_frozen && !nr_user) {
> +			printk("sync");
> +			sys_sync();
> +		}
> +		user_frozen = !nr_user;
>  		yield();			/* Yield is okay here */
>  		if (todo && time_after(jiffies, start_time + TIMEOUT)) {
>  			printk( "\n" );
}-- debugging stuff snipped --{

Greetings,
Rafael