Hi, Peter,
On 2023/3/7 δΈε4:48, Peter Xu wrote:
On Mon, Mar 06, 2023 at 08:48:05PM +0800, Chuang Xu wrote:
Hi, Peter,
On 2023/3/6 δΈε6:05, Peter Xu wrote:
1.virtio_load->virtio_init_region_cache
2.virtio_load->virtio_set_features_nocheck
What is this one specifically? I failed to see quickly why this needs to
reference the flatview.
0x560f3bb26510 : memory_region_transaction_do_commit+0x0/0x1c0
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bb26e45 : address_space_get_flatview+0x95/0x100
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bb296b6 : memory_listener_register+0xf6/0x300
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3baec59f : virtio_device_realize+0x12f/0x1a0
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbb3b1f : device_set_realized+0x2ff/0x660
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbb6ec6 : property_set_bool+0x46/0x70
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbb9173 : object_property_set+0x73/0x100
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbbc1ef : object_property_set_qobject+0x2f/0x50
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbb93e4 : object_property_set_bool+0x34/0xa0
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3b9830d9 : virtio_pci_realize+0x299/0x4e0
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3b901204 : pci_qdev_realize+0x7e4/0x1090
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbb3b1f : device_set_realized+0x2ff/0x660
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbb6ec6 : property_set_bool+0x46/0x70
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbb9173 : object_property_set+0x73/0x100
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbbc1ef : object_property_set_qobject+0x2f/0x50
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbb93e4 : object_property_set_bool+0x34/0xa0
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3b99e4a0 : qdev_device_add_from_qdict+0xb00/0xc40
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bac0c75 : virtio_net_set_features+0x385/0x490
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3baec238 : virtio_set_features_nocheck+0x58/0x70
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3baf1e9e : virtio_load+0x33e/0x820
[/data00/migration/qemu-open/build/qemu-system-x86_64]
I think this one can also be avoided. Basically any memory listener
related op can avoid referencing the latest flatview because even if it's
during depth>0 it'll be synced again when depth==0.
3.vapic_post_load
Same confusion to this one..
0x557a57b0e510 : memory_region_transaction_do_commit+0x0/0x1c0
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x557a57b0e9b5 : memory_region_find_rcu+0x2e5/0x310
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x557a57b11165 : memory_region_find+0x55/0xf0
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x557a57a07638 : vapic_prepare+0x58/0x250
[/data00/migration/qemu-open/build/qemu-system-x86_64]
0x557a57a07a7b : vapic_post_load+0x1b/0x80
[/data00/migration/qemu-open/build/qemu-system-x86_64]
AFAIU this one is the other one that truly need to reference the latest
flatview; the other one is (5) on AHCI. It's a pity that it uses
memory_region_find_rcu() even if it must already have BQL so it's kind of
unclear (and enforced commit should never need to happen with RCU
logically..).
4.tcg_commit
This is not enabled if kvm is on, right?
Yes.
I obtained these results from qtests. And some qtests enabled tcg.π
5.ahci_state_post_load
During my test, virtio_init_region_cache() will frequently trigger
do_commit() in address_space_to_flatview(), which will reduce the
optimization effect of v6 compared with v1.
IIU above 1 & 4 could leverage one address_space_to_flatview_rcu() which
can keep the old semantics of address_space_to_flatview() by just assert
rcu read lock and do qatomic_rcu_read() (e.g., tcg_commit() will run again
at last stage of vm load). Not sure how much it'll help.
This can really improve the performance of the existing patch, which is
basically the same as that of v1, but it needs to add
address_space_to_flatview_rcu()
and address_space_get_flatview_rcu(). I have also considered this, but will
others find it confusing? Because there will be to_flatview(), to_flatview_raw()
and to_flatview_rcu()..
Why do we need address_space_get_flatview_rcu()? I'm not sure whether you
address_space_cahce_init() uses address_space_get_flatview() to acquire
a ref-ed flatview. If we want to use address_space_to_flatview_rcu() and
make the flatview ref-ed, maybe we need to add address_space_get_flatview_rcu()?
Or if we use address_space_to_flatview_rcu() directly, then we'll get a
unref-ed flatview, I'm not sure wheather it's safe in other cases.. At least
I don't want the assertion to be triggered one day.
mean the two calls in memory listener add/del, if so would you consider a
fixup that I attached in this reply and squash it into patch 1 of mine? I
assume that'll also remove case (2) above, and also remove the need to have
address_space_get_flatview_rcu().
Later fixed in v7.
Having address_space_to_flatview_rcu() alone is fine to me (which is
actually the original address_space_to_flatview). Again IMHO it should
only be called in the case where a stall flatview doesn't hurt.
Thanks!
