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

[Igor Mammedov]

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.

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.
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)

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.


> 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;