Re: [PATCH v2 2/2] mm: kvmalloc: align kvrealloc() with krealloc()

[Michal Hocko <mhocko@suse.com>]

On Mon 22-07-24 18:29:24, Danilo Krummrich wrote:
> Besides the obvious (and desired) difference between krealloc() and
> kvrealloc(), there is some inconsistency in their function signatures
> and behavior:
> 
>  - krealloc() frees the memory when the requested size is zero, whereas
>    kvrealloc() simply returns a pointer to the existing allocation.
> 
>  - krealloc() behaves like kmalloc() if a NULL pointer is passed, whereas
>    kvrealloc() does not accept a NULL pointer at all and, if passed,
>    would fault instead.
> 
>  - krealloc() is self-contained, whereas kvrealloc() relies on the caller
>    to provide the size of the previous allocation.
> 
> Inconsistent behavior throughout allocation APIs is error prone, hence make
> kvrealloc() behave like krealloc(), which seems superior in all mentioned
> aspects.

I completely agree with this. Fortunately the number of existing callers
is small and none of them really seem to depend on the current behavior
in that aspect.
 
> Besides that, implementing kvrealloc() by making use of krealloc() and
> vrealloc() provides oppertunities to grow (and shrink) allocations more
> efficiently. For instance, vrealloc() can be optimized to allocate and
> map additional pages to grow the allocation or unmap and free unused
> pages to shrink the allocation.

This seems like a change that is independent on the above and should be
a patch on its own.

[...]

> diff --git a/mm/util.c b/mm/util.c
> index bc488f0121a7..0ff5898cc6de 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -608,6 +608,28 @@ unsigned long vm_mmap(struct file *file, unsigned long addr,
>  }
>  EXPORT_SYMBOL(vm_mmap);
>  
> +static gfp_t kmalloc_gfp_adjust(gfp_t flags, size_t size)

This seems like a generally useful helper which it is not. I would call
it something like __kvmalloc_gfp_adjust or something similar so that it is
clear that this is just a helper to adjust gfp flag for slab allocator
path

[...]
> -void *kvrealloc_noprof(const void *p, size_t oldsize, size_t newsize, gfp_t flags)
> +/**
> + * kvrealloc - reallocate memory; contents remain unchanged
> + * @p: object to reallocate memory for
> + * @size: the size to reallocate
> + * @flags: the flags for the page level allocator
> + *
> + * The contents of the object pointed to are preserved up to the lesser of the
> + * new and old size (__GFP_ZERO flag is effectively ignored).
> + *
> + * If @p is %NULL, kvrealloc() behaves exactly like kvmalloc(). If @size is 0
> + * and @p is not a %NULL pointer, the object pointed to is freed.
> + *
> + * Return: pointer to the allocated memory or %NULL in case of error
> + */
> +void *kvrealloc_noprof(const void *p, size_t size, gfp_t flags)
>  {
> -	void *newp;
> +	void *n;
> +

	if (!size && p) {
		kvfree(p);
		return NULL;
	}

would make this code flow slightly easier to read because the freeing
path would be shared for all compbinations IMO.

> +	if (is_vmalloc_addr(p))
> +		return vrealloc_noprof(p, size, flags);
> +
> +	n = krealloc_noprof(p, size, kmalloc_gfp_adjust(flags, size));
> +	if (!n) {
> +		/* We failed to krealloc(), fall back to kvmalloc(). */
> +		n = kvmalloc_noprof(size, flags);

Why don't you simply use vrealloc_noprof here?

> +		if (!n)
> +			return NULL;
> +
> +		if (p) {
> +			/* We already know that `p` is not a vmalloc address. */
> +			memcpy(n, p, ksize(p));
> +			kfree(p);
> +		}
> +	}
>  
> -	if (oldsize >= newsize)
> -		return (void *)p;
> -	newp = kvmalloc_noprof(newsize, flags);
> -	if (!newp)
> -		return NULL;
> -	memcpy(newp, p, oldsize);
> -	kvfree(p);
> -	return newp;
> +	return n;
>  }
>  EXPORT_SYMBOL(kvrealloc_noprof);
>  
> -- 
> 2.45.2

-- 
Michal Hocko
SUSE Labs