Re: [Xen-devel] [PATCH] libxc: Replace alloca() with mmap() in linux_privcmd_map_foreign_bulk()

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-04-19 at 23:24 +0100, Aravindh Puthiyaparambil wrote:
> When mapping in large amounts of pages (in the GB range) from a guest
> in to Dom0 using xc_map_foreign_bulk(), a segfault occurs in the libxc
> client application. This is because the pfn array in
> linux_privcmd_map_foreign_bulk() is being allocated using alloca() and
> the subsequent memcpy causes the stack to blow. This patch replaces
> the alloca() with mmap().

The original reason for switching to alloca from malloc was because the
similar gntmap path was a hot path and it seemed like if it was
reasonable there it would be reasonable here too.

So I have a couple of questions...

Is it possible that we also introduced this same issue at the other
callsites which were changed in that patch or are there other
constraints on the size of the allocation which make it not a problem?

Where does this mmap approach fall in terms of performance relative to
just switching back to malloc? I presume that alloca is faster than
both, but if it doesn't work reliably then it's not an option.

If mmap and malloc have roughly equivalent performance properties, or
the fast one is still too slow for Santosh's use case, then maybe we
need to have a think about other options.

e.g. use alloca for small numbers of mappings and mmap/malloc for larger
ones. Or do large numbers of mappings in multiple batches.

Ian.

> 
> Signed-off-by: Aravindh Puthiyaparambil <aravindh@xxxxxxxxxxxx>
> Acked-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
> 
> diff -r 7c777cb8f705 -r 2f68aefc46c3 tools/libxc/xc_linux_osdep.c
> --- a/tools/libxc/xc_linux_osdep.c    Wed Apr 18 16:49:55 2012 +0100
> +++ b/tools/libxc/xc_linux_osdep.c    Thu Apr 19 15:21:43 2012 -0700
> @@ -39,6 +39,7 @@
>  #include "xenctrl.h"
>  #include "xenctrlosdep.h"
>  
> +#define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & 
> ~((1UL<<(_w))-1))
>  #define ERROR(_m, _a...)  xc_osdep_log(xch,XTL_ERROR,XC_INTERNAL_ERROR,_m , 
> ## _a )
>  #define PERROR(_m, _a...) xc_osdep_log(xch,XTL_ERROR,XC_INTERNAL_ERROR,_m \
>                    " (%d = %s)", ## _a , errno, xc_strerror(xch, errno))
> @@ -286,7 +287,14 @@ static void *linux_privcmd_map_foreign_b
>           * IOCTL_PRIVCMD_MMAPBATCH.
>           */
>          privcmd_mmapbatch_t ioctlx;
> -        xen_pfn_t *pfn = alloca(num * sizeof(*pfn));
> +        xen_pfn_t *pfn = mmap(NULL, ROUNDUP((num * sizeof(*pfn)), 
> XC_PAGE_SHIFT),
> +                              PROT_READ | PROT_WRITE,
> +                              MAP_PRIVATE | MAP_ANON | MAP_POPULATE, -1, 0);
> +        if ( pfn == MAP_FAILED )
> +        {
> +            PERROR("xc_map_foreign_bulk: mmap of pfn array failed");
> +            return NULL;
> +        }
>  
>          memcpy(pfn, arr, num * sizeof(*arr));
>  
> @@ -328,6 +336,8 @@ static void *linux_privcmd_map_foreign_b
>              break;
>          }
>  
> +        munmap(pfn, ROUNDUP((num * sizeof(*pfn)), XC_PAGE_SHIFT));
> +
>          if ( rc == -ENOENT && i == num )
>              rc = 0;
>          else if ( rc )
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



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