Hi,
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).
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.
Greetings,
Rafael
kernel/workqueue.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
Index: linux-2.6.21-rc1/kernel/workqueue.c
===================================================================
--- 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);
+ if (cwq->thread != current) {
+ spin_unlock_irq(&cwq->lock);
+ break;
+ }
+ spin_unlock_irq(&cwq->lock);
+ }
add_wait_queue(&cwq->more_work, &wait);
if (list_empty(&cwq->worklist))
@@ -539,6 +550,9 @@ static void cleanup_workqueue_thread(str
cwq = per_cpu_ptr(wq->cpu_wq, cpu);
spin_lock_irqsave(&cwq->lock, flags);
p = cwq->thread;
+ if (p && frozen(p))
+ p = NULL;
+
cwq->thread = NULL;
spin_unlock_irqrestore(&cwq->lock, flags);
if (p)
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.
On Wed, 2007-02-28 at 02:28 +0300, Oleg Nesterov wrote:
> Ugh. I know nothing, nothing, nothing about suspend. I'll try to guess.
>
> Commit: ed746e3b18f4df18afa3763155972c5835f284c5
> Yes? with the patch above, _cpu_down() called _after_ freeze_processes() ???
perfect :)
See also my original mail and the thread:
http://thread.gmane.org/gmane.linux.power-management.general/4314
> How about other kthread_stop()s ? For example, kernel/softirq.c:cpu_callback() ?
I'd they should be affected as well. For me, I guess I haven't run into
them because xfs is enough to freeze the box ;) As for Rafael, I don't
know why he isn't seeing that... I haven't been able to test this patch
on my quad powermac so far unfortunately, been sick and away from the
machine. I'll probably be able to test tomorrow.
> I think we need a general "cpu_down() after freeze" implementation,
Yeah, I'd think so.
> this is what
> Gautham and Srivatsa are working on, right?
That I don't know.
johannes
On Wednesday, 28 February 2007 00:28, Oleg Nesterov wrote:
> 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
Yes, but not only this one. Please see:
http://kernel.org/git/gitweb.cgi?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=e3c7db621bed4afb8e231cb005057f2feb5db557
> [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() ???
Yes, that's what we want to do.
> Honestly, I can't understand this (yes, I know nothing, nothing, nothing...).
Well, we used the CPU hotplug for disabling nonboot CPUs before
freeze_processes() was called, because the freezer used to be totally unsafe
on SMP. However, what we wanted from the beginning was to freeze tasks with
all CPUs on line (this way, for example, userland tasks should not notice that
we have played with the CPUs under them, but there are other reasons).
> > 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() ?
They all are PF_NOFREEZE, I suppose. If we make all workqueues nonfreezable
(as they were before), the problem won't appear.
> I think we need a general "cpu_down() after freeze" implementation, this is what
> Gautham and Srivatsa are working on, right?
Yes, certainly.
> > --- 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.
Okay, in that case I'd suggest removing create_freezeable_workqueue() and
make all workqueues nonfreezable once again for 2.6.21 (as far as I know, only
the two XFS workqueues are affected).
Pavel, would that be acceptable?
Rafael
On Wednesday, 28 February 2007 00:36, Johannes Berg wrote:
> On Wed, 2007-02-28 at 02:28 +0300, Oleg Nesterov wrote:
>
> > Ugh. I know nothing, nothing, nothing about suspend. I'll try to guess.
> >
> > Commit: ed746e3b18f4df18afa3763155972c5835f284c5
>
> > Yes? with the patch above, _cpu_down() called _after_ freeze_processes() ???
>
> perfect :)
> See also my original mail and the thread:
> http://thread.gmane.org/gmane.linux.power-management.general/4314
>
> > How about other kthread_stop()s ? For example, kernel/softirq.c:cpu_callback() ?
>
> I'd they should be affected as well.
They won't be, if they have PF_NOFREEZE set.
> For me, I guess I haven't run into them because xfs is enough to freeze the
> box ;)
I think you've hit the only thing that could have triggered the issue. ;-)
Greetings,
Rafael
On Wed, 2007-02-28 at 01:00 +0100, Rafael J. Wysocki wrote:
> > > How about other kthread_stop()s ? For example, kernel/softirq.c:cpu_callback() ?
> >
> > I'd they should be affected as well.
>
> They won't be, if they have PF_NOFREEZE set.
Yup, I missed that.
johannes
On Wed, 2007-02-28 at 00:57 +0100, Rafael J. Wysocki wrote:
> Okay, in that case I'd suggest removing create_freezeable_workqueue() and
> make all workqueues nonfreezable once again for 2.6.21 (as far as I know, only
> the two XFS workqueues are affected).
I think Nigel might object but I forgot what specific trouble XFS was
causing him.
johannes
On Wednesday, 28 February 2007 01:01, Johannes Berg wrote:
> On Wed, 2007-02-28 at 00:57 +0100, Rafael J. Wysocki wrote:
>
> > Okay, in that case I'd suggest removing create_freezeable_workqueue() and
> > make all workqueues nonfreezable once again for 2.6.21 (as far as I know, only
> > the two XFS workqueues are affected).
>
> I think Nigel might object but I forgot what specific trouble XFS was
> causing him.
We suspected that the XFS' worker threads might commit I/O after
freeze_processes() has returned, but that hasn't been supported by evidence,
as far as I can recall.
Also, making them freezable was controversial ...
Greetings,
Rafael
Hi.
On Wed, 2007-02-28 at 01:08 +0100, Rafael J. Wysocki wrote:
> On Wednesday, 28 February 2007 01:01, Johannes Berg wrote:
> > On Wed, 2007-02-28 at 00:57 +0100, Rafael J. Wysocki wrote:
> >
> > > Okay, in that case I'd suggest removing create_freezeable_workqueue() and
> > > make all workqueues nonfreezable once again for 2.6.21 (as far as I know, only
> > > the two XFS workqueues are affected).
> >
> > I think Nigel might object but I forgot what specific trouble XFS was
> > causing him.
>
> We suspected that the XFS' worker threads might commit I/O after
> freeze_processes() has returned, but that hasn't been supported by evidence,
> as far as I can recall.
>
> Also, making them freezable was controversial ...
Controversy is no reason to give in! Nevertheless, I think you're right
- I believe the XFS guys said they fixed the issue that had caused I/O
to be submitted post-freeze. Well, we'll see if it appears again, won't
we?
Regards,
Nigel
On Tue, Feb 27, 2007 at 10:51:27PM +0100, 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).
This problem (of kthread_stopping a frozen thread) was there when we
implemented freezer-based cpu hotplug. We worked around that in the
callbacks by thawing the worker thread first before kthread_stopping it,
which is working pretty neatly.
Should that fix the issue?
--
Regards,
vatsa
On Wed, Feb 28, 2007 at 12:57:35AM +0100, Rafael J. Wysocki wrote:
> > How about other kthread_stop()s ? For example, kernel/softirq.c:cpu_callback() ?
>
> They all are PF_NOFREEZE, I suppose. If we make all workqueues nonfreezable
> (as they were before), the problem won't appear.
We can just thaw the worker thread selectively before kthread_stopping
them. This will let us freeze all worker threads (which we want to for
hotplug anyway).
> > I think we need a general "cpu_down() after freeze" implementation, this is what
> > Gautham and Srivatsa are working on, right?
>
> Yes, certainly.
Hmm ..good point. So can we assume that disable/enable_nonboot_cpus() are called
with processes frozen already?
Gautham, you need to take this into account in your patchset!
> > 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.
>
> Okay, in that case I'd suggest removing create_freezeable_workqueue() and
> make all workqueues nonfreezable once again for 2.6.21 (as far as I know, only
> the two XFS workqueues are affected).
See above suggestion of thawing worker thread before kthread_stopping
it.
--
Regards,
vatsa
On Wed, Feb 28, 2007 at 08:31:13AM +0530, Srivatsa Vaddagiri wrote:
> This problem (of kthread_stopping a frozen thread) was there when we
> implemented freezer-based cpu hotplug. We worked around that in the
> callbacks by thawing the worker thread first before kthread_stopping it,
> which is working pretty neatly.
>
> Should that fix the issue?
In addition to thawing worker thread before kthread_stopping it, there
are minor changes required in worker threads, to check for
is_cpu_offline(bind_cpu) when they come out of refrigerator and jump to
wait_to_die if so (ex: softirq.c).
I guess you would need these changes before freezer-based hotplug is
merged, in which case Gautham can send those patches out first.
--
Regards,
vatsa
On 02/28, Srivatsa Vaddagiri wrote:
>
> On Wed, Feb 28, 2007 at 12:57:35AM +0100, Rafael J. Wysocki wrote:
> > > How about other kthread_stop()s ? For example, kernel/softirq.c:cpu_callback() ?
> >
> > They all are PF_NOFREEZE, I suppose. If we make all workqueues nonfreezable
> > (as they were before), the problem won't appear.
>
> We can just thaw the worker thread selectively before kthread_stopping
> them. This will let us freeze all worker threads (which we want to for
> hotplug anyway).
I am not sure this is a good change for 2.6.21.
I strongly believe it is better to change XFS so that it doesn't use
create_freezeable_workqueue() as Rafael suggested. Besides, freezeable
workqueues are buggy anyway in 2.6.21-rc,
http://marc.theaimsgroup.com/?l=linux-kernel&m=116855740612755
This means that workqueues become non-freezeable after suspend/resume
anyway (if I understand disable_nonboot_cpus() correctly).
Oleg.
Hi!
> > 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.
>
> Okay, in that case I'd suggest removing create_freezeable_workqueue() and
> make all workqueues nonfreezable once again for 2.6.21 (as far as I know, only
> the two XFS workqueues are affected).
>
> Pavel, would that be acceptable?
Not sure... I really dislike XFS running while we are doing
swsusp. I'd like to move in direction of freezeable workqueues in the
future.
Anyway, if it gets us out of current trouble... yes I guess we can do
it.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed, Feb 28, 2007 at 11:48:59AM +0300, Oleg Nesterov wrote:
> On 02/28, Srivatsa Vaddagiri wrote:
> > We can just thaw the worker thread selectively before kthread_stopping
> > them. This will let us freeze all worker threads (which we want to for
> > hotplug anyway).
>
> I am not sure this is a good change for 2.6.21.
So we make that change when merging the freezer-based hotplug patchset?
> I strongly believe it is better to change XFS so that it doesn't use
> create_freezeable_workqueue() as Rafael suggested.
Ok no issues. But when we enable freezer-based hotplug, we expect all
non-singlethreaded worker threads to be frozen (for hotplug atleast).
> Besides, freezeable workqueues are buggy anyway in 2.6.21-rc,
>
> http://marc.theaimsgroup.com/?l=linux-kernel&m=116855740612755
>
> This means that workqueues become non-freezeable after suspend/resume
> anyway (if I understand disable_nonboot_cpus() correctly).
Ah ok. When is the above patch expected to be merged?
--
Regards,
vatsa
On 02/28, Srivatsa Vaddagiri wrote:
>
> On Wed, Feb 28, 2007 at 11:48:59AM +0300, Oleg Nesterov wrote:
> > On 02/28, Srivatsa Vaddagiri wrote:
> > > We can just thaw the worker thread selectively before kthread_stopping
> > > them. This will let us freeze all worker threads (which we want to for
> > > hotplug anyway).
> >
> > I am not sure this is a good change for 2.6.21.
>
> So we make that change when merging the freezer-based hotplug patchset?
>
> > I strongly believe it is better to change XFS so that it doesn't use
> > create_freezeable_workqueue() as Rafael suggested.
>
> Ok no issues. But when we enable freezer-based hotplug, we expect all
> non-singlethreaded worker threads to be frozen (for hotplug atleast).
Yes, we already discussed this :) This is btw another indication we
should kill create_freezeable_workqueue() eventually, so it would be
nice to change the only one user not to use it.
> > Besides, freezeable workqueues are buggy anyway in 2.6.21-rc,
> >
> > http://marc.theaimsgroup.com/?l=linux-kernel&m=116855740612755
> >
> > This means that workqueues become non-freezeable after suspend/resume
> > anyway (if I understand disable_nonboot_cpus() correctly).
>
> Ah ok. When is the above patch expected to be merged?
Oh, I don't know. Note that this particular patch makes little sense if
we change XFS now (because we have no other users), but we can't drop it,
this will break subsequent patches.
Oleg.
On Wednesday, 28 February 2007 02:14, Nigel Cunningham wrote:
> Hi.
>
> On Wed, 2007-02-28 at 01:08 +0100, Rafael J. Wysocki wrote:
> > On Wednesday, 28 February 2007 01:01, Johannes Berg wrote:
> > > On Wed, 2007-02-28 at 00:57 +0100, Rafael J. Wysocki wrote:
> > >
> > > > Okay, in that case I'd suggest removing create_freezeable_workqueue() and
> > > > make all workqueues nonfreezable once again for 2.6.21 (as far as I know, only
> > > > the two XFS workqueues are affected).
> > >
> > > I think Nigel might object but I forgot what specific trouble XFS was
> > > causing him.
> >
> > We suspected that the XFS' worker threads might commit I/O after
> > freeze_processes() has returned, but that hasn't been supported by evidence,
> > as far as I can recall.
> >
> > Also, making them freezable was controversial ...
>
> Controversy is no reason to give in! Nevertheless, I think you're right
> - I believe the XFS guys said they fixed the issue that had caused I/O
> to be submitted post-freeze. Well, we'll see if it appears again, won't
> we?
Sure, we will. :-)
Rafael
On Wednesday, 28 February 2007 10:10, Srivatsa Vaddagiri wrote:
> On Wed, Feb 28, 2007 at 11:48:59AM +0300, Oleg Nesterov wrote:
> > On 02/28, Srivatsa Vaddagiri wrote:
> > > We can just thaw the worker thread selectively before kthread_stopping
> > > them. This will let us freeze all worker threads (which we want to for
> > > hotplug anyway).
> >
> > I am not sure this is a good change for 2.6.21.
>
> So we make that change when merging the freezer-based hotplug patchset?
Well, if we want to have freezable workqueues eventually, and I think we do,
they should be taken into consideration in designing this patchset, as well as
the suspend code ordering (ie. freeze_processes(SUSPEND) before
freeze_processes(CPU_HOTPLUG)).
Greetings,
Rafael
On Wednesday, 28 February 2007 04:51, Srivatsa Vaddagiri wrote:
> On Wed, Feb 28, 2007 at 08:31:13AM +0530, Srivatsa Vaddagiri wrote:
> > This problem (of kthread_stopping a frozen thread) was there when we
> > implemented freezer-based cpu hotplug. We worked around that in the
> > callbacks by thawing the worker thread first before kthread_stopping it,
> > which is working pretty neatly.
> >
> > Should that fix the issue?
>
> In addition to thawing worker thread before kthread_stopping it, there
> are minor changes required in worker threads, to check for
> is_cpu_offline(bind_cpu) when they come out of refrigerator and jump to
> wait_to_die if so (ex: softirq.c).
>
> I guess you would need these changes before freezer-based hotplug is
> merged, in which case Gautham can send those patches out first.
Yes, please, if that's possible.
On Wed, Feb 28, 2007 at 12:11:03PM +0100, Rafael J. Wysocki wrote:
> > In addition to thawing worker thread before kthread_stopping it, there
> > are minor changes required in worker threads, to check for
> > is_cpu_offline(bind_cpu) when they come out of refrigerator and jump to
> > wait_to_die if so (ex: softirq.c).
> >
> > I guess you would need these changes before freezer-based hotplug is
> > merged, in which case Gautham can send those patches out first.
>
> Yes, please, if that's possible.
After looking at the current workqueue code, the above minor change I
suggested is not required.
So you should be able to fix your "kthread_stop on a frozen worker
thread hangs" problem by just a simple patch like this (against
2.6.20-mm2):
--- workqueue.c.org 2007-02-28 18:32:48.000000000 +0530
+++ workqueue.c 2007-02-28 18:44:23.000000000 +0530
@@ -718,6 +718,8 @@ static void cleanup_workqueue_thread(str
insert_wq_barrier(cwq, &barr, 1);
cwq->should_stop = 1;
alive = 1;
+ if (frozen(cwq->thread))
+ thaw(cwq->thread);
}
spin_unlock_irq(&cwq->lock);
Can you test with this?
Note that as Oleg commented, freezable workqueues are broken w/o his
patch here:
http://marc.theaimsgroup.com/?l=linux-kernel&m=116855740612755
So you need to have Andrew/Linux pick up the above patch first to have
correctly functioning freezable workqueues.
--
Regards,
vatsa
On Wed, Feb 28, 2007 at 06:47:21PM +0530, Srivatsa Vaddagiri wrote:
> --- workqueue.c.org 2007-02-28 18:32:48.000000000 +0530
> +++ workqueue.c 2007-02-28 18:44:23.000000000 +0530
> @@ -718,6 +718,8 @@ static void cleanup_workqueue_thread(str
> insert_wq_barrier(cwq, &barr, 1);
> cwq->should_stop = 1;
> alive = 1;
> + if (frozen(cwq->thread))
> + thaw(cwq->thread);
I meant thaw_process(cwq->thread)
> }
> spin_unlock_irq(&cwq->lock);
--
Regards,
vatsa
On Wednesday, 28 February 2007 14:17, Srivatsa Vaddagiri wrote:
> On Wed, Feb 28, 2007 at 12:11:03PM +0100, Rafael J. Wysocki wrote:
> > > In addition to thawing worker thread before kthread_stopping it, there
> > > are minor changes required in worker threads, to check for
> > > is_cpu_offline(bind_cpu) when they come out of refrigerator and jump to
> > > wait_to_die if so (ex: softirq.c).
> > >
> > > I guess you would need these changes before freezer-based hotplug is
> > > merged, in which case Gautham can send those patches out first.
> >
> > Yes, please, if that's possible.
>
> After looking at the current workqueue code, the above minor change I
> suggested is not required.
>
> So you should be able to fix your "kthread_stop on a frozen worker
> thread hangs" problem by just a simple patch like this (against
> 2.6.20-mm2):
>
>
> --- workqueue.c.org 2007-02-28 18:32:48.000000000 +0530
> +++ workqueue.c 2007-02-28 18:44:23.000000000 +0530
> @@ -718,6 +718,8 @@ static void cleanup_workqueue_thread(str
> insert_wq_barrier(cwq, &barr, 1);
> cwq->should_stop = 1;
> alive = 1;
> + if (frozen(cwq->thread))
> + thaw(cwq->thread);
> }
> spin_unlock_irq(&cwq->lock);
>
>
> Can you test with this?
Sure, I will.
> Note that as Oleg commented, freezable workqueues are broken w/o his
> patch here:
>
> http://marc.theaimsgroup.com/?l=linux-kernel&m=116855740612755
>
> So you need to have Andrew/Linux pick up the above patch first to have
> correctly functioning freezable workqueues.
This one already is in -mm.
On Wednesday, 28 February 2007 14:27, Srivatsa Vaddagiri wrote:
> On Wed, Feb 28, 2007 at 06:47:21PM +0530, Srivatsa Vaddagiri wrote:
> > --- workqueue.c.org 2007-02-28 18:32:48.000000000 +0530
> > +++ workqueue.c 2007-02-28 18:44:23.000000000 +0530
> > @@ -718,6 +718,8 @@ static void cleanup_workqueue_thread(str
> > insert_wq_barrier(cwq, &barr, 1);
> > cwq->should_stop = 1;
> > alive = 1;
> > + if (frozen(cwq->thread))
> > + thaw(cwq->thread);
>
> I meant thaw_process(cwq->thread)
I thought so, but thanks anyway. ;-)
>
> > }
> > spin_unlock_irq(&cwq->lock);
>
On Wed, Feb 28, 2007 at 12:36:52AM +0100, Johannes Berg wrote:
> On Wed, 2007-02-28 at 02:28 +0300, Oleg Nesterov wrote:
>
> > Ugh. I know nothing, nothing, nothing about suspend. I'll try to guess.
> >
> > Commit: ed746e3b18f4df18afa3763155972c5835f284c5
>
> > Yes? with the patch above, _cpu_down() called _after_ freeze_processes() ???
>
> perfect :)
> See also my original mail and the thread:
> http://thread.gmane.org/gmane.linux.power-management.general/4314
>
> > How about other kthread_stop()s ? For example, kernel/softirq.c:cpu_callback() ?
>
> I'd they should be affected as well. For me, I guess I haven't run into
> them because xfs is enough to freeze the box ;) As for Rafael, I don't
> know why he isn't seeing that... I haven't been able to test this patch
> on my quad powermac so far unfortunately, been sick and away from the
> machine. I'll probably be able to test tomorrow.
They will be affected. In our first implemetentation of
general "cpu_down after freeze"(http://lkml.org/lkml/2007/2/14/107)
we had a new state known CPU_DEAD_KILL_THREADS,
the notifications for which were being sent
*after* we thawed the processes in __cpu_down.
However, in the version which we are working on, we are thawing processes
individually in CPU_DEAD before cleaning/stopping them.
I haven't observed any bad lockups/freeze chills with this approach.
But I need to test it a bit more before posting them.
regards
gautham.
--
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"
On Wed, Feb 28, 2007 at 08:37:26AM +0530, Srivatsa Vaddagiri wrote:
>
> Hmm ..good point. So can we assume that disable/enable_nonboot_cpus() are called
> with processes frozen already?
>
> Gautham, you need to take this into account in your patchset!
Yup. That would mean making the freezer reentrant since we will
be freezing twice (once for suspend and later on for hotplug). This is
ok since the api in my patches looks like
freeze_processes(int freeze_event);
But thaw will be interesting. If we are thawing for hotplug, we gotta
only thaw processes which were frozen *only* for hotplug.
Rafael, does that mean more status flags?!
Thanks and Regards
gautham.
--
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"
On Wednesday, 28 February 2007 19:17, Gautham R Shenoy wrote:
> On Wed, Feb 28, 2007 at 08:37:26AM +0530, Srivatsa Vaddagiri wrote:
> >
> > Hmm ..good point. So can we assume that disable/enable_nonboot_cpus() are called
> > with processes frozen already?
> >
> > Gautham, you need to take this into account in your patchset!
>
> Yup. That would mean making the freezer reentrant since we will
> be freezing twice (once for suspend and later on for hotplug). This is
> ok since the api in my patches looks like
> freeze_processes(int freeze_event);
>
> But thaw will be interesting. If we are thawing for hotplug, we gotta
> only thaw processes which were frozen *only* for hotplug.
>
> Rafael, does that mean more status flags?!
Well, I don't really think so, but we need to store some information in the
freezer (eg. in a status variable). Namely, we can define a variable, say
tasks_frozen, the value of which will be the bitwise or of the flags
SPE_SUSPEND, SPE_HOTPLUG etc. In a fully functional system, tasks_frozen
is equal to zero. If freeze_processes(SPE_SUSPEND) is run, it does
tasks_frozen |= SPE_SUSPEND and analogously for SPE_HOTPLUG etc.
If tasks_frozen is equal to SPE_SUSPEND|SPE_HOTPLUG, for example, and
thaw_tasks(SPE_HOTPLUG) runs, it only thaws the tasks that need not stay frozen
for the suspend and does tasks_frozen &= ~SPE_SUSPEND etc.
I think something like this should work.
Greetings,
Rafael
On Wednesday, 28 February 2007 14:17, Srivatsa Vaddagiri wrote:
> On Wed, Feb 28, 2007 at 12:11:03PM +0100, Rafael J. Wysocki wrote:
> > > In addition to thawing worker thread before kthread_stopping it, there
> > > are minor changes required in worker threads, to check for
> > > is_cpu_offline(bind_cpu) when they come out of refrigerator and jump to
> > > wait_to_die if so (ex: softirq.c).
> > >
> > > I guess you would need these changes before freezer-based hotplug is
> > > merged, in which case Gautham can send those patches out first.
> >
> > Yes, please, if that's possible.
>
> After looking at the current workqueue code, the above minor change I
> suggested is not required.
>
> So you should be able to fix your "kthread_stop on a frozen worker
> thread hangs" problem by just a simple patch like this (against
> 2.6.20-mm2):
>
>
> --- workqueue.c.org 2007-02-28 18:32:48.000000000 +0530
> +++ workqueue.c 2007-02-28 18:44:23.000000000 +0530
> @@ -718,6 +718,8 @@ static void cleanup_workqueue_thread(str
> insert_wq_barrier(cwq, &barr, 1);
> cwq->should_stop = 1;
> alive = 1;
> + if (frozen(cwq->thread))
> + thaw(cwq->thread);
> }
> spin_unlock_irq(&cwq->lock);
>
>
> Can you test with this?
Unfortunately, the above code is mm-only. Is the analogous fix for 2.6.21-rc2
viable?
Rafael
On 02/28, Rafael J. Wysocki wrote:
>
> > --- workqueue.c.org 2007-02-28 18:32:48.000000000 +0530
> > +++ workqueue.c 2007-02-28 18:44:23.000000000 +0530
> > @@ -718,6 +718,8 @@ static void cleanup_workqueue_thread(str
> > insert_wq_barrier(cwq, &barr, 1);
> > cwq->should_stop = 1;
> > alive = 1;
> > + if (frozen(cwq->thread))
> > + thaw(cwq->thread);
> > }
> > spin_unlock_irq(&cwq->lock);
>
> Unfortunately, the above code is mm-only. Is the analogous fix for 2.6.21-rc2
> viable?
I am sorry, I lost track of this problem. As for 2.6.21, create_freezeable_workqueue
doesn't work and conflict with suspend. Why can't we remove it from XFS as you
suggested before?
Iirc,
On 02/28, Nigel Cunningham wrote:
>
> On Wed, 2007-02-28 at 01:08 +0100, Rafael J. Wysocki wrote:
> > On Wednesday, 28 February 2007 01:01, Johannes Berg wrote:
> > > On Wed, 2007-02-28 at 00:57 +0100, Rafael J. Wysocki wrote:
> > >
> > > > Okay, in that case I'd suggest removing create_freezeable_workqueue() and
> > > > make all workqueues nonfreezable once again for 2.6.21 (as far as I know, only
> > > > the two XFS workqueues are affected).
> > >
> > > I think Nigel might object but I forgot what specific trouble XFS was
> > > causing him.
> >
> > We suspected that the XFS' worker threads might commit I/O after
> > freeze_processes() has returned, but that hasn't been supported by evidence,
> > as far as I can recall.
> >
> > Also, making them freezable was controversial ...
>
> Controversy is no reason to give in! Nevertheless, I think you're right
> - I believe the XFS guys said they fixed the issue that had caused I/O
> to be submitted post-freeze. Well, we'll see if it appears again, won't
> we?
Oleg.
On Wednesday, 28 February 2007 20:32, Oleg Nesterov wrote:
> On 02/28, Rafael J. Wysocki wrote:
> >
> > > --- workqueue.c.org 2007-02-28 18:32:48.000000000 +0530
> > > +++ workqueue.c 2007-02-28 18:44:23.000000000 +0530
> > > @@ -718,6 +718,8 @@ static void cleanup_workqueue_thread(str
> > > insert_wq_barrier(cwq, &barr, 1);
> > > cwq->should_stop = 1;
> > > alive = 1;
> > > + if (frozen(cwq->thread))
> > > + thaw(cwq->thread);
> > > }
> > > spin_unlock_irq(&cwq->lock);
> >
> > Unfortunately, the above code is mm-only. Is the analogous fix for 2.6.21-rc2
> > viable?
>
> I am sorry, I lost track of this problem. As for 2.6.21, create_freezeable_workqueue
> doesn't work and conflict with suspend. Why can't we remove it from XFS as you
> suggested before?
Yes, we can (preparing a patch). I was just curious. :-)
Rafael
On 02/28, Rafael J. Wysocki wrote:
>
> On Wednesday, 28 February 2007 20:32, Oleg Nesterov wrote:
> >
> > I am sorry, I lost track of this problem. As for 2.6.21, create_freezeable_workqueue
> > doesn't work and conflict with suspend. Why can't we remove it from XFS as you
> > suggested before?
>
> Yes, we can (preparing a patch). I was just curious. :-)
OK, thanks.
We can (I think) do pretty much the same with some additional complications
in worker_thread() (check !cpu_online() after try_to_freeze() and break).
Oleg.
On Wednesday, 28 February 2007 21:08, Oleg Nesterov wrote:
> On 02/28, Rafael J. Wysocki wrote:
> >
> > On Wednesday, 28 February 2007 20:32, Oleg Nesterov wrote:
> > >
> > > I am sorry, I lost track of this problem. As for 2.6.21, create_freezeable_workqueue
> > > doesn't work and conflict with suspend. Why can't we remove it from XFS as you
> > > suggested before?
> >
> > Yes, we can (preparing a patch). I was just curious. :-)
>
> OK, thanks.
>
> We can (I think) do pretty much the same with some additional complications
> in worker_thread() (check !cpu_online() after try_to_freeze() and break).
Okay, but I've just finished the patch that removes the freezability of
workqueues (appended), so can we please do this in a separate one?
Rafael
---
Since freezable workqueues are broken in 2.6.21-rc
(cf. http://marc.theaimsgroup.com/?l=linux-kernel&m=116855740612755,
http://marc.theaimsgroup.com/?l=linux-kernel&m=117261312523921&w=2)
it's better to remove them altogether for 2.6.21 and change the only user of
them (XFS) accordingly.
---
fs/xfs/linux-2.6/xfs_buf.c | 4 ++--
include/linux/workqueue.h | 8 +++-----
kernel/workqueue.c | 21 +++++++--------------
3 files changed, 12 insertions(+), 21 deletions(-)
Index: linux-2.6.21-rc2/kernel/workqueue.c
===================================================================
--- linux-2.6.21-rc2.orig/kernel/workqueue.c
+++ linux-2.6.21-rc2/kernel/workqueue.c
@@ -59,7 +59,6 @@ struct cpu_workqueue_struct {
int run_depth; /* Detect run_workqueue() recursion depth */
- int freezeable; /* Freeze the thread during suspend */
} ____cacheline_aligned;
/*
@@ -352,8 +351,7 @@ static int worker_thread(void *__cwq)
struct k_sigaction sa;
sigset_t blocked;
- if (!cwq->freezeable)
- current->flags |= PF_NOFREEZE;
+ current->flags |= PF_NOFREEZE;
set_user_nice(current, -5);
@@ -376,9 +374,6 @@ static int worker_thread(void *__cwq)
set_current_state(TASK_INTERRUPTIBLE);
while (!kthread_should_stop()) {
- if (cwq->freezeable)
- try_to_freeze();
-
add_wait_queue(&cwq->more_work, &wait);
if (list_empty(&cwq->worklist))
schedule();
@@ -454,8 +449,8 @@ void fastcall flush_workqueue(struct wor
}
EXPORT_SYMBOL_GPL(flush_workqueue);
-static struct task_struct *create_workqueue_thread(struct workqueue_struct *wq,
- int cpu, int freezeable)
+static struct task_struct
+*create_workqueue_thread(struct workqueue_struct *wq, int cpu)
{
struct cpu_workqueue_struct *cwq = per_cpu_ptr(wq->cpu_wq, cpu);
struct task_struct *p;
@@ -465,7 +460,6 @@ static struct task_struct *create_workqu
cwq->thread = NULL;
cwq->insert_sequence = 0;
cwq->remove_sequence = 0;
- cwq->freezeable = freezeable;
INIT_LIST_HEAD(&cwq->worklist);
init_waitqueue_head(&cwq->more_work);
init_waitqueue_head(&cwq->work_done);
@@ -480,8 +474,7 @@ static struct task_struct *create_workqu
return p;
}
-struct workqueue_struct *__create_workqueue(const char *name,
- int singlethread, int freezeable)
+struct workqueue_struct *__create_workqueue(const char *name, int singlethread)
{
int cpu, destroy = 0;
struct workqueue_struct *wq;
@@ -501,7 +494,7 @@ struct workqueue_struct *__create_workqu
mutex_lock(&workqueue_mutex);
if (singlethread) {
INIT_LIST_HEAD(&wq->list);
- p = create_workqueue_thread(wq, singlethread_cpu, freezeable);
+ p = create_workqueue_thread(wq, singlethread_cpu);
if (!p)
destroy = 1;
else
@@ -509,7 +502,7 @@ struct workqueue_struct *__create_workqu
} else {
list_add(&wq->list, &workqueues);
for_each_online_cpu(cpu) {
- p = create_workqueue_thread(wq, cpu, freezeable);
+ p = create_workqueue_thread(wq, cpu);
if (p) {
kthread_bind(p, cpu);
wake_up_process(p);
@@ -760,7 +753,7 @@ static int __devinit workqueue_cpu_callb
mutex_lock(&workqueue_mutex);
/* Create a new workqueue thread for it. */
list_for_each_entry(wq, &workqueues, list) {
- if (!create_workqueue_thread(wq, hotcpu, 0)) {
+ if (!create_workqueue_thread(wq, hotcpu)) {
printk("workqueue for %i failed\n", hotcpu);
return NOTIFY_BAD;
}
Index: linux-2.6.21-rc2/include/linux/workqueue.h
===================================================================
--- linux-2.6.21-rc2.orig/include/linux/workqueue.h
+++ linux-2.6.21-rc2/include/linux/workqueue.h
@@ -159,11 +159,9 @@ struct execute_work {
extern struct workqueue_struct *__create_workqueue(const char *name,
- int singlethread,
- int freezeable);
-#define create_workqueue(name) __create_workqueue((name), 0, 0)
-#define create_freezeable_workqueue(name) __create_workqueue((name), 0, 1)
-#define create_singlethread_workqueue(name) __create_workqueue((name), 1, 0)
+ int singlethread);
+#define create_workqueue(name) __create_workqueue((name), 0)
+#define create_singlethread_workqueue(name) __create_workqueue((name), 1)
extern void destroy_workqueue(struct workqueue_struct *wq);
Index: linux-2.6.21-rc2/fs/xfs/linux-2.6/xfs_buf.c
===================================================================
--- linux-2.6.21-rc2.orig/fs/xfs/linux-2.6/xfs_buf.c
+++ linux-2.6.21-rc2/fs/xfs/linux-2.6/xfs_buf.c
@@ -1829,11 +1829,11 @@ xfs_buf_init(void)
if (!xfs_buf_zone)
goto out_free_trace_buf;
- xfslogd_workqueue = create_freezeable_workqueue("xfslogd");
+ xfslogd_workqueue = create_workqueue("xfslogd");
if (!xfslogd_workqueue)
goto out_free_buf_zone;
- xfsdatad_workqueue = create_freezeable_workqueue("xfsdatad");
+ xfsdatad_workqueue = create_workqueue("xfsdatad");
if (!xfsdatad_workqueue)
goto out_destroy_xfslogd_workqueue;
On 02/28, Rafael J. Wysocki wrote:
>
> Okay, but I've just finished the patch that removes the freezability of
> workqueues (appended), so can we please do this in a separate one?
Please, please, no. This patch is of course correct, but it breaks _a lot_
of patches in -mm tree.
May I ask you to send just
> ===================================================================
> --- linux-2.6.21-rc2.orig/fs/xfs/linux-2.6/xfs_buf.c
> +++ linux-2.6.21-rc2/fs/xfs/linux-2.6/xfs_buf.c
> @@ -1829,11 +1829,11 @@ xfs_buf_init(void)
> if (!xfs_buf_zone)
> goto out_free_trace_buf;
>
> - xfslogd_workqueue = create_freezeable_workqueue("xfslogd");
> + xfslogd_workqueue = create_workqueue("xfslogd");
> if (!xfslogd_workqueue)
> goto out_free_buf_zone;
>
> - xfsdatad_workqueue = create_freezeable_workqueue("xfsdatad");
> + xfsdatad_workqueue = create_workqueue("xfsdatad");
> if (!xfsdatad_workqueue)
> goto out_destroy_xfslogd_workqueue;
>
>
this bit?
After that, we can do the "removes the freezability of workqueues" patch
against -mm tree.
Oleg.
On Wed, 2007-02-28 at 12:14 +1100, Nigel Cunningham wrote:
> Controversy is no reason to give in! Nevertheless, I think you're right
> - I believe the XFS guys said they fixed the issue that had caused I/O
> to be submitted post-freeze. Well, we'll see if it appears again, won't
> we?
I get to be the guinea pig, right? :P Unfortunately I was sick for the
better part of the past few days and can only test all this stuff early
next week.
johannes
Hi!
> > OK, thanks.
> >
> > We can (I think) do pretty much the same with some additional complications
> > in worker_thread() (check !cpu_online() after try_to_freeze() and break).
>
> Okay, but I've just finished the patch that removes the freezability of
> workqueues (appended), so can we please do this in a separate one?
Hmm, nothing obviously wrong with the patch (ACK), but xfs people
should ack this one, too: 'is it okay to let xfs run while suspending'
is not a trivial question.
> Since freezable workqueues are broken in 2.6.21-rc
> (cf. http://marc.theaimsgroup.com/?l=linux-kernel&m=116855740612755,
> http://marc.theaimsgroup.com/?l=linux-kernel&m=117261312523921&w=2)
> it's better to remove them altogether for 2.6.21 and change the only user of
> them (XFS) accordingly.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wednesday, 28 February 2007 21:35, Oleg Nesterov wrote:
> On 02/28, Rafael J. Wysocki wrote:
> >
> > Okay, but I've just finished the patch that removes the freezability of
> > workqueues (appended), so can we please do this in a separate one?
>
> Please, please, no. This patch is of course correct, but it breaks _a lot_
> of patches in -mm tree.
>
> May I ask you to send just
>
> > ===================================================================
> > --- linux-2.6.21-rc2.orig/fs/xfs/linux-2.6/xfs_buf.c
> > +++ linux-2.6.21-rc2/fs/xfs/linux-2.6/xfs_buf.c
> > @@ -1829,11 +1829,11 @@ xfs_buf_init(void)
> > if (!xfs_buf_zone)
> > goto out_free_trace_buf;
> >
> > - xfslogd_workqueue = create_freezeable_workqueue("xfslogd");
> > + xfslogd_workqueue = create_workqueue("xfslogd");
> > if (!xfslogd_workqueue)
> > goto out_free_buf_zone;
> >
> > - xfsdatad_workqueue = create_freezeable_workqueue("xfsdatad");
> > + xfsdatad_workqueue = create_workqueue("xfsdatad");
> > if (!xfsdatad_workqueue)
> > goto out_destroy_xfslogd_workqueue;
> >
> >
>
> this bit?
>
> After that, we can do the "removes the freezability of workqueues" patch
> against -mm tree.
Okay, if that's better.
Pavel, is that acceptable to you?
Rafael
On Wed 2007-02-28 23:39:30, Rafael J. Wysocki wrote:
> On Wednesday, 28 February 2007 21:35, Oleg Nesterov wrote:
> > On 02/28, Rafael J. Wysocki wrote:
> > >
> > > Okay, but I've just finished the patch that removes the freezability of
> > > workqueues (appended), so can we please do this in a separate one?
> >
> > Please, please, no. This patch is of course correct, but it breaks _a lot_
> > of patches in -mm tree.
> >
> > May I ask you to send just
> >
> > > ===================================================================
> > > --- linux-2.6.21-rc2.orig/fs/xfs/linux-2.6/xfs_buf.c
> > > +++ linux-2.6.21-rc2/fs/xfs/linux-2.6/xfs_buf.c
> > > @@ -1829,11 +1829,11 @@ xfs_buf_init(void)
> > > if (!xfs_buf_zone)
> > > goto out_free_trace_buf;
> > >
> > > - xfslogd_workqueue = create_freezeable_workqueue("xfslogd");
> > > + xfslogd_workqueue = create_workqueue("xfslogd");
> > > if (!xfslogd_workqueue)
> > > goto out_free_buf_zone;
> > >
> > > - xfsdatad_workqueue = create_freezeable_workqueue("xfsdatad");
> > > + xfsdatad_workqueue = create_workqueue("xfsdatad");
> > > if (!xfsdatad_workqueue)
> > > goto out_destroy_xfslogd_workqueue;
> > >
> > >
> >
> > this bit?
> >
> > After that, we can do the "removes the freezability of workqueues" patch
> > against -mm tree.
>
> Okay, if that's better.
>
> Pavel, is that acceptable to you?
No problem, but get that acked-by: from XFS people ;-).
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Since freezable workqueues are broken in 2.6.21-rc
(cf. http://marc.theaimsgroup.com/?l=linux-kernel&m=116855740612755,
http://marc.theaimsgroup.com/?l=linux-kernel&m=117261312523921&w=2)
it's better to change the only user of them, which is XFS, to use "normal"
nonfreezable workqueues.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
fs/xfs/linux-2.6/xfs_buf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Index: linux-2.6.21-rc2/fs/xfs/linux-2.6/xfs_buf.c
===================================================================
--- linux-2.6.21-rc2.orig/fs/xfs/linux-2.6/xfs_buf.c
+++ linux-2.6.21-rc2/fs/xfs/linux-2.6/xfs_buf.c
@@ -1829,11 +1829,11 @@ xfs_buf_init(void)
if (!xfs_buf_zone)
goto out_free_trace_buf;
- xfslogd_workqueue = create_freezeable_workqueue("xfslogd");
+ xfslogd_workqueue = create_workqueue("xfslogd");
if (!xfslogd_workqueue)
goto out_free_buf_zone;
- xfsdatad_workqueue = create_freezeable_workqueue("xfsdatad");
+ xfsdatad_workqueue = create_workqueue("xfsdatad");
if (!xfsdatad_workqueue)
goto out_destroy_xfslogd_workqueue;