Re: Oops in pdflush

Keith Owens
2004-02-28T09:45:38Z

On Fri, 27 Feb 2004 22:52:46 -0800,
David Mosberger <davidm@napali.hpl.hp.com> wrote:
>>>>>> On Sat, 28 Feb 2004 00:58:20 +1100, Keith Owens <kaos@sgi.com> said:
> Keith> Ouch. rbs and stack have collided, kernel stack overflow. rbs shows
> Keith> a normal start, then it loops with the same data over and over again
>
>So if I'm reading this right, we get a case that looks like unbounded
>recursion:
>
> pdflush -> start_one_pdflush_thread -> kernel_thread -> pdflush ...
>
>Except, I don't think this is real recursion. Instead, we effectively
>get a (potentially unbounded) sequence of one kernel thread creating
>another thread. Each new kernel thread gets nested one deeper,
>eventually leading to a stack overflow...
>
>Hmmh, I think perhaps the right way to fix this is to use a separate
>continuation function, which will then take care of doing the
>child-specific actions. Let me see if I can come up with something.

Separate the pdflush thread creation and move it to a single master
thread. This restricts the maximum stack depth already in use when
starting a worker pdflush thread.

--- 2.6.3-pristine/mm/pdflush.c Thu Dec 18 14:00:02 2003
+++ 2.6.3-pdflush/mm/pdflush.c Sat Feb 28 20:42:04 2004
@@ -5,6 +5,9 @@
*
* 09Apr2002 akpm@zip.com.au
* Initial version
+ * 28Feb2004 kaos@sgi.com
+ * Move worker thread creation to a master thread to avoid chewing
+ * up stack space with nested calls to kernel_thread.
*/

#include <linux/sched.h>
@@ -18,6 +21,7 @@
#include <linux/fs.h> // Needed by writeback.h
#include <linux/writeback.h> // Prototypes pdflush_operation()

+#include <asm/semaphore.h>

/*
* Minimum and maximum number of pdflush instances
@@ -58,6 +62,11 @@ int nr_pdflush_threads = 0;
static unsigned long last_empty_jifs;

/*
+ * up() this to start a new pdflush thread.
+ */
+static struct semaphore new_pdflush;
+
+/*
* The pdflush thread.
*
* Thread pool management algorithm:
@@ -207,13 +216,31 @@ int pdflush_operation(void (*fn)(unsigne

static void start_one_pdflush_thread(void)
{
- kernel_thread(pdflush, NULL, CLONE_KERNEL);
+ up(&new_pdflush);
+}
+
+/*
+ * Create all pdflush worker threads from a single master thread. Creating
+ * worker threads from inside worker threads chews up kernel stack space and
+ * eventually overflows the kernel stack.
+ */
+static int pdflush_master(void *dummy)
+{
+ daemonize("pdflush_master");
+ while (1) {
+ if (down_interruptible(&new_pdflush))
+ continue;
+ kernel_thread(pdflush, NULL, CLONE_KERNEL);
+ }
+ return 0;
}

static int __init pdflush_init(void)
{
int i;

+ kernel_thread(pdflush_master, NULL, CLONE_KERNEL);
+
for (i = 0; i < MIN_PDFLUSH_THREADS; i++)
start_one_pdflush_thread();
return 0;

===========================

This is what the ia64 stack for a pdflush worker thread looks like now.
It has used 560 bytes of stack from creation to sleep.

0xe00000003db08000 16 1 0 2 S 0xe00000003db08570 pdflush
0xa0000001000830f0 schedule+0xf30
0xa0000001000dab70 __pdflush+0x230
0xa0000001000daf00 pdflush+0x20
0xa000000100016bc0 kernel_thread+0x100
0xa0000001000db250 pdflush_master+0xb0
0xa000000100016bc0 kernel_thread+0x100
0xa000000100650670 pdflush_init+0x30
0xa000000100641100 do_initcalls+0xc0
0xa000000100009300 init+0xe0
0xa000000100016bc0 kernel_thread+0x100
0xa000000100009090 rest_init+0x30
0xa000000100640f80 start_kernel+0x460
0xa0000001000085a0 _start+0x280

It would be nice if kernel_thread reset the stack every time it was
called, but that requires arch specific helper code. Until that is
available for every arch, avoid recursive calls to kernel_thread.