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
