[Qemu-devel] [PATCH 0/1] KVM: Retry KVM_CREATE_VM on EINTR or EAGAIN

[Qemu-devel] [PATCH 0/1] KVM: Retry KVM_CREATE_VM on EINTR or EAGAIN

[thomas knych]

Hi -

I've encountered occassional instability on qemu/kvm startup when the host is heavily loaded and already managing many guest OS's.

In this cases I've seen the KVM_CREATE_VM ioctl  return EINTR or EAGAIN and thats gets treated as fatal.

Adding a retry in these cases has greatly improved our startup reliablity.

I've submitted this patch to Android's qemu source tree and it seemed useful to push it upstream as well.


Thanks,
-Tom

thomas knych (1):
  KVM: Retry KVM_CREATE_VM on EINTR or EAGAIN

 kvm-all.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
1.8.5.1

[Qemu-devel] [PATCH 1/1] KVM: Retry KVM_CREATE_VM on EINTR or EAGAIN

[thomas knych]

Upstreaming this change from Android (https://android-review.googlesource.com/54211).

On heavily loaded machines with many VM instances we see KVM_CREATE_VM
failing with EINTR/EAGAIN retrying the system call greatly improves
reliability.

Signed-off-by: thomas knych <thomaswk@google.com>
---
 kvm-all.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kvm-all.c b/kvm-all.c
index 3937754..29787ae 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1442,8 +1442,14 @@ int kvm_init(void)
         nc++;
     }
 
-    s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, 0);
+    do {
+        s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, 0);
+    } while (s->vmfd < 0 && (-EINTR == s->vmfd || -EAGAIN == s->vmfd));
+
     if (s->vmfd < 0) {
+        fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d %s\n", -s->vmfd,
+                strerror(-s->vmfd));
+
 #ifdef TARGET_S390X
         fprintf(stderr, "Please add the 'switch_amode' kernel parameter to "
                         "your host kernel command line\n");
-- 
1.8.5.1

Re: [Qemu-devel] [PATCH 1/1] KVM: Retry KVM_CREATE_VM on EINTR or EAGAIN

[Paolo Bonzini]

Il 09/01/2014 22:14, thomas knych ha scritto:
> Upstreaming this change from Android (https://android-review.googlesource.com/54211).
> 
> On heavily loaded machines with many VM instances we see KVM_CREATE_VM
> failing with EINTR/EAGAIN retrying the system call greatly improves
> reliability.
> 
> Signed-off-by: thomas knych <thomaswk@google.com>
> ---
>  kvm-all.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 3937754..29787ae 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1442,8 +1442,14 @@ int kvm_init(void)
>          nc++;
>      }
>  
> -    s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, 0);
> +    do {
> +        s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, 0);
> +    } while (s->vmfd < 0 && (-EINTR == s->vmfd || -EAGAIN == s->vmfd));

No yoda conditionals---please write "s->vmfd == -EINTR".

However, I would like to understand where in the KVM module the error
originates and is propagated from.  Especially EAGAIN seems weird.

Paolo

> +
>      if (s->vmfd < 0) {
> +        fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d %s\n", -s->vmfd,
> +                strerror(-s->vmfd));
> +
>  #ifdef TARGET_S390X
>          fprintf(stderr, "Please add the 'switch_amode' kernel parameter to "
>                          "your host kernel command line\n");
> 

Re: [Qemu-devel] [PATCH 1/1] KVM: Retry KVM_CREATE_VM on EINTR or EAGAIN

[Tom Knych]

