From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932108AbbCFGHk (ORCPT ); Fri, 6 Mar 2015 01:07:40 -0500 Received: from mail-wi0-f173.google.com ([209.85.212.173]:34954 "EHLO mail-wi0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755903AbbCFGHc (ORCPT ); Fri, 6 Mar 2015 01:07:32 -0500 Message-ID: <54F9441B.20408@gmail.com> Date: Fri, 06 Mar 2015 08:07:23 +0200 From: Alex Dowad User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: David Rientjes 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 References: <1425472814-5127-1-git-send-email-alexinbeijing@gmail.com> <54F83B81.5060003@gmail.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/03/15 22:29, David Rientjes wrote: > 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. Do others concur with this idea? Personally, I feel the code will be more readable/maintainable if the naming of args/variables/etc reflects what they are actually used for. (Case in point: on IA64, copy_thread() adds the kernel thread arg to the user stack pointer. The kernel thread arg is always 0 when forking a user process, so this "works", but it's certainly not what the author intended. Good names make it harder to write buggy code!) For readability, using the same arg for 2 different purposes is a bad practice (though it might be good for keeping the object code small). I hate to think that "arg" might be co-opted for another purpose again.