[PATCH] coredump: kill mm->core_done

[PATCH] coredump: kill mm->core_done

[Oleg Nesterov]

Now that we have core_state->dumper list we can use it to wake up the
sub-threads waiting for the coredump completion.

This uglifies the code and .text grows by 47 bytes, but otoh mm_struct
lessens by sizeof(struct completion). Also, with this change we can
decouple exit_mm() from the coredumping code.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

 include/linux/mm_types.h |    4 +---
 kernel/exit.c            |    8 +++++++-
 fs/exec.c                |   25 ++++++++++++++++++++++---
 3 files changed, 30 insertions(+), 7 deletions(-)

--- 26-rc2/include/linux/mm_types.h~5_KILL_CORE_DONE	2008-07-13 18:28:36.000000000 +0400
+++ 26-rc2/include/linux/mm_types.h	2008-07-15 17:06:58.000000000 +0400
@@ -229,9 +229,7 @@ struct mm_struct {
 
 	unsigned long flags; /* Must use atomic bitops to access the bits */
 
-	/* coredumping support */
-	struct core_state *core_state;
-	struct completion core_done;
+	struct core_state *core_state; /* coredumping support */
 
 	/* aio bits */
 	rwlock_t		ioctx_list_lock;	/* aio lock */
--- 26-rc2/kernel/exit.c~5_KILL_CORE_DONE	2008-07-13 19:58:19.000000000 +0400
+++ 26-rc2/kernel/exit.c	2008-07-15 17:06:58.000000000 +0400
@@ -680,7 +680,13 @@ static void exit_mm(struct task_struct *
 		if (atomic_dec_and_test(&core_state->nr_threads))
 			complete(&core_state->startup);
 
-		wait_for_completion(&mm->core_done);
+		for (;;) {
+			set_task_state(tsk, TASK_UNINTERRUPTIBLE);
+			if (!self.task) /* see coredump_finish() */
+				break;
+			schedule();
+		}
+		__set_task_state(tsk, TASK_UNINTERRUPTIBLE);
 		down_read(&mm->mmap_sem);
 	}
 	atomic_inc(&mm->mm_count);
--- 26-rc2/fs/exec.c~5_KILL_CORE_DONE	2008-07-13 18:43:39.000000000 +0400
+++ 26-rc2/fs/exec.c	2008-07-15 17:54:45.000000000 +0400
@@ -1597,7 +1597,6 @@ static int coredump_wait(int exit_code, 
 	struct completion *vfork_done;
 	int core_waiters;
 
-	init_completion(&mm->core_done);
 	init_completion(&core_state->startup);
 	core_state->dumper.task = tsk;
 	core_state->dumper.next = NULL;
@@ -1623,6 +1622,27 @@ fail:
 	return core_waiters;
 }
 
+static void coredump_finish(struct mm_struct *mm)
+{
+	struct core_thread *curr, *next;
+	struct task_struct *task;
+
+	next = mm->core_state->dumper.next;
+	while ((curr = next) != NULL) {
+		next = curr->next;
+		task = curr->task;
+		/*
+		 * see exit_mm(), curr->task must not see
+		 * ->task == NULL before we read ->next.
+		 */
+		smp_mb();
+		curr->task = NULL;
+		wake_up_process(task);
+	}
+
+	mm->core_state = NULL;
+}
+
 /*
  * set_dumpable converts traditional three-value dumpable to two flags and
  * stores them into mm->flags.  It modifies lower two bits of mm->flags, but
@@ -1807,8 +1827,7 @@ fail_unlock:
 		argv_free(helper_argv);
 
 	current->fsuid = fsuid;
-	complete_all(&mm->core_done);
-	mm->core_state = NULL;
+	coredump_finish(mm);
 fail:
 	return retval;
 }

Re: [PATCH] coredump: kill mm->core_done

[Oleg Nesterov]

On 07/15, Oleg Nesterov wrote:
>
> +		for (;;) {
> +			set_task_state(tsk, TASK_UNINTERRUPTIBLE);
> +			if (!self.task) /* see coredump_finish() */
> +				break;
> +			schedule();
> +		}
> +		__set_task_state(tsk, TASK_UNINTERRUPTIBLE);
                                      ^^^^^^^^^^^^^^^^^^^^
