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

[Michal Hocko <mhocko@suse.com>]

On Tue 23-07-24 12:42:17, Danilo Krummrich wrote:
> 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 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.

No I mean, that the change of the signature and semantic should be done along with
update to callers and the new implementation of the function itself
should be done in its own patch.

[...]
> > > +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.

I would really prefer to have it there because it makes the follow up
code easier.

> > > +	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;

With the early !size && p check we wouldn't right?

> 
> 	/* 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;
> 	}

I do not see why kvrealloc should have different set of constrains than
vrealloc in this regards.

> 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.

This is effectively open coding part of vrealloc without any good
reason. Please get rid of that.

-- 
Michal Hocko
SUSE Labs