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

[Danilo Krummrich]

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
>