| Message ID | 69778de6-3b94-64d1-99d9-1a0fcfa503fd@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xvmalloc() / x86 xstate area / x86 CPUID / AMX+XFD | expand |
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,...}? > --- /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. > + */ > + > +/* 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? > + > +/* 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. Thanks, Roger.
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
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? > 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 What do you mean with more powerful here? Thanks, Roger.
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
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.
--- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -37,7 +37,7 @@ #include <xen/iommu.h> #include <xen/paging.h> #include <xen/keyhandler.h> -#include <xen/vmap.h> +#include <xen/xvmalloc.h> #include <xen/nospec.h> #include <xsm/xsm.h> #include <asm/flushtlb.h> @@ -1749,8 +1749,8 @@ gnttab_populate_status_frames(struct dom if ( gt->status == ZERO_BLOCK_PTR ) { - gt->status = xzalloc_array(grant_status_t *, - grant_to_status_frames(gt->max_grant_frames)); + gt->status = xvzalloc_array(grant_status_t *, + grant_to_status_frames(gt->max_grant_frames)); if ( !gt->status ) { gt->status = ZERO_BLOCK_PTR; @@ -1780,7 +1780,7 @@ status_alloc_failed: } if ( !nr_status_frames(gt) ) { - xfree(gt->status); + xvfree(gt->status); gt->status = ZERO_BLOCK_PTR; } return -ENOMEM; @@ -1852,7 +1852,7 @@ gnttab_unpopulate_status_frames(struct d gt->nr_status_frames = 0; for ( i = 0; i < n; i++ ) free_xenheap_page(gt->status[i]); - xfree(gt->status); + xvfree(gt->status); gt->status = ZERO_BLOCK_PTR; return 0; @@ -1966,21 +1966,22 @@ int grant_table_init(struct domain *d, i d->grant_table = gt; /* Active grant table. */ - gt->active = xzalloc_array(struct active_grant_entry *, - max_nr_active_grant_frames(gt)); + gt->active = xvzalloc_array(struct active_grant_entry *, + max_nr_active_grant_frames(gt)); if ( gt->active == NULL ) goto out; /* Tracking of mapped foreign frames table */ if ( gt->max_maptrack_frames ) { - gt->maptrack = vzalloc(gt->max_maptrack_frames * sizeof(*gt->maptrack)); + gt->maptrack = xvzalloc_array(struct grant_mapping *, + gt->max_maptrack_frames); if ( gt->maptrack == NULL ) goto out; } /* Shared grant table. */ - gt->shared_raw = xzalloc_array(void *, gt->max_grant_frames); + gt->shared_raw = xvzalloc_array(void *, gt->max_grant_frames); if ( gt->shared_raw == NULL ) goto out; @@ -3868,19 +3869,19 @@ grant_table_destroy( for ( i = 0; i < nr_grant_frames(t); i++ ) free_xenheap_page(t->shared_raw[i]); - xfree(t->shared_raw); + xvfree(t->shared_raw); for ( i = 0; i < nr_maptrack_frames(t); i++ ) free_xenheap_page(t->maptrack[i]); - vfree(t->maptrack); + xvfree(t->maptrack); for ( i = 0; i < nr_active_grant_frames(t); i++ ) free_xenheap_page(t->active[i]); - xfree(t->active); + xvfree(t->active); for ( i = 0; i < nr_status_frames(t); i++ ) free_xenheap_page(t->status[i]); - xfree(t->status); + xvfree(t->status); xfree(t); d->grant_table = NULL; --- a/xen/common/vmap.c +++ b/xen/common/vmap.c @@ -7,6 +7,7 @@ #include <xen/spinlock.h> #include <xen/types.h> #include <xen/vmap.h> +#include <xen/xvmalloc.h> #include <asm/page.h> static DEFINE_SPINLOCK(vm_lock); @@ -301,11 +302,29 @@ void *vzalloc(size_t size) return p; } -void vfree(void *va) +static void _vfree(const void *va, unsigned int pages, enum vmap_region type) { - unsigned int i, pages; + unsigned int i; struct page_info *pg; PAGE_LIST_HEAD(pg_list); + + ASSERT(pages); + + for ( i = 0; i < pages; i++ ) + { + pg = vmap_to_page(va + i * PAGE_SIZE); + ASSERT(pg); + page_list_add(pg, &pg_list); + } + vunmap(va); + + while ( (pg = page_list_remove_head(&pg_list)) != NULL ) + free_domheap_page(pg); +} + +void vfree(void *va) +{ + unsigned int pages; enum vmap_region type = VMAP_DEFAULT; if ( !va ) @@ -317,18 +336,71 @@ void vfree(void *va) type = VMAP_XEN; pages = vm_size(va, type); } - ASSERT(pages); - for ( i = 0; i < pages; i++ ) + _vfree(va, pages, type); +} + +void xvfree(void *va) +{ + unsigned int pages = vm_size(va, VMAP_DEFAULT); + + if ( pages ) + _vfree(va, pages, VMAP_DEFAULT); + else + xfree(va); +} + +void *_xvmalloc(size_t size, unsigned int align) +{ + ASSERT(align <= PAGE_SIZE); + return size <= PAGE_SIZE ? _xmalloc(size, align) : vmalloc(size); +} + +void *_xvzalloc(size_t size, unsigned int align) +{ + ASSERT(align <= PAGE_SIZE); + return size <= PAGE_SIZE ? _xzalloc(size, align) : vzalloc(size); +} + +void *_xvrealloc(void *va, size_t size, unsigned int align) +{ + size_t pages = vm_size(va, VMAP_DEFAULT); + void *ptr; + + ASSERT(align <= PAGE_SIZE); + + if ( !pages ) { - struct page_info *page = vmap_to_page(va + i * PAGE_SIZE); + if ( size <= PAGE_SIZE ) + return _xrealloc(va, size, align); - ASSERT(page); - page_list_add(page, &pg_list); + ptr = vmalloc(size); + if ( ptr && va && va != ZERO_BLOCK_PTR ) + { + /* + * xmalloc-based allocations up to PAGE_SIZE don't cross page + * boundaries. Therefore, without needing to know the exact + * prior allocation size, simply copy the entire tail of the + * page containing the earlier allocation. + */ + memcpy(ptr, va, PAGE_SIZE - PAGE_OFFSET(va)); + xfree(va); + } + } + else if ( pages == PFN_UP(size) ) + ptr = va; + else + { + ptr = _xvmalloc(size, align); + if ( ptr ) + { + memcpy(ptr, va, min(size, pages << PAGE_SHIFT)); + vfree(va); + } + else if ( pages > PFN_UP(size) ) + ptr = va; } - vunmap(va); - while ( (pg = page_list_remove_head(&pg_list)) != NULL ) - free_domheap_page(pg); + return ptr; } #endif --- /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. + */ + +/* 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) + +/* 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); + +static inline void *_xvmalloc_array( + size_t size, unsigned int align, unsigned long num) +{ + /* Check for overflow. */ + if ( size && num > UINT_MAX / size ) + return NULL; + return _xvmalloc(size * num, align); +} + +static inline void *_xvzalloc_array( + size_t size, unsigned int align, unsigned long num) +{ + /* Check for overflow. */ + if ( size && num > UINT_MAX / size ) + return NULL; + return _xvzalloc(size * num, align); +} + +#endif /* __XVMALLOC_H__ */
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. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Actually edit a copy-and-pasted comment in xvmalloc.h which was meant to be edited from the beginning. --- I'm unconvinced of the mentioning of "physically contiguous" in the comment at the top of the new header: I don't think xmalloc() provides such a guarantee. Any use assuming so would look (latently) broken to me.