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

[Michal Hocko <mhocko@suse.com>]

On Tue 23-07-24 13:55:48, Danilo Krummrich wrote:
> 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?

No, because this second patch reimplements kvrealloc wo to use krealloc
and vrealloc fallback. More clear now?
 
> > [...]
> > > > > +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?

Andrew will either apply follow up fixes are replace the series by a new
version.

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

It seems we are not on the same page here. Here is what I would like
kvrealloc to look like in the end:

void *kvrealloc_noprof(const void *p, size_t size, gfp_t flags)
{
        void *newp;

        if (!size && p) {
                kvfree(p);
                return NULL;
        }

        if (!is_vmalloc_addr(p))
                newp = krealloc_noprof(p, size, kmalloc_gfp_adjust(flags, size));

        if (newp)
                return newp;

        return vrealloc_noprof(p, size, flags);
}
EXPORT_SYMBOL(kvrealloc_noprof);

krealloc_noprof should be extended for the maximum allowed size and so
does vrealloc_noprof. The implementation of the kvrealloc cannot get any
easier and more straightforward AFAICS. See my point?
-- 
Michal Hocko
SUSE Labs