Re: [PATCH v3 6/7] util/oslib-posix: Support concurrent os_mem_prealloc() invocation

[Daniel P . Berrangé]

On Mon, Aug 16, 2021 at 11:47:38AM +0200, David Hildenbrand wrote:
> Add a mutex to protect the SIGBUS case, as we cannot mess concurrently
> with the sigbus handler and we have to manage the global variable
> sigbus_memset_context. The MADV_POPULATE_WRITE path can run
> concurrently.
> 
> Note that page_mutex and page_cond are shared between concurrent
> invocations, which shouldn't be a problem.
> 
> This is a preparation for future virtio-mem prealloc code, which will call
> os_mem_prealloc() asynchronously from an iothread when handling guest
> requests.
> 
> Add a comment that messing with the SIGBUS handler is frowned upon and
> can result in problems we fortunately haven't seen so far. Note that
> forwarding signals to the already installed SIGBUS handler isn't clean
> either, as that one might modify the SIGBUS handler again.

Even with the mutex, messing with SIGBUS post-startup still isn't safe
as we're clashing with SIGBUS usage in softmmu/cpus.c

IIUC, the virtio-mem prealloc code is something new that we've not
shipped yet. With this in mind, how about we simply enforce that
usage of this new feature is dependant on kernel support for
MADV_POPULATE_WRITE ?  If users want this feature they'll simply
need to update to a modern kernel.  This shouldn't break any existing
deployed QEMU guests IIUC

> 
> Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  util/oslib-posix.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index efa4f96d56..9829149e4b 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -95,6 +95,7 @@ typedef struct MemsetThread MemsetThread;
>  
>  /* used by sigbus_handler() */
>  static MemsetContext *sigbus_memset_context;
> +static QemuMutex sigbus_mutex;
>  
>  static QemuMutex page_mutex;
b>  static QemuCond page_cond;
> @@ -625,6 +626,7 @@ static bool madv_populate_write_possible(char *area, 
> size_t pagesize)
>  void os_mem_prealloc(int fd, char *area, size_t memory, int smp_cpus,
>                       Error **errp)
>  {
> +    static gsize initialized;
>      int ret;
>      struct sigaction act, oldact;
>      size_t hpagesize = qemu_fd_getpagesize(fd);
> @@ -638,6 +640,12 @@ void os_mem_prealloc(int fd, char *area, size_t memory, 
> int smp_cpus,
>      use_madv_populate_write = madv_populate_write_possible(area, hpagesize);
>  
>      if (!use_madv_populate_write) {
> +        if (g_once_init_enter(&initialized)) {
> +            qemu_mutex_init(&sigbus_mutex);
> +            g_once_init_leave(&initialized, 1);
> +        }
> +
> +        qemu_mutex_lock(&sigbus_mutex);
>          memset(&act, 0, sizeof(act));
>          act.sa_handler = &sigbus_handler;
>          act.sa_flags = 0;
> @@ -665,6 +673,7 @@ void os_mem_prealloc(int fd, char *area, size_t memory, 
> int smp_cpus,
>              perror("os_mem_prealloc: failed to reinstall signal handler");
>              exit(1);
>          }
> +        qemu_mutex_unlock(&sigbus_mutex);
>      }
>  }
>  
> -- 
> 2.31.1
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|