[-- Attachment #1: Type: text/plain, Size: 1938 bytes --]

I'll flip the conditional check

So I traced thru the code and the one path I saw returning EINTR was:

kvm_dev_ioctl_create_vm -> kvm_create_vm -> kvm_init_mmu_notifier ->
mmu_notifier_register ->  do_mmu_notifier_register -> mm_take_all_locks

Which checks if any signals have been raised while it was attaining locks
and returns EINTR.

Going thru my logs - all of my errors actually are EINTRs I'll remove the
EAGAIN

Thanks for the review
-Tom


On Fri, Jan 10, 2014 at 5:01 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 09/01/2014 22:14, thomas knych ha scritto:
> > Upstreaming this change from Android (
> https://android-review.googlesource.com/54211).
> >
> > On heavily loaded machines with many VM instances we see KVM_CREATE_VM
> > failing with EINTR/EAGAIN retrying the system call greatly improves
> > reliability.
> >
> > Signed-off-by: thomas knych <thomaswk@google.com>
> > ---
> >  kvm-all.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 3937754..29787ae 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -1442,8 +1442,14 @@ int kvm_init(void)
> >          nc++;
> >      }
> >
> > -    s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, 0);
> > +    do {
> > +        s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, 0);
> > +    } while (s->vmfd < 0 && (-EINTR == s->vmfd || -EAGAIN == s->vmfd));
>
> No yoda conditionals---please write "s->vmfd == -EINTR".
>
> However, I would like to understand where in the KVM module the error
> originates and is propagated from.  Especially EAGAIN seems weird.
>
> Paolo
>
> > +
> >      if (s->vmfd < 0) {
> > +        fprintf(stderr, "ioctl(KVM_CREATE_VM) failed: %d %s\n",
> -s->vmfd,
> > +                strerror(-s->vmfd));
> > +
> >  #ifdef TARGET_S390X
> >          fprintf(stderr, "Please add the 'switch_amode' kernel parameter
> to "
> >                          "your host kernel command line\n");
> >
>
>

[-- Attachment #2: Type: text/html, Size: 3120 bytes --]

Re: [Qemu-devel] [PATCH 1/1] KVM: Retry KVM_CREATE_VM on EINTR or EAGAIN

[Paolo Bonzini]

Il 10/01/2014 23:15, Tom Knych ha scritto:
> I'll flip the conditional check
> 
> So I traced thru the code and the one path I saw returning EINTR was:
> 
> kvm_dev_ioctl_create_vm -> kvm_create_vm -> kvm_init_mmu_notifier ->
> mmu_notifier_register ->  do_mmu_notifier_register -> mm_take_all_locks
> 
> Which checks if any signals have been raised while it was attaining
> locks and returns EINTR.
> 
> Going thru my logs - all of my errors actually are EINTRs I'll remove
> the EAGAIN

Andrea, what do you think here?  Is it intended that
kvm_init_mmu_notifier return an EINTR that percolates up to userspace?

Paolo

Re: [Qemu-devel] [PATCH 1/1] KVM: Retry KVM_CREATE_VM on EINTR or EAGAIN

[Andrea Arcangeli]

On Mon, Jan 13, 2014 at 12:16:11PM +0100, Paolo Bonzini wrote:
> Il 10/01/2014 23:15, Tom Knych ha scritto:
> > I'll flip the conditional check
> > 
> > So I traced thru the code and the one path I saw returning EINTR was:
> > 
> > kvm_dev_ioctl_create_vm -> kvm_create_vm -> kvm_init_mmu_notifier ->
> > mmu_notifier_register ->  do_mmu_notifier_register -> mm_take_all_locks
> > 
> > Which checks if any signals have been raised while it was attaining
> > locks and returns EINTR.
> > 
> > Going thru my logs - all of my errors actually are EINTRs I'll remove
> > the EAGAIN
> 
> Andrea, what do you think here?  Is it intended that
> kvm_init_mmu_notifier return an EINTR that percolates up to userspace?

It is intended yes. The reason being that mm_take_all_locks is
potentially a CPU intensive operation so if we don't return -EINTR and
break it immediately if a signal is pending, we'd be potentially
hanging the process for too long, if you press C^c or the task wouldn't
go away immediately, or if you kill -9 same problem.

All CPU intensive syscalls in the kernel should check for pending
signals and return -EINTR immediately to allow userland to remain
interactive.

EAGAIN shouldn't originate anywhere in those paths so yes they're all
EINTR for interactivity.

Why don't you mask the signals instead of looping? That would be more
efficient, what's the point of interrupting the syscall and restarting
it when you can just avoid being interrupted during it? If the signal
is blocked signal_pending won't be raised and EINTR won't be
generated. I think you only need a sigprocmask or equivalent around
the call.

It's important to check the retval and fail the startup of qemu if you
still get a error, but you shouldn't loop if you just mask the signal
instead of looping.

Re: [Qemu-devel] [PATCH 1/1] KVM: Retry KVM_CREATE_VM on EINTR or EAGAIN

[Tom Knych]

[-- Attachment #1: Type: text/plain, Size: 2003 bytes --]

Doing it with sigprocmask seems good I will send an updated patch
On Jan 14, 2014 10:57 AM, "Andrea Arcangeli" <aarcange@redhat.com> wrote:

> On Mon, Jan 13, 2014 at 12:16:11PM +0100, Paolo Bonzini wrote:
> > Il 10/01/2014 23:15, Tom Knych ha scritto:
> > > I'll flip the conditional check
> > >
> > > So I traced thru the code and the one path I saw returning EINTR was:
> > >
> > > kvm_dev_ioctl_create_vm -> kvm_create_vm -> kvm_init_mmu_notifier ->
> > > mmu_notifier_register ->  do_mmu_notifier_register -> mm_take_all_locks
> > >
> > > Which checks if any signals have been raised while it was attaining
> > > locks and returns EINTR.
> > >
> > > Going thru my logs - all of my errors actually are EINTRs I'll remove
> > > the EAGAIN
> >
> > Andrea, what do you think here?  Is it intended that
> > kvm_init_mmu_notifier return an EINTR that percolates up to userspace?
>
> It is intended yes. The reason being that mm_take_all_locks is
> potentially a CPU intensive operation so if we don't return -EINTR and
> break it immediately if a signal is pending, we'd be potentially
> hanging the process for too long, if you press C^c or the task wouldn't
> go away immediately, or if you kill -9 same problem.
>
> All CPU intensive syscalls in the kernel should check for pending
> signals and return -EINTR immediately to allow userland to remain
> interactive.
>
> EAGAIN shouldn't originate anywhere in those paths so yes they're all
> EINTR for interactivity.
>
> Why don't you mask the signals instead of looping? That would be more
> efficient, what's the point of interrupting the syscall and restarting
> it when you can just avoid being interrupted during it? If the signal
> is blocked signal_pending won't be raised and EINTR won't be
> generated. I think you only need a sigprocmask or equivalent around
> the call.
>
> It's important to check the retval and fail the startup of qemu if you
> still get a error, but you shouldn't loop if you just mask the signal
> instead of looping.
>

[-- Attachment #2: Type: text/html, Size: 2528 bytes --]

Re: [Qemu-devel] [PATCH 1/1] KVM: Retry KVM_CREATE_VM on EINTR or EAGAIN

[Paolo Bonzini]

Il 15/01/2014 03:09, Tom Knych ha scritto:
> Doing it with sigprocmask seems good I will send an updated patch

No need, I'll apply this patch.

Paolo