[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

[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

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 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 1/5] Split wait_noreap_copyout()

[Vitaly Mayatskikh]

Move getrusage() and put_user() code from wait_noreap_copyout()
to copy_wait_opts_to_user(). 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..9546362 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 copy_wait_opts_to_user(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 = copy_wait_opts_to_user(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()

[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

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

[Andrew Morton]

On Mon, 11 May 2009 15:25:50 +0200
Vitaly Mayatskikh <v.mayatskih@gmail.com> wrote:

> +	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);
> +	}

I'll assume that "More changes to come" includes making all this somewhat
less inefficient ;)

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

[Vitaly Mayatskikh]

At Mon, 11 May 2009 15:25:50 +0200, Vitaly Mayatskikh wrote:
> 
> Move getrusage() and put_user() code from wait_noreap_copyout()
> to copy_wait_opts_to_user(). 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..9546362 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 copy_wait_opts_to_user(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);
...
> +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 = copy_wait_opts_to_user(wo, p, pid, uid, why, status, SIGCHLD);
>  	put_task_struct(p);
> -	infop = wo->wo_info;
> -	if (!retval)
> -		retval = put_user(SIGCHLD, &infop->si_signo);
...

Oleg has pointed me to broken behaviour here. Previously
wait_noreap_copyout was doing unconditional put_user and was returning
EFAULT when infop is NULL. Now it uses copy_wait_opts_to_user, which
checks infop and return NULL in the same case. This change is visible
from userspace in waitid() function.

There're 2 opportunities how to deal with new behaviour:

1. Assume wait_task_zombie had a bug previously, and let this patch go.
2. Fix copy_wait_opts_to_user to old behaviour by something like:

	if (!retval && (infop || WNOWAIT)) {

What's your opinion?

-- 
wbr, Vitaly

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

[Oleg Nesterov]

On 05/20, Vitaly Mayatskikh wrote:
>
> At Mon, 11 May 2009 15:25:50 +0200, Vitaly Mayatskikh wrote:
> >
> Oleg has pointed me to broken behaviour here. Previously
> wait_noreap_copyout was doing unconditional put_user and was returning
> EFAULT when infop is NULL. Now it uses copy_wait_opts_to_user, which
> checks infop and return NULL in the same case. This change is visible
> from userspace in waitid() function.

To me, this behaviour change looks like the cleanup (if not fix) too.
But of course, this should be discussed and at least documented in the
changelog.

do_wait() && infop interaction is really strange before the patch.

When do_wait() is called without WNOWAIT, then infop == NULL is fine.

If WNOWAIT is set, we return -EFAULT. Except in WCONTINUED case
infop == NULL is fine again.


Can somebody explain what is the supposed behaviour?


Otherwise, in my opinion this series realy makes the code better,
and afaics it allows to simplify the code more.

Oleg.

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

[Ingo Molnar]

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

> At Mon, 11 May 2009 15:25:50 +0200, Vitaly Mayatskikh wrote:
> > 
> > Move getrusage() and put_user() code from wait_noreap_copyout()
> > to copy_wait_opts_to_user(). 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..9546362 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 copy_wait_opts_to_user(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);
> ...
> > +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 = copy_wait_opts_to_user(wo, p, pid, uid, why, status, SIGCHLD);
> >  	put_task_struct(p);
> > -	infop = wo->wo_info;
> > -	if (!retval)
> > -		retval = put_user(SIGCHLD, &infop->si_signo);
> ...
> 
> Oleg has pointed me to broken behaviour here. Previously
> wait_noreap_copyout was doing unconditional put_user and was returning
> EFAULT when infop is NULL. Now it uses copy_wait_opts_to_user, which
> checks infop and return NULL in the same case. This change is visible
> from userspace in waitid() function.
> 
> There're 2 opportunities how to deal with new behaviour:
> 
> 1. Assume wait_task_zombie had a bug previously, and let this patch go.
> 2. Fix copy_wait_opts_to_user to old behaviour by something like:
> 
> 	if (!retval && (infop || WNOWAIT)) {
> 
> What's your opinion?

I'd suggest a variant of 2: keep this large-ish patch an equivalent 
transformation - i.e. an impact: cleanup type of change.

Then queue up a patch that removes this quirk. Should this change 
break any user-space, the quirk can be reinstated promptly, without 
affecting the cleanups. It will also be a very clear, easy target to 
bisect to - not obscured by clean-up details.

Does this sound good to you?

	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: [PATCH 1/5] Split wait_noreap_copyout()

[Roland McGrath]

> do_wait() && infop interaction is really strange before the patch.

do_wait() underlies sys_wait4() and sys_waitid().  The original intent was
that all the infop==NULL cases are just for the sys_wait4() path.  In the
sys_waitid() path, infop comes from the user and NULL always ought to have
been invalid.

See http://lkml.org/lkml/2009/1/13/446 for the previous thread about this.
We wanted to clean it up, but Linus objected to changing the userland
behavior of passing NULL to waitid on the grounds of "never regress the
ABI, even if it was not supposed to be the ABI".

> When do_wait() is called without WNOWAIT, then infop == NULL is fine.
> 
> If WNOWAIT is set, we return -EFAULT. Except in WCONTINUED case
> infop == NULL is fine again.

WNOWAIT can only be set in the sys_waitid() path, not by sys_wait4().
Without WNOWAIT, it might be sys_wait4(), where infop==NULL is normal.
The WCONTINUED variance was unintended.

I would be fine with any way you want to clean this up.
But presumably Linus would object again if any combination of userland
arguments that is now permitted were to start returning an error.
I'm guessing he won't object to making the WNOWAIT case consistent
with other sys_waitid() calls that pass NULL (i.e. -EFAULT -> success
acceptable, but success -> -EFAULT not acceptable).


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())

[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: 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: [PATCH 1/5] Split wait_noreap_copyout()

[Oleg Nesterov]

On 05/20, Ingo Molnar wrote:
>
> * Vitaly Mayatskikh <v.mayatskih@gmail.com> wrote:
>
> > 2. Fix copy_wait_opts_to_user to old behaviour by something like:
> >
> > 	if (!retval && (infop || WNOWAIT)) {
> >
> > What's your opinion?
>
> I'd suggest a variant of 2: keep this large-ish patch an equivalent
> transformation - i.e. an impact: cleanup type of change.
>
> Then queue up a patch that removes this quirk.

Yes, this would be the best option.

The problem is, it is not trivial to keep the current behaviour and
make the patch which looks like a cleanup, not uglification.

copy_wait_opts_to_user() needs the new "called_from_wait_task_continued"
argument, and it should do

	if (called_from_wait_task_continued && !wo->wo_info)
		return -EFAULT;

Or we should add

	if (!infop && WNOWAIT)
		return -EFAULT;

to all callers except wait_task_continued().


Roland thinks that "-EFAULT -> success" change is acceptable, and I think
the same. So, to me the best option is just  change the changelog of this
patch and that is all.

Or. We can make a trivial patch which adds the behavior change first:

	Changelog: always accept the NULL infop, because it is not
	possible to understand the current behaviour ;)

	User-visible change! needs Acks!

	--- a/kernel/exit.c
	+++ b/kernel/exit.c
	@@ -1126,24 +1126,26 @@ static int eligible_child(struct wait_op
	 static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
					pid_t pid, uid_t uid, int why, int status)
	 {
	-	struct siginfo __user *infop;
		int retval = wo->wo_rusage
			? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
	-
		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 (wo->wo_info) {
	+		struct siginfo __user *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;

And then redo Vitaly's patches on top of this change.

What do you and Vitaly think?

Oleg.

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

[Vitaly Mayatskikh]

At Thu, 21 May 2009 16:12:50 +0200, Oleg Nesterov wrote:

> Roland thinks that "-EFAULT -> success" change is acceptable, and I think
> the same. So, to me the best option is just  change the changelog of this
> patch and that is all.
> 
> Or. We can make a trivial patch which adds the behavior change first:
> 
> 	Changelog: always accept the NULL infop, because it is not
> 	possible to understand the current behaviour ;)
> 
> 	User-visible change! needs Acks!
> 
> 	--- a/kernel/exit.c
> 	+++ b/kernel/exit.c
> 	@@ -1126,24 +1126,26 @@ static int eligible_child(struct wait_op
> 	 static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
> 					pid_t pid, uid_t uid, int why, int status)
> 	 {
> 	-	struct siginfo __user *infop;
> 		int retval = wo->wo_rusage
> 			? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
> 	-
> 		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 (wo->wo_info) {
> 	+		struct siginfo __user *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;
> 
> And then redo Vitaly's patches on top of this change.
> 
> What do you and Vitaly think?

I like your idea.

-- 
wbr, Vitaly

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

[Vitaly Mayatskikh]

Hi, Andrew!

Can you, please, update description of
wait_task_-cleanups-split-wait_noreap_copyout.patch?

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

User visible change: previously, waitid() was doing unconditional
put_user() in wait_noreap_copyout(), and was returning EFAULT
if infop pointer, provided by user, was NULL *and* WCONTINUED was not
set in options. Now all variants of wait() function use the same generic
copy_wait_opts_to_user(), and, thus, have the same behaviour regarding
to filling of siginfo structure. Starting from this commit waitid()
returns NULL if infop argument is NULL.
----------------------------------------------------------------------

Thanks!

> 
> Move getrusage() and put_user() code from wait_noreap_copyout()
> to copy_wait_opts_to_user(). 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..9546362 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 copy_wait_opts_to_user(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 = copy_wait_opts_to_user(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
> 
> 

-- 
wbr, Vitaly