[FOR REVIEW, PATCH 2/2] introduce "struct wait_opts" to simplify do_wait() pathes

[FOR REVIEW, PATCH 2/2] introduce "struct wait_opts" to simplify do_wait() pathes

[Oleg Nesterov]

Introduce "struct wait_opts" which holds the parameters for misc helpers
in do_wait() pathes.

This adds 17 lines to kernel/exit.c, but saves 256 bytes from .o and imho
makes the code much more readable.

(untested, not signed)

 kernel/exit.c |  211 +++++++++++++++++++++++++++++++---------------------------
 1 file changed, 114 insertions(+), 97 deletions(-)

--- PTRACE/kernel/exit.c~2_WOPTS	2009-05-06 01:48:51.000000000 +0200
+++ PTRACE/kernel/exit.c	2009-05-06 07:00:45.000000000 +0200
@@ -1076,6 +1076,18 @@ SYSCALL_DEFINE1(exit_group, int, error_c
 	return 0;
 }
 
+struct wait_opts {
+	enum pid_type		wtype;
+	struct pid		*wpid;
+	int			wflags;
+
+	struct siginfo __user	*winfo;
+	int __user		*wstat;
+	struct rusage __user	*wrusage;
+
+	int			notask_error;
+};
+
 static struct pid *task_pid_type(struct task_struct *task, enum pid_type type)
 {
 	struct pid *pid = NULL;
@@ -1086,13 +1098,12 @@ static struct pid *task_pid_type(struct 
 	return pid;
 }
 
-static int eligible_child(enum pid_type type, struct pid *pid, int options,
-			  struct task_struct *p)
+static int eligible_child(struct wait_opts *wopts, struct task_struct *p)
 {
 	int err;
 
-	if (type < PIDTYPE_MAX) {
-		if (task_pid_type(p, type) != pid)
+	if (wopts->wtype < PIDTYPE_MAX) {
+		if (task_pid_type(p, wopts->wtype) != wopts->wpid)
 			return 0;
 	}
 
@@ -1101,8 +1112,8 @@ static int eligible_child(enum pid_type 
 	 * set; otherwise, wait for non-clone children *only*.  (Note:
 	 * A "clone" child here is one that reports to its parent
 	 * using a signal other than SIGCHLD.) */
-	if (((p->exit_signal != SIGCHLD) ^ ((options & __WCLONE) != 0))
-	    && !(options & __WALL))
+	if (((p->exit_signal != SIGCHLD) ^ !!(wopts->wflags & __WCLONE))
+	    && !(wopts->wflags & __WALL))
 		return 0;
 
 	err = security_task_wait(p);
@@ -1112,14 +1123,15 @@ static int eligible_child(enum pid_type 
 	return 1;
 }
 
-static int wait_noreap_copyout(struct task_struct *p, pid_t pid, uid_t uid,
-			       int why, int status,
-			       struct siginfo __user *infop,
-			       struct rusage __user *rusagep)
+static int wait_noreap_copyout(struct wait_opts *wopts, struct task_struct *p,
+				pid_t pid, uid_t uid, int why, int status)
 {
-	int retval = rusagep ? getrusage(p, RUSAGE_BOTH, rusagep) : 0;
+	struct siginfo __user *infop;
+	int retval = wopts->wrusage
+		? getrusage(p, RUSAGE_BOTH, wopts->wrusage) : 0;
 
 	put_task_struct(p);
+	infop = wopts->winfo;
 	if (!retval)
 		retval = put_user(SIGCHLD, &infop->si_signo);
 	if (!retval)
@@ -1143,19 +1155,18 @@ static int wait_noreap_copyout(struct ta
  * the lock and this task is uninteresting.  If we return nonzero, we have
  * released the lock and the system call should return.
  */
-static int wait_task_zombie(struct task_struct *p, int options,
-			    struct siginfo __user *infop,
-			    int __user *stat_addr, struct rusage __user *ru)
+static int wait_task_zombie(struct wait_opts *wopts, struct task_struct *p)
 {
 	unsigned long state;
 	int retval, status, traced;
 	pid_t pid = task_pid_vnr(p);
 	uid_t uid = __task_cred(p)->uid;
+	struct siginfo __user *infop;
 
-	if (!likely(options & WEXITED))
+	if (!likely(wopts->wflags & WEXITED))
 		return 0;
 
-	if (unlikely(options & WNOWAIT)) {
+	if (unlikely(wopts->wflags & WNOWAIT)) {
 		int exit_code = p->exit_code;
 		int why, status;
 
@@ -1168,8 +1179,7 @@ static int wait_task_zombie(struct task_
 			why = (exit_code & 0x80) ? CLD_DUMPED : CLD_KILLED;
 			status = exit_code & 0x7f;
 		}
-		return wait_noreap_copyout(p, pid, uid, why,
-					   status, infop, ru);
+		return wait_noreap_copyout(wopts, p, pid, uid, why, status);
 	}
 
 	/*
@@ -1250,11 +1260,14 @@ static int wait_task_zombie(struct task_
 	 */
 	read_unlock(&tasklist_lock);
 
-	retval = ru ? getrusage(p, RUSAGE_BOTH, ru) : 0;
+	retval = wopts->wrusage
+		? getrusage(p, RUSAGE_BOTH, wopts->wrusage) : 0;
 	status = (p->signal->flags & SIGNAL_GROUP_EXIT)
 		? p->signal->group_exit_code : p->exit_code;
-	if (!retval && stat_addr)
-		retval = put_user(status, stat_addr);
+	if (!retval && wopts->wstat)
+		retval = put_user(status, wopts->wstat);
+
+	infop = wopts->winfo;
 	if (!retval && infop)
 		retval = put_user(SIGCHLD, &infop->si_signo);
 	if (!retval && infop)
@@ -1322,16 +1335,16 @@ static int *task_stopped_code(struct tas
  * the lock and this task is uninteresting.  If we return nonzero, we have
  * released the lock and the system call should return.
  */
-static int wait_task_stopped(int ptrace, struct task_struct *p,
-			     int options, struct siginfo __user *infop,
-			     int __user *stat_addr, struct rusage __user *ru)
+static int wait_task_stopped(struct wait_opts *wopts,
+				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;
 
 	/* Traditionally we see ptrace'd stopped tasks regardless of options */
-	if (!ptrace && !(options & WUNTRACED))
+	if (!ptrace && !(wopts->wflags & WUNTRACED))
 		return 0;
 
 	exit_code = 0;
@@ -1345,7 +1358,7 @@ static int wait_task_stopped(int ptrace,
 	if (!exit_code)
 		goto unlock_sig;
 
-	if (!unlikely(options & WNOWAIT))
+	if (!unlikely(wopts->wflags & WNOWAIT))
 		*p_code = 0;
 
 	/* don't need the RCU readlock here as we're holding a spinlock */
@@ -1367,14 +1380,15 @@ unlock_sig:
 	why = ptrace ? CLD_TRAPPED : CLD_STOPPED;
 	read_unlock(&tasklist_lock);
 
-	if (unlikely(options & WNOWAIT))
-		return wait_noreap_copyout(p, pid, uid,
-					   why, exit_code,
-					   infop, ru);
+	if (unlikely(wopts->wflags & WNOWAIT))
+		return wait_noreap_copyout(wopts, p, pid, uid, why, exit_code);
 
-	retval = ru ? getrusage(p, RUSAGE_BOTH, ru) : 0;
-	if (!retval && stat_addr)
-		retval = put_user((exit_code << 8) | 0x7f, stat_addr);
+	retval = wopts->wrusage
+		? getrusage(p, RUSAGE_BOTH, wopts->wrusage) : 0;
+	if (!retval && wopts->wstat)
+		retval = put_user((exit_code << 8) | 0x7f, wopts->wstat);
+
+	infop = wopts->winfo;
 	if (!retval && infop)
 		retval = put_user(SIGCHLD, &infop->si_signo);
 	if (!retval && infop)
@@ -1401,15 +1415,13 @@ unlock_sig:
  * the lock and this task is uninteresting.  If we return nonzero, we have
  * released the lock and the system call should return.
  */
-static int wait_task_continued(struct task_struct *p, int options,
-			       struct siginfo __user *infop,
-			       int __user *stat_addr, struct rusage __user *ru)
+static int wait_task_continued(struct wait_opts *wopts, struct task_struct *p)
 {
 	int retval;
 	pid_t pid;
 	uid_t uid;
 
-	if (!unlikely(options & WCONTINUED))
+	if (!unlikely(wopts->wflags & WCONTINUED))
 		return 0;
 
 	if (!(p->signal->flags & SIGNAL_STOP_CONTINUED))
@@ -1421,7 +1433,7 @@ static int wait_task_continued(struct ta
 		spin_unlock_irq(&p->sighand->siglock);
 		return 0;
 	}
-	if (!unlikely(options & WNOWAIT))
+	if (!unlikely(wopts->wflags & WNOWAIT))
 		p->signal->flags &= ~SIGNAL_STOP_CONTINUED;
 	uid = __task_cred(p)->uid;
 	spin_unlock_irq(&p->sighand->siglock);
@@ -1430,17 +1442,17 @@ static int wait_task_continued(struct ta
 	get_task_struct(p);
 	read_unlock(&tasklist_lock);
 
-	if (!infop) {
-		retval = ru ? getrusage(p, RUSAGE_BOTH, ru) : 0;
+	if (!wopts->winfo) {
+		retval = wopts->wrusage
+			? getrusage(p, RUSAGE_BOTH, wopts->wrusage) : 0;
 		put_task_struct(p);
-		if (!retval && stat_addr)
-			retval = put_user(0xffff, stat_addr);
+		if (!retval && wopts->wstat)
+			retval = put_user(0xffff, wopts->wstat);
 		if (!retval)
 			retval = pid;
 	} else {
-		retval = wait_noreap_copyout(p, pid, uid,
-					     CLD_CONTINUED, SIGCONT,
-					     infop, ru);
+		retval = wait_noreap_copyout(wopts, p, pid, uid,
+					     CLD_CONTINUED, SIGCONT);
 		BUG_ON(retval == 0);
 	}
 
@@ -1450,19 +1462,16 @@ static int wait_task_continued(struct ta
 /*
  * Consider @p for a wait by @parent.
  *
- * -ECHILD should be in *@notask_error before the first call.
+ * -ECHILD should be in ->notask_error before the first call.
  * Returns nonzero for a final return, when we have unlocked tasklist_lock.
  * Returns zero if the search for a child should continue;
- * then *@notask_error is 0 if @p is an eligible child,
+ * then ->notask_error is 0 if @p is an eligible child,
  * or another error from security_task_wait(), or still -ECHILD.
  */
-static int wait_consider_task(struct task_struct *parent, int ptrace,
-			      struct task_struct *p, int *notask_error,
-			      enum pid_type type, struct pid *pid, int options,
-			      struct siginfo __user *infop,
-			      int __user *stat_addr, struct rusage __user *ru)
+static int wait_consider_task(struct wait_opts *wopts, struct task_struct *parent,
+				int ptrace, struct task_struct *p)
 {
-	int ret = eligible_child(type, pid, options, p);
+	int ret = eligible_child(wopts, p);
 	if (!ret)
 		return ret;
 
@@ -1474,8 +1483,8 @@ static int wait_consider_task(struct tas
 		 * to look for security policy problems, rather
 		 * than for mysterious wait bugs.
 		 */
-		if (*notask_error)
-			*notask_error = ret;
+		if (wopts->notask_error)
+			wopts->notask_error = ret;
 		return 0;
 	}
 
@@ -1484,7 +1493,7 @@ static int wait_consider_task(struct tas
 		 * This child is hidden by ptrace.
 		 * We aren't allowed to see it now, but eventually we will.
 		 */
-		*notask_error = 0;
+		wopts->notask_error = 0;
 		return 0;
 	}
 
@@ -1495,34 +1504,30 @@ static int wait_consider_task(struct tas
 	 * We don't reap group leaders with subthreads.
 	 */
 	if (p->exit_state == EXIT_ZOMBIE && !delay_group_leader(p))
-		return wait_task_zombie(p, options, infop, stat_addr, ru);
+		return wait_task_zombie(wopts, p);
 
 	/*
 	 * It's stopped or running now, so it might
 	 * later continue, exit, or stop again.
 	 */
-	*notask_error = 0;
+	wopts->notask_error = 0;
 
 	if (task_stopped_code(p, ptrace))
-		return wait_task_stopped(ptrace, p, options,
-					 infop, stat_addr, ru);
+		return wait_task_stopped(wopts, ptrace, p);
 
-	return wait_task_continued(p, options, infop, stat_addr, ru);
+	return wait_task_continued(wopts, p);
 }
 
 /*
  * Do the work of do_wait() for one thread in the group, @tsk.
  *
- * -ECHILD should be in *@notask_error before the first call.
+ * -ECHILD should be in ->notask_error before the first call.
  * Returns nonzero for a final return, when we have unlocked tasklist_lock.
  * Returns zero if the search for a child should continue; then
- * *@notask_error is 0 if there were any eligible children,
+ * ->notask_error is 0 if there were any eligible children,
  * or another error from security_task_wait(), or still -ECHILD.
  */
-static int do_wait_thread(struct task_struct *tsk, int *notask_error,
-			  enum pid_type type, struct pid *pid, int options,
-			  struct siginfo __user *infop, int __user *stat_addr,
-			  struct rusage __user *ru)
+static int do_wait_thread(struct wait_opts *wopts, struct task_struct *tsk)
 {
 	struct task_struct *p;
 
@@ -1531,9 +1536,7 @@ static int do_wait_thread(struct task_st
 		 * Do not consider detached threads.
 		 */
 		if (!task_detached(p)) {
-			int ret = wait_consider_task(tsk, 0, p, notask_error,
-						     type, pid, options,
-						     infop, stat_addr, ru);
+			int ret = wait_consider_task(wopts, tsk, 0, p);
 			if (ret)
 				return ret;
 		}
@@ -1542,17 +1545,12 @@ static int do_wait_thread(struct task_st
 	return 0;
 }
 
-static int ptrace_do_wait(struct task_struct *tsk, int *notask_error,
-			  enum pid_type type, struct pid *pid, int options,
-			  struct siginfo __user *infop, int __user *stat_addr,
-			  struct rusage __user *ru)
+static int ptrace_do_wait(struct wait_opts *wopts, struct task_struct *tsk)
 {
 	struct task_struct *p;
 
 	list_for_each_entry(p, &tsk->ptraced, ptrace_entry) {
-		int ret = wait_consider_task(tsk, 1, p, notask_error,
-					     type, pid, options,
-					     infop, stat_addr, ru);
+		int ret = wait_consider_task(wopts, tsk, 1, p);
 		if (ret)
 			return ret;
 	}
@@ -1560,38 +1558,36 @@ static int ptrace_do_wait(struct task_st
 	return 0;
 }
 
-static long do_wait(enum pid_type type, struct pid *pid, int options,
-		    struct siginfo __user *infop, int __user *stat_addr,
-		    struct rusage __user *ru)
+static long do_wait(struct wait_opts *wopts)
 {
 	DECLARE_WAITQUEUE(wait, current);
 	struct task_struct *tsk;
 	int retval;
 
-	trace_sched_process_wait(pid);
+	trace_sched_process_wait(wopts->wpid);
 
 	add_wait_queue(&current->signal->wait_chldexit,&wait);
 repeat:
 	/*
 	 * If there is nothing that can match our critiera just get out.
-	 * We will clear @retval to zero if we see any child that might later
-	 * match our criteria, even if we are not able to reap it yet.
+	 * We will clear ->notask_error to zero if we see any child that
+	 * might later match our criteria, even if we are not able to reap
+	 * it yet.
 	 */
-	retval = -ECHILD;
-	if ((type < PIDTYPE_MAX) && (!pid || hlist_empty(&pid->tasks[type])))
+	retval = wopts->notask_error = -ECHILD;
+	if ((wopts->wtype < PIDTYPE_MAX) &&
+	   (!wopts->wpid || hlist_empty(&wopts->wpid->tasks[wopts->wtype])))
 		goto end;
 
 	current->state = TASK_INTERRUPTIBLE;
 	read_lock(&tasklist_lock);
 	tsk = current;
 	do {
-		int tsk_result = do_wait_thread(tsk, &retval,
-						type, pid, options,
-						infop, stat_addr, ru);
+		int tsk_result = do_wait_thread(wopts, tsk);
+
 		if (!tsk_result)
-			tsk_result = ptrace_do_wait(tsk, &retval,
-						    type, pid, options,
-						    infop, stat_addr, ru);
+			tsk_result = ptrace_do_wait(wopts, tsk);
+
 		if (tsk_result) {
 			/*
 			 * tasklist_lock is unlocked and we have a final result.
@@ -1600,25 +1596,27 @@ repeat:
 			goto end;
 		}
 
-		if (options & __WNOTHREAD)
+		if (wopts->wflags & __WNOTHREAD)
 			break;
 		tsk = next_thread(tsk);
 		BUG_ON(tsk->signal != current->signal);
 	} while (tsk != current);
 	read_unlock(&tasklist_lock);
 
-	if (!retval && !(options & WNOHANG)) {
+	retval = wopts->notask_error;
+	if (!retval && !(wopts->wflags & WNOHANG)) {
 		retval = -ERESTARTSYS;
 		if (!signal_pending(current)) {
 			schedule();
 			goto repeat;
 		}
 	}
-
 end:
 	current->state = TASK_RUNNING;
 	remove_wait_queue(&current->signal->wait_chldexit,&wait);
-	if (infop) {
+	if (wopts->winfo) {
+		struct siginfo __user *infop = wopts->winfo;
+
 		if (retval > 0)
 			retval = 0;
 		else {
@@ -1647,6 +1645,7 @@ end:
 SYSCALL_DEFINE5(waitid, int, which, pid_t, upid, struct siginfo __user *,
 		infop, int, options, struct rusage __user *, ru)
 {
+	struct wait_opts wopts;
 	struct pid *pid = NULL;
 	enum pid_type type;
 	long ret;
@@ -1676,7 +1675,16 @@ SYSCALL_DEFINE5(waitid, int, which, pid_
 
 	if (type < PIDTYPE_MAX)
 		pid = find_get_pid(upid);
-	ret = do_wait(type, pid, options, infop, NULL, ru);
+
+	wopts.wtype = type;
+	wopts.wpid = pid;
+	wopts.wflags = options;
+
+	wopts.winfo = infop;
+	wopts.wstat = NULL;
+	wopts.wrusage = ru;
+
+	ret = do_wait(&wopts);
 	put_pid(pid);
 
 	/* avoid REGPARM breakage on x86: */
@@ -1687,6 +1695,7 @@ SYSCALL_DEFINE5(waitid, int, which, pid_
 SYSCALL_DEFINE4(wait4, pid_t, upid, int __user *, stat_addr,
 		int, options, struct rusage __user *, ru)
 {
+	struct wait_opts wopts;
 	struct pid *pid = NULL;
 	enum pid_type type;
 	long ret;
@@ -1708,7 +1717,15 @@ SYSCALL_DEFINE4(wait4, pid_t, upid, int 
 		pid = find_get_pid(upid);
 	}
 
-	ret = do_wait(type, pid, options | WEXITED, NULL, stat_addr, ru);
+	wopts.wtype = type;
+	wopts.wpid = pid;
+	wopts.wflags = options | WEXITED;
+
+	wopts.winfo = NULL;
+	wopts.wstat = stat_addr;
+	wopts.wrusage = ru;
+
+	ret = do_wait(&wopts);
 	put_pid(pid);
 
 	/* avoid REGPARM breakage on x86: */

Re: [FOR REVIEW, PATCH 2/2] introduce "struct wait_opts" to simplify do_wait() pathes

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> Introduce "struct wait_opts" which holds the parameters for misc 
> helpers in do_wait() pathes.
> 
> This adds 17 lines to kernel/exit.c, but saves 256 bytes from .o 
> and imho makes the code much more readable.
> 
> (untested, not signed)
> 
>  kernel/exit.c |  211 +++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 114 insertions(+), 97 deletions(-)

> +struct wait_opts {
> +	enum pid_type		wtype;
> +	struct pid		*wpid;
> +	int			wflags;
> +
> +	struct siginfo __user	*winfo;
> +	int __user		*wstat;
> +	struct rusage __user	*wrusage;
> +
> +	int			notask_error;
> +};

Nice idea!

One small nit with the definition above: when using vertical spacing 
(which really looks nice) we tend to put the asterix to the type 
itself, not to the variable. I.e.:

	enum pid_type		wtype;
	struct pid *		wpid;
	int			wflags;

( This is done to separate the field name from the type - the 
  pointer nature of the field is part of the type, not part of the
  name. )

It's impressive how well the function prototypes get compressed and 
cleaned up by this helper structure:

> -static int eligible_child(enum pid_type type, struct pid *pid, int options,
> -			  struct task_struct *p)
> +static int eligible_child(struct wait_opts *wopts, struct task_struct *p)

> -static int wait_noreap_copyout(struct task_struct *p, pid_t pid, uid_t uid,
> -			       int why, int status,
> -			       struct siginfo __user *infop,
> -			       struct rusage __user *rusagep)
> +static int wait_noreap_copyout(struct wait_opts *wopts, struct task_struct *p,
> +				pid_t pid, uid_t uid, int why, int status)

> -static int wait_task_zombie(struct task_struct *p, int options,
> -			    struct siginfo __user *infop,
> -			    int __user *stat_addr, struct rusage __user *ru)
> +static int wait_task_zombie(struct wait_opts *wopts, struct task_struct *p)

> -static int wait_task_stopped(int ptrace, struct task_struct *p,
> -			     int options, struct siginfo __user *infop,
> -			     int __user *stat_addr, struct rusage __user *ru)
> +static int wait_task_stopped(struct wait_opts *wopts,
> +				int ptrace, struct task_struct *p)

> -static int wait_task_continued(struct task_struct *p, int options,
> -			       struct siginfo __user *infop,
> -			       int __user *stat_addr, struct rusage __user *ru)
> +static int wait_task_continued(struct wait_opts *wopts, struct task_struct *p)

> -static int wait_consider_task(struct task_struct *parent, int ptrace,
> -			      struct task_struct *p, int *notask_error,
> -			      enum pid_type type, struct pid *pid, int options,
> -			      struct siginfo __user *infop,
> -			      int __user *stat_addr, struct rusage __user *ru)
> +static int wait_consider_task(struct wait_opts *wopts, struct task_struct *parent,
> +				int ptrace, struct task_struct *p)

> -static int do_wait_thread(struct task_struct *tsk, int *notask_error,
> -			  enum pid_type type, struct pid *pid, int options,
> -			  struct siginfo __user *infop, int __user *stat_addr,
> -			  struct rusage __user *ru)
> +static int do_wait_thread(struct wait_opts *wopts, struct task_struct *tsk)

> -static int ptrace_do_wait(struct task_struct *tsk, int *notask_error,
> -			  enum pid_type type, struct pid *pid, int options,
> -			  struct siginfo __user *infop, int __user *stat_addr,
> -			  struct rusage __user *ru)
> +static int ptrace_do_wait(struct wait_opts *wopts, struct task_struct *tsk)

> -static long do_wait(enum pid_type type, struct pid *pid, int options,
> -		    struct siginfo __user *infop, int __user *stat_addr,
> -		    struct rusage __user *ru)
> +static long do_wait(struct wait_opts *wopts)

One small (style) detail here as well:

> -	ret = do_wait(type, pid, options, infop, NULL, ru);
> +
> +	wopts.wtype = type;
> +	wopts.wpid = pid;
> +	wopts.wflags = options;
> +
> +	wopts.winfo = infop;
> +	wopts.wstat = NULL;
> +	wopts.wrusage = ru;
> +
> +	ret = do_wait(&wopts);

it makes sense to write this as:

> +	wopts.wtype	= type;
> +	wopts.wpid	= pid;
> +	wopts.wflags	= options;
> +
> +	wopts.winfo	= infop;
> +	wopts.wstat	= NULL;
> +	wopts.wrusage	= ru;
> +
> +	ret = do_wait(&wopts);

(and in other places as well). Vertical spacing for assignments 
looks messy if done for 1-3 assignment lines, but in the case above 
we've got 6 of them so it has a nice vertical structure already that 
helps readability.

Regarding the patch itself: i guess we could do it as-is - but if 
you think there's regression risks, a safer approach would be to 
create 5-6 patches to build up all the structure parameters one by 
one. Such a series is a lot easier to check (and a lot easier to 
bisect to).

Anyway ... provided you give it some testing:

Reviewed-by: Ingo Molnar <mingo@elte.hu>

	Ingo

Re: [FOR REVIEW, PATCH 2/2] introduce "struct wait_opts" to simplify do_wait() pathes

[Roland McGrath]

I like the idea just fine.  Drop the "w" on every field name.
If you want them to have a greppable uniqueness/prefix, use "wo_".
I'd go with a shorter name for the canonical pointer parameter,
like "w" or "wo".

It looks cleaner to me to keep the several args to do_wait() and have
it use a single initializer with = { .foo = a, .bar = b } syntax there.


Thanks,
Roland

Re: [FOR REVIEW, PATCH 2/2] introduce "struct wait_opts" to simplify do_wait() pathes

[Oleg Nesterov]

On 05/06, Ingo Molnar wrote:
>
> One small nit with the definition above: when using vertical spacing
> (which really looks nice) we tend to put the asterix to the type
> itself, not to the variable. I.e.:
>
> 	enum pid_type		wtype;
> 	struct pid *		wpid;
> 	int			wflags;
>
> ( This is done to separate the field name from the type - the
>   pointer nature of the field is part of the type, not part of the
>   name. )

Indeed, I like this more too. But checkpatch.pl disagrees!

> it makes sense to write this as:
>
> > +	wopts.wtype	= type;
> > +	wopts.wpid	= pid;
> > +	wopts.wflags	= options;
> > +
> > +	wopts.winfo	= infop;
> > +	wopts.wstat	= NULL;
> > +	wopts.wrusage	= ru;
> > +
> > +	ret = do_wait(&wopts);
>
> (and in other places as well). Vertical spacing for assignments
> looks messy if done for 1-3 assignment lines, but in the case above
> we've got 6 of them so it has a nice vertical structure already that
> helps readability.

Done.

> Regarding the patch itself: i guess we could do it as-is - but if
> you think there's regression risks, a safer approach would be to
> create 5-6 patches to build up all the structure parameters one by
> one.

Oh, I tried to do it this way first. But I got lost and decided to
make a single patch. Besides, if I make 6 patches I should try to test
each one...

> Anyway ... provided you give it some testing:

Well, I did now. But of course this needs more testing. As you see,
the patch is trivial, it "must" be correct. Except some silly typos
are possible.

> Reviewed-by: Ingo Molnar <mingo@elte.hu>

Thanks!

Oleg.

Re: [FOR REVIEW, PATCH 2/2] introduce "struct wait_opts" to simplify do_wait() pathes

[Oleg Nesterov]

On 05/06, Roland McGrath wrote:
>
> I like the idea just fine.  Drop the "w" on every field name.
> If you want them to have a greppable uniqueness/prefix,

Yes!

> use "wo_".
> I'd go with a shorter name for the canonical pointer parameter,
> like "w" or "wo".

Done.

> It looks cleaner to me to keep the several args to do_wait() and have
> it use a single initializer with = { .foo = a, .bar = b } syntax there.

Yes, I considered this option too. But since (I hope) you do not have
a strong opinion on this, I'd prefer to keep the code as is. This way
do_wait() looks more symmetrical wrt to other helpers. And we don't
copy args twice.

Oleg.

Re: [FOR REVIEW, PATCH 2/2] introduce "struct wait_opts" to simplify do_wait() pathes

[Roland McGrath]

> Yes, I considered this option too. But since (I hope) you do not have
> a strong opinion on this, I'd prefer to keep the code as is. This way
> do_wait() looks more symmetrical wrt to other helpers. And we don't
> copy args twice.

I don't feel strongly.  But I do think that those two repeated assignment
blocks are more to read and harder to read, and more error-prone for drift
in future changes (vs prototype changes getting quick compilation errors).
do_wait() is not "another helper", it's the main function.  On machines
with 6 argument registers (everything but x86-32?), the compiler probably
does fine making the callers' register shuffling be free.  On x86-32, a few
cache-hot stack stores and loads are in the tiny noise vs the whole cost of
this hairy syscall, and IMHO don't compare to source maintainability issues.


Thanks,
Roland

Re: [FOR REVIEW, PATCH 2/2] introduce "struct wait_opts" to simplify do_wait() pathes

[Oleg Nesterov]

On 05/07, Roland McGrath wrote:
>
> > Yes, I considered this option too. But since (I hope) you do not have
> > a strong opinion on this, I'd prefer to keep the code as is. This way
> > do_wait() looks more symmetrical wrt to other helpers. And we don't
> > copy args twice.
>
> I don't feel strongly.  But I do think that those two repeated assignment
> blocks are more to read and harder to read, and more error-prone for drift
> in future changes (vs prototype changes getting quick compilation errors).
> do_wait() is not "another helper", it's the main function.

I must admit, I do not agree. I feel the opposite. Yes we have 2 repeated
assignment blocks, but there are not exactly equal, and imho the difference
is more visible this way.

That said. This is not the technical issue, I can't "prove" I am right and
of course I may be wrong. I think we should follow the "maintaner is always
right" rule ;)

I'll send this change as another cleanup on top of the new series.

OK ?

> On machines
> with 6 argument registers (everything but x86-32?), the compiler probably
> does fine making the callers' register shuffling be free.  On x86-32, a few
> cache-hot stack stores and loads are in the tiny noise vs the whole cost of
> this hairy syscall, and IMHO don't compare to source maintainability issues.

Yes, I agree.

Oleg.

Re: [FOR REVIEW, PATCH 2/2] introduce "struct wait_opts" to simplify do_wait() pathes

[Roland McGrath]

> I must admit, I do not agree. I feel the opposite. Yes we have 2 repeated
> assignment blocks, but there are not exactly equal, and imho the difference
> is more visible this way.

Ok, whatever.

> That said. This is not the technical issue, I can't "prove" I am right and
> of course I may be wrong. I think we should follow the "maintaner is always
> right" rule ;)
> 
> I'll send this change as another cleanup on top of the new series.

Nah.  The "he who wrote the last patch is the real maintainer" rule is
better.  If the theoretical pitfalls I imagine ever materialize under our
feet, I'll tell everybody it was your fault (git will, anyway). ;-)


Thanks,
Roland

Re: [FOR REVIEW, PATCH 2/2] introduce "struct wait_opts" to simplify do_wait() pathes

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 05/06, Ingo Molnar wrote:
> >
> > One small nit with the definition above: when using vertical spacing
> > (which really looks nice) we tend to put the asterix to the type
> > itself, not to the variable. I.e.:
> >
> > 	enum pid_type		wtype;
> > 	struct pid *		wpid;
> > 	int			wflags;
> >
> > ( This is done to separate the field name from the type - the
> >   pointer nature of the field is part of the type, not part of the
> >   name. )
> 
> Indeed, I like this more too. But checkpatch.pl disagrees!

That's probably a checkpatch bug mistaking * for multiplication - 
ignore checkpatch in that case and please report it to Andy 
Withcroft as well as well.

> > Regarding the patch itself: i guess we could do it as-is - but 
> > if you think there's regression risks, a safer approach would be 
> > to create 5-6 patches to build up all the structure parameters 
> > one by one.
> 
> Oh, I tried to do it this way first. But I got lost and decided to 
> make a single patch. Besides, if I make 6 patches I should try to 
> test each one...

One way to do it is to build-test them on a common config (say 
64-bit defconfig), and then boot test the final result. That makes 
it fully bisectable in 90% of the cases.

But ... it's really up to you. I'd be cautious out of box, as the 
quirk density in exit.c is still enormous and it's very easy to have 
some unintended side-effect.

	Ingo

Re: [FOR REVIEW, PATCH 2/2] introduce "struct wait_opts" to simplify do_wait() pathes

[Oleg Nesterov]

(add Andy)

On 05/07, Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg@redhat.com> wrote:
>
> > On 05/06, Ingo Molnar wrote:
> > >
> > > One small nit with the definition above: when using vertical spacing
> > > (which really looks nice) we tend to put the asterix to the type
> > > itself, not to the variable. I.e.:
> > >
> > > 	enum pid_type		wtype;
> > > 	struct pid *		wpid;
> > > 	int			wflags;
> > >
> > > ( This is done to separate the field name from the type - the
> > >   pointer nature of the field is part of the type, not part of the
> > >   name. )
> >
> > Indeed, I like this more too. But checkpatch.pl disagrees!
>
> That's probably a checkpatch bug mistaking * for multiplication -
> ignore checkpatch in that case and please report it to Andy
> Withcroft as well as well.

No, this is not a bug. From scripts/checkpatch.pl

	1667                          # Should not end with a space.
	1668                          $to =~ s/\s+$//;

checkpatch explicitely dislikes "type * name". I think this is
not really right, but I won't insist. Perhaps it is better to
allow tabs before name at least, because this likely means the
code really tries ro look good.


Btw, can't resist,

	while ($to =~ s/\*\s+\*/\*\*/) {
	}

I think this can be simplified to

	$to =~ s/(?<=\*)\s+//g;

Oleg.

Re: [FOR REVIEW, PATCH 2/2] introduce "struct wait_opts" to simplify do_wait() pathes

[Andy Whitcroft]

On Sat, May 09, 2009 at 06:15:06PM +0200, Oleg Nesterov wrote:
> (add Andy)
> 
> On 05/07, Ingo Molnar wrote:
> >
> > * Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > On 05/06, Ingo Molnar wrote:
> > > >
> > > > One small nit with the definition above: when using vertical spacing
> > > > (which really looks nice) we tend to put the asterix to the type
> > > > itself, not to the variable. I.e.:
> > > >
> > > > 	enum pid_type		wtype;
> > > > 	struct pid *		wpid;
> > > > 	int			wflags;

My expectation would normally be that this would be written more as:

 	enum pid_type		wtype;
 	struct pid		*wpid;
 	int			wflags;

(more explanation below)

> > > >
> > > > ( This is done to separate the field name from the type - the
> > > >   pointer nature of the field is part of the type, not part of the
> > > >   name. )
> > >
> > > Indeed, I like this more too. But checkpatch.pl disagrees!
> >
> > That's probably a checkpatch bug mistaking * for multiplication -
> > ignore checkpatch in that case and please report it to Andy
> > Withcroft as well as well.

This is a C'ism.  The pointerness is not part of the type in C, where as
it is in C++.  In C a char * is a pointer to the type char (classically
written as "char *var").  In C++ char * is of type pointer to char
(classically written as "char* var").

> No, this is not a bug. From scripts/checkpatch.pl
> 
> 	1667                          # Should not end with a space.
> 	1668                          $to =~ s/\s+$//;
> 
> checkpatch explicitely dislikes "type * name". I think this is
> not really right, but I won't insist. Perhaps it is better to
> allow tabs before name at least, because this likely means the
> code really tries ro look good.

So yes the current behaviour is as designed.  We are aiming for a
consistent style not necessarily one any of us actually wholy agrees
with.  If the right people think this deserves relaxing we can revisit
that decision.

> Btw, can't resist,
> 
> 	while ($to =~ s/\*\s+\*/\*\*/) {
> 	}
> 
> I think this can be simplified to
> 
> 	$to =~ s/(?<=\*)\s+//g;

Heh quite possibly.  But the first is enough like line noise for me!
The latter is primarily scary.

> Oleg.

Thanks.

-apw

Re: [FOR REVIEW, PATCH 2/2] introduce "struct wait_opts" to simplify do_wait() pathes

[Ingo Molnar]

* Andy Whitcroft <apw@canonical.com> wrote:

> On Sat, May 09, 2009 at 06:15:06PM +0200, Oleg Nesterov wrote:
> > (add Andy)
> > 
> > On 05/07, Ingo Molnar wrote:
> > >
> > > * Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > > On 05/06, Ingo Molnar wrote:
> > > > >
> > > > > One small nit with the definition above: when using vertical spacing
> > > > > (which really looks nice) we tend to put the asterix to the type
> > > > > itself, not to the variable. I.e.:
> > > > >
> > > > > 	enum pid_type		wtype;
> > > > > 	struct pid *		wpid;
> > > > > 	int			wflags;
> 
> My expectation would normally be that this would be written more as:
> 
>  	enum pid_type		wtype;
>  	struct pid		*wpid;
>  	int			wflags;

Correct - i was confused there.

	Ingo