Re: [PATCH 2/2] x86/mm: replace a goto by merging two if clause
On Mon, Sep 23, 2019 at 11:22:31AM +0200, Peter Zijlstra wrote: >On Thu, Sep 19, 2019 at 10:08:44AM +0800, Wei Yang wrote: >> There is only one place to use good_area jump, which could be reduced by >> merging the following two if clause. >> >> Signed-off-by: Wei Yang >> --- >> arch/x86/mm/fault.c | 11 +-- >> 1 file changed, 5 insertions(+), 6 deletions(-) >> >> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c >> index 9d18b73b5f77..72ce6c69e195 100644 >> --- a/arch/x86/mm/fault.c >> +++ b/arch/x86/mm/fault.c >> @@ -1390,18 +1390,17 @@ void do_user_addr_fault(struct pt_regs *regs, >> vma = find_vma(mm, address); >> if (unlikely(!vma)) >> goto bad_area; >> -if (likely(vma->vm_start <= address)) >> -goto good_area; >> -if (unlikely(!(vma->vm_flags & VM_GROWSDOWN))) >> -goto bad_area; >> -if (unlikely(expand_stack(vma, address))) >> +if (likely(vma->vm_start <= address)) { >> +/* good area, do nothing */ >> +} else if (unlikely(!(vma->vm_flags & VM_GROWSDOWN)) || >> + unlikely(expand_stack(vma, address))) { >> goto bad_area; >> +} >> >> /* >> * Ok, we have a good vm_area for this memory access, so >> * we can handle it.. >> */ >> -good_area: >> if (unlikely(access_error(hw_error_code, vma))) { >> bad_area_access_error(regs, hw_error_code, address, vma); >> return; > >I find the old code far easier to read... is there any actual reason to >do this? Hi, Peter, Do you have some comment for the Patch 1? -- Wei Yang Help you, Help me
Re: [PATCH 2/2] x86/mm: replace a goto by merging two if clause
On Mon, Sep 23, 2019 at 11:22:31AM +0200, Peter Zijlstra wrote: >On Thu, Sep 19, 2019 at 10:08:44AM +0800, Wei Yang wrote: >> There is only one place to use good_area jump, which could be reduced by >> merging the following two if clause. >> >> Signed-off-by: Wei Yang >> --- >> arch/x86/mm/fault.c | 11 +-- >> 1 file changed, 5 insertions(+), 6 deletions(-) >> >> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c >> index 9d18b73b5f77..72ce6c69e195 100644 >> --- a/arch/x86/mm/fault.c >> +++ b/arch/x86/mm/fault.c >> @@ -1390,18 +1390,17 @@ void do_user_addr_fault(struct pt_regs *regs, >> vma = find_vma(mm, address); >> if (unlikely(!vma)) >> goto bad_area; >> -if (likely(vma->vm_start <= address)) >> -goto good_area; >> -if (unlikely(!(vma->vm_flags & VM_GROWSDOWN))) >> -goto bad_area; >> -if (unlikely(expand_stack(vma, address))) >> +if (likely(vma->vm_start <= address)) { >> +/* good area, do nothing */ >> +} else if (unlikely(!(vma->vm_flags & VM_GROWSDOWN)) || >> + unlikely(expand_stack(vma, address))) { >> goto bad_area; >> +} >> >> /* >> * Ok, we have a good vm_area for this memory access, so >> * we can handle it.. >> */ >> -good_area: >> if (unlikely(access_error(hw_error_code, vma))) { >> bad_area_access_error(regs, hw_error_code, address, vma); >> return; > >I find the old code far easier to read... is there any actual reason to >do this? No, just want to make it easy to read. -- Wei Yang Help you, Help me
Re: [PATCH 2/2] x86/mm: replace a goto by merging two if clause
On Thu, Sep 19, 2019 at 10:08:44AM +0800, Wei Yang wrote: > There is only one place to use good_area jump, which could be reduced by > merging the following two if clause. > > Signed-off-by: Wei Yang > --- > arch/x86/mm/fault.c | 11 +-- > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c > index 9d18b73b5f77..72ce6c69e195 100644 > --- a/arch/x86/mm/fault.c > +++ b/arch/x86/mm/fault.c > @@ -1390,18 +1390,17 @@ void do_user_addr_fault(struct pt_regs *regs, > vma = find_vma(mm, address); > if (unlikely(!vma)) > goto bad_area; > - if (likely(vma->vm_start <= address)) > - goto good_area; > - if (unlikely(!(vma->vm_flags & VM_GROWSDOWN))) > - goto bad_area; > - if (unlikely(expand_stack(vma, address))) > + if (likely(vma->vm_start <= address)) { > + /* good area, do nothing */ > + } else if (unlikely(!(vma->vm_flags & VM_GROWSDOWN)) || > +unlikely(expand_stack(vma, address))) { > goto bad_area; > + } > > /* >* Ok, we have a good vm_area for this memory access, so >* we can handle it.. >*/ > -good_area: > if (unlikely(access_error(hw_error_code, vma))) { > bad_area_access_error(regs, hw_error_code, address, vma); > return; I find the old code far easier to read... is there any actual reason to do this?
[PATCH 2/2] x86/mm: replace a goto by merging two if clause
There is only one place to use good_area jump, which could be reduced by merging the following two if clause. Signed-off-by: Wei Yang --- arch/x86/mm/fault.c | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c index 9d18b73b5f77..72ce6c69e195 100644 --- a/arch/x86/mm/fault.c +++ b/arch/x86/mm/fault.c @@ -1390,18 +1390,17 @@ void do_user_addr_fault(struct pt_regs *regs, vma = find_vma(mm, address); if (unlikely(!vma)) goto bad_area; - if (likely(vma->vm_start <= address)) - goto good_area; - if (unlikely(!(vma->vm_flags & VM_GROWSDOWN))) - goto bad_area; - if (unlikely(expand_stack(vma, address))) + if (likely(vma->vm_start <= address)) { + /* good area, do nothing */ + } else if (unlikely(!(vma->vm_flags & VM_GROWSDOWN)) || + unlikely(expand_stack(vma, address))) { goto bad_area; + } /* * Ok, we have a good vm_area for this memory access, so * we can handle it.. */ -good_area: if (unlikely(access_error(hw_error_code, vma))) { bad_area_access_error(regs, hw_error_code, address, vma); return; -- 2.17.1