| Message ID | 1559133285-27986-5-git-send-email-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/mm: Fixes and cleanups for do_page_fault() | expand |
On Wed, May 29, 2019 at 06:04:45PM +0530, Anshuman Khandual wrote: > __do_page_fault() is over complicated with multiple goto statements. This > cleans up code flow and while there drops the vm_fault_t argument. > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: James Morse <james.morse@arm.com> > Cc: Andrey Konovalov <andreyknvl@google.com> > --- > arch/arm64/mm/fault.c | 38 ++++++++++++++++---------------------- > 1 file changed, 16 insertions(+), 22 deletions(-) > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > index 170c71f..a53a30e 100644 > --- a/arch/arm64/mm/fault.c > +++ b/arch/arm64/mm/fault.c > @@ -397,37 +397,31 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re > static vm_fault_t __do_page_fault(struct mm_struct *mm, unsigned long addr, > unsigned int mm_flags, unsigned long vm_flags) > { > - struct vm_area_struct *vma; > - vm_fault_t fault; > + struct vm_area_struct *vma = find_vma(mm, addr); > > - vma = find_vma(mm, addr); > - fault = VM_FAULT_BADMAP; > if (unlikely(!vma)) > - goto out; > - if (unlikely(vma->vm_start > addr)) > - goto check_stack; > + return VM_FAULT_BADMAP; > > /* > - * Ok, we have a good vm_area for this memory access, so we can handle > - * it. > + * Check if the VMA has got the required permssion with respect > + * to the access fault here. > */ We already had a perfectly good comment for this check: /* * Check that the permissions on the VMA allow for the fault which * occurred. */ ... so please keep that and minimize the diff. > -good_area: > + if (!(vma->vm_flags & vm_flags)) > + return VM_FAULT_BADACCESS; > + > /* > - * Check that the permissions on the VMA allow for the fault which > - * occurred. > + * There is a valid VMA for this access. But before proceeding > + * make sure that it has required flags if there is an attempt > + * to expand the stack downwards. > */ I think we can drop this comment, given we didn't have it previously. > - if (!(vma->vm_flags & vm_flags)) { > - fault = VM_FAULT_BADACCESS; > - goto out; > - } > + if (unlikely(vma->vm_start > addr)) { > + if (!(vma->vm_flags & VM_GROWSDOWN)) > + return VM_FAULT_BADMAP; > > + if (expand_stack(vma, addr)) > + return VM_FAULT_BADMAP; You can drop the line space between these two if statements. > + } > return handle_mm_fault(vma, addr & PAGE_MASK, mm_flags); > - > -check_stack: > - if (vma->vm_flags & VM_GROWSDOWN && !expand_stack(vma, addr)) > - goto good_area; > -out: > - return fault; We used to check the stack before the checknig the rest of the vm_flags, so this changes the precedence of the VM_FAULT_BADMAP and VM_FAULT_BADACCESS return codes. Please check the stack before checking the other vm_flags. Otherwise, this looks like a nice cleanup -- the old control flow was hideous. Thanks, Mark.
On Wed, May 29, 2019 at 06:04:45PM +0530, Anshuman Khandual wrote: > __do_page_fault() is over complicated with multiple goto statements. This > cleans up code flow and while there drops the vm_fault_t argument. There is no argument dropped anywhere, just a local variable.
On 05/30/2019 12:04 PM, Christoph Hellwig wrote: > On Wed, May 29, 2019 at 06:04:45PM +0530, Anshuman Khandual wrote: >> __do_page_fault() is over complicated with multiple goto statements. This >> cleans up code flow and while there drops the vm_fault_t argument. > > There is no argument dropped anywhere, just a local variable. > You are right. Will fix both subject line and the commit message.
On 05/29/2019 08:41 PM, Mark Rutland wrote: > On Wed, May 29, 2019 at 06:04:45PM +0530, Anshuman Khandual wrote: >> __do_page_fault() is over complicated with multiple goto statements. This >> cleans up code flow and while there drops the vm_fault_t argument. >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will.deacon@arm.com> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Cc: James Morse <james.morse@arm.com> >> Cc: Andrey Konovalov <andreyknvl@google.com> >> --- >> arch/arm64/mm/fault.c | 38 ++++++++++++++++---------------------- >> 1 file changed, 16 insertions(+), 22 deletions(-) >> >> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c >> index 170c71f..a53a30e 100644 >> --- a/arch/arm64/mm/fault.c >> +++ b/arch/arm64/mm/fault.c >> @@ -397,37 +397,31 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re >> static vm_fault_t __do_page_fault(struct mm_struct *mm, unsigned long addr, >> unsigned int mm_flags, unsigned long vm_flags) >> { >> - struct vm_area_struct *vma; >> - vm_fault_t fault; >> + struct vm_area_struct *vma = find_vma(mm, addr); >> >> - vma = find_vma(mm, addr); >> - fault = VM_FAULT_BADMAP; >> if (unlikely(!vma)) >> - goto out; >> - if (unlikely(vma->vm_start > addr)) >> - goto check_stack; >> + return VM_FAULT_BADMAP; >> >> /* >> - * Ok, we have a good vm_area for this memory access, so we can handle >> - * it. >> + * Check if the VMA has got the required permssion with respect >> + * to the access fault here. >> */ > > We already had a perfectly good comment for this check: > > /* > * Check that the permissions on the VMA allow for the fault which > * occurred. > */ > > ... so please keep that and minimize the diff. Sure, will keep all the existing comments here. > >> -good_area: >> + if (!(vma->vm_flags & vm_flags)) >> + return VM_FAULT_BADACCESS; >> + >> /* >> - * Check that the permissions on the VMA allow for the fault which >> - * occurred. >> + * There is a valid VMA for this access. But before proceeding >> + * make sure that it has required flags if there is an attempt >> + * to expand the stack downwards. >> */ > > I think we can drop this comment, given we didn't have it previously. Okay. > >> - if (!(vma->vm_flags & vm_flags)) { >> - fault = VM_FAULT_BADACCESS; >> - goto out; >> - } >> + if (unlikely(vma->vm_start > addr)) { >> + if (!(vma->vm_flags & VM_GROWSDOWN)) >> + return VM_FAULT_BADMAP; >> >> + if (expand_stack(vma, addr)) >> + return VM_FAULT_BADMAP; > > You can drop the line space between these two if statements. Will do. > >> + } >> return handle_mm_fault(vma, addr & PAGE_MASK, mm_flags); >> - >> -check_stack: >> - if (vma->vm_flags & VM_GROWSDOWN && !expand_stack(vma, addr)) >> - goto good_area; >> -out: >> - return fault; > > We used to check the stack before the checknig the rest of the vm_flags, > so this changes the precedence of the VM_FAULT_BADMAP and > VM_FAULT_BADACCESS return codes. > > Please check the stack before checking the other vm_flags. Though it makes some sense to move VMA permission check earlier in the function as it is the quicker one but I understand need to maintain the existing code flow in a clean up patch like this. Will retain the existing flow.
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index 170c71f..a53a30e 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c @@ -397,37 +397,31 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re static vm_fault_t __do_page_fault(struct mm_struct *mm, unsigned long addr, unsigned int mm_flags, unsigned long vm_flags) { - struct vm_area_struct *vma; - vm_fault_t fault; + struct vm_area_struct *vma = find_vma(mm, addr); - vma = find_vma(mm, addr); - fault = VM_FAULT_BADMAP; if (unlikely(!vma)) - goto out; - if (unlikely(vma->vm_start > addr)) - goto check_stack; + return VM_FAULT_BADMAP; /* - * Ok, we have a good vm_area for this memory access, so we can handle - * it. + * Check if the VMA has got the required permssion with respect + * to the access fault here. */ -good_area: + if (!(vma->vm_flags & vm_flags)) + return VM_FAULT_BADACCESS; + /* - * Check that the permissions on the VMA allow for the fault which - * occurred. + * There is a valid VMA for this access. But before proceeding + * make sure that it has required flags if there is an attempt + * to expand the stack downwards. */ - if (!(vma->vm_flags & vm_flags)) { - fault = VM_FAULT_BADACCESS; - goto out; - } + if (unlikely(vma->vm_start > addr)) { + if (!(vma->vm_flags & VM_GROWSDOWN)) + return VM_FAULT_BADMAP; + if (expand_stack(vma, addr)) + return VM_FAULT_BADMAP; + } return handle_mm_fault(vma, addr & PAGE_MASK, mm_flags); - -check_stack: - if (vma->vm_flags & VM_GROWSDOWN && !expand_stack(vma, addr)) - goto good_area; -out: - return fault; } static bool is_el0_instruction_abort(unsigned int esr)
__do_page_fault() is over complicated with multiple goto statements. This cleans up code flow and while there drops the vm_fault_t argument. Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: James Morse <james.morse@arm.com> Cc: Andrey Konovalov <andreyknvl@google.com> --- arch/arm64/mm/fault.c | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-)