Re: [Qemu-devel] [PATCH v3 1/7] Add -mem-share option

[Edgar E. Iglesias]

On Fri, Dec 13, 2013 at 12:14:31PM +0100, Antonios Motakis wrote:
> This option complements -mem-path. It implies -mem-prealloc. If specified,
> the guest RAM is allocated as a shared memory object. If both -mem-path
> and -mem-share are provided, the memory is allocated from the HugeTLBFS
> supplied path, and then mmapped with MAP_SHARED.

Hi,

Interesting, I've got a similar use-case here where I've added a -mem-shared
option. I've got a few comments/questions.

Why do you imply -mem-prealloc? cant the user keep controlling that through
-mem-prealloc?

I'd prefer if -mem-share did not use shm_open but took a directory path as arg
and created the backing files there. I'd also prefer if the files had
deterministic names and where not unlinked after creation. I.e, let the user
delete them when no longer needed.

The reason for this is that it makes it easier to use apps that are not
aware of shm or QEMU specifics to manipulate the memory backing. I understand
that there might be issues (e.g filling up the disk, slow access over NFS etc)
but these are at the choice of the user.

Any thoughts around this?

Best regards,
Edgar



> 
> Signed-off-by: Antonios Motakis <address@hidden>
> Signed-off-by: Nikolay Nikolaev <address@hidden>
> ---
>  exec.c                 | 72 
> +++++++++++++++++++++++++++++++++++---------------
>  include/exec/cpu-all.h |  1 +
>  qemu-options.hx        |  9 +++++++
>  vl.c                   |  5 ++++
>  4 files changed, 66 insertions(+), 21 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index f4b9ef2..10720f1 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -883,6 +883,7 @@ void qemu_mutex_unlock_ramlist(void)
>  #include <sys/vfs.h>
>  
>  #define HUGETLBFS_MAGIC       0x958458f6
> +#define MIN_HUGE_PAGE_SIZE    (2*1024*1024)
>  
>  static long gethugepagesize(const char *path)
>  {
> @@ -920,15 +921,24 @@ static void *file_ram_alloc(RAMBlock *block,
>      char *c;
>      void *area;
>      int fd;
> +    int flags;
>      unsigned long hpagesize;
>  
> -    hpagesize = gethugepagesize(path);
> -    if (!hpagesize) {
> -        return NULL;
> -    }
> +    if (path) {
> +        hpagesize = gethugepagesize(path);
>  
> -    if (memory < hpagesize) {
> -        return NULL;
> +        if (!hpagesize) {
> +            return NULL ;
> +        }
> +
> +        if (memory < hpagesize) {
> +            return NULL ;
> +        }
> +    } else {
> +        if (memory < MIN_HUGE_PAGE_SIZE) {
> +            return NULL ;
> +        }
> +        hpagesize = MIN_HUGE_PAGE_SIZE;
>      }
>  
>      if (kvm_enabled() && !kvm_has_sync_mmu()) {
> @@ -943,20 +953,37 @@ static void *file_ram_alloc(RAMBlock *block,
>              *c = '_';
>      }
>  
> -    filename = g_strdup_printf("%s/qemu_back_mem.%s.XXXXXX", path,
> -                               sanitized_name);
> -    g_free(sanitized_name);
> +    if (path) {
> +
> +        filename = g_strdup_printf("%s/qemu_back_mem.%s.XXXXXX", path,
> +                sanitized_name);
> +        g_free(sanitized_name);
>  
> -    fd = mkstemp(filename);
> -    if (fd < 0) {
> -        perror("unable to create backing store for hugepages");
> +        fd = mkstemp(filename);
> +        if (fd < 0) {
> +            perror("unable to create backing store for huge");
> +            g_free(filename);
> +            return NULL ;
> +        }
> +        unlink(filename);
>          g_free(filename);
> +        memory = (memory + hpagesize - 1) & ~(hpagesize - 1);
> +    } else if (mem_share) {
> +        filename = g_strdup_printf("qemu_back_mem.%s.XXXXXX", 
> sanitized_name);
> +        g_free(sanitized_name);
> +        fd = shm_open(filename, O_CREAT | O_RDWR | O_EXCL,
> +                      S_IRWXU | S_IRWXG | S_IRWXO);
> +        if (fd < 0) {
> +            perror("unable to create backing store for shared memory");
> +            g_free(filename);
> +            return NULL ;
> +        }
> +        shm_unlink(filename);
> +        g_free(filename);
> +    } else {
> +        fprintf(stderr, "-mem-path or -mem-share required\n");
>          return NULL;
>      }
> -    unlink(filename);
> -    g_free(filename);
> -
> -    memory = (memory+hpagesize-1) & ~(hpagesize-1);
>  
>      /*
>       * ftruncate is not supported by hugetlbfs in older
> @@ -967,7 +994,8 @@ static void *file_ram_alloc(RAMBlock *block,
>      if (ftruncate(fd, memory))
>          perror("ftruncate");
>  
> -    area = mmap(0, memory, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
> +    flags = mem_share ? MAP_SHARED : MAP_PRIVATE;
> +    area = mmap(0, memory, PROT_READ | PROT_WRITE, flags, fd, 0);
>      if (area == MAP_FAILED) {
>          perror("file_ram_alloc: can't mmap RAM pages");
>          close(fd);
> @@ -1150,13 +1178,14 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, 
> void *host,
>          new_block->host = host;
>          new_block->flags |= RAM_PREALLOC_MASK;
>      } else if (xen_enabled()) {
> -        if (mem_path) {
> -            fprintf(stderr, "-mem-path not supported with Xen\n");
> +        if (mem_path || mem_share) {
> +            fprintf(stderr,
> +                    "-mem-path and -mem-share not supported with Xen\n");
>              exit(1);
>          }
>          xen_ram_alloc(new_block->offset, size, mr);
>      } else {
> -        if (mem_path) {
> +        if (mem_path || mem_share) {
>              if (phys_mem_alloc != qemu_anon_ram_alloc) {
>                  /*
>                   * file_ram_alloc() needs to allocate just like
> @@ -1164,7 +1193,8 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, 
> void *host,
>                   * a hook there.
>                   */
>                  fprintf(stderr,
> -                        "-mem-path not supported with this accelerator\n");
> +                        "-mem-path and -mem-share "
> +                        "not supported with this accelerator\n");
>                  exit(1);
>              }
>              new_block->host = file_ram_alloc(new_block, size, mem_path);
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index b6998f0..05ad0ee 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -469,6 +469,7 @@ extern RAMList ram_list;
>  
>  extern const char *mem_path;
>  extern int mem_prealloc;
> +extern int mem_share;
>  
>  /* Flags stored in the low bits of the TLB virtual address.  These are
>     defined so that fast path ram access is all zeros.  */
> diff --git a/qemu-options.hx b/qemu-options.hx
> index af34483..bd70777 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -237,6 +237,15 @@ STEXI
>  Preallocate memory when using -mem-path.
>  ETEXI
>  
> +DEF("mem-share", 0, QEMU_OPTION_mem_share,
> +    "-mem-share   share guest memory (implies -mem-prealloc)\n",
> +    QEMU_ARCH_ALL)
> +STEXI
> address@hidden -mem-share
> address@hidden -mem-share
> +Allocate guest RAM as a shared memory object.
> +ETEXI
> +
>  DEF("k", HAS_ARG, QEMU_OPTION_k,
>      "-k language     use keyboard layout (for example 'fr' for French)\n",
>      QEMU_ARCH_ALL)
> diff --git a/vl.c b/vl.c
> index b0399de..d2f7403 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -189,6 +189,7 @@ const char* keyboard_layout = NULL;
>  ram_addr_t ram_size;
>  const char *mem_path = NULL;
>  int mem_prealloc = 0; /* force preallocation of physical target memory */
> +int mem_share = 0; /* allocate shared memory */
>  int nb_nics;
>  NICInfo nd_table[MAX_NICS];
>  int autostart;
> @@ -3212,6 +3213,10 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_mem_prealloc:
>                  mem_prealloc = 1;
>                  break;
> +            case QEMU_OPTION_mem_share:
> +                mem_share = 1;
> +                mem_prealloc = 1;
> +                break;
>              case QEMU_OPTION_d:
>                  log_mask = optarg;
>                  break;
> -- 
> 1.8.3.2
> 
>