Re: 2.6.22-rc1: Broken suspend on SMP with tifm

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

On Sunday, 13 May 2007 22:08, Oleg Nesterov wrote:
> On 05/13, Rafael J. Wysocki wrote:
> > 
> > The suspend/hibernation is broken on SMP due to:
> > 
> > commit 3540af8ffddcdbc7573451ac0b5cd57a2eaf8af5
> > tifm: replace per-adapter kthread with freezeable workqueue
> > 
> > Well, it looks like freezable worqueues still deadlock with CPU hotplug
> > when worker threads are frozen.
> 
> Ugh. I thought we deprecated create_freezeable_workqueue(), exactly
> because suspend was changed to call _cpu_down() after freeze().

Well, apparently no one has told it to Alex ...

> It is not that "looks like freezable worqueues still deadlock", it
> is "of course, freezable worqueues deadlocks" on CPU_DEAD.
> 
> The ->freezeable is still here just because of incoming "cpu-hotplug
> using freezer" rework.
> 
> No?

Yes, but we failed to communicate that to the others clearly enough.

> > --- linux-2.6.22-rc1.orig/kernel/workqueue.c
> > +++ linux-2.6.22-rc1/kernel/workqueue.c
> > @@ -799,9 +799,7 @@ static int __devinit workqueue_cpu_callb
> >  	struct cpu_workqueue_struct *cwq;
> >  	struct workqueue_struct *wq;
> >  
> > -	action &= ~CPU_TASKS_FROZEN;
> > -
> > -	switch (action) {
> > +	switch (action & ~CPU_TASKS_FROZEN) {
> 
> Confused. How can we see, say CPU_UP_PREPARE_FROZEN, if we cleared
> CPU_TASKS_FROZEN bit?

There's another 'switch ()' in there where the flag is not cleared
(that's why I removed the 'action &= ~CPU_TASKS_FROZEN' above).

> >  	case CPU_LOCK_ACQUIRE:
> >  		mutex_lock(&workqueue_mutex);
> >  		return NOTIFY_OK;
> > @@ -819,20 +817,29 @@ static int __devinit workqueue_cpu_callb
> >  
> >  		switch (action) {
> >  		case CPU_UP_PREPARE:
> > +		case CPU_UP_PREPARE_FROZEN:
> >  			if (!create_workqueue_thread(cwq, cpu))
> >  				break;
> >  			printk(KERN_ERR "workqueue for %i failed\n", cpu);
> >  			return NOTIFY_BAD;
> >  
> >  		case CPU_ONLINE:
> > +		case CPU_ONLINE_FROZEN:
> >  			start_workqueue_thread(cwq, cpu);
> >  			break;
> >  
> >  		case CPU_UP_CANCELED:
> > +		case CPU_UP_CANCELED_FROZEN:
> >  			start_workqueue_thread(cwq, -1);
> >  		case CPU_DEAD:
> >  			cleanup_workqueue_thread(cwq, cpu);
> >  			break;
> > +
> > +		case CPU_DEAD_FROZEN:
> > +			if (wq->freezeable)
> > +				thaw_process(cwq->thread);
> > +			cleanup_workqueue_thread(cwq, cpu);
> > +			break;
> >  		}
> >  	}
> 
> Minor, but can't we do
> 
> 		...
> 		case CPU_UP_CANCELED:
> 		case CPU_UP_CANCELED_FROZEN:
> 			start_workqueue_thread(cwq, -1);
> 		case CPU_DEAD_FROZEN:
> 			if (wq->freezeable)
> 				// we can't see PF_FROZEN if it was CPU_UP_CANCELED
> 				thaw_process(cwq->thread);
> 		case CPU_DEAD:
> 			cleanup_workqueue_thread(cwq, cpu);
> 			break;
> 
> ?

Yes, we can, but that means one redundant check more in the CPU_UP_CANCELLED
path.  Besides, I prefer having different cases clearly separated if that makes
sense.

Greetings,
Rafael