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
