Re: [Xen-devel] [PATCH v5 1/4] xen: introduce a helper to allocate non-contiguous memory

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.05.15 at 16:34, <roger.pau@xxxxxxxxxx> wrote:
> --- a/xen/common/vmap.c
> +++ b/xen/common/vmap.c
> @@ -146,7 +146,7 @@ static unsigned int vm_index(const void *va)
>             test_bit(idx, vm_bitmap) ? idx : 0;
>  }
>  
> -static unsigned int vm_size(const void *va)
> +unsigned int vm_size(const void *va)

I was hoping you would avoid this. I really think vmalloc() and vfree()
should go into vmap.c. They certainly don't belong into ...

> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c

... here.

> +void *vmalloc(unsigned long size)
> +{
> +    unsigned long *mfn;
> +    unsigned long pages, i;
> +    struct page_info *pg;
> +    void *va = NULL;
> +
> +    ASSERT(!in_irq());

I don't think you explicitly need this here - the xzalloc_array() below
will catch that case, and the function here doesn't itself depend on
that.

> +    if ( size == 0 )
> +        return ZERO_BLOCK_PTR;

If you really wanted this, vfree() would need to cope. But I think it's
better to simply ASSERT(size) here.

> +    pages = DIV_ROUND_UP(size, PAGE_SIZE);

I think this would better use PFN_UP().

> +    mfn = xzalloc_array(unsigned long, pages);
> +    if ( mfn == NULL )
> +        return NULL;
> +
> +    for ( i = 0; i < pages; i++ )
> +    {
> +        pg = alloc_domheap_pages(NULL, 1, 0);
> +        if ( pg == NULL )
> +            goto error;
> +        mfn[i] = page_to_mfn(pg);
> +    }
> +
> +    va = vmap(mfn, pages);
> +    if ( va == NULL )
> +        goto error;
> +
> +    xfree(mfn);
> +    return va;
> +
> + error:
> +    vunmap(va);

I don't think vunmap() copes with NULL passed into it. But I also
don't see why you call it in the first place - no path leads here
with va != NULL.

> +void vfree(void *va)
> +{
> +    unsigned int i, pages = vm_size(va);
> +
> +    if ( pages == 0 )
> +        return;

This then shouldn't be needed either (or become an ASSERT()).

> --- a/xen/include/xen/xmalloc.h
> +++ b/xen/include/xen/xmalloc.h
> @@ -30,6 +30,10 @@ extern void xfree(void *);
>  extern void *_xmalloc(unsigned long size, unsigned long align);
>  extern void *_xzalloc(unsigned long size, unsigned long align);
>  
> +void *vmalloc(unsigned long size);
> +void *vzalloc(unsigned long size);
> +void vfree(void *va);

And consequently this is the wrong place then too.

Jan


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