Am 19/09/2022 um 19:30 schrieb David Hildenbrand: > On 19.09.22 09:53, David Hildenbrand wrote: >> On 18.09.22 18:13, Emanuele Giuseppe Esposito wrote: >>> >>> >>> Am 09/09/2022 um 16:30 schrieb Sean Christopherson: >>>> On Fri, Sep 09, 2022, Emanuele Giuseppe Esposito wrote: >>>>> KVM is currently capable of receiving a single memslot update through >>>>> the KVM_SET_USER_MEMORY_REGION ioctl. >>>>> The problem arises when we want to atomically perform multiple >>>>> updates, >>>>> so that readers of memslot active list avoid seeing incomplete states. >>>>> >>>>> For example, in RHBZ >>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1979276 >>>> >>>> I don't have access. Can you provide a TL;DR? >>> >>> You should be able to have access to it now. >>> >>>> >>>>> we see how non atomic updates cause boot failure, because vcpus >>>>> will se a partial update (old memslot delete, new one not yet created) >>>>> and will crash. >>>> >>>> Why not simply pause vCPUs in this scenario? This is an awful lot >>>> of a complexity >>>> to take on for something that appears to be solvable in userspace. >>>> >>> >>> I think it is not that easy to solve in userspace: see >>> https://lore.kernel.org/qemu-devel/20200312161217.3590-1-david@xxxxxxxxxx/ >>> >>> >>> >>> "Using pause_all_vcpus()/resume_all_vcpus() is not possible, as it will >>> temporarily drop the BQL - something most callers can't handle (esp. >>> when called from vcpu context e.g., in virtio code)." >> >> Can you please comment on the bigger picture? The patch from me works >> around *exactly that*, and for that reason, contains that comment. >> > > FWIW, I hacked up my RFC to perform atomic updates on any memslot > transactions (not just resizes) where ranges do add overlap with ranges > to remove. > > https://github.com/davidhildenbrand/qemu/tree/memslot > > > I only performed simple boot check under x86-64 (where I can see region > resizes) and some make checks -- pretty sure it has some rough edges; > but should indicate what's possible and what the possible price might > be. [one could wire up a new KVM ioctl and call it conditionally on > support if really required] > A benefit of my ioctl implementation is that could be also used by other hypervisors, which then do not need to share this kind of "hack". However, after also talking with Maxim and Paolo, we all agreed that the main disadvantage of your approach is that is not scalable with the number of vcpus. It is also inferior to stop *all* vcpus just to allow a memslot update (KVM only pauses vCPUs that access the modified memslots instead). So I took some measurements, to see what is the performance difference between my implementation and yours. I used a machine where I could replicate the bug mentioned in bugzilla, an AMD EPYC 7413 24-Core Processor with kernel 5.19.0 (+ my patches). Then I measured the time it takes that QEMU spends in kvm_commit (ie in memslot updates) while booting a VM. In other words, if kvm_commit takes 10 ms and QEMU calls it 20 times, "time to boot" is 200ms. kvm_commit is not called anymore after boot, so this measurement is easy to compare over multiple invocations of QEMU. I ran the tests with different amount of cores: 1,2,4,8,16,32. QEMU command is the same to replicate the bug: ./qemu-system-x86_64 --overcommit cpu-pm=on --smp $v --accel kvm --display none >> ~/smp_$v; Each boot is reproduced 100 times, and then from results I measure average and stddev (in milliseconds). ioctl: -smp 1: Average: 2.1ms Stdev: 0.8ms -smp 2: Average: 2.5ms Stdev: 1.5ms -smp 4: Average: 2.2ms Stdev: 1.1ms -smp 8: Average: 2.4ms Stdev: 0.7ms -smp 16: Average: 3.6ms Stdev: 2.4ms (1000 repetitions) -smp 24: Average: 12.5ms Stdev: 0.9ms (1000 repetitions) pause/resume: (https://github.com/davidhildenbrand/qemu/tree/memslot) -smp 1: Average: 2.2ms Stdev: 1.2ms -smp 2: Average: 3.0ms Stdev: 1.4ms -smp 4: Average: 3.1ms Stdev: 1.3m -smp 8: Average: 3.4ms Stdev: 1.4ms -smp 16: Average: 12ms Stdev: 7.0ms (1000 repetitions) -smp 24: Average: 20ms Stdev: 7.3ms (1000 repetitions) Above 24 vCPUs performance gets worse quickly but I think it's already quite clear that the results for ioctl scale better as the number of vcpus increases, while pausing the vCPUs becomes slower already with 16 vcpus. Thank you, Emanuele
