[not for merging, but for discussion; this is something I found when
looking at another issue on Chuang's optimization for migration downtime]
Summary: we tried to access memory_listeners, address_spaces, etc. in RCU
way. However we didn't implement them with RCU-safety. This patchset is
trying to do that; at least making it closer.
NOTE! It's doing it wrongly for now, so please feel free to see this as a
thread to start discussing this problem, as in subject.
The core problem here is how to make sure memory listeners will be freed in
RCU ways, per when unlinking them from the global memory_listeners list.
The current patchset (in patch 1) did it with drain_call_rcu(), but of
course it's wrong, because of at least two things:
(1) drain_call_rcu() will release BQL; currently there's no way to me to
guarantee that releasing BQL is safe here.
(2) memory_listener_unregister() can be called within a RCU read lock
itself (we're so happy to take rcu read lock in many places but we
don't think much on how long it'll be taken; at least not as strict
as the kernel variance, so we're just less care about that fact yet).
It means, drain_call_rcu() should deadlock there waiting for itself.
For an example, see Appendix A.
Side question to Stefan / Maxim: why do we need drain_call_rcu() and what's
its difference from synchronize_rcu() in API level besides releasing and
retaking BQL when taken?
I have a few solutions in mind, none of them look pretty, though. Before
going further, I think I should raise this to the list and discuss. The
good thing is I don't think the fix is urgently needed, because hitting the
bug should be very rare in real life (I do think we have yet another bug to
sometimes not taking RCU lock for memory_region_clear_dirty_bitmap too; see
patch 3, which I think is only used during virtio-mem / virtio-balloon
migrating). However I think it should still be important to discuss this
or it'll be even harder to maintain in the future with more things piled
up.
Solution A
==========
We can logically release everything that contains a MemoryListener with
g_free_rcu() rather than g_free(). It can be actually quite
straightforward for some cases (e.g. VFIOContainer, where we can simply do
s/g_free/g_free_rcu/ with the two call sites), but not for some others
(vhost_dev, where there can be tons of other structures wrapping vhost_dev
and it's sometimes not straightforward when the object got freed).
Solution B
==========
Can we omit taking RCU read lock if we already hold BQL? Logically it's
safe for most of the cases we're using RCU with BQL modifying the protected
strucuture, but it may have issue when we extend RCU to outside-BQL usages,
IOW, we may need to have a BQL-flavoured rcu_read_lock_bql() that we use to
reference things that will be protected by BQL, then replace most of the
RCU read locks in QEMU to use that, then I think maybe we can safely not
taking RCU read lock when BQL is taken, but I am really not that sure yet.
Thoughts? Is there any better way to do so? Or am I perhaps over worried?
Thanks,
Appendix A
==========
Sample of calling memory_listener_unregister() with both BQL & RCU held
(RCU held in address_space_write).
3 0x0000562b8da6bd60 in memory_listener_unregister (listener=0x562b8feeae48)
at ../softmmu/memory.c:3065
4 0x0000562b8da1ad47 in virtio_device_unrealize (dev=0x562b8feead60) at
../hw/virtio/virtio.c:3631
5 0x0000562b8db1e2f1 in device_set_realized (obj=0x562b8feead60,
value=false, errp=0x562b8e617138 <error_abort>) at ../hw/core/qdev.c:599
6 0x0000562b8db27e45 in property_set_bool (obj=0x562b8feead60,
v=0x562b90715670, name=0x562b8dec2b19 "realized", opaque=0x562b8fc46070,
errp=0x562b8e617138 <error_abort>)
at ../qom/object.c:2285
7 0x0000562b8db25e16 in object_property_set (obj=0x562b8feead60,
name=0x562b8dec2b19 "realized", v=0x562b90715670, errp=0x562b8e617138
<error_abort>) at ../qom/object.c:1420
8 0x0000562b8db2a2ad in object_property_set_qobject (obj=0x562b8feead60,
name=0x562b8dec2b19 "realized", value=0x562b8ff09800, errp=0x562b8e617138
<error_abort>)
at ../qom/qom-qobject.c:28
9 0x0000562b8db26181 in object_property_set_bool (obj=0x562b8feead60,
name=0x562b8dec2b19 "realized", value=false, errp=0x562b8e617138 <error_abort>)
at ../qom/object.c:1489
10 0x0000562b8db1d82a in qdev_unrealize (dev=0x562b8feead60) at
../hw/core/qdev.c:306
11 0x0000562b8db1a500 in bus_set_realized (obj=0x562b8feeace0, value=false,
errp=0x562b8e617138 <error_abort>) at ../hw/core/bus.c:205
12 0x0000562b8db27e45 in property_set_bool (obj=0x562b8feeace0,
v=0x562b90e71800, name=0x562b8dec21f8 "realized", opaque=0x562b908fc660,
errp=0x562b8e617138 <error_abort>)
at ../qom/object.c:2285
13 0x0000562b8db25e16 in object_property_set (obj=0x562b8feeace0,
name=0x562b8dec21f8 "realized", v=0x562b90e71800, errp=0x562b8e617138
<error_abort>) at ../qom/object.c:1420
14 0x0000562b8db2a2ad in object_property_set_qobject (obj=0x562b8feeace0,
name=0x562b8dec21f8 "realized", value=0x562b8ff097e0, errp=0x562b8e617138
<error_abort>)
at ../qom/qom-qobject.c:28
15 0x0000562b8db26181 in object_property_set_bool (obj=0x562b8feeace0,
name=0x562b8dec21f8 "realized", value=false, errp=0x562b8e617138 <error_abort>)
at ../qom/object.c:1489
16 0x0000562b8db1a3eb in qbus_unrealize (bus=0x562b8feeace0) at
../hw/core/bus.c:179
17 0x0000562b8db1e25f in device_set_realized (obj=0x562b8fee29a0,
value=false, errp=0x562b8e617138 <error_abort>) at ../hw/core/qdev.c:593
18 0x0000562b8db27e45 in property_set_bool (obj=0x562b8fee29a0,
v=0x562b90e6eac0, name=0x562b8dec2b19 "realized", opaque=0x562b8fc46070,
errp=0x562b8e617138 <error_abort>)
at ../qom/object.c:2285
19 0x0000562b8db25e16 in object_property_set (obj=0x562b8fee29a0,
name=0x562b8dec2b19 "realized", v=0x562b90e6eac0, errp=0x562b8e617138
<error_abort>) at ../qom/object.c:1420
20 0x0000562b8db2a2ad in object_property_set_qobject (obj=0x562b8fee29a0,
name=0x562b8dec2b19 "realized", value=0x562b908fc800, errp=0x562b8e617138
<error_abort>)
at ../qom/qom-qobject.c:28
21 0x0000562b8db26181 in object_property_set_bool (obj=0x562b8fee29a0,
name=0x562b8dec2b19 "realized", value=false, errp=0x562b8e617138 <error_abort>)
at ../qom/object.c:1489
22 0x0000562b8db1d82a in qdev_unrealize (dev=0x562b8fee29a0) at
../hw/core/qdev.c:306
23 0x0000562b8d603a4a in acpi_pcihp_device_unplug_cb
(hotplug_dev=0x562b90add210, s=0x562b90ade510, dev=0x562b8fee29a0,
errp=0x562b8e617138 <error_abort>) at ../hw/acpi/pcihp.c:415
24 0x0000562b8d601397 in piix4_device_unplug_cb (hotplug_dev=0x562b90add210,
dev=0x562b8fee29a0, errp=0x562b8e617138 <error_abort>) at ../hw/acpi/piix4.c:394
25 0x0000562b8db2265d in hotplug_handler_unplug (plug_handler=0x562b90add210,
plugged_dev=0x562b8fee29a0, errp=0x562b8e617138 <error_abort>) at
../hw/core/hotplug.c:56
26 0x0000562b8d603386 in acpi_pcihp_eject_slot (s=0x562b90ade510, bsel=0,
slots=64) at ../hw/acpi/pcihp.c:251
27 0x0000562b8d603e5f in pci_write (opaque=0x562b90ade510, addr=8, data=64,
size=4) at ../hw/acpi/pcihp.c:528
28 0x0000562b8da641cd in memory_region_write_accessor (mr=0x562b90adf120,
addr=8, value=0x7fff5484aae8, size=4, shift=0, mask=4294967295, attrs=...) at
../softmmu/memory.c:493
29 0x0000562b8da6441a in access_with_adjusted_size
(addr=8, value=0x7fff5484aae8, size=4, access_size_min=1,
access_size_max=4, access_fn=0x562b8da640d7 <memory_region_write_accessor>,
mr=0x562b90adf120, attrs=...) at ../softmmu/memory5
30 0x0000562b8da676ba in memory_region_dispatch_write (mr=0x562b90adf120,
addr=8, data=64, op=MO_32, attrs=...) at ../softmmu/memory.c:1515
31 0x0000562b8da752a4 in flatview_write_continue (fv=0x562b8fefc190,
addr=44552, attrs=..., ptr=0x7fff5484ac64, len=4, addr1=8, l=4,
mr=0x562b90adf120) at ../softmmu/physmem.c:2826
32 0x0000562b8da75407 in flatview_write (fv=0x562b8fefc190, addr=44552,
attrs=..., buf=0x7fff5484ac64, len=4) at ../softmmu/physmem.c:2868
-Type <RET> for more, q to quit, c to continue without paging--
33 0x0000562b8da757b7 in address_space_write (as=0x562b8e5f9f60
<address_space_io>, addr=44552, attrs=..., buf=0x7fff5484ac64, len=4) at
../softmmu/physmem.c:2964
34 0x0000562b8da60ad9 in cpu_outl (addr=44552, val=64) at
../softmmu/ioport.c:80
35 0x0000562b8da7a952 in qtest_process_command (chr=0x562b8fc49070,
words=0x562b90e57cb0) at ../softmmu/qtest.c:482
36 0x0000562b8da7c565 in qtest_process_inbuf (chr=0x562b8fc49070,
inbuf=0x562b8fe11da0) at ../softmmu/qtest.c:802
37 0x0000562b8da7c5f6 in qtest_read (opaque=0x562b8fc49070,
buf=0x7fff5484afc0 "outl 0xae08 0x40\n306c\n", size=17) at
../softmmu/qtest.c:814
38 0x0000562b8dc3bc25 in qemu_chr_be_write_impl (s=0x562b8fe3f800,
buf=0x7fff5484afc0 "outl 0xae08 0x40\n306c\n", len=17) at ../chardev/char.c:202
39 0x0000562b8dc3bc89 in qemu_chr_be_write (s=0x562b8fe3f800,
buf=0x7fff5484afc0 "outl 0xae08 0x40\n306c\n", len=17) at ../chardev/char.c:214
40 0x0000562b8dc37846 in tcp_chr_read (chan=0x562b8fdf5e80, cond=G_IO_IN,
opaque=0x562b8fe3f800) at ../chardev/char-socket.c:508
41 0x0000562b8db30e65 in qio_channel_fd_source_dispatch
(source=0x562b90028ea0, callback=0x562b8dc376cb <tcp_chr_read>,
user_data=0x562b8fe3f800) at ../io/channel-watch.c:84
42 0x00007f1d80e7cfaf in g_main_dispatch (context=0x562b8fc43e80) at
../glib/gmain.c:3417
43 g_main_context_dispatch (context=0x562b8fc43e80) at ../glib/gmain.c:4135
44 0x0000562b8dd1d9c4 in glib_pollfds_poll () at ../util/main-loop.c:294
45 0x0000562b8dd1da41 in os_host_main_loop_wait (timeout=27462700) at
../util/main-loop.c:317
46 0x0000562b8dd1db4f in main_loop_wait (nonblocking=0) at
../util/main-loop.c:603
47 0x0000562b8d822785 in qemu_main_loop () at ../softmmu/runstate.c:730
48 0x0000562b8d5ad486 in qemu_default_main () at ../softmmu/main.c:37
49 0x0000562b8d5ad4bc in main (argc=21, argv=0x7fff5484c2c8) at
../softmmu/main.c:48
Peter Xu (4):
memory: Make memory_listeners RCU-safe for real
memory: Use rcu list variance for address_spaces modifications
memory: Protect memory_region_clear_dirty_bitmap with RCU
memory: Use rcu traversal in memory_region_to_address_space
softmmu/memory.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
--
2.39.1
