From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755375AbbCEU3V (ORCPT ); Thu, 5 Mar 2015 15:29:21 -0500 Received: from mail-ie0-f181.google.com ([209.85.223.181]:39753 "EHLO mail-ie0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752983AbbCEU3T (ORCPT ); Thu, 5 Mar 2015 15:29:19 -0500 Date: Thu, 5 Mar 2015 12:29:16 -0800 (PST) From: David Rientjes X-X-Sender: rientjes@chino.kir.corp.google.com To: Alex Dowad cc: Andrew Morton , Oleg Nesterov , Peter Zijlstra , Ingo Molnar , Rik van Riel , Vladimir Davydov , "Kirill A. Shutemov" , Thomas Gleixner , Kees Cook , Aaron Tomlin , open list Subject: Re: [PATCH] do_fork(): Rename 'stack_size' argument to reflect actual use In-Reply-To: <54F83B81.5060003@gmail.com> Message-ID: References: <1425472814-5127-1-git-send-email-alexinbeijing@gmail.com> <54F83B81.5060003@gmail.com> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 5 Mar 2015, Alex Dowad wrote: > > > 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"? > I would recommend exactly "arg" since it can be used for multiple purposes and if the formal could ever be used for a third purpose we don't want to go through another re-naming patch to change it from sp_or_fn or usp_or_fn. If that's done, then the comment above the function could define what arg can represent.