Re: patch: qemu + hugetlbfs..

[Anthony Liguori <anthony@codemonkey.ws>]

john cooper wrote:
> Anthony Liguori wrote:
>> Marcelo Tosatti wrote:
>>> This is Linux's behaviour for all filesystems. There is no error
>>> checking on MAP_POPULATE's attempt to prefault pages.
>>>   
>>
>> I thought that mmap() will fail with hugetlbfs if enough large pages 
>> can't be reserved?
>
> This appears to be the case for MAP_SHARED.  For MAP_PRIVATE
> the accounting is side-stepped, the result of which was the
> best-effort prealloc/populate behavior we had see seen.
>
> Marcelo and I batted this around some yesterday.  And there
> doesn't appear to be any concern using MAP_SHARED for the
> phys_ram map as potential access to the backing file is
> still regulated by filesystem permissions.
>
> Doing so dispenses with the syscall/useraddr hack to fault
> in pages since mmap() fails as expected in the case
> sufficient pages are unavailable immediately to back
> MAP_SHARED pages.  We also decided to defer the map-fallback
> logic.  This is essentially behavior we'd like to see
> provided by kvm, ideally on a dynamic (guest fault) basis
> and was technically beyond the issue we were trying to
> target.
>
> Attached is the resulting patch which also addresses the
> other concerns which arose.
>
> -john
>
> --- a/qemu/vl.c
> +++ b/qemu/vl.c
> @@ -45,6 +45,7 @@
>  #include <signal.h>
>  #include <time.h>
>  #include <errno.h>
> +#include <asm/param.h>
>   

I don't think this is necessary anymore.  Depending on a Linux headers 
breaks the QEMU build on other unices so it's a bad thing.

>  #include <sys/time.h>
>  #include <zlib.h>
>  
> @@ -234,6 +235,7 @@ int autostart = 1;
>  int time_drift_fix = 0;
>  unsigned int kvm_shadow_memory = 0;
>  const char *mem_path = NULL;
> +int mem_prealloc = 1;	/* force preallocation of physical target memory */
>  int hpagesize = 0;
>  const char *cpu_vendor_string;
>  #ifdef TARGET_ARM
> @@ -7809,7 +7811,10 @@ static void help(int exitcode)
>  #endif
>             "-tdf            inject timer interrupts that got lost\n"
>             "-kvm-shadow-memory megs set the amount of shadow pages to be allocated\n"
> -           "-mem-path       set the path to hugetlbfs/tmpfs mounted directory, also enables allocation of guest memory with huge pages\n"
> +           "-mem-path       set the path to hugetlbfs/tmpfs mounted directory, also\n"
> +           "                enables allocation of guest memory with huge pages\n"
> +           "-mem-prealloc   toggles preallocation of huge page physical memory at\n"
> +           "                startup when -mem-path is specified.  Default is enabled.\n"
>   

This really isn't huge page specific.  It should work equally well with 
tmpfs, no?

>  	   "-option-rom rom load a file, rom, into the option ROM space\n"
>  #ifdef TARGET_SPARC
>             "-prom-env variable=value  set OpenBIOS nvram variables\n"
> @@ -7932,6 +7937,7 @@ enum {
>      QEMU_OPTION_tdf,
>      QEMU_OPTION_kvm_shadow_memory,
>      QEMU_OPTION_mempath,
> +    QEMU_OPTION_mem_prealloc
>  };
>  
>  typedef struct QEMUOption {
> @@ -8059,6 +8065,7 @@ const QEMUOption qemu_options[] = {
>      { "startdate", HAS_ARG, QEMU_OPTION_startdate },
>      { "tb-size", HAS_ARG, QEMU_OPTION_tb_size },
>      { "mem-path", HAS_ARG, QEMU_OPTION_mempath },
> +    { "mem-prealloc", 0, QEMU_OPTION_mem_prealloc },
>      { NULL },
>  };
>  
> @@ -8276,11 +8283,13 @@ static int gethugepagesize(void)
>      return hugepagesize;
>  }
>  
> -void *alloc_mem_area(unsigned long memory, const char *path)
> +/* attempt to allocate memory mmap'ed to mem_path
> + */
> +void *alloc_hpage_mem(unsigned long memory, const char *path)
>  {
>      char *filename;
>      void *area;
> -    int fd;
> +    int fd, flags;
>  
>      if (asprintf(&filename, "%s/kvm.XXXXXX", path) == -1)
>  	return NULL;
> @@ -8308,26 +8317,24 @@ void *alloc_mem_area(unsigned long memor
>       */
>      ftruncate(fd, memory);
>  
> -    area = mmap(0, memory, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);
> -    if (area == MAP_FAILED) {
> -	perror("mmap");
> -	close(fd);
> -	return NULL;
> -    }
> -
> -    return area;
> +    /* NB: MAP_POPULATE won't exhaustively alloc all phys pages in the case
> +     * MAP_PRIVATE is requested.  For mem_prealloc we mmap as MAP_SHARED
> +     * to sidestep this quirk.
> +     */
> +    flags = mem_prealloc ? MAP_POPULATE|MAP_SHARED : MAP_PRIVATE;
> +    area = mmap(0, memory, PROT_READ|PROT_WRITE, flags, fd, 0);
> +    if (area != MAP_FAILED)
> +	return (area);
> +    perror("alloc_hpage_mem: can't mmap hugetlbfs pages");
> +    close(fd);
> +    return (NULL);
>  }
>  
> -void *qemu_alloc_physram(unsigned long memory)
> +/* allocate guest memory as requested
> + */
> +void *qemu_alloc_physram(unsigned long size)
>  {
> -    void *area = NULL;
> -
> -    if (mem_path)
> -	area = alloc_mem_area(memory, mem_path);
> -    if (!area)
> -	area = qemu_vmalloc(memory);
> -
> -    return area;
> +    return (mem_path ? alloc_hpage_mem(size, mem_path) : qemu_vmalloc(size));
>  }
>   

I'd prefer if you didn't convert from a if/else to conditional.  hpage 
is a misnomer too as we aren't actually dependent on huge pages (this 
code should work equally well for tmpfs).

Regards,

Anthony Liguori

>  int main(int argc, char **argv)
> @@ -8962,6 +8969,9 @@ int main(int argc, char **argv)
>              case QEMU_OPTION_mempath:
>  		mem_path = optarg;
>  		break;
> +            case QEMU_OPTION_mem_prealloc:
> +		mem_prealloc = !mem_prealloc;
> +		break;
>              case QEMU_OPTION_name:
>                  qemu_name = optarg;
>                  break;
>