Re: [PATCH] QEMU/KVM: large page support

Anthony Liguori
2008-02-23T18:29:27Z

Hi Marcelo,

Marcelo Tosatti wrote:
> Add an option so the user can specify the hugetlbfs mounted path, with
> fallback to 4k pages on error.
>
> Align the 4GB+ memslot on large page boundary.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> Index: kvm-userspace.gitpara/qemu/hw/pc.c
> ===================================================================
> --- kvm-userspace.gitpara.orig/qemu/hw/pc.c
> +++ kvm-userspace.gitpara/qemu/hw/pc.c
> @@ -854,6 +854,15 @@ static void pc_init1(ram_addr_t ram_size
> /* above 4giga memory allocation */
> if (above_4g_mem_size > 0) {
> ram_addr = qemu_ram_alloc(above_4g_mem_size);
> + if (hpagesize) {
> + if (ram_addr & (hpagesize-1)) {
> + unsigned long aligned_addr;
> + aligned_addr = (ram_addr + hpagesize - 1) &
> + ~(hpagesize-1);
> + qemu_ram_alloc(aligned_addr - ram_addr);
> + ram_addr = aligned_addr;
> + }
> + }
>

In general, I don't think it causes any real harm if we always align the
ram address to a large page boundary. If we aren't on Linux (and can't
determine what the large page size is), we can just set hpagesize to
getpagesize(). I think there's a good reason for this that I'll explain
below.

> +
> +void *alloc_huge_area(unsigned long memory, const char *path)
> +{
> + void *area;
> + int fd;
> + char *filename;
> + char *tmpfile = "/kvm.XXXXXX";
> +
> + filename = qemu_malloc(4096);
> + if (!filename)
> + return NULL;
> +
> + memset(filename, 0, 4096);
> + strncpy(filename, path, 4096 - strlen(tmpfile) - 1);
> + strcat(filename, tmpfile);
> +
> + hpagesize = gethugepagesize() * 1024;
> + if (!hpagesize)
> + return NULL;
> +
> + mkstemp(filename);
>

mkstemp returns a file descriptor so the following open is not required.

> + fd = open(filename, O_RDWR);
> + if (fd < 0) {
> + perror("open");
> + hpagesize = 0;
> + exit(0);
> + }
> + memory = (memory+hpagesize-1) & ~(hpagesize-1);
>

I'm a little surprised that hugetlbfs doesn't require an ftruncate()
before the mmap(). Does an ftruncate() do any harm? If so, it would be
better to have one.

> + area = mmap(0, memory, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);
>

I think a MAP_SHARED may have some advantages in that it then becomes
possible to pass this file descriptor around to other processes so they
can mmap() the same memory region. I don't know if that works with
hugetlbfs but it certainly does with tmpfs. My thinking is that this
code can be made generic so it works with either hugetlbfs or tmpfs.

Furthermore, I think it would be interesting if we defaulted to trying
to create the memory file in something like /dev/kvm-mem or something
more appropriately named. An administrator can then either mount a
hugetlbfs or tmpfs mount there. We still probably want to provide an
option to override where to create the file and we want to be able to
fall back to a normal malloc() of course but at least this makes it
possible for the distros to set things up so that it Just Work without
the user having to understand things like hugetlbfs and tmpfs.

Maybe we'll even see something that merges the two filesystems in the
future so that if a huge page allocation fails, it falls back to
creating a normal tmpfs file. Perhaps that's a reasonable mount option
to add to hugetlbfs.

> + if (area == MAP_FAILED) {
> + perror("mmap");
> + hpagesize = 0;
> + exit(0);
> + }
> +
> + hugetlbfile = filename;
> + atexit(cleanup_hugetlb);
>

Instead of registering an atexit() handler, I think it would be better
to unlink immediately after doing the mkstemp(). This reduces the
possibility of leaking the file in the event of catastrophe (like a kill
-SIGKILL).

Regards,

Anthony Liguori