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

[Daniel P . Berrangé]

On Tue, Sep 28, 2021 at 05:56:41PM +0100, Daniel P. Berrangé wrote:
> 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

Oh, I should have read ahead to the next patch where you address
this issue.

With that in mind I'm ok adding

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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

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