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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Having seen similar naming elsewhere, I did propose xnew() /
xdelete() (plus array and flex-struct counterparts) as the single
new recommended interface; didn't hear back yet. But we'd switch to
that gradually, so intermediately there would still be a larger set
of interfaces.

I'm not convinced we should continue to have byte-granular allocation
functions producing physically contiguous memory. I think the page
allocator should be used directly in such cases.

>> --- /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
respectively other lower level function, that might be an argument in
favor of always using the new interfaces.

>> + */
>> +
>> +/* Allocate space for typed object. */
>> +#define xvmalloc(_type) ((_type *)_xvmalloc(sizeof(_type), 
>> __alignof__(_type)))
>> +#define xvzalloc(_type) ((_type *)_xvzalloc(sizeof(_type), 
>> __alignof__(_type)))
>> +
>> +/* Allocate space for array of typed objects. */
>> +#define xvmalloc_array(_type, _num) \
>> +    ((_type *)_xvmalloc_array(sizeof(_type), __alignof__(_type), _num))
>> +#define xvzalloc_array(_type, _num) \
>> +    ((_type *)_xvzalloc_array(sizeof(_type), __alignof__(_type), _num))
>> +
>> +/* Allocate space for a structure with a flexible array of typed objects. */
>> +#define xvzalloc_flex_struct(type, field, nr) \
>> +    ((type *)_xvzalloc(offsetof(type, field[nr]), __alignof__(type)))
>> +
>> +#define xvmalloc_flex_struct(type, field, nr) \
>> +    ((type *)_xvmalloc(offsetof(type, field[nr]), __alignof__(type)))
>> +
>> +/* Re-allocate space for a structure with a flexible array of typed 
>> objects. */
>> +#define xvrealloc_flex_struct(ptr, field, nr)                          \
>> +    ((typeof(ptr))_xvrealloc(ptr, offsetof(typeof(*(ptr)), field[nr]), \
>> +                             __alignof__(typeof(*(ptr)))))
>> +
>> +/* Allocate untyped storage. */
>> +#define xvmalloc_bytes(_bytes) _xvmalloc(_bytes, SMP_CACHE_BYTES)
>> +#define xvzalloc_bytes(_bytes) _xvzalloc(_bytes, SMP_CACHE_BYTES)
> 
> I see xmalloc does the same, wouldn't it be enough to align to a lower
> value? Seems quite wasteful to align to 128 on x86 by default?

Yes, it would. Personally (see "[PATCH v2 0/8] assorted replacement of
x[mz]alloc_bytes()") I think these ..._bytes() wrappers should all go
away. Hence I don't think it's very important how exactly they behave,
and in turn it's then best to have them match x[mz]alloc_bytes().

>> +
>> +/* Free any of the above. */
>> +extern void xvfree(void *);
>> +
>> +/* Free an allocation, and zero the pointer to it. */
>> +#define XVFREE(p) do { \
>> +    xvfree(p);         \
>> +    (p) = NULL;        \
>> +} while ( false )
>> +
>> +/* Underlying functions */
>> +extern void *_xvmalloc(size_t size, unsigned int align);
>> +extern void *_xvzalloc(size_t size, unsigned int align);
>> +extern void *_xvrealloc(void *ptr, size_t size, unsigned int align);
> 
> Nit: I would drop the 'extern' keyword from the function prototypes.

Ah yes, will do. Simply a result of taking the other header as basis.

Jan