Re: [Xen-devel] [PATCH v9 08/27] arm/x86/vmap: Add v[z|m]alloc_xen and vm_init_type

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.04.16 at 04:38, <konrad.wilk@xxxxxxxxxx> wrote:
>>  With vm_alloc() getting removed, vm_free() should get removed
>> here too. And with that, vm_alloc_type() and vm_free_type() can
>> then just become vm_alloc() and vm_free() respectively (as static
>> internal functions).
> 
> Please take a look at this inline one:

Better, and it can have my ack, but it's still doing more changes than
really needed:

> +static void vunmap_pages(const void *va, unsigned int pages)
> +{
> +#ifndef _PAGE_NONE
> +    unsigned long addr = (unsigned long)va;
> +
> +    destroy_xen_mappings(addr, addr + PAGE_SIZE * pages);
> +#else /* Avoid tearing down intermediate page tables. */
> +    map_pages_to_xen((unsigned long)va, 0, pages, _PAGE_NONE);
> +#endif
> +    vm_free(va);
> +}

There's no real reason to break this out and move up here - the
two callers other than vunmap() could easily continue to call
vunmap(). The more that you do not similarly leverage knowing
the type here already (all callers of vunmap_pages() already
know the type, and hence could pass it here).

> +void vunmap(const void *va)
> +{
> +    enum vmap_region type = VMAP_DEFAULT;

If vunmap_pages() was to stay, and was to continue to not have a
type parameter, this local variable is pointless.

> @@ -266,16 +308,32 @@ void *vzalloc(size_t size)
>      return p;
>  }
>  
> +void *vzalloc(size_t size)
> +{
> +    return vzalloc_type(size, VMAP_DEFAULT);
> +}
> +
> +void *vzalloc_xen(size_t size)
> +{
> +    return vzalloc_type(size, VMAP_XEN);
> +}

I didn't look at your replies to the later patches yet, but considering
my reply to the one using vzalloc_xen() I wonder whether in fact
you still need this flavor (and hence vzalloc_type()).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel