Re: Problem with freezable workqueues

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

On 02/27, Rafael J. Wysocki wrote:
>
> We have a problem with freezable workqueues in 2.6.21-rc1 and in -mm
> (there are only two of them, in XFS, but still).  Namely, their worker threads
> deadlock with workqueue_cpu_callback() that gets called during the CPU hotplug,
> becuase workqueue_cpu_callback() tries to stop these threads while they are
> frozen (disable_nonboot_cpus() happens after we've frozen tasks).

Ugh. I know nothing, nothing, nothing about suspend. I'll try to guess.

   Commit: ed746e3b18f4df18afa3763155972c5835f284c5

   [PATCH] swsusp: Change code ordering in disk.c

   Change the ordering of code in kernel/power/disk.c so that device_suspend() is
   called before disable_nonboot_cpus() and platform_finish() is called after
   enable_nonboot_cpus() and before device_resume(), as indicated by the recent
   discussion on Linux-PM (cf.
   http://lists.osdl.org/pipermail/linux-pm/2006-November/004164.html).

   The changes here only affect the built-in swsusp.

Yes? with the patch above, _cpu_down() called _after_ freeze_processes() ???
Honestly, I can't understand this (yes, I know nothing, nothing, nothing...).

> For 2.6.21-rc1 I've invented the appended workaround (works for me, waiting for
> Johannes to confirm it works for him too), but I think we need something better
> for -mm and future kernels.

How about other kthread_stop()s ? For example, kernel/softirq.c:cpu_callback() ?

I think we need a general "cpu_down() after freeze" implementation, this is what
Gautham and Srivatsa are working on, right?

> --- linux-2.6.21-rc1.orig/kernel/workqueue.c	2007-02-24 10:17:57.000000000 +0100
> +++ linux-2.6.21-rc1/kernel/workqueue.c	2007-02-24 20:00:22.000000000 +0100
> @@ -376,8 +376,19 @@ static int worker_thread(void *__cwq)
>  
>  	set_current_state(TASK_INTERRUPTIBLE);
>  	while (!kthread_should_stop()) {
> -		if (cwq->freezeable)
> -			try_to_freeze();
> +		if (try_to_freeze()) {
> +			/* We've just left the refrigerator.  If our CPU is
> +			 * a nonboot one, we might have been replaced.
> +			 * The lock is taken to prevent the race with
> +			 * cleanup_workqueue_thread() from happening
> +			 */
> +			spin_lock_irq(&cwq->lock);

I'm afraid this is racy. We can't touch *cwq, it may be freed. Suppose
that another thread does destroy_workqueue(), and we thaw that thread
before cwq->thread.

Oleg.