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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
