Sorry.
my e-mail was rejected by vger.kernel.org server.
resend my e-mail.
2013/11/27 DaeSeok Youn <daeseok.youn@xxxxxxxxx>
>
> Thank you for reviewing my patch.
>
> I am a newbie who want to know kernel source deeply. :)
>
> I agree your comments but I want to explain my patch why it is sent.
>
> I think "ENOMEM" is enough to explain to fail by dup_mm().
> So I replace the goto line to "return -ENOMEM" in if(!mm).
>
> And I think oldmm doesn't use any meaning control in copy_mm(),
> so re-use local 'mm' variable. If the source has less comprehensible,
> how about adding a comment like 'parent and child share address space'
> above "mm = current->mm" in my patch.
>
> just my opinion, let me know if i am wrong.
> Actually I need to learn about kernel source code.
>
> Thanks.
> Daeseok Youn.
>
>
>
> 2013/11/27 Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
>
>> Daeseok Youn <daeseok.youn@xxxxxxxxx> writes:
>>
>> > From cec2f201f0dc99a33a58d9d1e0452140bb0993a1 Mon Sep 17 00:00:00 2001
>> > From: Daeseok Youn <daeseok.youn@xxxxxxx>
>> > Date: Wed, 27 Nov 2013 09:54:41 +0900
>> > Subject: [PATCH] kernel/fork.c : remove local 'oldmm' and retval
>> >
>> > Local oldmm is used only for increaing mm_users field
>> > in current->mm. When clone_flags have a CLONE_VM flag,
>> > current->mm is assigning to local 'mm'.
>> > Local retval is used only for returning -ENOMEM value.
>> > When dup_mm() is failed, just return -ENOMEM.
>>
>> You are making the generated code worse, and the source less
>> comprehensible.
>>
>> You are adding additional exit points making it harder to analyze the
>> function.
>>
>> You are introducing races and expense by not caching current->mm in
>> oldmm.
>>
>> This looks like code churn for no good reason, and that will result in
>> worse code.
>>
>> ick.
>>
>> Eric
>>
>> > Signed-off-by: Daeseok Youn <daeseok.youn@xxxxxxxxx>
>> > ---
>> > kernel/fork.c | 16 +++++-----------
>> > 1 file changed, 5 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/kernel/fork.c b/kernel/fork.c
>> > index 728d5be..022a0af 100644
>> > --- a/kernel/fork.c
>> > +++ b/kernel/fork.c
>> > @@ -857,8 +857,7 @@ fail_nocontext:
>> >
>> > static int copy_mm(unsigned long clone_flags, struct task_struct *tsk)
>> > {
>> > - struct mm_struct *mm, *oldmm;
>> > - int retval;
>> > + struct mm_struct *mm;
>> >
>> > tsk->min_flt = tsk->maj_flt = 0;
>> > tsk->nvcsw = tsk->nivcsw = 0;
>> > @@ -874,28 +873,23 @@ static int copy_mm(unsigned long clone_flags, struct task_struct *tsk)
>> > *
>> > * We need to steal a active VM for that..
>> > */
>> > - oldmm = current->mm;
>> > - if (!oldmm)
>> > + if (!current->mm)
>> > return 0;
>> >
>> > if (clone_flags & CLONE_VM) {
>> > - atomic_inc(&oldmm->mm_users);
>> > - mm = oldmm;
>> > + mm = current->mm;
>> > + atomic_inc(&mm->mm_users);
>> > goto good_mm;
>> > }
>> >
>> > - retval = -ENOMEM;
>> > mm = dup_mm(tsk);
>> > if (!mm)
>> > - goto fail_nomem;
>> > + return -ENOMEM;
>> >
>> > good_mm:
>> > tsk->mm = mm;
>> > tsk->active_mm = mm;
>> > return 0;
>> > -
>> > -fail_nomem:
>> > - return retval;
>> > }
>> >
>> > static int copy_fs(unsigned long clone_flags, struct task_struct *tsk)
>
>
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
