Re: [PATCH v2 1/2] vmalloc: Factor vmap_alloc() out of vm_map_ram()

[Christoph Hellwig <hch@infradead.org>]

On Tue, Nov 01, 2022 at 08:18:27PM +0000, Matthew Wilcox (Oracle) wrote:
> Introduce vmap_alloc() to simply get the address space.  This allows
> for code sharing in the next patch.
> 
> Suggested-by: Uladzislau Rezki <urezki@gmail.com>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  mm/vmalloc.c | 41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index ccaa461998f3..dcab1d3cf185 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2230,6 +2230,27 @@ void vm_unmap_ram(const void *mem, unsigned int count)
>  }
>  EXPORT_SYMBOL(vm_unmap_ram);
>  
> +static void *vmap_alloc(size_t size, int node)
> +{
> +	void *mem;
> +
> +	if (likely(size <= (VMAP_MAX_ALLOC * PAGE_SIZE))) {
> +		mem = vb_alloc(size, GFP_KERNEL);
> +		if (IS_ERR(mem))
> +			mem = NULL;
> +	} else {
> +		struct vmap_area *va;
> +		va = alloc_vmap_area(size, PAGE_SIZE,
> +				VMALLOC_START, VMALLOC_END, node, GFP_KERNEL);
> +		if (IS_ERR(va))
> +			mem = NULL;
> +		else
> +			mem = (void *)va->va_start;
> +	}
> +
> +	return mem;

This reads really strange, why not return the ERR_PTR and do:

static void *vmap_alloc(size_t size, int node)
{
	if (unlikely(size > VMAP_MAX_ALLOC * PAGE_SIZE)) {
		struct vmap_area *va;

		va = alloc_vmap_area(size, PAGE_SIZE, VMALLOC_START,
				     VMALLOC_END, node, GFP_KERNEL);
		if (IS_ERR(va))
			return ERR_CAST(va);
		return (void *)va->va_start;
	}

	return vb_alloc(size, GFP_KERNEL);
}

> @@ -2247,24 +2268,8 @@ EXPORT_SYMBOL(vm_unmap_ram);
>  void *vm_map_ram(struct page **pages, unsigned int count, int node)
>  {
>  	unsigned long size = (unsigned long)count << PAGE_SHIFT;
> +	void *mem = vmap_alloc(size, node);
> +	unsigned long addr = (unsigned long)mem;

And here we still need the error check anyway, no matter if it is for
NULL or an ERR_PTR.