lkml.org 
[lkml]   [2007]   [May]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: 2.6.22-rc1: Broken suspend on SMP with tifm
    Date
    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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-05-13 22:31    [W:0.033 / U:2.948 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site