| Message ID | 20240722163111.4766-3-dakr@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Align kvrealloc() with krealloc() | expand |
On Mon, 22 Jul 2024 18:29:24 +0200 Danilo Krummrich <dakr@kernel.org> 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. The old kvrealloc() behavior actually sounds somewhat useful. You've checked that no existing sites were relying on this? And that all existing kvrealloc() callers were (incorrectly) checking for NULL? Seems that way.
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. [...] > 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 [...] > -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. > + 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? > + 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
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 >
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.
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 >
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?
On Tue, Jul 23, 2024 at 02:12:23PM +0200, Michal Hocko wrote: > 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? I'm very sorry, but no. The second patch just changes kvrealloc(), how do you want to split it up? > > > > > > + 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); This looks weird. The fact that you're passing p to vrealloc_noprof() if krealloc_noprof() fails, implies that vrealloc_noprof() must be able to deal with pointers to kmalloc'd memory. Also, you never migrate from kmalloc memory to vmalloc memory and never free p. Given the above, do you mean to say that vrealloc_noprof() should do all that? If so, I strongly disagree here. vrealloc() should only deal with vmalloc memory. > > krealloc_noprof should be extended for the maximum allowed size krealloc_noprof() already has a maximum allowed size. > and so does vrealloc_noprof. Probably, but I don't think this series is the correct scope for this change. I'd offer to send a separate patch for this though. > The implementation of the kvrealloc cannot get any > easier and more straightforward AFAICS. See my point? > -- > Michal Hocko > SUSE Labs >
On Mon, Jul 22, 2024 at 06:43:48PM -0700, Andrew Morton wrote: > On Mon, 22 Jul 2024 18:29:24 +0200 Danilo Krummrich <dakr@kernel.org> 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. > > The old kvrealloc() behavior actually sounds somewhat useful. You've > checked that no existing sites were relying on this? Yes, I did. > > And that all existing kvrealloc() callers were (incorrectly) checking > for NULL? Seems that way. You mean for the initial allocation? Yes, but I also noticed that as long as the old kvrealloc() is called with p == NULL and oldsize == 0 it should work as well.
On Tue 23-07-24 15:33:32, Danilo Krummrich wrote: > On Tue, Jul 23, 2024 at 02:12:23PM +0200, Michal Hocko wrote: > > On Tue 23-07-24 13:55:48, Danilo Krummrich wrote: [...] > > 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); > > This looks weird. The fact that you're passing p to vrealloc_noprof() if > krealloc_noprof() fails, implies that vrealloc_noprof() must be able to deal > with pointers to kmalloc'd memory. You are right I have oversimplified this. I was hoping to follow kvmalloc model with a clear fallback and that should be possible but it would require more changes. Scratch that.
On 7/22/24 6:29 PM, 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. > > 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. > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> Acked-by: Vlastimil Babka <vbabka@suse.cz> with same caveat about the __GFP_ZERO comment on kvrealloc_noprof()
diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c index de789e0f1ae9..1ff3079aabc9 100644 --- a/arch/arm64/kvm/nested.c +++ b/arch/arm64/kvm/nested.c @@ -62,7 +62,6 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu) */ num_mmus = atomic_read(&kvm->online_vcpus) * S2_MMU_PER_VCPU; tmp = kvrealloc(kvm->arch.nested_mmus, - size_mul(sizeof(*kvm->arch.nested_mmus), kvm->arch.nested_mmus_size), size_mul(sizeof(*kvm->arch.nested_mmus), num_mmus), GFP_KERNEL_ACCOUNT | __GFP_ZERO); if (!tmp) diff --git a/arch/powerpc/platforms/pseries/papr-vpd.c b/arch/powerpc/platforms/pseries/papr-vpd.c index c29e85db5f35..1574176e3ffc 100644 --- a/arch/powerpc/platforms/pseries/papr-vpd.c +++ b/arch/powerpc/platforms/pseries/papr-vpd.c @@ -156,10 +156,7 @@ static int vpd_blob_extend(struct vpd_blob *blob, const char *data, size_t len) const char *old_ptr = blob->data; char *new_ptr; - new_ptr = old_ptr ? - kvrealloc(old_ptr, old_len, new_len, GFP_KERNEL_ACCOUNT) : - kvmalloc(len, GFP_KERNEL_ACCOUNT); - + new_ptr = kvrealloc(old_ptr, new_len, GFP_KERNEL_ACCOUNT); if (!new_ptr) return -ENOMEM; diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c index 2da094bdf8a4..18e366cc4993 100644 --- a/drivers/gpu/drm/drm_exec.c +++ b/drivers/gpu/drm/drm_exec.c @@ -145,8 +145,7 @@ static int drm_exec_obj_locked(struct drm_exec *exec, size_t size = exec->max_objects * sizeof(void *); void *tmp; - tmp = kvrealloc(exec->objects, size, size + PAGE_SIZE, - GFP_KERNEL); + tmp = kvrealloc(exec->objects, size + PAGE_SIZE, GFP_KERNEL); if (!tmp) return -ENOMEM; diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index 4423dd344239..1997981827fb 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -2128,7 +2128,7 @@ xlog_recover_add_to_cont_trans( old_ptr = item->ri_buf[item->ri_cnt-1].i_addr; old_len = item->ri_buf[item->ri_cnt-1].i_len; - ptr = kvrealloc(old_ptr, old_len, len + old_len, GFP_KERNEL); + ptr = kvrealloc(old_ptr, len + old_len, GFP_KERNEL); if (!ptr) return -ENOMEM; memcpy(&ptr[old_len], dp, len); diff --git a/include/linux/slab.h b/include/linux/slab.h index eb2bf4629157..c9cb42203183 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -841,8 +841,8 @@ kvmalloc_array_node_noprof(size_t n, size_t size, gfp_t flags, int node) #define kvcalloc_node(...) alloc_hooks(kvcalloc_node_noprof(__VA_ARGS__)) #define kvcalloc(...) alloc_hooks(kvcalloc_noprof(__VA_ARGS__)) -extern void *kvrealloc_noprof(const void *p, size_t oldsize, size_t newsize, gfp_t flags) - __realloc_size(3); +void *kvrealloc_noprof(const void *p, size_t size, gfp_t flags) + __realloc_size(2); #define kvrealloc(...) alloc_hooks(kvrealloc_noprof(__VA_ARGS__)) extern void kvfree(const void *addr); diff --git a/kernel/resource.c b/kernel/resource.c index 14777afb0a99..9f747bb7cd03 100644 --- a/kernel/resource.c +++ b/kernel/resource.c @@ -450,8 +450,7 @@ int walk_system_ram_res_rev(u64 start, u64 end, void *arg, /* re-alloc */ struct resource *rams_new; - rams_new = kvrealloc(rams, rams_size * sizeof(struct resource), - (rams_size + 16) * sizeof(struct resource), + rams_new = kvrealloc(rams, (rams_size + 16) * sizeof(struct resource), GFP_KERNEL); if (!rams_new) goto out; diff --git a/lib/fortify_kunit.c b/lib/fortify_kunit.c index f9ad60a9c7bd..ecb638d4cde1 100644 --- a/lib/fortify_kunit.c +++ b/lib/fortify_kunit.c @@ -306,8 +306,7 @@ DEFINE_ALLOC_SIZE_TEST_PAIR(vmalloc) orig = kvmalloc(prev_size, gfp); \ KUNIT_EXPECT_TRUE(test, orig != NULL); \ checker(((expected_pages) * PAGE_SIZE) * 2, \ - kvrealloc(orig, prev_size, \ - ((alloc_pages) * PAGE_SIZE) * 2, gfp), \ + kvrealloc(orig, ((alloc_pages) * PAGE_SIZE) * 2, gfp), \ kvfree(p)); \ } while (0) DEFINE_ALLOC_SIZE_TEST_PAIR(kvmalloc) 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) +{ + /* + * We want to attempt a large physically contiguous block first because + * it is less likely to fragment multiple larger blocks and therefore + * contribute to a long term fragmentation less than vmalloc fallback. + * However make sure that larger requests are not too disruptive - no + * OOM killer and no allocation failure warnings as we have a fallback. + */ + if (size > PAGE_SIZE) { + flags |= __GFP_NOWARN; + + if (!(flags & __GFP_RETRY_MAYFAIL)) + flags |= __GFP_NORETRY; + + /* nofail semantic is implemented by the vmalloc fallback */ + flags &= ~__GFP_NOFAIL; + } + + return flags; +} + /** * __kvmalloc_node - attempt to allocate physically contiguous memory, but upon * failure, fall back to non-contiguous (vmalloc) allocation. @@ -627,32 +649,15 @@ EXPORT_SYMBOL(vm_mmap); */ void *__kvmalloc_node_noprof(DECL_BUCKET_PARAMS(size, b), gfp_t flags, int node) { - gfp_t kmalloc_flags = flags; void *ret; - /* - * We want to attempt a large physically contiguous block first because - * it is less likely to fragment multiple larger blocks and therefore - * contribute to a long term fragmentation less than vmalloc fallback. - * However make sure that larger requests are not too disruptive - no - * OOM killer and no allocation failure warnings as we have a fallback. - */ - if (size > PAGE_SIZE) { - kmalloc_flags |= __GFP_NOWARN; - - if (!(kmalloc_flags & __GFP_RETRY_MAYFAIL)) - kmalloc_flags |= __GFP_NORETRY; - - /* nofail semantic is implemented by the vmalloc fallback */ - kmalloc_flags &= ~__GFP_NOFAIL; - } - - ret = __kmalloc_node_noprof(PASS_BUCKET_PARAMS(size, b), kmalloc_flags, node); - /* * It doesn't really make sense to fallback to vmalloc for sub page * requests */ + ret = __kmalloc_node_noprof(PASS_BUCKET_PARAMS(size, b), + kmalloc_gfp_adjust(flags, size), + node); if (ret || size <= PAGE_SIZE) return ret; @@ -715,18 +720,42 @@ void kvfree_sensitive(const void *addr, size_t len) } EXPORT_SYMBOL(kvfree_sensitive); -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 (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); + 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);
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. 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. Signed-off-by: Danilo Krummrich <dakr@kernel.org> --- arch/arm64/kvm/nested.c | 1 - arch/powerpc/platforms/pseries/papr-vpd.c | 5 +- drivers/gpu/drm/drm_exec.c | 3 +- fs/xfs/xfs_log_recover.c | 2 +- include/linux/slab.h | 4 +- kernel/resource.c | 3 +- lib/fortify_kunit.c | 3 +- mm/util.c | 89 +++++++++++++++-------- 8 files changed, 66 insertions(+), 44 deletions(-)