Re: [PATCH v1 3/3] util/oslib-posix: Support concurrent os_mem_prealloc() invocation

[David Hildenbrand <david@redhat.com>]

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.

Fortunately, running into that race seems to be rare, at least I never 
hear reports.

-- 
Thanks,

David / dhildenb