Re: [Qemu-devel] [PATCH] mmap-alloc: use same backend for all mappings

[Michael S. Tsirkin]

On Tue, Dec 01, 2015 at 11:42:15AM +0100, Greg Kurz wrote:
> On Mon, 30 Nov 2015 15:12:08 +0200
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Mon, Nov 30, 2015 at 11:53:41AM +0100, Paolo Bonzini wrote:
> > > 
> > > 
> > > On 30/11/2015 11:51, Greg Kurz wrote:
> > > > Since commit 8561c9244ddf1122d "exec: allocate PROT_NONE pages on top 
> > > > of RAM",
> > > > it is no longer possible to back guest RAM with hugepages on ppc64 
> > > > hosts:
> > > > 
> > > > mmap(NULL, 285212672, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 
> > > > 0x3fff57000000
> > > > mmap(0x3fff57000000, 268435456, PROT_READ|PROT_WRITE, 
> > > > MAP_PRIVATE|MAP_FIXED, 19, 0) = -1 EBUSY (Device or resource busy)
> > > > 
> > > > This is due to a limitation on ppc64 that requires MAP_FIXED mappings 
> > > > to have
> > > > the same page size as other mappings already present in the same 
> > > > "slice" of
> > > > virtual address space (Cc'ing Ben for details). This is exactly what 
> > > > happens
> > > > when calling mmap() above: first one uses native host page size (64k) 
> > > > and
> > > > second one uses huge page size (16M).
> > > > 
> > > > To be sure we always have the same page size, let's use the same 
> > > > backend for
> > > > both calls to mmap(): this is enough to fix the ppc64 issue.
> > > > 
> > > > This has no effect on RAM based mappings.
> > > > 
> > > > Signed-off-by: Greg Kurz <address@hidden>
> > > > ---
> > > > 
> > > > This is a bug fix for 2.5
> > > > 
> > > >  util/mmap-alloc.c |    3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> > > > index c37acbe58ede..0ff221dd94f4 100644
> > > > --- a/util/mmap-alloc.c
> > > > +++ b/util/mmap-alloc.c
> > > > @@ -21,7 +21,8 @@ void *qemu_ram_mmap(int fd, size_t size, size_t 
> > > > align, bool shared)
> > > >       * space, even if size is already aligned.
> > > >       */
> > > >      size_t total = size + align;
> > > > -    void *ptr = mmap(0, total, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, 
> > > > -1, 0);
> > > > +    void *ptr = mmap(0, total, PROT_NONE,
> > > > +                     (fd == -1 ? MAP_ANONYMOUS : 0) | MAP_PRIVATE, fd, 
> > > > 0);
> > > >      size_t offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - 
> > > > (uintptr_t)ptr;
> > > >      void *ptr1;
> > > >  
> > > > 
> > > 
> > > Acked-by: Paolo Bonzini <address@hidden>
> > 
> > But why does this patch have any effect?
> > I'm worried that extra memory is still allocated
> > with this, even if it's not accessible.
> > 
> 
> And you are right because that is exactly what is happening
> with hugetlbfs_file_mmap()->hugetlb_reserve_pages() :-\

By the way, this also means we were wasting a bunch of
memory when trying to get aligned pages.

> > If yes, we are better off disabling the protection for ppc.
> > 
> 
> Yes, this is the only alternative... I'll send a patch ASAP.
> 
> Thanks !

Does MAP_HUGETLB does anything?
I would expect it to get a slice with the correct page size.
If not, this might be a reasonable thing to implement in kernel.

> --
> Greg