[PATCH v2] mm: vmalloc: Clean up vunmap to avoid pgtable ops twice

[PATCH v2] mm: vmalloc: Clean up vunmap to avoid pgtable ops twice

[Chintan Pandya]

vunmap does page table clear operations twice in the
case when DEBUG_PAGEALLOC_ENABLE_DEFAULT is enabled.

So, clean up the code as that is unintended.

As a perf gain, we save few us. Below ftrace data was
obtained while doing 1 MB of vmalloc/vfree on ARM64
based SoC *without* this patch applied. After this
patch, we can save ~3 us (on 1 extra vunmap_page_range).

  CPU  DURATION                  FUNCTION CALLS
  |     |   |                     |   |   |   |
 6)               |  __vunmap() {
 6)               |    vmap_debug_free_range() {
 6)   3.281 us    |      vunmap_page_range();
 6) + 45.468 us   |    }
 6)   2.760 us    |    vunmap_page_range();
 6) ! 505.105 us  |  }

Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
---
 mm/vmalloc.c | 25 +++----------------------
 1 file changed, 3 insertions(+), 22 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index ebff729..6729400 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -603,26 +603,6 @@ static void unmap_vmap_area(struct vmap_area *va)
 	vunmap_page_range(va->va_start, va->va_end);
 }
 
