diff mbox

[v3,1/7] Add -mem-share option

Message ID 1386933277-20003-2-git-send-email-a.motakis@virtualopensystems.com
State New
Headers show

Commit Message

Antonios Motakis Dec. 13, 2013, 11:14 a.m. UTC 
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.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 exec.c                 | 72 +++++++++++++++++++++++++++++++++++---------------
 include/exec/cpu-all.h |  1 +
 qemu-options.hx        |  9 +++++++
 vl.c                   |  5 ++++
 4 files changed, 66 insertions(+), 21 deletions(-)

Comments

Eric Blake Dec. 14, 2013, 3:53 a.m. UTC | #1 
On 12/13/2013 04:14 AM, 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.
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> ---

> +++ b/qemu-options.hx
> @@ -237,6 +237,15 @@ STEXI
>  Preallocate memory when using -mem-path.
>  ETEXI
>  
> +DEF("mem-share", 0, QEMU_OPTION_mem_share,

Ouch.  Doesn't this mean you are defining a boolean option (absent or
present) as opposed to a qemuOpts option?  I've already been complaining
that other boolean opts are currently undiscoverable to QMP; they also
have the drawback of having no way to turn the option back off if an
alias turned it on earlier in the command line.  Can we use qemuOpts
here (so query-command-line-options can see it), and with a boolean
on/off argument (so it's not a one-way switch)?
Paolo Bonzini Dec. 14, 2013, 11:09 a.m. UTC | #2 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 14/12/2013 04:53, Eric Blake ha scritto:
>>> +DEF("mem-share", 0, QEMU_OPTION_mem_share,
> Ouch.  Doesn't this mean you are defining a boolean option (absent
> or present) as opposed to a qemuOpts option?  I've already been
> complaining that other boolean opts are currently undiscoverable to
> QMP; they also have the drawback of having no way to turn the
> option back off if an alias turned it on earlier in the command
> line.  Can we use qemuOpts here (so query-command-line-options can
> see it), and with a boolean on/off argument (so it's not a one-way
> switch)?

This can be replaced with a memdev property as soon as memdev gets in.

We'll need to provide a way to introspect the properties of a QOM
object, though.

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSrDx+AAoJEBvWZb6bTYbyf1oP/imUGPTzhoAOVb8MfR+2UnJ5
JQBQa6000nOLH54NnIPtTepJEQ+01sRCqJHykUEQD2q4WNL/ToBfu3FEwJbu7Wyx
PHlt3BNG33LY+tM5pANgu/+Pliqe2dzPScTbfnkAawOUDSGFxkQWhJ/dFMHMfmiw
HtrfmNF72liz9OJ8VEvt7JmQFW5lGeFpd3XzibDyC3VtjN7tX3KVe8lZ4oUjjNvD
z2Q2pFs7fIHvKTTgMn1QQeKj42yXlzq+fNQPwuSlQQmlqqOjyDET9fCe2fYeqlGX
b94iTzpqGNUwCRw2OluLftdNnScQKy2LEkokd5LAIqsyJEAVRZnAikLF2JyfdhTy
lIGg9VX0VnqIVeauVOcpoBH1HqlDDMcen2DV5Sr5mTHrJ9m2EMm/aqVMYt/kPjP1
5IeYdJ44RxxAXr6aPnpK2/JjaJP+LfjB+xXUYqukjFOibDDXMr4wprKOgkDd5K/9
SkC8KSBsxK2pwUdztr4W1pSxnsYHwlsBKZM0i8MTZv5GsAe4oG3Authp5Uy15JmR
xjmm6YfbRGRH/Ov+InlQ8BbzzCVRQkoaGb8B8GosKrDe+7Fnh+GUV2pjxvgySYk1
kUyo9RpeT9KebCpvg4RvasLX5A1yiDdh07KAp+Lyw1JXrw5zIeOPzf02LQJBtfmM
5MS3PXn4GbvmsgW/e1bA
=Ed70
-----END PGP SIGNATURE-----
Edgar E. Iglesias Dec. 16, 2013, 7:32 a.m. UTC | #3 
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 <a.motakis@virtualopensystems.com>
> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> ---
>  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
> +@item -mem-share
> +@findex -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
> 
>
Paolo Bonzini Dec. 16, 2013, 10:17 a.m. UTC | #4 
Il 16/12/2013 08:32, Edgar E. Iglesias ha scritto:
> 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?

I agree entirely with you.

Paolo
Antonios Motakis Dec. 16, 2013, 3:20 p.m. UTC | #5 
On Sat, Dec 14, 2013 at 4:53 AM, Eric Blake <eblake@redhat.com> wrote:

> On 12/13/2013 04:14 AM, 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.
> >
> > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> > Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> > ---
>
> > +++ b/qemu-options.hx
> > @@ -237,6 +237,15 @@ STEXI
> >  Preallocate memory when using -mem-path.
> >  ETEXI
> >
> > +DEF("mem-share", 0, QEMU_OPTION_mem_share,
>
> Ouch.  Doesn't this mean you are defining a boolean option (absent or
> present) as opposed to a qemuOpts option?  I've already been complaining
> that other boolean opts are currently undiscoverable to QMP; they also
> have the drawback of having no way to turn the option back off if an
> alias turned it on earlier in the command line.  Can we use qemuOpts
> here (so query-command-line-options can see it), and with a boolean
> on/off argument (so it's not a one-way switch)?
>

Our implementation of mem-share complements mem-path and mem-prealloc.
Maybe these options should be combined as one QemuOpts with arguments?
Is this the memdev that is referred to by Paolo?


> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>
Antonios Motakis Dec. 16, 2013, 3:21 p.m. UTC | #6 
On Mon, Dec 16, 2013 at 8:32 AM, Edgar E. Iglesias <edgar.iglesias@gmail.com
> wrote:

> 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.
>
>
Currently -mem-path implies HugeTLBFS for the supplied path. Maybe we
should change
its behavior to do what you suggest:

-mem-path - specify a directory where to allocate the mmap-ed ram files
(and don't unlink them)
-mem-hugetlbfs - check mem-path directory for HugeTLBFS (do we need this
one?)
-mem-share - add MAP_SHARED to mmap
-mem-prealloc - preallocate the memory

How does this sound?



> Any thoughts around this?
>
> Best regards,
> Edgar
>
>
>
> >
> > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> > Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> > ---
> >  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
> > +@item -mem-share
> > +@findex -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
> >
> >
>
Igor Mammedov Dec. 16, 2013, 3:47 p.m. UTC | #7 
On Mon, 16 Dec 2013 16:20:04 +0100
Antonios Motakis <a.motakis@virtualopensystems.com> wrote:

> On Sat, Dec 14, 2013 at 4:53 AM, Eric Blake <eblake@redhat.com> wrote:
> 
> > On 12/13/2013 04:14 AM, 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.
> > >
> > > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> > > Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> > > ---
> >
> > > +++ b/qemu-options.hx
> > > @@ -237,6 +237,15 @@ STEXI
> > >  Preallocate memory when using -mem-path.
> > >  ETEXI
> > >
> > > +DEF("mem-share", 0, QEMU_OPTION_mem_share,
> >
> > Ouch.  Doesn't this mean you are defining a boolean option (absent or
> > present) as opposed to a qemuOpts option?  I've already been complaining
> > that other boolean opts are currently undiscoverable to QMP; they also
> > have the drawback of having no way to turn the option back off if an
> > alias turned it on earlier in the command line.  Can we use qemuOpts
> > here (so query-command-line-options can see it), and with a boolean
> > on/off argument (so it's not a one-way switch)?
> >
> 
> Our implementation of mem-share complements mem-path and mem-prealloc.
> Maybe these options should be combined as one QemuOpts with arguments?
> Is this the memdev that is referred to by Paolo?
memdev is introduced here: http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg02532.html

as for mem-path & mem-prealloc, I was thinking about adding HugePageMem backend
to handle hugepage specifics. mem-share could be a part of ShareMem backend
or something like this.

> 
> 
> > --
> > Eric Blake   eblake redhat com    +1-919-301-3266
> > Libvirt virtualization library http://libvirt.org
> >
> >
Edgar E. Iglesias Dec. 17, 2013, 1:55 a.m. UTC | #8 
On Mon, Dec 16, 2013 at 04:21:28PM +0100, Antonios Motakis wrote:
>    On Mon, Dec 16, 2013 at 8:32 AM, Edgar E. Iglesias
>    <edgar.iglesias@gmail.com> wrote:
> 
>      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.
> 
>    Currently -mem-path implies HugeTLBFS for the supplied path. Maybe we
>    should change
>    its behavior to do what you suggest:
> 
>    -mem-path - specify a directory where to allocate the mmap-ed ram files
>    (and don't unlink them)
>    -mem-hugetlbfs - check mem-path directory for HugeTLBFS (do we need this
>    one?)
>    -mem-share - add MAP_SHARED to mmap
>    -mem-prealloc - preallocate the memory
>    How does this sound?


It sounds good to me, but I'm not sure its a good thing to change the
behaviour of -mem-path and break backwards compatibility. Maybe we
could relax the hugetlbfs impl for -mem-path in that it uses it the
huge page sizes if the underlying fs is hugetlbfs and otherwise not?

In my local implementation mem-share takes an argument by itself and replaces
the need for -mem-path, not very intuitive I agree but its an alternative.

Cheers,
Edgar
Paolo Bonzini Dec. 17, 2013, 11:11 a.m. UTC | #9 
Il 16/12/2013 16:47, Igor Mammedov ha scritto:
> memdev is introduced here: http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg02532.html
> 
> as for mem-path & mem-prealloc, I was thinking about adding HugePageMem backend
> to handle hugepage specifics. mem-share could be a part of ShareMem backend
> or something like this.

We need three backends, something like the following:

- anonymous mmap (-object memory-ram,size=128M)

- file mmap (-object memory-file,shared=on/off,size=128M,file=FILENAME),
creates file if size property provided.

- mkstemp + file mmap (-object memory-hugepage,size=128M,path=PATH)

I don't think shared=on/off is useful in the third case, but it would be
trivial to add.

Paolo
diff mbox

Patch

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
+@item -mem-share
+@findex -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;