Re: [PATCH v3 2/6] mm/vmalloc: set area's page_order after allocation succeeds

[Dev Jain <dev.jain@arm.com>]

On 03/09/26 2:58 pm, Mike Rapoport (Microsoft) wrote:
> __vmalloc_area_node() calls set_vm_area_page_order() to set area's
> page_order before actually allocating pages to populate the area.
> 
> If allocation of large pages in HUGE_VMAP case fails midway, this leaves
> the area with elevated page_order throughout the cleanup path.
> 
> There is no actual issue with this because the only place that currently
> relies on area->page_order on the cleanup path is the loop calculating
> the direct map alias range in vm_reset_perms() and it anyway skips
> unpopulated pages.
> 
> But having set_vm_area_page_order() in the middle of __vmalloc_area_node()
> makes things very obscure, hard to reason about and error prone against
> future changes of the cleanup path.
> 
> Move the call to set_vm_area_page_order() just before the successful
> return from __vmalloc_area_node() where page order is guaranteed.
> 
> While on it, initialize local page_order variable with its declaration.
> 
> Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> ---
>  mm/vmalloc.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 0380917392e68..22aa554d123f5 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -3879,7 +3879,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
>  	unsigned long size = get_vm_area_size(area);
>  	unsigned long array_size;
>  	unsigned long nr_small_pages = size >> PAGE_SHIFT;
> -	unsigned int page_order;
> +	unsigned int page_order = page_shift - PAGE_SHIFT;
>  	unsigned int flags;
>  	int ret;
>  
> @@ -3907,9 +3907,6 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
>  		goto fail;
>  	}
>  
> -	set_vm_area_page_order(area, page_shift - PAGE_SHIFT);
> -	page_order = vm_area_page_order(area);

vm_area_page_order() returns zero in case !CONFIG_HAVE_ARCH_HUGE_VMALLOC. So
it may happen that after this patch the page_shift passed to the page-allocator
is > PAGE_SHIFT but that got rejected by vm_area_page_order.

But that won't happen because for page_shift > PAGE_SHIFT to be passed to
__vmalloc_area_node(), we need "vmap_huge" to be true, which needs
CONFIG_HAVE_ARCH_HUGE_VMALLOC.

Writing this out because CONFIG_HAVE_ARCH_HUGE_VMALLOC and CONFIG_HAVE_ARCH_HUGE_VMAP
along with mixing variable names with vmap is extremely confusing.

Reviewed-by: Dev Jain <dev.jain@arm.com>

> -
>  	/*
>  	 * High-order nofail allocations are really expensive and
>  	 * potentially dangerous (pre-mature OOM, disruptive reclaim
> @@ -3964,6 +3961,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
>  		goto fail;
>  	}
>  
> +	set_vm_area_page_order(area, page_order);
>  	return area->addr;
>  
>  fail:
>