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é <[email protected]> > > > > > Reviewed-by: Pankaj Gupta <[email protected]> > > Signed-off-by: David Hildenbrand <[email protected]> > > --- > > 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 :|