-static void vmap_debug_free_range(unsigned long start, unsigned long end)
-{
-	/*
-	 * Unmap page tables and force a TLB flush immediately if pagealloc
-	 * debugging is enabled.  This catches use after free bugs similarly to
-	 * those in linear kernel virtual address space after a page has been
-	 * freed.
-	 *
-	 * All the lazy freeing logic is still retained, in order to minimise
-	 * intrusiveness of this debugging feature.
-	 *
-	 * This is going to be *slow* (linear kernel virtual address debugging
-	 * doesn't do a broadcast TLB flush so it is a lot faster).
-	 */
-	if (debug_pagealloc_enabled()) {
-		vunmap_page_range(start, end);
-		flush_tlb_kernel_range(start, end);
-	}
-}
-
 /*
  * lazy_max_pages is the maximum amount of virtual address space we gather up
  * before attempting to purge with a TLB flush.
@@ -756,6 +736,9 @@ static void free_unmap_vmap_area(struct vmap_area *va)
 {
 	flush_cache_vunmap(va->va_start, va->va_end);
 	unmap_vmap_area(va);
+	if (debug_pagealloc_enabled())
+		flush_tlb_kernel_range(va->va_start, va->va_end);
+
 	free_vmap_area_noflush(va);
 }
 
@@ -1142,7 +1125,6 @@ void vm_unmap_ram(const void *mem, unsigned int count)
 	BUG_ON(!PAGE_ALIGNED(addr));
 
 	debug_check_no_locks_freed(mem, size);
-	vmap_debug_free_range(addr, addr+size);
 
 	if (likely(count <= VMAP_MAX_ALLOC)) {
 		vb_free(mem, size);
@@ -1499,7 +1481,6 @@ struct vm_struct *remove_vm_area(const void *addr)
 		va->flags |= VM_LAZY_FREE;
 		spin_unlock(&vmap_area_lock);
 
-		vmap_debug_free_range(va->va_start, va->va_end);
 		kasan_free_shadow(vm);
 		free_unmap_vmap_area(va);
 
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project

Re: [PATCH v2] mm: vmalloc: Clean up vunmap to avoid pgtable ops twice

[Andrew Morton]

On Mon, 16 Apr 2018 16:29:02 +0530 Chintan Pandya <cpandya@codeaurora.org> wrote:

> vunmap does page table clear operations twice in the
> case when DEBUG_PAGEALLOC_ENABLE_DEFAULT is enabled.
> 
> So, clean up the code as that is unintended.
> 
> As a perf gain, we save few us. Below ftrace data was
> obtained while doing 1 MB of vmalloc/vfree on ARM64
> based SoC *without* this patch applied. After this
> patch, we can save ~3 us (on 1 extra vunmap_page_range).
> 
>   CPU  DURATION                  FUNCTION CALLS
>   |     |   |                     |   |   |   |
>  6)               |  __vunmap() {
>  6)               |    vmap_debug_free_range() {
>  6)   3.281 us    |      vunmap_page_range();
>  6) + 45.468 us   |    }
>  6)   2.760 us    |    vunmap_page_range();
>  6) ! 505.105 us  |  }

It's been a long time since I looked at the vmap code :(

> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -603,26 +603,6 @@ static void unmap_vmap_area(struct vmap_area *va)
>  	vunmap_page_range(va->va_start, va->va_end);
>  }
>  
> -static void vmap_debug_free_range(unsigned long start, unsigned long end)
> -{
> -	/*
> -	 * Unmap page tables and force a TLB flush immediately if pagealloc
> -	 * debugging is enabled.  This catches use after free bugs similarly to
> -	 * those in linear kernel virtual address space after a page has been
> -	 * freed.
> -	 *
> -	 * All the lazy freeing logic is still retained, in order to minimise
> -	 * intrusiveness of this debugging feature.
> -	 *
> -	 * This is going to be *slow* (linear kernel virtual address debugging
> -	 * doesn't do a broadcast TLB flush so it is a lot faster).
> -	 */
> -	if (debug_pagealloc_enabled()) {
> -		vunmap_page_range(start, end);
> -		flush_tlb_kernel_range(start, end);
> -	}
> -}
> -
>  /*
>   * lazy_max_pages is the maximum amount of virtual address space we gather up
>   * before attempting to purge with a TLB flush.
> @@ -756,6 +736,9 @@ static void free_unmap_vmap_area(struct vmap_area *va)
>  {
>  	flush_cache_vunmap(va->va_start, va->va_end);
>  	unmap_vmap_area(va);
> +	if (debug_pagealloc_enabled())
> +		flush_tlb_kernel_range(va->va_start, va->va_end);
> +
>  	free_vmap_area_noflush(va);
>  }
>  
> @@ -1142,7 +1125,6 @@ void vm_unmap_ram(const void *mem, unsigned int count)
>  	BUG_ON(!PAGE_ALIGNED(addr));
>  
>  	debug_check_no_locks_freed(mem, size);
> -	vmap_debug_free_range(addr, addr+size);

This appears to be a functional change: if (count <= VMAP_MAX_ALLOC)
and we're in debug mode then the
vunmap_page_range/flush_tlb_kernel_range will no longer be performed. 
Why is this ok?

>  	if (likely(count <= VMAP_MAX_ALLOC)) {
>  		vb_free(mem, size);
> @@ -1499,7 +1481,6 @@ struct vm_struct *remove_vm_area(const void *addr)
>  		va->flags |= VM_LAZY_FREE;
>  		spin_unlock(&vmap_area_lock);
>  
> -		vmap_debug_free_range(va->va_start, va->va_end);
>  		kasan_free_shadow(vm);
>  		free_unmap_vmap_area(va);
>  

Re: [PATCH v2] mm: vmalloc: Clean up vunmap to avoid pgtable ops twice

[Chintan Pandya]

On 5/1/2018 4:22 AM, Andrew Morton wrote:
> On Mon, 16 Apr 2018 16:29:02 +0530 Chintan Pandya <cpandya@codeaurora.org> wrote:
> 
>> vunmap does page table clear operations twice in the
>> case when DEBUG_PAGEALLOC_ENABLE_DEFAULT is enabled.
>>
>> So, clean up the code as that is unintended.
>>
>> As a perf gain, we save few us. Below ftrace data was
>> obtained while doing 1 MB of vmalloc/vfree on ARM64
>> based SoC *without* this patch applied. After this
>> patch, we can save ~3 us (on 1 extra vunmap_page_range).
>>
>>    CPU  DURATION                  FUNCTION CALLS
>>    |     |   |                     |   |   |   |
>>   6)               |  __vunmap() {
>>   6)               |    vmap_debug_free_range() {
>>   6)   3.281 us    |      vunmap_page_range();
>>   6) + 45.468 us   |    }
>>   6)   2.760 us    |    vunmap_page_range();
>>   6) ! 505.105 us  |  }
> 
> It's been a long time since I looked at the vmap code :(
> 
>> --- a/mm/vmalloc.c
>> +++ b/mm/vmalloc.c
>> @@ -603,26 +603,6 @@ static void unmap_vmap_area(struct vmap_area *va)
>>   	vunmap_page_range(va->va_start, va->va_end);
>>   }
>>   
>> -static void vmap_debug_free_range(unsigned long start, unsigned long end)
>> -{
>> -	/*
>> -	 * Unmap page tables and force a TLB flush immediately if pagealloc
>> -	 * debugging is enabled.  This catches use after free bugs similarly to
>> -	 * those in linear kernel virtual address space after a page has been
>> -	 * freed.
>> -	 *
>> -	 * All the lazy freeing logic is still retained, in order to minimise
>> -	 * intrusiveness of this debugging feature.
>> -	 *
>> -	 * This is going to be *slow* (linear kernel virtual address debugging
>> -	 * doesn't do a broadcast TLB flush so it is a lot faster).
>> -	 */
>> -	if (debug_pagealloc_enabled()) {
>> -		vunmap_page_range(start, end);
>> -		flush_tlb_kernel_range(start, end);
>> -	}
>> -}
>> -
>>   /*
>>    * lazy_max_pages is the maximum amount of virtual address space we gather up
>>    * before attempting to purge with a TLB flush.
>> @@ -756,6 +736,9 @@ static void free_unmap_vmap_area(struct vmap_area *va)
>>   {
>>   	flush_cache_vunmap(va->va_start, va->va_end);
>>   	unmap_vmap_area(va);
>> +	if (debug_pagealloc_enabled())
>> +		flush_tlb_kernel_range(va->va_start, va->va_end);
>> +
>>   	free_vmap_area_noflush(va);
>>   }
>>   
>> @@ -1142,7 +1125,6 @@ void vm_unmap_ram(const void *mem, unsigned int count)
>>   	BUG_ON(!PAGE_ALIGNED(addr));
>>   
>>   	debug_check_no_locks_freed(mem, size);
>> -	vmap_debug_free_range(addr, addr+size);
> 
> This appears to be a functional change: if (count <= VMAP_MAX_ALLOC)
> and we're in debug mode then the
> vunmap_page_range/flush_tlb_kernel_range will no longer be performed.
> Why is this ok?
> 

Yes, you are right. In vb_free(), we do vunmap_page_range() but not
flush_tlb_kernel_range(). I will add this stub for debug benefits and
share v3.

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 6729400..781ce02 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1036,6 +1036,10 @@ static void vb_free(const void *addr, unsigned 
long size)

         vunmap_page_range((unsigned long)addr, (unsigned long)addr + size);

+       if (debug_pagealloc_enabled())
+               flush_tlb_kernel_range((unsigned long)addr,
+                                       (unsigned long)addr + size);
+
         spin_lock(&vb->lock);

         /* Expand dirty range */



>>   	if (likely(count <= VMAP_MAX_ALLOC)) {
>>   		vb_free(mem, size);
>> @@ -1499,7 +1481,6 @@ struct vm_struct *remove_vm_area(const void *addr)
>>   		va->flags |= VM_LAZY_FREE;
>>   		spin_unlock(&vmap_area_lock);
>>   
>> -		vmap_debug_free_range(va->va_start, va->va_end);
>>   		kasan_free_shadow(vm);
>>   		free_unmap_vmap_area(va);
>>   
> 

Chintan
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation
Collaborative Project