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

[Igor Mammedov]

On Fri, 29 Nov 2019 00:31:32 +0400
Marc-André Lureau <address@hidden> wrote:

> Hi
> 
> On Thu, Nov 28, 2019 at 9:25 PM Igor Mammedov <address@hidden> wrote:
> >
> > On Thu, 28 Nov 2019 18:15:18 +0400
> > Marc-André Lureau <address@hidden> 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).
on positive side it makes behavior consistent (and need to be documented
in one place (memdev)), i.e. user knows that he will get what he specified 
on CLI.

With global options like you propose, good luck with figuring out
(and documenting it in user documentation will be nightmare)
if your config actually does what it's supposed to do or not.
That's unfortunate the state we are in right now with
mem-path/prealloc.

> > 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.
Well, it adds more invariants to already complex RAM allocation
I have to untangle. So lets look into it after memdev re-factoring.
I'll plan to post it right after merge window is open.

> > 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.
Complexity is only one side of issue, I'm mainly refactoring it because
different approaches to allocate RAM lead to subtle bugs that's hard to
spot and fix. That's why I'm against adding more invariants to the pile.

> > 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.
(I still do not like idea of convenience options but it won't
get onto the way much if implemented as "global property" to memdev,
so I won't object if there is real demand for it)

Simplicity in global options is deceiving once you have mixed
RAM (main and as -device), good luck with documenting how it
works in all possible configs and making it robust (so it
won't explode later) (I wouldn't take on such task).
And then poor user will have to figure it out as well.

Verbose way makes allocating backing storage consistent,
which is much more important (including end user).
The rest could be scripted or one could use higher level
mgmt tools if simplicity is desired.


> thanks
> 
> >  
> > > Signed-off-by: Marc-André Lureau <address@hidden>
> > > ---
> > >  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;  
> >
> >  
> 
>