On Tue, Jul 23, 2024 at 09:50:13AM +0200, Michal Hocko wrote:
> 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.
The optimizations you mean? Yes, I intend to do this in a separate series. For
now, I put TODOs in vrealloc.
>
> [...]
>
> > 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
Christoph proposed this name, I think he wanted to encode the target of the
flags, whereas you want to encode where the function is intended to be called
from.
When I originally named this thing, I had the same conflict - encoding both
turns out clumsy - and came up with to_kmalloc_flags().
Personally, I'd be fine with __kvmalloc_gfp_adjust() too.
>
> [...]
> > -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.
Personally, I like it without. For me the simplicity comes from directing things
to either krealloc() or vrealloc(). But I'd be open to change it however.
>
> > + 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?
We could do that, but we'd also need to do the same checks kvmalloc() does, i.e.
/*
* It doesn't really make sense to fallback to vmalloc for sub page
* requests
*/
if (ret || size <= PAGE_SIZE)
return ret;
/* non-sleeping allocations are not supported by vmalloc */
if (!gfpflags_allow_blocking(flags))
return NULL;
/* Don't even allow crazy sizes */
if (unlikely(size > INT_MAX)) {
WARN_ON_ONCE(!(flags & __GFP_NOWARN));
return NULL;
}
Does the kmalloc() retry through kvmalloc() hurt us enough to do that? This
should only ever happen when we switch from a kmalloc buffer to a vmalloc
buffer, which we only do once, we never switch back.
>
> > + 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
>
