Re: [PATCH] do_fork(): Rename 'stack_size' argument to reflect actual use

From: Alex Dowad
Date: Thu Mar 05 2015 - 06:18:39 EST



On 05/03/15 01:07, David Rientjes wrote:
On Wed, 4 Mar 2015, Alex Dowad wrote:

The 'stack_size' argument is never used to pass a stack size. It's only used when
forking a kernel thread, in which case it is an argument which should be passed
to the 'main' function which the kernel thread executes. Hence, rename it to
'kthread_arg'.

Signed-off-by: Alex Dowad <alexinbeijing@xxxxxxxxx>
---

Hi,

Please have a look at this patch. If this is accepted, I have a series of patches
ready for a similar cleanup to all the arch-specific implementations of copy_thread()
(as suggested by Andrew Morton in a private e-mail).

Thank you,
Alex Dowad

kernel/fork.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index cf65139..b38a2ae 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1186,10 +1186,12 @@ init_task_pid(struct task_struct *task, enum pid_type type, struct pid *pid)
* It copies the registers, and all the appropriate
* parts of the process environment (as per the clone
* flags). The actual kick-off is left to the caller.
+ *
+ * When copying a kernel thread, 'stack_start' is the function to run.
*/
static struct task_struct *copy_process(unsigned long clone_flags,
unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long kthread_arg,
int __user *child_tidptr,
struct pid *pid,
int trace)
@@ -1401,7 +1403,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
retval = copy_io(clone_flags, p);
if (retval)
goto bad_fork_cleanup_namespaces;
- retval = copy_thread(clone_flags, stack_start, stack_size, p);
+ retval = copy_thread(clone_flags, stack_start, kthread_arg, p);
if (retval)
goto bad_fork_cleanup_io;
@@ -1629,8 +1631,8 @@ struct task_struct *fork_idle(int cpu)
* it and waits for it to finish using the VM if required.
*/
long do_fork(unsigned long clone_flags,
- unsigned long stack_start,
- unsigned long stack_size,
+ unsigned long stack_start, /* or function for kthread to run */
+ unsigned long kthread_arg,
int __user *parent_tidptr,
int __user *child_tidptr)
{
Looks fine, but I'm not sure about commenting functional formals. Since
copy_process() and do_fork() can have formals with different meanings,
then why not just rename them "arg1" and "arg2" respectively and then
define in the comment above the function what the possible combinations
are?

The second argument is *only* ever used for one thing: an argument passed to a kernel thread. That's why I would like to rename it to "kthread_arg". The previous argument (currently named "stack_start") is indeed used for 2 different things: a new stack pointer for a user thread, or a function to be executed by a kernel thread. Rather than "arg1", what would you think of something like "sp_or_fn", or "usp_or_fn"?

Thanks for your feedback!


@@ -1656,7 +1658,7 @@ long do_fork(unsigned long clone_flags,
trace = 0;
}
- p = copy_process(clone_flags, stack_start, stack_size,
+ p = copy_process(clone_flags, stack_start, kthread_arg,
child_tidptr, NULL, trace);
/*
* Do this prior waking up the new thread - the thread pointer
@@ -1740,7 +1742,7 @@ SYSCALL_DEFINE5(clone, unsigned long, newsp, unsigned long, clone_flags,
int, tls_val)
#elif defined(CONFIG_CLONE_BACKWARDS3)
SYSCALL_DEFINE6(clone, unsigned long, clone_flags, unsigned long, newsp,
- int, stack_size,
+ int, ignored,
int __user *, parent_tidptr,
int __user *, child_tidptr,
int, tls_val)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/