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

["Andres Lagar-Cavilla" <andres@xxxxxxxxxxxxxxxx>]

> 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?

I don't know how arbitrary or large can the buffer sizes be in the other
callsites. It would seem that they can be large enough.
>
> 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.

It's hard to dig out exactly how alloca is implemented, but it would seem
to simply move the stack pointer and return the old one (it's arguable
whether there is a buggy check for limits or a buggy lack of check against
the guard page going on -- man page says "behavior undefined"). So, if you
need new pages in the stack as a result of the stack pointer motion, those
pages will be demand allocated.

mmap just short circuits straight to page allocation. Note that malloc may
or may not wind up calling mmap if it needs more pages, depending on its
internal machinations.

MAP_POPULATE tells the mmap syscall to allocate right now, avoiding demand
allocation. Presumably this will batch all page allocations in the single
syscall, and avoid page faults down the line.

Short story, I suggested mmap with MAP_POPULATE because it will do the
allocation of pages in the most optimal fashion, even better than malloc
or alloca. But it's only worth if the buffer is big enough such that new
pages will need to be allocated.

I think a reasonable compromise would be to do alloca if the buffer is
less than one page (a fairly common case, single pfn buffers, etc), and do
mmap(MAP_POPULATE) for buffers larger than a page.

Andres

>
> 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