| Message ID | 20221111154758.1372674-3-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: allow listener to stop all vcpus before | expand |
On 11.11.22 16:47, Emanuele Giuseppe Esposito wrote: > Using the new accel-blocker API, mark where ioctls are being called > in KVM. Next, we will implement the critical section that will take > care of performing memslots modifications atomically, therefore > preventing any new ioctl from running and allowing the running ones > to finish. > > Signed-off-by: David Hildenbrand <david@redhat.com> You might want to drop that and instead mention something like "This patch is based on a protoype patch by David Hildenbrand". > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > accel/kvm/kvm-all.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index f99b0becd8..ff660fd469 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -2310,6 +2310,7 @@ static int kvm_init(MachineState *ms) > assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size()); > > s->sigmask_len = 8; > + accel_blocker_init(); > > #ifdef KVM_CAP_SET_GUEST_DEBUG > QTAILQ_INIT(&s->kvm_sw_breakpoints); > @@ -3014,7 +3015,9 @@ int kvm_vm_ioctl(KVMState *s, int type, ...) > va_end(ap); > > trace_kvm_vm_ioctl(type, arg); > + accel_ioctl_begin(); > ret = ioctl(s->vmfd, type, arg); > + accel_ioctl_end(); > if (ret == -1) { > ret = -errno; > } > @@ -3032,7 +3035,9 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, ...) > va_end(ap); > > trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg); > + accel_cpu_ioctl_begin(cpu); > ret = ioctl(cpu->kvm_fd, type, arg); > + accel_cpu_ioctl_end(cpu); > if (ret == -1) { > ret = -errno; > } > @@ -3050,7 +3055,9 @@ int kvm_device_ioctl(int fd, int type, ...) > va_end(ap); > > trace_kvm_device_ioctl(fd, type, arg); > + accel_ioctl_begin(); > ret = ioctl(fd, type, arg); > + accel_ioctl_end(); > if (ret == -1) { > ret = -errno; > } I recall that I had some additional patches that tried to catch some of more calls: https://lore.kernel.org/qemu-devel/20200312161217.3590-2-david@redhat.com/ https://lore.kernel.org/qemu-devel/20200312161217.3590-3-david@redhat.com/ Do they still apply? Is there more?
Am 17/11/2022 um 20:27 schrieb David Hildenbrand: > On 11.11.22 16:47, Emanuele Giuseppe Esposito wrote: >> Using the new accel-blocker API, mark where ioctls are being called >> in KVM. Next, we will implement the critical section that will take >> care of performing memslots modifications atomically, therefore >> preventing any new ioctl from running and allowing the running ones >> to finish. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> > > You might want to drop that and instead mention something like "This > patch is based on a protoype patch by David Hildenbrand". > >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> --- >> accel/kvm/kvm-all.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >> index f99b0becd8..ff660fd469 100644 >> --- a/accel/kvm/kvm-all.c >> +++ b/accel/kvm/kvm-all.c >> @@ -2310,6 +2310,7 @@ static int kvm_init(MachineState *ms) >> assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size()); >> s->sigmask_len = 8; >> + accel_blocker_init(); >> #ifdef KVM_CAP_SET_GUEST_DEBUG >> QTAILQ_INIT(&s->kvm_sw_breakpoints); >> @@ -3014,7 +3015,9 @@ int kvm_vm_ioctl(KVMState *s, int type, ...) >> va_end(ap); >> trace_kvm_vm_ioctl(type, arg); >> + accel_ioctl_begin(); >> ret = ioctl(s->vmfd, type, arg); >> + accel_ioctl_end(); >> if (ret == -1) { >> ret = -errno; >> } >> @@ -3032,7 +3035,9 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, ...) >> va_end(ap); >> trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg); >> + accel_cpu_ioctl_begin(cpu); >> ret = ioctl(cpu->kvm_fd, type, arg); >> + accel_cpu_ioctl_end(cpu); >> if (ret == -1) { >> ret = -errno; >> } >> @@ -3050,7 +3055,9 @@ int kvm_device_ioctl(int fd, int type, ...) >> va_end(ap); >> trace_kvm_device_ioctl(fd, type, arg); >> + accel_ioctl_begin(); >> ret = ioctl(fd, type, arg); >> + accel_ioctl_end(); >> if (ret == -1) { >> ret = -errno; >> } > > I recall that I had some additional patches that tried to catch some of > more calls: > > https://lore.kernel.org/qemu-devel/20200312161217.3590-2-david@redhat.com/ > > https://lore.kernel.org/qemu-devel/20200312161217.3590-3-david@redhat.com/ > > Do they still apply? Is there more? > Apologies, what do you mean with "do they still apply"? Looks fine to me Emanuele
On Fri, 2022-11-11 at 10:47 -0500, Emanuele Giuseppe Esposito wrote: > Using the new accel-blocker API, mark where ioctls are being called > in KVM. Next, we will implement the critical section that will take > care of performing memslots modifications atomically, therefore > preventing any new ioctl from running and allowing the running ones > to finish. > > Signed-off-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > accel/kvm/kvm-all.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index f99b0becd8..ff660fd469 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -2310,6 +2310,7 @@ static int kvm_init(MachineState *ms) > assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size()); > > s->sigmask_len = 8; > + accel_blocker_init(); > > #ifdef KVM_CAP_SET_GUEST_DEBUG > QTAILQ_INIT(&s->kvm_sw_breakpoints); > @@ -3014,7 +3015,9 @@ int kvm_vm_ioctl(KVMState *s, int type, ...) > va_end(ap); > > trace_kvm_vm_ioctl(type, arg); > + accel_ioctl_begin(); > ret = ioctl(s->vmfd, type, arg); > + accel_ioctl_end(); > if (ret == -1) { > ret = -errno; > } > @@ -3032,7 +3035,9 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, > ...) > va_end(ap); > > trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg); > + accel_cpu_ioctl_begin(cpu); Does this mean that kvm_region_commit() can inhibit any other vcpus doing any ioctls? > ret = ioctl(cpu->kvm_fd, type, arg); > + accel_cpu_ioctl_end(cpu); > if (ret == -1) { > ret = -errno; > } > @@ -3050,7 +3055,9 @@ int kvm_device_ioctl(int fd, int type, ...) > va_end(ap); > > trace_kvm_device_ioctl(fd, type, arg); > + accel_ioctl_begin(); > ret = ioctl(fd, type, arg); > + accel_ioctl_end(); > if (ret == -1) { > ret = -errno; > }
Am 02/12/2022 um 07:54 schrieb Robert Hoo: > On Fri, 2022-11-11 at 10:47 -0500, Emanuele Giuseppe Esposito wrote: >> Using the new accel-blocker API, mark where ioctls are being called >> in KVM. Next, we will implement the critical section that will take >> care of performing memslots modifications atomically, therefore >> preventing any new ioctl from running and allowing the running ones >> to finish. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> --- >> accel/kvm/kvm-all.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >> index f99b0becd8..ff660fd469 100644 >> --- a/accel/kvm/kvm-all.c >> +++ b/accel/kvm/kvm-all.c >> @@ -2310,6 +2310,7 @@ static int kvm_init(MachineState *ms) >> assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size()); >> >> s->sigmask_len = 8; >> + accel_blocker_init(); >> >> #ifdef KVM_CAP_SET_GUEST_DEBUG >> QTAILQ_INIT(&s->kvm_sw_breakpoints); >> @@ -3014,7 +3015,9 @@ int kvm_vm_ioctl(KVMState *s, int type, ...) >> va_end(ap); >> >> trace_kvm_vm_ioctl(type, arg); >> + accel_ioctl_begin(); >> ret = ioctl(s->vmfd, type, arg); >> + accel_ioctl_end(); >> if (ret == -1) { >> ret = -errno; >> } >> @@ -3032,7 +3035,9 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, >> ...) >> va_end(ap); >> >> trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg); >> + accel_cpu_ioctl_begin(cpu); > > Does this mean that kvm_region_commit() can inhibit any other vcpus > doing any ioctls? Yes, because we must prevent any vcpu from reading memslots while we are updating them. > >> ret = ioctl(cpu->kvm_fd, type, arg); >> + accel_cpu_ioctl_end(cpu); >> if (ret == -1) { >> ret = -errno; >> } >> @@ -3050,7 +3055,9 @@ int kvm_device_ioctl(int fd, int type, ...) >> va_end(ap); >> >> trace_kvm_device_ioctl(fd, type, arg); >> + accel_ioctl_begin(); >> ret = ioctl(fd, type, arg); >> + accel_ioctl_end(); >> if (ret == -1) { >> ret = -errno; >> } >
On Fri, 2022-12-02 at 13:03 +0100, Emanuele Giuseppe Esposito wrote: ... > > > @@ -3032,7 +3035,9 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, > > > ...) > > > va_end(ap); > > > > > > trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg); > > > + accel_cpu_ioctl_begin(cpu); > > > > Does this mean that kvm_region_commit() can inhibit any other vcpus > > doing any ioctls? > > Yes, because we must prevent any vcpu from reading memslots while we > are > updating them. > But do most other vm/vcpu ioctls contend with memslot operations?
Am 02/12/2022 um 14:32 schrieb Robert Hoo: > On Fri, 2022-12-02 at 13:03 +0100, Emanuele Giuseppe Esposito wrote: > ... >>>> @@ -3032,7 +3035,9 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, >>>> ...) >>>> va_end(ap); >>>> >>>> trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg); >>>> + accel_cpu_ioctl_begin(cpu); >>> >>> Does this mean that kvm_region_commit() can inhibit any other vcpus >>> doing any ioctls? >> >> Yes, because we must prevent any vcpu from reading memslots while we >> are >> updating them. >> > But do most other vm/vcpu ioctls contend with memslot operations? > I think this is the simplest way. I agree not all ioctls contend with memslot operations, but there are also not so many memslot operations too. Instead of going one by one in all possible ioctls, covering all of them is the simplest way and it covers also the case of a new ioctl reading memslots that could be added in the future (alternatively we would be always updating the list of ioctls to block).
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index f99b0becd8..ff660fd469 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -2310,6 +2310,7 @@ static int kvm_init(MachineState *ms) assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size()); s->sigmask_len = 8; + accel_blocker_init(); #ifdef KVM_CAP_SET_GUEST_DEBUG QTAILQ_INIT(&s->kvm_sw_breakpoints); @@ -3014,7 +3015,9 @@ int kvm_vm_ioctl(KVMState *s, int type, ...) va_end(ap); trace_kvm_vm_ioctl(type, arg); + accel_ioctl_begin(); ret = ioctl(s->vmfd, type, arg); + accel_ioctl_end(); if (ret == -1) { ret = -errno; } @@ -3032,7 +3035,9 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, ...) va_end(ap); trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg); + accel_cpu_ioctl_begin(cpu); ret = ioctl(cpu->kvm_fd, type, arg); + accel_cpu_ioctl_end(cpu); if (ret == -1) { ret = -errno; } @@ -3050,7 +3055,9 @@ int kvm_device_ioctl(int fd, int type, ...) va_end(ap); trace_kvm_device_ioctl(fd, type, arg); + accel_ioctl_begin(); ret = ioctl(fd, type, arg); + accel_ioctl_end(); if (ret == -1) { ret = -errno; }