On Tue, Jul 20, 2021 at 04:27:32PM +0200, David Hildenbrand wrote: > On 20.07.21 16:22, Daniel P. Berrangé wrote: > > On Wed, Jul 14, 2021 at 01:23:06PM +0200, David Hildenbrand wrote: > > > Add a mutext to protect the SIGBUS case, as we cannot mess concurrently > > > > typo s/mutext/mutex/ > > > > > 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. > > > > Hmm, I'm wondering how the need to temporarily play with SIGBUS > > at runtime for mem preallocation will interact with the SIGBUS > > handler installed by softmmu/cpus.c. > > That's exactly why I came up with MADV_POPULATE_WRITE, to avoid having to > mess with different kinds of sigbus at the same time. You can only get it > wrong. > > > > > The SIGBUS handler the preallocation code is installed just > > blindly assumes the SIGBUS is related to the preallocation > > work being done. This is a fine assumption during initially > > startup where we're single threaded and not running guest > > CPUs. I'm less clear on whether that's a valid assumption > > at runtime once guest CPUs are running. > > I assume it's quite broken, for example, already when hotplugging a DIMM and > prallocating memory for the memory backend. > > > > > If the sigbus_handler method in softmmu/cpus.c is doing > > something important for QEMU, then why is it ok for us to > > periodically disable that handler and replace it with > > something else that takes a completely different action ? > > I don't think it is ok. I assume it has been broken for a long time, just > nobody ever ran into that race. > > > > > Of course with the madvise impl we're bypassing the SIGBUS > > dance entirely. This is good for people with new kernels, > > but is this SIGBUS stuff safe for older kernels ? > > It remains broken with old kernels I guess. There isn't too much that we can > do: disabling prealloc=on once the VM is running breaks existing use cases.
Ok, while refactoring this, could you add a scary warning next to the sigaction calls mentioning that this code is not likely to play well with qemu's other handling of sigbus, as a reminder to future reviewers. > Fortunately, running into that race seems to be rare, at least I never hear > reports. The failure mode is likely to be silent or easily mis-interpreted Is there any value in emitting a one-time per process warning message on stderr if we take the old codepath post-startup ? 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 :|
