[PATCH 0/5] wait_task_* cleanups

[PATCH 0/5] wait_task_* cleanups

[Vitaly Mayatskikh]

These patches eliminate duplicate code for filling rusage and siginfo
structures in wait_task_*() routines. The work is based on top of recent
Oleg's changes in do_wait().

I'm running kernel with these changes on my laptop for one day, nothing
exploded (yet).

More changes to come.

Vitaly Mayatskikh (5):
  Split wait_noreap_copyout()
  Use wait_copyout() in wait_task_stopped()
  Use wait_copyout() in do_wait()
  Use wait_copyout() in wait_task_zombie()
  Use wait_copyout() in wait_task_continued()

 kernel/exit.c |  136 +++++++++++++++++++-------------------------------------
 1 files changed, 46 insertions(+), 90 deletions(-)

[PATCH 1/5] Split wait_noreap_copyout()

[Vitaly Mayatskikh]

Move getrusage() and put_user() code from wait_noreap_copyout()
to wait_copyout(). The same code is spreaded across all
wait_task_*() routines, it's better to reuse one copy.

Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
---
 kernel/exit.c |   39 +++++++++++++++++++++++----------------
 1 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 25782da..cbc5623 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1123,27 +1123,34 @@ static int eligible_child(struct wait_opts *wo, struct task_struct *p)
 	return 1;
 }
 
-static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
-				pid_t pid, uid_t uid, int why, int status)
+static int wait_copyout(struct wait_opts *wo, struct task_struct *p,
+			pid_t pid, uid_t uid, int why, int status, int signal)
 {
-	struct siginfo __user *infop;
+	struct siginfo __user *infop = wo->wo_info;
 	int retval = wo->wo_rusage
 		? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
 
+	if (!retval && infop) {
+		retval = put_user(signal, &infop->si_signo);
+		if (!retval)
+			retval = put_user(0, &infop->si_errno);
+		if (!retval)
+			retval = put_user((short)why, &infop->si_code);
+		if (!retval)
+			retval = put_user(pid, &infop->si_pid);
+		if (!retval)
+			retval = put_user(uid, &infop->si_uid);
+		if (!retval)
+			retval = put_user(status, &infop->si_status);
+	}
+	return retval;
+}
+
+static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
+				pid_t pid, uid_t uid, int why, int status)
+{
+	int retval = wait_copyout(wo, p, pid, uid, why, status, SIGCHLD);
 	put_task_struct(p);
-	infop = wo->wo_info;
-	if (!retval)
-		retval = put_user(SIGCHLD, &infop->si_signo);
-	if (!retval)
-		retval = put_user(0, &infop->si_errno);
-	if (!retval)
-		retval = put_user((short)why, &infop->si_code);
-	if (!retval)
-		retval = put_user(pid, &infop->si_pid);
-	if (!retval)
-		retval = put_user(uid, &infop->si_uid);
-	if (!retval)
-		retval = put_user(status, &infop->si_status);
 	if (!retval)
 		retval = pid;
 	return retval;
-- 
1.6.2.2

Re: [PATCH 1/5] Split wait_noreap_copyout()

[Ingo Molnar]

* Vitaly Mayatskikh <v.mayatskih@gmail.com> wrote:

> -static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
> -				pid_t pid, uid_t uid, int why, int status)
> +static int wait_copyout(struct wait_opts *wo, struct task_struct *p,
> +			pid_t pid, uid_t uid, int why, int status, int signal)

Nice cleanups. Would be nice to fix the naming here too while at it.

Right now it's two verbs and a straightforward reading of it suggest 
that we 'wait for some copyout to occur', which is nonsensical and 
confusing.

So please put the main action as the first verb (this is an internal 
symbol so no subsystem differentiator is needed). Something like:

  copy_wait_opts_to_user()

... and it becomes a whole lot easier to read. This matches the 
copy*to_user idioms we have elsewhere so it nicely wibes with the 
sound of those.

( Now repeat similar measures on the whole tree, a hundred thousand 
  times or so, and enforce it for all new patches, and we'd have
  crisp, consistent, highly readable and enjoyable kernel source ;-)

