lkml.org 
[lkml]   [2009]   [May]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [FOR REVIEW, PATCH 2/2] introduce "struct wait_opts" to simplify do_wait() pathes

* 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


\
 
 \ /
  Last update: 2009-05-06 09:31    [W:1.293 / U:1.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site