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

[Alex Dowad]

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 <alexinbeij...@gmail.com>
---

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/