| Message ID | 1559544085-7502-5-git-send-email-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/mm: Clean ups for do_page_fault() | expand |
On Mon, Jun 03, 2019 at 12:11:25PM +0530, Anshuman Khandual wrote: > __do_page_fault() is over complicated with multiple goto statements. This > cleans up the code flow and while there drops local variable vm_fault_t. I'd change the subject as well here to something like refactor or simplify __do_page_fault(). > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > index 4bb65f3..41fa905 100644 > --- a/arch/arm64/mm/fault.c > +++ b/arch/arm64/mm/fault.c > @@ -397,37 +397,29 @@ 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. > */ > -good_area: > + 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 could have a single return here: if (unlikely(vma->vm_start > addr) && (!(vma->vm_flags & VM_GROWSDOWN) || expand_stack(vma, addr))) return VM_FAULT_BADMAP; Not sure it's any clearer though.
On 06/04/2019 08:26 PM, Catalin Marinas wrote: > On Mon, Jun 03, 2019 at 12:11:25PM +0530, Anshuman Khandual wrote: >> __do_page_fault() is over complicated with multiple goto statements. This >> cleans up the code flow and while there drops local variable vm_fault_t. > > I'd change the subject as well here to something like refactor or > simplify __do_page_fault(). Sure. > >> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c >> index 4bb65f3..41fa905 100644 >> --- a/arch/arm64/mm/fault.c >> +++ b/arch/arm64/mm/fault.c >> @@ -397,37 +397,29 @@ 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. >> */ >> -good_area: >> + 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 could have a single return here: > > if (unlikely(vma->vm_start > addr) && > (!(vma->vm_flags & VM_GROWSDOWN) || expand_stack(vma, addr))) > return VM_FAULT_BADMAP; > > Not sure it's any clearer though. > TBH the proposed one seems clearer as it separates effect (vma->vm_start > addr) from required permission check (vma->vm_flags & VM_GROWSDOWN) and required action (expand_stack(vma, addr)). But I am happy to change as you have mentioned if that is preferred.
On Thu, Jun 06, 2019 at 10:24:01AM +0530, Anshuman Khandual wrote: > On 06/04/2019 08:26 PM, Catalin Marinas wrote: > > On Mon, Jun 03, 2019 at 12:11:25PM +0530, Anshuman Khandual wrote: > >> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > >> index 4bb65f3..41fa905 100644 > >> --- a/arch/arm64/mm/fault.c > >> +++ b/arch/arm64/mm/fault.c > >> @@ -397,37 +397,29 @@ 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. > >> */ > >> -good_area: > >> + 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 could have a single return here: > > > > if (unlikely(vma->vm_start > addr) && > > (!(vma->vm_flags & VM_GROWSDOWN) || expand_stack(vma, addr))) > > return VM_FAULT_BADMAP; > > > > Not sure it's any clearer though. > > TBH the proposed one seems clearer as it separates effect (vma->vm_start > addr) > from required permission check (vma->vm_flags & VM_GROWSDOWN) and required action > (expand_stack(vma, addr)). But I am happy to change as you have mentioned if that > is preferred. Not bothered really. You can leave them as in your proposal (I was just seeing the VM_GROWSDOWN check tightly coupled with the expand_stack(), it's fine either way).
On Thu, Jun 06, 2019 at 12:27:40PM +0100, Catalin Marinas wrote: > On Thu, Jun 06, 2019 at 10:24:01AM +0530, Anshuman Khandual wrote: > > On 06/04/2019 08:26 PM, Catalin Marinas wrote: > > > On Mon, Jun 03, 2019 at 12:11:25PM +0530, Anshuman Khandual wrote: > > >> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > > >> index 4bb65f3..41fa905 100644 > > >> --- a/arch/arm64/mm/fault.c > > >> +++ b/arch/arm64/mm/fault.c > > >> @@ -397,37 +397,29 @@ 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. > > >> */ > > >> -good_area: > > >> + 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 could have a single return here: > > > > > > if (unlikely(vma->vm_start > addr) && > > > (!(vma->vm_flags & VM_GROWSDOWN) || expand_stack(vma, addr))) > > > return VM_FAULT_BADMAP; > > > > > > Not sure it's any clearer though. > > > > TBH the proposed one seems clearer as it separates effect (vma->vm_start > addr) > > from required permission check (vma->vm_flags & VM_GROWSDOWN) and required action > > (expand_stack(vma, addr)). But I am happy to change as you have mentioned if that > > is preferred. > > Not bothered really. You can leave them as in your proposal (I was just > seeing the VM_GROWSDOWN check tightly coupled with the expand_stack(), > it's fine either way). Personally, I find it clearer as separate statements, so I'd suggest keeping it as per Anshuman's proposal. Thanks, Mark.
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index 4bb65f3..41fa905 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c @@ -397,37 +397,29 @@ 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. */ -good_area: + 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; + } + /* * Check that the permissions on the VMA allow for the fault which * occurred. */ - if (!(vma->vm_flags & vm_flags)) { - fault = VM_FAULT_BADACCESS; - goto out; - } - + if (!(vma->vm_flags & vm_flags)) + return VM_FAULT_BADACCESS; 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 the code flow and while there drops local variable vm_fault_t. 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> Cc: Christoph Hellwig <hch@infradead.org> --- arch/arm64/mm/fault.c | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-)