[V2,4/4] arm64/mm: Drop local variable vm_fault_t from __do_page_fault()
diff mbox series

Message ID 1559544085-7502-5-git-send-email-anshuman.khandual@arm.com
State New
Headers show
Series
  • arm64/mm: Clean ups for do_page_fault()
Related show

Commit Message

Anshuman Khandual June 3, 2019, 6:41 a.m. UTC
__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(-)

Comments

Catalin Marinas June 4, 2019, 2:56 p.m. UTC | #1
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.
Anshuman Khandual June 6, 2019, 4:54 a.m. UTC | #2
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.
Catalin Marinas June 6, 2019, 11:27 a.m. UTC | #3
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).
Mark Rutland June 6, 2019, 11:31 a.m. UTC | #4
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.

Patch
diff mbox series

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)