Thanks,

	Ingo

Re: [PATCH 1/5] Split wait_noreap_copyout()

[Vitaly Mayatskikh]

At Mon, 11 May 2009 12:20:50 +0200, Ingo Molnar wrote:

> > -static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
> > -				pid_t pid, uid_t uid, int why, int status)
> > +static int wait_copyout(struct wait_opts *wo, struct task_struct *p,
> > +			pid_t pid, uid_t uid, int why, int status, int signal)
> 
> Nice cleanups. Would be nice to fix the naming here too while at it.
> 
> Right now it's two verbs and a straightforward reading of it suggest 
> that we 'wait for some copyout to occur', which is nonsensical and 
> confusing.
> 
> So please put the main action as the first verb (this is an internal 
> symbol so no subsystem differentiator is needed). Something like:
> 
>   copy_wait_opts_to_user()
> 
> ... and it becomes a whole lot easier to read. This matches the 
> copy*to_user idioms we have elsewhere so it nicely wibes with the 
> sound of those.

Sure, will repost with naming fixes soon.
-- 
wbr, Vitaly

Re: [PATCH 1/5] Split wait_noreap_copyout()

[Christoph Hellwig]

On Mon, May 11, 2009 at 12:12:40PM +0200, Vitaly Mayatskikh wrote:
> +static int wait_copyout(struct wait_opts *wo, struct task_struct *p,
> +			pid_t pid, uid_t uid, int why, int status, int signal)
>  {
> -	struct siginfo __user *infop;
> +	struct siginfo __user *infop = wo->wo_info;
>  	int retval = wo->wo_rusage
>  		? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
>  
> +	if (!retval && infop) {
> +		retval = put_user(signal, &infop->si_signo);
> +		if (!retval)
> +			retval = put_user(0, &infop->si_errno);
> +		if (!retval)
> +			retval = put_user((short)why, &infop->si_code);
> +		if (!retval)
> +			retval = put_user(pid, &infop->si_pid);
> +		if (!retval)
> +			retval = put_user(uid, &infop->si_uid);
> +		if (!retval)
> +			retval = put_user(status, &infop->si_status);
> +	}
> +	return retval;

wouldn't this better be written as:

static int wait_copyout(struct wait_opts *wo, struct task_struct *p,
			pid_t pid, uid_t uid, int why, int status, int signal)
{
	struct siginfo __user *infop = wo->wo_info;

	if (wo->wo_rusage) {
		int retval = getrusage(p, RUSAGE_BOTH, wo->wo_rusage);
		if (retval)
			return retval;
	}

	if (!infop)
		return 0;

	if (put_user(signal, &infop->si_signo) ||
	    put_user(0, &infop->si_errno) ||
	    put_user((short)why, &infop->si_code) ||
	    put_user(pid, &infop->si_pid) ||
	    put_user(uid, &infop->si_uid) ||
	    put_user(status, &infop->si_status))
		return -EFAULT;
	return 0;
}

Re: [PATCH 1/5] Split wait_noreap_copyout()

[Ingo Molnar]

* Christoph Hellwig <hch@infradead.org> wrote:

> On Mon, May 11, 2009 at 12:12:40PM +0200, Vitaly Mayatskikh wrote:
> > +static int wait_copyout(struct wait_opts *wo, struct task_struct *p,
> > +			pid_t pid, uid_t uid, int why, int status, int signal)
> >  {
> > -	struct siginfo __user *infop;
> > +	struct siginfo __user *infop = wo->wo_info;
> >  	int retval = wo->wo_rusage
> >  		? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
> >  
> > +	if (!retval && infop) {
> > +		retval = put_user(signal, &infop->si_signo);
> > +		if (!retval)
> > +			retval = put_user(0, &infop->si_errno);
> > +		if (!retval)
> > +			retval = put_user((short)why, &infop->si_code);
> > +		if (!retval)
> > +			retval = put_user(pid, &infop->si_pid);
> > +		if (!retval)
> > +			retval = put_user(uid, &infop->si_uid);
> > +		if (!retval)
> > +			retval = put_user(status, &infop->si_status);
> > +	}
> > +	return retval;
> 
> wouldn't this better be written as:
> 
> static int wait_copyout(struct wait_opts *wo, struct task_struct *p,
> 			pid_t pid, uid_t uid, int why, int status, int signal)
> {
> 	struct siginfo __user *infop = wo->wo_info;
> 
> 	if (wo->wo_rusage) {
> 		int retval = getrusage(p, RUSAGE_BOTH, wo->wo_rusage);
> 		if (retval)
> 			return retval;
> 	}
> 
> 	if (!infop)
> 		return 0;
> 
> 	if (put_user(signal, &infop->si_signo) ||
> 	    put_user(0, &infop->si_errno) ||
> 	    put_user((short)why, &infop->si_code) ||
> 	    put_user(pid, &infop->si_pid) ||
> 	    put_user(uid, &infop->si_uid) ||
> 	    put_user(status, &infop->si_status))
> 		return -EFAULT;

For best assembly code this should generally be written as a series 
of:

   __uaccess_err |= __put_user(x, ptr);
   __uaccess_err |= __put_user(y, ptr);
   __uaccess_err |= __put_user(z, ptr);

As this generates non-dependent, compressed, branch-less code.

See the (new) put_user_try / put_user_ex() / put_user_catch() 
abstraction in arch/x86/include/asm/uaccess.h, and how all the x86 
signal code makes use of that to optimize such patterns of per field 
user copies.

	Ingo

Re: [PATCH 1/5] Split wait_noreap_copyout()

[Vitaly Mayatskikh]

At Mon, 11 May 2009 14:17:08 +0200, Ingo Molnar wrote:

> > 	if (put_user(signal, &infop->si_signo) ||
> > 	    put_user(0, &infop->si_errno) ||
> > 	    put_user((short)why, &infop->si_code) ||
> > 	    put_user(pid, &infop->si_pid) ||
> > 	    put_user(uid, &infop->si_uid) ||
> > 	    put_user(status, &infop->si_status))
> > 		return -EFAULT;
> 
> For best assembly code this should generally be written as a series 
> of:
> 
>    __uaccess_err |= __put_user(x, ptr);
>    __uaccess_err |= __put_user(y, ptr);
>    __uaccess_err |= __put_user(z, ptr);
> 
> As this generates non-dependent, compressed, branch-less code.

Yeah, my first intention was to eliminate a lot of branches in one
place. It's terrible for CPU pipeline, I bet.

> See the (new) put_user_try / put_user_ex() / put_user_catch() 
> abstraction in arch/x86/include/asm/uaccess.h, and how all the x86 
> signal code makes use of that to optimize such patterns of per field 
> user copies.

So, there's catch block to handle GPF and the code inside of `try'
block is still branch-less, right? I was thinking of minimized version
of struct siginfo (up to si_uid) and copying it with single
copy_to_user(), but the idea with try/catch is definitely much
better.

-- 
wbr, Vitaly

Re: [PATCH 1/5] Split wait_noreap_copyout()

[Ingo Molnar]

* Vitaly Mayatskikh <v.mayatskih@gmail.com> wrote:

> At Mon, 11 May 2009 14:17:08 +0200, Ingo Molnar wrote:
> 
> > > 	if (put_user(signal, &infop->si_signo) ||
> > > 	    put_user(0, &infop->si_errno) ||
> > > 	    put_user((short)why, &infop->si_code) ||
> > > 	    put_user(pid, &infop->si_pid) ||
> > > 	    put_user(uid, &infop->si_uid) ||
> > > 	    put_user(status, &infop->si_status))
> > > 		return -EFAULT;
> > 
> > For best assembly code this should generally be written as a series 
> > of:
> > 
> >    __uaccess_err |= __put_user(x, ptr);
> >    __uaccess_err |= __put_user(y, ptr);
> >    __uaccess_err |= __put_user(z, ptr);
> > 
> > As this generates non-dependent, compressed, branch-less code.
> 
> Yeah, my first intention was to eliminate a lot of branches in one
> place. It's terrible for CPU pipeline, I bet.
> 
> > See the (new) put_user_try / put_user_ex() / put_user_catch() 
> > abstraction in arch/x86/include/asm/uaccess.h, and how all the 
> > x86 signal code makes use of that to optimize such patterns of 
> > per field user copies.
> 
> So, there's catch block to handle GPF and the code inside of `try' 
> block is still branch-less, right? I was thinking of minimized 
> version of struct siginfo (up to si_uid) and copying it with 
> single copy_to_user(), but the idea with try/catch is definitely 
> much better.

It creates really nice assembly code. Hiroshi-san experimented with 
it a lot until he found this form.

Regarding potentially generalizing that facility into generic code, 
it relies on the exception code filling in 
current_thread_info()->uaccess_err with -EFAULT. So it needs 
architecture level support. It also kind of relies on 
current_thread_info()->uaccess_err being super-optimal - which it is 
on x86. (the assembler can optimize it)

But a compatible wrapper could be added, for architectures that dont 
support, or that dont need support.

	Ingo

Q: put_user_try & co (Was: [PATCH 1/5] Split wait_noreap_copyout())

[Oleg Nesterov]

On 05/11, Ingo Molnar wrote:
>
> See the (new) put_user_try / put_user_ex() / put_user_catch()
> abstraction in arch/x86/include/asm/uaccess.h, and how all the x86
> signal code makes use of that to optimize such patterns of per field
> user copies.

Just curious, can't we simplify put_user_{try,ex,catch} ?

Pseudo-code:

	#define put_user_try				\
		do {					\
			__label__  __efault_label;	\


	#define put_user_catch(err)			\
			err = 0;			\
			if (0) {			\
			__efault_label:			\
				err = -EFAULT;		\
			}				\
		while (0)
		

	#define __put_user_asm_ex(...)				\
		asm volatile(					\
			"1:	mov ..."			\
			_ASM_EXTABLE(1b, &__efault_label)	\
		     	: : ...)


Now, we don't need thread_info->uaccess_err, and we don't need the
special "if (fixup->fixup < 16)" hack in fixup_exception(). Once
any put_user_ex() fails, we jump to the __efault_label and set
err = -EFAULT.

This also means that we skip other put_user_ex's after the faulted
one. Not very important, this is unlikely case, but imho nice anyway.

Can this work? (warning: my asm skills is almost zero ;)

Oleg.

Re: Q: put_user_try & co (Was: [PATCH 1/5] Split wait_noreap_copyout())

[Roland McGrath]

> 	#define __put_user_asm_ex(...)				\
> 		asm volatile(					\
> 			"1:	mov ..."			\
> 			_ASM_EXTABLE(1b, &__efault_label)	\
> 		     	: : ...)

You mean &&__efault_label here (it's a funny syntax, but that's how it is).
&&label is a GCC extension that I'm not sure the kernel has used before.

I think it can be touchy to have an asm jump into compiled code that way.
e.g., perhaps the compiler produced:

	mov reg, 40(sp)
	mov $123, reg
	#APP
	... inside of your asm ...
	#NO_APP
	mov 40(sp), reg

or some such thing.  If you jump away from inside the asm, you won't ever
do "mov 40(sp), reg".  But the compiler might think that reg has its
original value at the __efault_label: code location.

Perhaps more important than any particular compiler-confusion scenario we
can come up with is simply that this would be an obscure corner of code
generation in the compiler that the kernel has not evoked before.  There
might be bugs or oddities in various compilers of various vintages, that
we don't know about because they never came up before.


Thanks,
Roland

Re: Q: put_user_try & co (Was: [PATCH 1/5] Split wait_noreap_copyout())

[H. Peter Anvin]

Roland McGrath wrote:
> 
> You mean &&__efault_label here (it's a funny syntax, but that's how it is).
> &&label is a GCC extension that I'm not sure the kernel has used before.
> 
> I think it can be touchy to have an asm jump into compiled code that way.
> e.g., perhaps the compiler produced:
> 
> 	mov reg, 40(sp)
> 	mov $123, reg
> 	#APP
> 	... inside of your asm ...
> 	#NO_APP
> 	mov 40(sp), reg
> 
> or some such thing.  If you jump away from inside the asm, you won't ever
> do "mov 40(sp), reg".  But the compiler might think that reg has its
> original value at the __efault_label: code location.
> 
> Perhaps more important than any particular compiler-confusion scenario we
> can come up with is simply that this would be an obscure corner of code
> generation in the compiler that the kernel has not evoked before.  There
> might be bugs or oddities in various compilers of various vintages, that
> we don't know about because they never came up before.
> 

Yes, it seems like a bad idea to me.

	-hpa

Re: Q: put_user_try & co (Was: [PATCH 1/5] Split wait_noreap_copyout())

[Oleg Nesterov]

On 05/20, Roland McGrath wrote:
>
> > 	#define __put_user_asm_ex(...)				\
> > 		asm volatile(					\
> > 			"1:	mov ..."			\
> > 			_ASM_EXTABLE(1b, &__efault_label)	\
> > 		     	: : ...)
>
> You mean &&__efault_label here (it's a funny syntax, but that's how it is).
> &&label is a GCC extension that I'm not sure the kernel has used before.
>
> I think it can be touchy to have an asm jump into compiled code that way.
> e.g., perhaps the compiler produced:
>
> 	mov reg, 40(sp)
> 	mov $123, reg
> 	#APP
> 	... inside of your asm ...
> 	#NO_APP
> 	mov 40(sp), reg
>
> or some such thing.  If you jump away from inside the asm, you won't ever
> do "mov 40(sp), reg".  But the compiler might think that reg has its
> original value at the __efault_label: code location.
>
> Perhaps more important than any particular compiler-confusion scenario we
> can come up with is simply that this would be an obscure corner of code
> generation in the compiler that the kernel has not evoked before.  There
> might be bugs or oddities in various compilers of various vintages, that
> we don't know about because they never came up before.

Yes, agreed. Thanks to all for replies.

Oleg.

Re: Q: put_user_try & co (Was: [PATCH 1/5] Split wait_noreap_copyout())

[Andreas Schwab]

Oleg Nesterov <oleg@redhat.com> writes:

> Pseudo-code:
>
> 	#define put_user_try				\
> 		do {					\
> 			__label__  __efault_label;	\
>
>
> 	#define put_user_catch(err)			\
> 			err = 0;			\
> 			if (0) {			\
> 			__efault_label:			\
> 				err = -EFAULT;		\
> 			}				\
> 		while (0)
> 		
>
> 	#define __put_user_asm_ex(...)				\
> 		asm volatile(					\
> 			"1:	mov ..."			\
> 			_ASM_EXTABLE(1b, &__efault_label)	\
> 		     	: : ...)

The address of local labels can only be used in connection with computed
gotos, otherwise you get unspecified results.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 1/5] Split wait_noreap_copyout()

[Vitaly Mayatskikh]

At Mon, 11 May 2009 08:04:18 -0400, Christoph Hellwig wrote:

> wouldn't this better be written as:
> 
> static int wait_copyout(struct wait_opts *wo, struct task_struct *p,
> 			pid_t pid, uid_t uid, int why, int status, int signal)
> {
> 	struct siginfo __user *infop = wo->wo_info;
> 
> 	if (wo->wo_rusage) {
> 		int retval = getrusage(p, RUSAGE_BOTH, wo->wo_rusage);
> 		if (retval)
> 			return retval;
> 	}
> 
> 	if (!infop)
> 		return 0;
> 
> 	if (put_user(signal, &infop->si_signo) ||
> 	    put_user(0, &infop->si_errno) ||
> 	    put_user((short)why, &infop->si_code) ||
> 	    put_user(pid, &infop->si_pid) ||
> 	    put_user(uid, &infop->si_uid) ||
> 	    put_user(status, &infop->si_status))
> 		return -EFAULT;
> 	return 0;
> }

Yes. But I'm planning to get rid of put_user() in next patches.

-- 
wbr, Vitaly

[PATCH 2/5] Use wait_copyout() in wait_task_stopped()

[Vitaly Mayatskikh]

All copy-paste getrusage() and put_user() code in wait_task_* functions
is replaced by call to wait_copyout()

Also, there's no reason to have two almost similar branches of copyout
in wait_task_stopped(). The later branch also puts stat_addr to user,
but it can't affect WNOWAIT flag, and it's ok to merge both branches.

Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
---
 kernel/exit.c |   20 +-------------------
 1 files changed, 1 insertions(+), 19 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index cbc5623..3b2fdd9 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1345,7 +1345,6 @@ static int *task_stopped_code(struct task_struct *p, bool ptrace)
 static int wait_task_stopped(struct wait_opts *wo,
 				int ptrace, struct task_struct *p)
 {
-	struct siginfo __user *infop;
 	int retval, exit_code, *p_code, why;
 	uid_t uid = 0; /* unneeded, required by compiler */
 	pid_t pid;
@@ -1389,27 +1388,10 @@ unlock_sig:
 	why = ptrace ? CLD_TRAPPED : CLD_STOPPED;
 	read_unlock(&tasklist_lock);
 
-	if (unlikely(wo->wo_flags & WNOWAIT))
-		return wait_noreap_copyout(wo, p, pid, uid, why, exit_code);
+	retval = wait_copyout(wo, p, pid, uid, why, exit_code, SIGCHLD);
 
-	retval = wo->wo_rusage
-		? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
 	if (!retval && wo->wo_stat)
 		retval = put_user((exit_code << 8) | 0x7f, wo->wo_stat);
-
-	infop = wo->wo_info;
-	if (!retval && infop)
-		retval = put_user(SIGCHLD, &infop->si_signo);
-	if (!retval && infop)
-		retval = put_user(0, &infop->si_errno);
-	if (!retval && infop)
-		retval = put_user((short)why, &infop->si_code);
-	if (!retval && infop)
-		retval = put_user(exit_code, &infop->si_status);
-	if (!retval && infop)
-		retval = put_user(pid, &infop->si_pid);
-	if (!retval && infop)
-		retval = put_user(uid, &infop->si_uid);
 	if (!retval)
 		retval = pid;
 	put_task_struct(p);
-- 
1.6.2.2

[PATCH 3/5] Use wait_copyout() in do_wait()

[Vitaly Mayatskikh]

All copy-paste getrusage() and put_user() code in wait_task_* functions
is replaced by call to wait_copyout()

Use wait_copyout() in do_wait() to clean up user's siginfo structure.

Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
---
 kernel/exit.c |   15 +--------------
 1 files changed, 1 insertions(+), 14 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 3b2fdd9..6ee6b53 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1600,8 +1600,6 @@ end:
 	__set_current_state(TASK_RUNNING);
 	remove_wait_queue(&current->signal->wait_chldexit,&wait);
 	if (wo->wo_info) {
-		struct siginfo __user *infop = wo->wo_info;
-
 		if (retval > 0)
 			retval = 0;
 		else {
@@ -1610,18 +1608,7 @@ end:
 			 * we would set so the user can easily tell the
 			 * difference.
 			 */
-			if (!retval)
-				retval = put_user(0, &infop->si_signo);
-			if (!retval)
-				retval = put_user(0, &infop->si_errno);
-			if (!retval)
-				retval = put_user(0, &infop->si_code);
-			if (!retval)
-				retval = put_user(0, &infop->si_pid);
-			if (!retval)
-				retval = put_user(0, &infop->si_uid);
-			if (!retval)
-				retval = put_user(0, &infop->si_status);
+			retval = wait_copyout(wo, 0, 0, 0, 0, 0, 0);
 		}
 	}
 	return retval;
-- 
1.6.2.2

[PATCH 4/5] Use wait_copyout() in wait_task_zombie()

[Vitaly Mayatskikh]

All copy-paste getrusage() and put_user() code in wait_task_* functions
is replaced by call to wait_copyout()

Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
---
 kernel/exit.c |   39 +++++++++++----------------------------
 1 files changed, 11 insertions(+), 28 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 6ee6b53..1ff490d 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1165,17 +1165,15 @@ static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
 static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
 {
 	unsigned long state;
-	int retval, status, traced;
+	int retval, why, status, traced;
 	pid_t pid = task_pid_vnr(p);
 	uid_t uid = __task_cred(p)->uid;
-	struct siginfo __user *infop;
 
 	if (!likely(wo->wo_flags & WEXITED))
 		return 0;
 
 	if (unlikely(wo->wo_flags & WNOWAIT)) {
 		int exit_code = p->exit_code;
-		int why, status;
 
 		get_task_struct(p);
 		read_unlock(&tasklist_lock);
@@ -1267,36 +1265,21 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
 	 */
 	read_unlock(&tasklist_lock);
 
-	retval = wo->wo_rusage
-		? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
 	status = (p->signal->flags & SIGNAL_GROUP_EXIT)
 		? p->signal->group_exit_code : p->exit_code;
-	if (!retval && wo->wo_stat)
+	if (wo->wo_stat)
 		retval = put_user(status, wo->wo_stat);
 
-	infop = wo->wo_info;
-	if (!retval && infop)
-		retval = put_user(SIGCHLD, &infop->si_signo);
-	if (!retval && infop)
-		retval = put_user(0, &infop->si_errno);
-	if (!retval && infop) {
-		int why;
-
-		if ((status & 0x7f) == 0) {
-			why = CLD_EXITED;
-			status >>= 8;
-		} else {
-			why = (status & 0x80) ? CLD_DUMPED : CLD_KILLED;
-			status &= 0x7f;
-		}
-		retval = put_user((short)why, &infop->si_code);
-		if (!retval)
-			retval = put_user(status, &infop->si_status);
+	if ((status & 0x7f) == 0) {
+		why = CLD_EXITED;
+		status >>= 8;
+	} else {
+		why = (status & 0x80) ? CLD_DUMPED : CLD_KILLED;
+		status &= 0x7f;
 	}
-	if (!retval && infop)
-		retval = put_user(pid, &infop->si_pid);
-	if (!retval && infop)
-		retval = put_user(uid, &infop->si_uid);
+
+	retval = wait_copyout(wo, p, pid, uid, why, status, SIGCHLD);
+
 	if (!retval)
 		retval = pid;
 
-- 
1.6.2.2

[PATCH 5/5] Use wait_copyout() in wait_task_continued()

[Vitaly Mayatskikh]

All copy-paste getrusage() and put_user() code in wait_task_* functions
is replaced by call to wait_copyout()

It makes no sense to do conditional siginfo's fill, wait_copyout() knows
how to handle it right.

Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
---
 kernel/exit.c |   23 ++++++++++-------------
 1 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 1ff490d..690a1a6 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1416,19 +1416,16 @@ static int wait_task_continued(struct wait_opts *wo, struct task_struct *p)
 	get_task_struct(p);
 	read_unlock(&tasklist_lock);
 
-	if (!wo->wo_info) {
-		retval = wo->wo_rusage
-			? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
-		put_task_struct(p);
-		if (!retval && wo->wo_stat)
-			retval = put_user(0xffff, wo->wo_stat);
-		if (!retval)
-			retval = pid;
-	} else {
-		retval = wait_noreap_copyout(wo, p, pid, uid,
-					     CLD_CONTINUED, SIGCONT);
-		BUG_ON(retval == 0);
-	}
+	retval = wait_copyout(wo, p, pid, uid,
+			      CLD_CONTINUED, SIGCONT, SIGCHLD);
+	put_task_struct(p);
+
+	if (!retval && wo->wo_stat)
+		retval = put_user(0xffff, wo->wo_stat);
+	if (!retval)
+		retval = pid;
+
+	BUG_ON(retval == 0);
 
 	return retval;
 }
-- 
1.6.2.2