Ugh, sorry, this should be TASK_RUNNING. Please find the fixed patch below.


[PATCH] coredump: kill mm->core_done

Now that we have core_state->dumper list we can use it to wake up the
sub-threads waiting for the coredump completion.

This uglifies the code and .text grows by 47 bytes, but otoh mm_struct
lessens by sizeof(struct completion). Also, with this change we can
decouple exit_mm() from the coredumping code.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

 include/linux/mm_types.h |    4 +---
 kernel/exit.c            |    8 +++++++-
 fs/exec.c                |   25 ++++++++++++++++++++++---
 3 files changed, 30 insertions(+), 7 deletions(-)

--- 26-rc2/include/linux/mm_types.h~5_KILL_CORE_DONE	2008-07-13 18:28:36.000000000 +0400
+++ 26-rc2/include/linux/mm_types.h	2008-07-15 17:06:58.000000000 +0400
@@ -229,9 +229,7 @@ struct mm_struct {
 
 	unsigned long flags; /* Must use atomic bitops to access the bits */
 
-	/* coredumping support */
-	struct core_state *core_state;
-	struct completion core_done;
+	struct core_state *core_state; /* coredumping support */
 
 	/* aio bits */
 	rwlock_t		ioctx_list_lock;	/* aio lock */
--- 26-rc2/kernel/exit.c~5_KILL_CORE_DONE	2008-07-13 19:58:19.000000000 +0400
+++ 26-rc2/kernel/exit.c	2008-07-15 20:17:28.000000000 +0400
@@ -680,7 +680,13 @@ static void exit_mm(struct task_struct *
 		if (atomic_dec_and_test(&core_state->nr_threads))
 			complete(&core_state->startup);
 
-		wait_for_completion(&mm->core_done);
+		for (;;) {
+			set_task_state(tsk, TASK_UNINTERRUPTIBLE);
+			if (!self.task) /* see coredump_finish() */
+				break;
+			schedule();
+		}
+		__set_task_state(tsk, TASK_RUNNING);
 		down_read(&mm->mmap_sem);
 	}
 	atomic_inc(&mm->mm_count);
--- 26-rc2/fs/exec.c~5_KILL_CORE_DONE	2008-07-13 18:43:39.000000000 +0400
+++ 26-rc2/fs/exec.c	2008-07-15 20:16:36.000000000 +0400
@@ -1597,7 +1597,6 @@ static int coredump_wait(int exit_code, 
 	struct completion *vfork_done;
 	int core_waiters;
 
-	init_completion(&mm->core_done);
 	init_completion(&core_state->startup);
 	core_state->dumper.task = tsk;
 	core_state->dumper.next = NULL;
@@ -1623,6 +1622,27 @@ fail:
 	return core_waiters;
 }
 
+static void coredump_finish(struct mm_struct *mm)
+{
+	struct core_thread *curr, *next;
+	struct task_struct *task;
+
+	next = mm->core_state->dumper.next;
+	while ((curr = next) != NULL) {
+		next = curr->next;
+		task = curr->task;
+		/*
+		 * see exit_mm(), curr->task must not see
+		 * ->task == NULL before we read ->next.
+		 */
+		smp_mb();
+		curr->task = NULL;
+		wake_up_process(task);
+	}
+
+	mm->core_state = NULL;
+}
+
 /*
  * set_dumpable converts traditional three-value dumpable to two flags and
  * stores them into mm->flags.  It modifies lower two bits of mm->flags, but
@@ -1807,8 +1827,7 @@ fail_unlock:
 		argv_free(helper_argv);
 
 	current->fsuid = fsuid;
-	complete_all(&mm->core_done);
-	mm->core_state = NULL;
+	coredump_finish(mm);
 fail:
 	return retval;
 }

Re: [PATCH] coredump: kill mm->core_done

[Andrew Morton]

On Tue, 15 Jul 2008 20:21:50 +0400
Oleg Nesterov <oleg@tv-sign.ru> wrote:

> Now that we have core_state->dumper list we can use it to wake up the
> sub-threads waiting for the coredump completion.
> 
> This uglifies the code and .text grows by 47 bytes, but otoh mm_struct
> lessens by sizeof(struct completion). Also, with this change we can
> decouple exit_mm() from the coredumping code.

This conflicts with your
coredump-simplify-core_state-nr_threads-calculation.patch

fs/exec.c:

***************
*** 1597,1603 ****
  	struct completion *vfork_done;
  	int core_waiters;
  
- 	init_completion(&mm->core_done);
  	init_completion(&core_state->startup);
  	core_state->dumper.task = tsk;
  	core_state->dumper.next = NULL;
--- 1597,1602 ----
  	struct completion *vfork_done;
  	int core_waiters;
  
  	init_completion(&core_state->startup);
  	core_state->dumper.task = tsk;
  	core_state->dumper.next = NULL;
***************
*** 1812,1819 ****
  		argv_free(helper_argv);
  
  	current->fsuid = fsuid;
- 	complete_all(&mm->core_done);
- 	mm->core_state = NULL;
  fail:
  	return retval;
  }
--- 1832,1838 ----
  		argv_free(helper_argv);
  
  	current->fsuid = fsuid;
+ 	coredump_finish(mm);
  fail:
  	return retval;
  }

The second hunk is a bit worrisome.  The

	mm->core_state = NULL;

isn't there any more.


I have a bad feelnig that I have a coredump patch which should have
been dropped.  Can you please check everything?

Current queue:

#
# coredump
#
introduce-pf_kthread-flag.patch
kill-pf_borrowed_mm-in-favour-of-pf_kthread.patch
coredump-zap_threads-must-skip-kernel-threads.patch
coredump-elf_core_dump-skip-kernel-threads.patch
#
coredump-turn-mm-core_startup_done-into-the-pointer-to-struct-core_state.patch
coredump-move-mm-core_waiters-into-struct-core_state.patch
coredump-simplify-core_state-nr_threads-calculation.patch
coredump-turn-core_state-nr_threads-into-atomic_t.patch
coredump-kill-mm-core_done.patch


All at http://userweb.kernel.org/~akpm/mmotm/

Re: [PATCH] coredump: kill mm->core_done

[Johannes Weiner]

Hi,

Oleg Nesterov <oleg@tv-sign.ru> writes:

> Now that we have core_state->dumper list we can use it to wake up the
> sub-threads waiting for the coredump completion.
>
> This uglifies the code and .text grows by 47 bytes, but otoh mm_struct
> lessens by sizeof(struct completion). Also, with this change we can
> decouple exit_mm() from the coredumping code.
>
> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
>
>  include/linux/mm_types.h |    4 +---
>  kernel/exit.c            |    8 +++++++-
>  fs/exec.c                |   25 ++++++++++++++++++++++---
>  3 files changed, 30 insertions(+), 7 deletions(-)
>
> --- 26-rc2/include/linux/mm_types.h~5_KILL_CORE_DONE	2008-07-13 18:28:36.000000000 +0400
> +++ 26-rc2/include/linux/mm_types.h	2008-07-15 17:06:58.000000000 +0400
> @@ -229,9 +229,7 @@ struct mm_struct {
>  
>  	unsigned long flags; /* Must use atomic bitops to access the bits */
>  
> -	/* coredumping support */
> -	struct core_state *core_state;
> -	struct completion core_done;
> +	struct core_state *core_state; /* coredumping support */
>  
>  	/* aio bits */
>  	rwlock_t		ioctx_list_lock;	/* aio lock */
> --- 26-rc2/kernel/exit.c~5_KILL_CORE_DONE	2008-07-13 19:58:19.000000000 +0400
> +++ 26-rc2/kernel/exit.c	2008-07-15 17:06:58.000000000 +0400
> @@ -680,7 +680,13 @@ static void exit_mm(struct task_struct *
>  		if (atomic_dec_and_test(&core_state->nr_threads))
>  			complete(&core_state->startup);
>  
> -		wait_for_completion(&mm->core_done);
> +		for (;;) {
> +			set_task_state(tsk, TASK_UNINTERRUPTIBLE);
> +			if (!self.task) /* see coredump_finish() */

kernel/exit.c: In function `exit_mm':
kernel/exit.c:686: error: `self' undeclared (first use in this function)
kernel/exit.c:686: error: (Each undeclared identifier is reported only once
kernel/exit.c:686: error: for each function it appears in.)
make[1]: *** [kernel/exit.o] Error 1

	Hannes

Re: [PATCH] coredump: kill mm->core_done

[Roland McGrath]

It is indeed ugly.  I think the place to start is with moving all the
coredump code out of exit_mm() into a new subfunction that is just passed
the mm pointer, and doing the same with coredump_finish() as you've done,
but leaving the completion logic inside those functions the same.  If all
references to the core_* fields in mm_struct are inside coredump_*()
functions, then that is a good basis for the future cleanups.


Thanks,
Roland

Re: [PATCH] coredump: kill mm->core_done

[Oleg Nesterov]

On 07/20, Johannes Weiner wrote:
> 
> Oleg Nesterov <oleg@tv-sign.ru> writes:
> 
> > --- 26-rc2/kernel/exit.c~5_KILL_CORE_DONE	2008-07-13 19:58:19.000000000 +0400
> > +++ 26-rc2/kernel/exit.c	2008-07-15 17:06:58.000000000 +0400
> > @@ -680,7 +680,13 @@ static void exit_mm(struct task_struct *
> >  		if (atomic_dec_and_test(&core_state->nr_threads))
> >  			complete(&core_state->startup);
> >  
> > -		wait_for_completion(&mm->core_done);
> > +		for (;;) {
> > +			set_task_state(tsk, TASK_UNINTERRUPTIBLE);
> > +			if (!self.task) /* see coredump_finish() */
> 
> kernel/exit.c: In function `exit_mm':
> kernel/exit.c:686: error: `self' undeclared (first use in this function)
> kernel/exit.c:686: error: (Each undeclared identifier is reported only once
> kernel/exit.c:686: error: for each function it appears in.)
> make[1]: *** [kernel/exit.o] Error 1

This is on top of other patches in -mm tree,

         coredump-zap_threads-comments-use-while_each_thread.patch
         introduce-pf_kthread-flag.patch
         kill-pf_borrowed_mm-in-favour-of-pf_kthread.patch
         coredump-zap_threads-must-skip-kernel-threads.patch
         coredump-elf_core_dump-skip-kernel-threads.patch
         coredump-turn-mm-core_startup_done-into-the-pointer-to-struct-core_state.patch
         coredump-move-mm-core_waiters-into-struct-core_state.patch
         coredump-simplify-core_state-nr_threads-calculation.patch
         coredump-turn-core_state-nr_threads-into-atomic_t.patch
         coredump-make-mm-core_state-visible-to-core_dump.patch
         coredump-construct-the-list-of-coredumping-threads-at-startup-time.patch
         coredump-elf_core_dump-use-core_state-dumper-list.patch
         coredump-elf_fdpic_core_dump-use-core_state-dumper-list.patch
         coredump-kill-mm-core_done.patch

Oleg.

Re: [PATCH] coredump: kill mm->core_done

[Oleg Nesterov]

On 07/19, Roland McGrath wrote:
>
> It is indeed ugly.  I think the place to start is with moving all the
> coredump code out of exit_mm() into a new subfunction that is just passed
> the mm pointer, and doing the same with coredump_finish() as you've done,
> but leaving the completion logic inside those functions the same.

The next patch does this.

This patch goes first because it kills the last field in mm_struct which
is needed for the coredump code, so new new function (exit_coredump) doesn't
need the mm pointer at all.

Oleg.

Re: [PATCH] coredump: kill mm->core_done

[Johannes Weiner]

Hi,

Oleg Nesterov <oleg@tv-sign.ru> writes:

> On 07/20, Johannes Weiner wrote:
>> 
>> Oleg Nesterov <oleg@tv-sign.ru> writes:
>> 
>> > --- 26-rc2/kernel/exit.c~5_KILL_CORE_DONE	2008-07-13 19:58:19.000000000 +0400
>> > +++ 26-rc2/kernel/exit.c	2008-07-15 17:06:58.000000000 +0400
>> > @@ -680,7 +680,13 @@ static void exit_mm(struct task_struct *
>> >  		if (atomic_dec_and_test(&core_state->nr_threads))
>> >  			complete(&core_state->startup);
>> >  
>> > -		wait_for_completion(&mm->core_done);
>> > +		for (;;) {
>> > +			set_task_state(tsk, TASK_UNINTERRUPTIBLE);
>> > +			if (!self.task) /* see coredump_finish() */
>> 
>> kernel/exit.c: In function `exit_mm':
>> kernel/exit.c:686: error: `self' undeclared (first use in this function)
>> kernel/exit.c:686: error: (Each undeclared identifier is reported only once
>> kernel/exit.c:686: error: for each function it appears in.)
>> make[1]: *** [kernel/exit.o] Error 1
>
> This is on top of other patches in -mm tree,
>
>          coredump-zap_threads-comments-use-while_each_thread.patch
>          introduce-pf_kthread-flag.patch
>          kill-pf_borrowed_mm-in-favour-of-pf_kthread.patch
>          coredump-zap_threads-must-skip-kernel-threads.patch
>          coredump-elf_core_dump-skip-kernel-threads.patch
>          coredump-turn-mm-core_startup_done-into-the-pointer-to-struct-core_state.patch
>          coredump-move-mm-core_waiters-into-struct-core_state.patch
>          coredump-simplify-core_state-nr_threads-calculation.patch
>          coredump-turn-core_state-nr_threads-into-atomic_t.patch
>          coredump-make-mm-core_state-visible-to-core_dump.patch
>          coredump-construct-the-list-of-coredumping-threads-at-startup-time.patch
>          coredump-elf_core_dump-use-core_state-dumper-list.patch
>          coredump-elf_fdpic_core_dump-use-core_state-dumper-list.patch
>          coredump-kill-mm-core_done.patch

Sorry, I should have given more information.

I was building -mm directly with all these patches applied.  I did not
cherry-pick this exact patch into some other tree.

But I still can not find anything defining `self' in these patches:

$ grep self coredump-*.patch \
	introduce-pf_kthread-flag.patch \
	kill-pf_borrowed_mm-in-favour-of-pf_kthread.patch
coredump-kill-mm-core_done.patch:+                      if (!self.task) /* see coredump_finish() */

cscope finds some other definitions of self in the tree, but nothing
relevant.

	Hannes

Re: [PATCH] coredump: kill mm->core_done

[Oleg Nesterov]

On 07/20, Johannes Weiner wrote:
> 
> Oleg Nesterov <oleg@tv-sign.ru> writes:
> 
> > On 07/20, Johannes Weiner wrote:
> >> 
> >> Oleg Nesterov <oleg@tv-sign.ru> writes:
> >> 
> >> > --- 26-rc2/kernel/exit.c~5_KILL_CORE_DONE	2008-07-13 19:58:19.000000000 +0400
> >> > +++ 26-rc2/kernel/exit.c	2008-07-15 17:06:58.000000000 +0400
> >> > @@ -680,7 +680,13 @@ static void exit_mm(struct task_struct *
> >> >  		if (atomic_dec_and_test(&core_state->nr_threads))
> >> >  			complete(&core_state->startup);
> >> >  
> >> > -		wait_for_completion(&mm->core_done);
> >> > +		for (;;) {
> >> > +			set_task_state(tsk, TASK_UNINTERRUPTIBLE);
> >> > +			if (!self.task) /* see coredump_finish() */
> >> 
> >> kernel/exit.c: In function `exit_mm':
> >> kernel/exit.c:686: error: `self' undeclared (first use in this function)
> >> kernel/exit.c:686: error: (Each undeclared identifier is reported only once
> >> kernel/exit.c:686: error: for each function it appears in.)
> >> make[1]: *** [kernel/exit.o] Error 1
> >
> > This is on top of other patches in -mm tree,
> >
> >          coredump-zap_threads-comments-use-while_each_thread.patch
> >          introduce-pf_kthread-flag.patch
> >          kill-pf_borrowed_mm-in-favour-of-pf_kthread.patch
> >          coredump-zap_threads-must-skip-kernel-threads.patch
> >          coredump-elf_core_dump-skip-kernel-threads.patch
> >          coredump-turn-mm-core_startup_done-into-the-pointer-to-struct-core_state.patch
> >          coredump-move-mm-core_waiters-into-struct-core_state.patch
> >          coredump-simplify-core_state-nr_threads-calculation.patch
> >          coredump-turn-core_state-nr_threads-into-atomic_t.patch
> >          coredump-make-mm-core_state-visible-to-core_dump.patch
> >          coredump-construct-the-list-of-coredumping-threads-at-startup-time.patch
> >          coredump-elf_core_dump-use-core_state-dumper-list.patch
> >          coredump-elf_fdpic_core_dump-use-core_state-dumper-list.patch
> >          coredump-kill-mm-core_done.patch
>
> Sorry, I should have given more information.
>
> I was building -mm directly with all these patches applied.  I did not
> cherry-pick this exact patch into some other tree.
>
> But I still can not find anything defining `self' in these patches:

Because it was merged into -mm before the previous 4 patches by mistake,
sorry.

Just drop this one:

	http://userweb.kernel.org/~akpm/mmotm/broken-out/coredump-kill-mm-core_done.patch

Oleg.

Re: [PATCH] coredump: kill mm->core_done

[Johannes Weiner]

Hi,

Oleg Nesterov <oleg@tv-sign.ru> writes:

> On 07/20, Johannes Weiner wrote:
>> 
>> Oleg Nesterov <oleg@tv-sign.ru> writes:
>> 
>> > On 07/20, Johannes Weiner wrote:
>> >> 
>> >> Oleg Nesterov <oleg@tv-sign.ru> writes:
>> >> 
>> >> > --- 26-rc2/kernel/exit.c~5_KILL_CORE_DONE	2008-07-13 19:58:19.000000000 +0400
>> >> > +++ 26-rc2/kernel/exit.c	2008-07-15 17:06:58.000000000 +0400
>> >> > @@ -680,7 +680,13 @@ static void exit_mm(struct task_struct *
>> >> >  		if (atomic_dec_and_test(&core_state->nr_threads))
>> >> >  			complete(&core_state->startup);
>> >> >  
>> >> > -		wait_for_completion(&mm->core_done);
>> >> > +		for (;;) {
>> >> > +			set_task_state(tsk, TASK_UNINTERRUPTIBLE);
>> >> > +			if (!self.task) /* see coredump_finish() */
>> >> 
>> >> kernel/exit.c: In function `exit_mm':
>> >> kernel/exit.c:686: error: `self' undeclared (first use in this function)
>> >> kernel/exit.c:686: error: (Each undeclared identifier is reported only once
>> >> kernel/exit.c:686: error: for each function it appears in.)
>> >> make[1]: *** [kernel/exit.o] Error 1
>> >
>> > This is on top of other patches in -mm tree,
>> >
>> >          coredump-zap_threads-comments-use-while_each_thread.patch
>> >          introduce-pf_kthread-flag.patch
>> >          kill-pf_borrowed_mm-in-favour-of-pf_kthread.patch
>> >          coredump-zap_threads-must-skip-kernel-threads.patch
>> >          coredump-elf_core_dump-skip-kernel-threads.patch
>> >          coredump-turn-mm-core_startup_done-into-the-pointer-to-struct-core_state.patch
>> >          coredump-move-mm-core_waiters-into-struct-core_state.patch
>> >          coredump-simplify-core_state-nr_threads-calculation.patch
>> >          coredump-turn-core_state-nr_threads-into-atomic_t.patch
>> >          coredump-make-mm-core_state-visible-to-core_dump.patch
>> >          coredump-construct-the-list-of-coredumping-threads-at-startup-time.patch
>> >          coredump-elf_core_dump-use-core_state-dumper-list.patch
>> >          coredump-elf_fdpic_core_dump-use-core_state-dumper-list.patch
>> >          coredump-kill-mm-core_done.patch
>>
>> Sorry, I should have given more information.
>>
>> I was building -mm directly with all these patches applied.  I did not
>> cherry-pick this exact patch into some other tree.
>>
>> But I still can not find anything defining `self' in these patches:
>
> Because it was merged into -mm before the previous 4 patches by mistake,
> sorry.
>
> Just drop this one:
>
> 	http://userweb.kernel.org/~akpm/mmotm/broken-out/coredump-kill-mm-core_done.patch

Alright, thanks!

	Hannes