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

[Danilo Krummrich <dakr@kernel.org>]

On Tue, Jul 23, 2024 at 12:55:45PM +0200, Michal Hocko wrote:
> 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.

Sorry, it seems like you lost me a bit.

There is one patch that implements vrealloc() and one patch that does the change
of krealloc()'s signature, semantics and the corresponding update to the
callers.

Isn't that already what you ask for?

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

I don't think it does (see below).

Either way, I got notified that Andrew applied the patches to mm-unstable. How
to proceed from there for further changes, if any?

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

I think that's unrelated. Your proposed early check checks for size == 0 to free
and return early. Whereas this check bails out if we fail kmalloc() with
size <= PAGE_SIZE, because a subsequent vmalloc() wouldn't make a lot of sense.

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

Those constraints come from kvmalloc() and hence should also apply for
kvrealloc(). What you seem to question here is whether they should be moved from
kvmalloc() to vmalloc() (and hence implicitly to vrealloc()).

As for the gfpflags_allow_blocking() check, it seems like this one was suggested
by you for kvmalloc() [1]. It seems that some people call kvmalloc() with
GPF_ATOMIC (which seems a bit weird at a first glance, but maybe makes sense in
some generic code paths). Hence, kvrealloc() must be able to handle it as well.

As for the size > INT_MAX check, please see the discussion in commit
0708a0afe291 ("mm: Consider __GFP_NOWARN flag for oversized kvmalloc() calls").

But again, whether those checks should be moved to vmalloc() is probably a
different topic.

[1] https://lore.kernel.org/all/20220926151650.15293-1-fw@strlen.de/T/#u

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