Re: [PATCH 1/2] mm: vmalloc: implement vrealloc()

[Danilo Krummrich]

On Wed, Jul 17, 2024 at 08:16:29PM -0700, Christoph Hellwig wrote:
> On Thu, Jul 18, 2024 at 12:24:01AM +0200, Danilo Krummrich wrote:
> > +extern void * __must_check vrealloc_noprof(const void *p, size_t size,
> > +					   gfp_t flags) __realloc_size(2);
> 
> No need for the extern here.
> 
> It would also help readability a bit to keep the __realloc_size on a
> separate like, aka:
> 
> void *__must_check vrealloc_noprof(const void *p, size_t size, gfp_t flags)
> 		__realloc_size(2);

Sure, will change that.

> 
> > +	if (size <= old_size) {
> > +		/* TODO: Can we optimize and shrink the allocation? What would
> > +		 * be a good metric for when to shrink the vm_area?
> > +		 */
> 
> Kernel comment style is to keep the
> 
> 		/*
> 
> on a line of it's own.

I think it differs a bit throughout subsystems - will change.


> A realloc that doesn't shrink is a bit pointless.

Indeed - the idea was to mostly clean up kvrealloc() in this series first and
implement proper shrinking and growing in a subsequent one. Does that make
sense?

> The same heuristics as for krealloc should probably apply
> here, adjust to the fact that we always deal with whole pages.

Sounds reasonable, but are there any? In __do_krealloc() if ks > new_size we
only call into kasan_krealloc() to poison things and return the original
pointer. Do I miss anything?

> 
> 
> > +	/* TODO: Can we optimize and extend the existing allocation if we have
> > +	 * enough contiguous space left in the virtual address space?
> > +	 */
> 
> I don't understand this comment.

I meant to say that we could optimze by allocating additional pages and map them
at the end of the existing allocation, if there is enough space in the VA space.

If that doesn't work, we can still allocate additional pages and remap
everything.

> 
> > +EXPORT_SYMBOL(vrealloc_noprof);
> 
> New interfaces should be EXPORT_SYMBOL_GPL.  That being said for this
> to be added an exported we'll need an actual user to start with anyway.

Besides kvrealloc(), the Rust abstractions will be a user soon, but for that we
probably don't need the export either. I will remove it for now.