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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-04-20 at 15:08 +0100, Andres Lagar-Cavilla wrote:
> > 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 in these cases the whole buffer will always be used, since they
are sized precisely for what they need to contain.

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

I agree.

Ian.

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