Re: [PATCH 2/2] Add -mem-shared option

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

Hi

On Thu, Nov 28, 2019 at 9:25 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Thu, 28 Nov 2019 18:15:18 +0400
> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>
> > Add an option to simplify shared memory / vhost-user setup.
> >
> > Currently, using vhost-user requires NUMA setup such as:
> > -m 4G -object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on -numa node,memdev=mem
> >
> > As there is no other way to allocate shareable RAM, afaik.
> >
> > -mem-shared aims to have a simple way instead: -m 4G -mem-shared
> User always can write a wrapper script if verbose CLI is too much,
> and we won't have to deal with myriad permutations to maintain.

Sure, but that's not exactly making it easier for the user,
documentation etc (or machine that do not support numa as David
mentionned).

>
> Also current -mem-path/prealloc in combination with memdevs is
> the source of problems (as ram allocation uses 2 different paths).
> It's possible to fix with a kludge but I'd rather fix it properly.

I agree, however I think it's a separate problems. We don't have to
fix both simultaneously. The semantic of a new CLI -mem-shared (or
shared=on etc) can be defined and implemented in a simple way, before
internal refactoring.

> So during 5.0, I'm planning to consolidate -mem-path/prealloc
> handling around memory backend internally (and possibly deprecate them),
> so the only way to allocate RAM for guest would be via memdevs.
> (reducing number of options an globals that they use)
>

That would be great indeed. I tried to look at that in the past, but
was a it overwhelmed by the amount of details and/or code complexity.

> So user who wants something non trivial could override default
> non-numa behavior with
>   -object memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on \
>   -machine memdev=mem
> or use any other backend that suits theirs needs.

That's nice, but not as friendly as a simple -mem-shared.

thanks

>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  exec.c                  | 11 ++++++++++-
> >  hw/core/numa.c          | 16 +++++++++++++++-
> >  include/sysemu/sysemu.h |  1 +
> >  qemu-options.hx         | 10 ++++++++++
> >  vl.c                    |  4 ++++
> >  5 files changed, 40 insertions(+), 2 deletions(-)
> >
> > diff --git a/exec.c b/exec.c
> > index ffdb518535..4e53937eaf 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -72,6 +72,10 @@
> >  #include "qemu/mmap-alloc.h"
> >  #endif
> >
> > +#ifdef CONFIG_POSIX
> > +#include "qemu/memfd.h"
> > +#endif
> > +
> >  #include "monitor/monitor.h"
> >
> >  //#define DEBUG_SUBPAGE
> > @@ -2347,7 +2351,12 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
> >      bool created;
> >      RAMBlock *block;
> >
> > -    fd = file_ram_open(mem_path, memory_region_name(mr), &created, errp);
> > +    if (mem_path) {
> > +        fd = file_ram_open(mem_path, memory_region_name(mr), &created, errp);
> > +    } else {
> > +        fd = qemu_memfd_open(mr->name, size,
> > +                             F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL, errp);
> > +    }
>
> that's what I'm mostly against, as it spills out memdev impl. details
> into generic code.
>
> >      if (fd < 0) {
> >          return NULL;
> >      }
> > diff --git a/hw/core/numa.c b/hw/core/numa.c
> > index e3332a984f..6f72cddb1c 100644
> > --- a/hw/core/numa.c
> > +++ b/hw/core/numa.c
> > @@ -493,7 +493,8 @@ static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
> >      if (mem_path) {
> >  #ifdef __linux__
> >          Error *err = NULL;
> > -        memory_region_init_ram_from_file(mr, owner, name, ram_size, 0, 0,
> > +        memory_region_init_ram_from_file(mr, owner, name, ram_size, 0,
> > +                                         mem_shared ? RAM_SHARED : 0,
> >                                           mem_path, &err);
> this will be gone and replaced by memory region that memdev initializes.
>
> >          if (err) {
> >              error_report_err(err);
> > @@ -513,6 +514,19 @@ static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
> >  #else
> >          fprintf(stderr, "-mem-path not supported on this host\n");
> >          exit(1);
> > +#endif
> > +    } else if (mem_shared) {
> > +#ifdef CONFIG_POSIX
> > +        Error *err = NULL;
> > +        memory_region_init_ram_from_file(mr, owner, NULL, ram_size, 0,
> > +                                         RAM_SHARED, NULL, &err);
> > +        if (err) {
> > +            error_report_err(err);
> > +            exit(1);
> > +        }
> > +#else
> > +        fprintf(stderr, "-mem-shared not supported on this host\n");
> > +        exit(1);
> >  #endif
> >      } else {
> >          memory_region_init_ram_nomigrate(mr, owner, name, ram_size, &error_fatal);
> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > index 80c57fdc4e..80db8465a9 100644
> > --- a/include/sysemu/sysemu.h
> > +++ b/include/sysemu/sysemu.h
> > @@ -55,6 +55,7 @@ extern bool enable_cpu_pm;
> >  extern QEMUClockType rtc_clock;
> >  extern const char *mem_path;
> >  extern int mem_prealloc;
> > +extern int mem_shared;
> >
> >  #define MAX_OPTION_ROMS 16
> >  typedef struct QEMUOptionRom {
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 65c9473b73..4c69b03ad3 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -394,6 +394,16 @@ STEXI
> >  Preallocate memory when using -mem-path.
> >  ETEXI
> >
> > +DEF("mem-shared", 0, QEMU_OPTION_mem_shared,
> > +    "-mem-shared     allocate shared memory\n", QEMU_ARCH_ALL)
> > +STEXI
> > +@item -mem-shared
> > +@findex -mem-shared
> > +Allocate guest RAM with shared mapping.  Whether the allocation is
> > +anonymous or not (with -mem-path), QEMU will allocate a shared memory that
> > +can be shared by unrelated processes, such as vhost-user backends.
> > +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 6a65a64bfd..53b1155455 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -143,6 +143,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_shared = 0;
> Also what happened to no more globals policy?
>
> >  bool enable_mlock = false;
> >  bool enable_cpu_pm = false;
> >  int nb_nics;
> > @@ -3172,6 +3173,9 @@ int main(int argc, char **argv, char **envp)
> >              case QEMU_OPTION_mem_prealloc:
> >                  mem_prealloc = 1;
> >                  break;
> > +            case QEMU_OPTION_mem_shared:
> > +                mem_shared = 1;
> > +                break;
> >              case QEMU_OPTION_d:
> >                  log_mask = optarg;
> >                  break;
>
>


-- 
Marc-André Lureau