Re: [PATCH v3 2/3] KVM: keep track of running ioctls
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).
Re: [PATCH v3 2/3] KVM: keep track of running ioctls
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?
Re: [PATCH v3 2/3] KVM: keep track of running ioctls
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
>> Signed-off-by: Emanuele Giuseppe Esposito
>> ---
>> 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(>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;
>> }
>
Re: [PATCH v3 2/3] KVM: keep track of running ioctls
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
> Signed-off-by: Emanuele Giuseppe Esposito
> ---
> 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(>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;
> }
Re: [PATCH v3 2/3] KVM: keep track of running ioctls
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
>
> 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
>> ---
>> 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(>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-da...@redhat.com/
>
> https://lore.kernel.org/qemu-devel/20200312161217.3590-3-da...@redhat.com/
>
> Do they still apply? Is there more?
>
Apologies, what do you mean with "do they still apply"?
Looks fine to me
Emanuele
Re: [PATCH v3 2/3] KVM: keep track of running ioctls
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
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
---
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(>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-da...@redhat.com/
https://lore.kernel.org/qemu-devel/20200312161217.3590-3-da...@redhat.com/
Do they still apply? Is there more?
--
Thanks,
David / dhildenb
