Re: [PATCH v3 01/22] mm: introduce xvmalloc() et al and use for grant table allocations

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, May 03, 2021 at 05:21:37PM +0200, Jan Beulich wrote:
> On 03.05.2021 16:54, Roger Pau Monné wrote:
> > On Mon, May 03, 2021 at 03:50:48PM +0200, Jan Beulich wrote:
> >> On 03.05.2021 13:31, Roger Pau Monné wrote:
> >>> On Thu, Apr 22, 2021 at 04:43:39PM +0200, Jan Beulich wrote:
> >>>> All of the array allocations in grant_table_init() can exceed a page's
> >>>> worth of memory, which xmalloc()-based interfaces aren't really suitable
> >>>> for after boot. We also don't need any of these allocations to be
> >>>> physically contiguous.. Introduce interfaces dynamically switching
> >>>> between xmalloc() et al and vmalloc() et al, based on requested size,
> >>>> and use them instead.
> >>>>
> >>>> All the wrappers in the new header get cloned mostly verbatim from
> >>>> xmalloc.h, with the sole adjustment to switch unsigned long to size_t
> >>>> for sizes and to unsigned int for alignments.
> >>>
> >>> We seem to be growing a non-trivial amount of memory allocation
> >>> families of functions: xmalloc, vmalloc and now xvmalloc.
> >>>
> >>> I think from a consumer PoV it would make sense to only have two of
> >>> those: one for allocations that require to be physically contiguous,
> >>> and one for allocation that don't require it.
> >>>
> >>> Even then, requesting for physically contiguous allocations could be
> >>> done by passing a flag to the same interface that's used for
> >>> non-contiguous allocations.
> >>>
> >>> Maybe another option would be to expand the existing
> >>> v{malloc,realloc,...} set of functions to have your proposed behaviour
> >>> for xv{malloc,realloc,...}?
> >>
> >> All of this and some of your remarks further down has already been
> >> discussed. A working group has been formed. No progress since. Yes,
> >> a smaller set of interfaces may be the way to go. Controlling
> >> behavior via flags, otoh, is very much not malloc()-like. Making
> >> existing functions have the intended new behavior is a no-go without
> >> auditing all present uses, to find those few which actually may need
> >> physically contiguous allocations.
> > 
> > But you could make your proposed xvmalloc logic the implementation
> > behind vmalloc, as that would still be perfectly fine and safe? (ie:
> > existing users of vmalloc already expect non-physically contiguous
> > memory). You would just optimize the size < PAGE_SIZE for that
> > interface?
> 
> Existing callers of vmalloc() may expect page alignment of the
> returned address.

Right - just looked and also the interface is different from
x{v}malloc, so you would have to fixup callers.

> >>>> --- /dev/null
> >>>> +++ b/xen/include/xen/xvmalloc.h
> >>>> @@ -0,0 +1,73 @@
> >>>> +
> >>>> +#ifndef __XVMALLOC_H__
> >>>> +#define __XVMALLOC_H__
> >>>> +
> >>>> +#include <xen/cache.h>
> >>>> +#include <xen/types.h>
> >>>> +
> >>>> +/*
> >>>> + * Xen malloc/free-style interface for allocations possibly exceeding a 
> >>>> page's
> >>>> + * worth of memory, as long as there's no need to have physically 
> >>>> contiguous
> >>>> + * memory allocated.  These should be used in preference to xmalloc() 
> >>>> et al
> >>>> + * whenever the size is not known to be constrained to at most a single 
> >>>> page.
> >>>
> >>> Even when it's know that size <= PAGE_SIZE this helpers are
> >>> appropriate as they would end up using xmalloc, so I think it's fine to
> >>> recommend them universally as long as there's no need to alloc
> >>> physically contiguous memory?
> >>>
> >>> Granted there's a bit more overhead from the logic to decide between
> >>> using xmalloc or vmalloc &c, but that's IMO not that big of a deal in
> >>> order to not recommend this interface globally for non-contiguous
> >>> alloc.
> >>
> >> As long as xmalloc() and vmalloc() are meant stay around as separate
> >> interfaces, I wouldn't want to "forbid" their use when it's sufficiently
> >> clear that they would be chosen by the new function anyway. Otoh, if the
> >> new function became more powerful in terms of falling back to the
> > 
> > What do you mean with more powerful here?
> 
> Well, right now the function is very simplistic, looking just at the size
> and doing no fallback attempts at all. Linux'es kvmalloc() goes a little
> farther. What I see as an option is for either form of allocation to fall
> back to the other form in case the first attempt fails. This would cover
> - out of memory Xen heap for small allocs,
> - out of VA space for large allocs.
> And of course, like Linux does (or at least did at the time I looked at
> their code), the choice which of the backing functions to call could also
> become more sophisticated over time.

I'm not opposed to any of this, but even your proposed code right now
seems no worse than using either vmalloc or xmalloc, as it's only a
higher level wrapper around those.

What I would prefer is to propose to use function foo for all
allocations that don't require contiguous physical memory, and
function bar for those that do require contiguous physical memory.
It's IMO awkward from a developer PoV to have to select an
allocation function based on the size to be allocated.

I wouldn't mind if you wanted to name this more generic wrapped straight
malloc.

Thanks, Roger.