Re: [Qemu-devel] [PATCH for-2.7] fix qemu exit on memory hotplug when allocation fails at prealloc time

[Markus Armbruster]

Igor Mammedov <address@hidden> writes:

> When adding hostmem backend at runtime, QEMU might exit with error:
>   "os_mem_prealloc: Insufficient free host memory pages available to allocate 
> guest RAM"
>
> It happens due to os_mem_prealloc() not handling errors gracefully.
>
> Fix it by passing errp argument so that os_mem_prealloc() could
> report error to callers and undo performed allocation when
> os_mem_prealloc() fails.
>
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
>  include/qemu/osdep.h |  2 +-
>  backends/hostmem.c   | 18 ++++++++++++++----
>  exec.c               | 10 ++++++++--
>  util/oslib-posix.c   | 25 ++++++++++++-------------
>  util/oslib-win32.c   |  2 +-
>  5 files changed, 36 insertions(+), 21 deletions(-)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index fbb8759..d7c111d 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -379,7 +379,7 @@ unsigned long qemu_getauxval(unsigned long type);
>  
>  void qemu_set_tty_echo(int fd, bool echo);
>  
> -void os_mem_prealloc(int fd, char *area, size_t sz);
> +void os_mem_prealloc(int fd, char *area, size_t sz, Error **errp);
>  
>  int qemu_read_password(char *buf, int buf_size);
>  
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index ac80257..b7a208d 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -203,6 +203,7 @@ static bool host_memory_backend_get_prealloc(Object *obj, 
> Error **errp)
>  static void host_memory_backend_set_prealloc(Object *obj, bool value,
>                                               Error **errp)
>  {
> +    Error *local_err = NULL;
>      HostMemoryBackend *backend = MEMORY_BACKEND(obj);
>  
>      if (backend->force_prealloc) {
> @@ -223,7 +224,11 @@ static void host_memory_backend_set_prealloc(Object 
> *obj, bool value,
>          void *ptr = memory_region_get_ram_ptr(&backend->mr);
>          uint64_t sz = memory_region_size(&backend->mr);
>  
> -        os_mem_prealloc(fd, ptr, sz);
> +        os_mem_prealloc(fd, ptr, sz, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
>          backend->prealloc = true;
>      }
>  }
> @@ -286,8 +291,7 @@ host_memory_backend_memory_complete(UserCreatable *uc, 
> Error **errp)
>      if (bc->alloc) {
>          bc->alloc(backend, &local_err);
>          if (local_err) {
> -            error_propagate(errp, local_err);
> -            return;
> +            goto out;

I'd leave the tail merging optimization to the compiler, i.e. don't
touch the code here, and ...

>          }
>  
>          ptr = memory_region_get_ram_ptr(&backend->mr);
> @@ -343,9 +347,15 @@ host_memory_backend_memory_complete(UserCreatable *uc, 
> Error **errp)
>           * specified NUMA policy in place.
>           */
>          if (backend->prealloc) {
> -            os_mem_prealloc(memory_region_get_fd(&backend->mr), ptr, sz);
> +            os_mem_prealloc(memory_region_get_fd(&backend->mr), ptr, sz,
> +                            &local_err);
> +            if (local_err) {
> +                goto out;

... have the obvious error_propagate() + return here.  Matter of taste,
between you and the maintainer.  Except there is none.  Inexcusable for
a file created in 2014.  Suggest you appoint yourself.

> +            }
>          }
>      }
> +out:
> +    error_propagate(errp, local_err);
>  }
>  
>  static bool
> diff --git a/exec.c b/exec.c
> index 60cf46a..bf1446c 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1268,7 +1268,7 @@ static void *file_ram_alloc(RAMBlock *block,
>      char *filename;
>      char *sanitized_name;
>      char *c;
> -    void *area;
> +    void *area = MAP_FAILED;
>      int fd = -1;
>      int64_t page_size;
>  
> @@ -1356,13 +1356,19 @@ static void *file_ram_alloc(RAMBlock *block,
>      }
>  
>      if (mem_prealloc) {
> -        os_mem_prealloc(fd, area, memory);
> +        os_mem_prealloc(fd, area, memory, errp);
> +        if (errp && *errp) {
> +            goto error;
> +        }
>      }
>  
>      block->fd = fd;
>      return area;
>  
>  error:
> +    if (area != MAP_FAILED) {
> +        qemu_ram_munmap(area, memory);
> +    }
>      if (unlink_on_error) {
>          unlink(path);
>      }
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 6d70d9a..a60ac97 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -318,7 +318,7 @@ static void sigbus_handler(int signal)
>      siglongjmp(sigjump, 1);
>  }
>  
> -void os_mem_prealloc(int fd, char *area, size_t memory)
> +void os_mem_prealloc(int fd, char *area, size_t memory, Error **errp)
>  {
>      int ret;
>      struct sigaction act, oldact;
> @@ -330,8 +330,9 @@ void os_mem_prealloc(int fd, char *area, size_t memory)
>  
>      ret = sigaction(SIGBUS, &act, &oldact);
>      if (ret) {
> -        perror("os_mem_prealloc: failed to install signal handler");
> -        exit(1);
> +        error_setg_errno(errp, errno,
> +            "os_mem_prealloc: failed to install signal handler");
> +        return;
>      }
>  
>      /* unblock SIGBUS */
> @@ -340,9 +341,8 @@ void os_mem_prealloc(int fd, char *area, size_t memory)
>      pthread_sigmask(SIG_UNBLOCK, &set, &oldset);
>  
>      if (sigsetjmp(sigjump, 1)) {
> -        fprintf(stderr, "os_mem_prealloc: Insufficient free host memory "
> -                        "pages available to allocate guest RAM\n");
> -        exit(1);
> +        error_setg(errp, "os_mem_prealloc: Insufficient free host memory "
> +            "pages available to allocate guest RAM\n");
>      } else {
>          int i;
>          size_t hpagesize = qemu_fd_getpagesize(fd);
> @@ -352,15 +352,14 @@ void os_mem_prealloc(int fd, char *area, size_t memory)
>          for (i = 0; i < numpages; i++) {
>              memset(area + (hpagesize * i), 0, 1);
>          }
> +    }
>  
> -        ret = sigaction(SIGBUS, &oldact, NULL);
> -        if (ret) {
> -            perror("os_mem_prealloc: failed to reinstall signal handler");
> -            exit(1);
> -        }
> -
> -        pthread_sigmask(SIG_SETMASK, &oldset, NULL);
> +    ret = sigaction(SIGBUS, &oldact, NULL);
> +    if (ret) {
> +        perror("os_mem_prealloc: failed to reinstall signal handler");
> +        exit(1);

I guess you're keeping the exit() here, because you can't recover
cleanly from this error.  I should never happen anyway.  Worth a
comment, I think.

>      }
> +    pthread_sigmask(SIG_SETMASK, &oldset, NULL);
>  }
>  
>  
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 6debc2b..4c1dcf1 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -539,7 +539,7 @@ int getpagesize(void)
>      return system_info.dwPageSize;
>  }
>  
> -void os_mem_prealloc(int fd, char *area, size_t memory)
> +void os_mem_prealloc(int fd, char *area, size_t memory, Error **errp)
>  {
>      int i;
>      size_t pagesize = getpagesize();

Reviewed-by: Markus Armbruster <address@hidden>