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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan