Re: [PATCH] kvm: don't take vcpu mutex for obviously invalid vcpu ioctls

2014-09-23 Thread Gleb Natapov
On Mon, Sep 22, 2014 at 09:29:19PM +0200, Paolo Bonzini wrote:
 Il 22/09/2014 21:20, Christian Borntraeger ha scritto:
  while using trinity to fuzz KVM, we noticed long stalls on invalid ioctls. 
  Lets bail out early on invalid ioctls. or similar?
 
 Okay.  David, can you explain how you found it so that I can make up my
 mind?
 
 Gleb and Marcelo, a fourth and fifth opinion? :)
 
I agree with Christian that simpler fix is better here.
The overhead is minimal. If we ever notice this overhead
we can revert the patch all together since the problem it
fixes can only be inflicted on userspace by itself and there
are myriads other ways userspace can hurt itself.

--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] kvm: don't take vcpu mutex for obviously invalid vcpu ioctls

2014-09-23 Thread Christian Borntraeger
On 09/23/2014 08:49 AM, Gleb Natapov wrote:
 On Mon, Sep 22, 2014 at 09:29:19PM +0200, Paolo Bonzini wrote:
 Il 22/09/2014 21:20, Christian Borntraeger ha scritto:
 while using trinity to fuzz KVM, we noticed long stalls on invalid ioctls. 
 Lets bail out early on invalid ioctls. or similar?

 Okay.  David, can you explain how you found it so that I can make up my
 mind?

 Gleb and Marcelo, a fourth and fifth opinion? :)

 I agree with Christian that simpler fix is better here.
 The overhead is minimal. If we ever notice this overhead
 we can revert the patch all together since the problem it
 fixes can only be inflicted on userspace by itself and there
 are myriads other ways userspace can hurt itself.


Yes. Davids explanation also makes sense as a commit message. Paolo, if you use 
David patch with a better description of the why I am fine with this patch.

Christian

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] kvm: don't take vcpu mutex for obviously invalid vcpu ioctls

2014-09-23 Thread Paolo Bonzini
Il 23/09/2014 10:06, Christian Borntraeger ha scritto:
 Yes. Davids explanation also makes sense as a commit message. Paolo,
 if you use David patch with a better description of the why I am
 fine with this patch.

Done, thanks everybody!

Paolo
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] kvm: don't take vcpu mutex for obviously invalid vcpu ioctls

2014-09-22 Thread Paolo Bonzini
Il 20/09/2014 01:03, David Matlack ha scritto:
 vcpu ioctls can hang the calling thread if issued while a vcpu is
 running. If we know ioctl is going to be rejected as invalid anyway,
 we can fail before trying to take the vcpu mutex.
 
 This patch does not change functionality, it just makes invalid ioctls
 fail faster.
 
 Signed-off-by: David Matlack dmatl...@google.com
 ---
  virt/kvm/kvm_main.c | 4 
  1 file changed, 4 insertions(+)
 
 diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
 index 96ec622..f9234e5 100644
 --- a/virt/kvm/kvm_main.c
 +++ b/virt/kvm/kvm_main.c
 @@ -52,6 +52,7 @@
  
  #include asm/processor.h
  #include asm/io.h
 +#include asm/ioctl.h
  #include asm/uaccess.h
  #include asm/pgtable.h
  
 @@ -1975,6 +1976,9 @@ static long kvm_vcpu_ioctl(struct file *filp,
   if (vcpu-kvm-mm != current-mm)
   return -EIO;
  
 + if (unlikely(_IOC_TYPE(ioctl) != KVMIO))
 + return -EINVAL;
 +
  #if defined(CONFIG_S390) || defined(CONFIG_PPC) || defined(CONFIG_MIPS)
   /*
* Special cases: vcpu ioctls that are asynchronous to vcpu execution,
 

Thanks, applying this patch.

Paolo
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] kvm: don't take vcpu mutex for obviously invalid vcpu ioctls

2014-09-22 Thread Christian Borntraeger
On 09/22/2014 12:50 PM, Paolo Bonzini wrote:
 Il 20/09/2014 01:03, David Matlack ha scritto:
 vcpu ioctls can hang the calling thread if issued while a vcpu is
 running. If we know ioctl is going to be rejected as invalid anyway,
 we can fail before trying to take the vcpu mutex.

 This patch does not change functionality, it just makes invalid ioctls
 fail faster.

 Signed-off-by: David Matlack dmatl...@google.com
 ---
  virt/kvm/kvm_main.c | 4 
  1 file changed, 4 insertions(+)

 diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
 index 96ec622..f9234e5 100644
 --- a/virt/kvm/kvm_main.c
 +++ b/virt/kvm/kvm_main.c
 @@ -52,6 +52,7 @@
  
  #include asm/processor.h
  #include asm/io.h
 +#include asm/ioctl.h
  #include asm/uaccess.h
  #include asm/pgtable.h
  
 @@ -1975,6 +1976,9 @@ static long kvm_vcpu_ioctl(struct file *filp,
  if (vcpu-kvm-mm != current-mm)
  return -EIO;
  
 +if (unlikely(_IOC_TYPE(ioctl) != KVMIO))
 +return -EINVAL;
 +
  #if defined(CONFIG_S390) || defined(CONFIG_PPC) || defined(CONFIG_MIPS)
  /*
   * Special cases: vcpu ioctls that are asynchronous to vcpu execution,

 
 Thanks, applying this patch.

Isnt that the wrong trade off?

We now have an extra condition check for every valid ioctl, to make an error 
case go faster.
I know, the extra check is just a 1 or 2 cycles if branch prediction is right, 
but still.

Christian

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] kvm: don't take vcpu mutex for obviously invalid vcpu ioctls

2014-09-22 Thread Paolo Bonzini
Il 22/09/2014 15:45, Christian Borntraeger ha scritto:
 We now have an extra condition check for every valid ioctl, to make an error 
 case go faster.
 I know, the extra check is just a 1 or 2 cycles if branch prediction is 
 right, but still.

I applied the patch because the delay could be substantial, depending on
what the other VCPU is doing.  Perhaps something like this would be
better?

(Untested, but Tested-by/Reviewed-bys are welcome).

Paolo

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 84e24b210273..ed31760d79fe 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -117,12 +117,10 @@ bool kvm_is_mmio_pfn(pfn_t pfn)
 /*
  * Switches to specified vcpu, until a matching vcpu_put()
  */
-int vcpu_load(struct kvm_vcpu *vcpu)
+static void __vcpu_load(struct kvm_vcpu *vcpu)
 {
int cpu;
 
-   if (mutex_lock_killable(vcpu-mutex))
-   return -EINTR;
if (unlikely(vcpu-pid != current-pids[PIDTYPE_PID].pid)) {
/* The thread running this VCPU changed. */
struct pid *oldpid = vcpu-pid;
@@ -136,6 +134,14 @@ int vcpu_load(struct kvm_vcpu *vcpu)
preempt_notifier_register(vcpu-preempt_notifier);
kvm_arch_vcpu_load(vcpu, cpu);
put_cpu();
+}
+
+int vcpu_load(struct kvm_vcpu *vcpu)
+{
+   if (mutex_lock_killable(vcpu-mutex))
+   return -EINTR;
+
+   __vcpu_load(vcpu);
return 0;
 }
 
@@ -1989,9 +1995,6 @@ static long kvm_vcpu_ioctl(struct file *filp,
if (vcpu-kvm-mm != current-mm)
return -EIO;
 
-   if (unlikely(_IOC_TYPE(ioctl) != KVMIO))
-   return -EINVAL;
-
 #if defined(CONFIG_S390) || defined(CONFIG_PPC) || defined(CONFIG_MIPS)
/*
 * Special cases: vcpu ioctls that are asynchronous to vcpu execution,
@@ -2001,8 +2004,21 @@ static long kvm_vcpu_ioctl(struct file *filp,
return kvm_arch_vcpu_ioctl(filp, ioctl, arg);
 #endif
 
+   if (!mutex_trylock(vcpu-mutex))) {
+   /*
+* Before a potentially long sleep, check if we'd exit anyway.
+* The common case is for the mutex not to be contended, when
+* this does not add overhead.
+*/
+   if (unlikely(_IOC_TYPE(ioctl) != KVMIO))
+   return -EINVAL;
+
+   if (mutex_lock_killable(vcpu-mutex))
+   return -EINTR;
+   }
+
 
-   r = vcpu_load(vcpu);
+   r = __vcpu_load(vcpu);
if (r)
return r;
switch (ioctl) {

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] kvm: don't take vcpu mutex for obviously invalid vcpu ioctls

2014-09-22 Thread David Matlack
On 09/22, Paolo Bonzini wrote:
 Il 22/09/2014 15:45, Christian Borntraeger ha scritto:
  We now have an extra condition check for every valid ioctl, to make an 
  error case go faster.
  I know, the extra check is just a 1 or 2 cycles if branch prediction is 
  right, but still.
 
 I applied the patch because the delay could be substantial, depending on
 what the other VCPU is doing.  Perhaps something like this would be
 better?

I'm happy with either approach.

 
 (Untested, but Tested-by/Reviewed-bys are welcome).

There were a few build bugs in your diff. Here's a working version that
I tested. Feel free to add my Tested-by and Reviewed-by if you go with
this.


diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index c71931f..fbdcdc2 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -133,12 +133,10 @@ bool kvm_is_mmio_pfn(pfn_t pfn)
 /*
  * Switches to specified vcpu, until a matching vcpu_put()
  */
-int vcpu_load(struct kvm_vcpu *vcpu)
+static void __vcpu_load(struct kvm_vcpu *vcpu)
 {
int cpu;
 
-   if (mutex_lock_killable(vcpu-mutex))
-   return -EINTR;
if (unlikely(vcpu-pid != current-pids[PIDTYPE_PID].pid)) {
/* The thread running this VCPU changed. */
struct pid *oldpid = vcpu-pid;
@@ -151,6 +149,14 @@ int vcpu_load(struct kvm_vcpu *vcpu)
preempt_notifier_register(vcpu-preempt_notifier);
kvm_arch_vcpu_load(vcpu, cpu);
put_cpu();
+}
+
+int vcpu_load(struct kvm_vcpu *vcpu)
+{
+   if (mutex_lock_killable(vcpu-mutex))
+   return -EINTR;
+
+   __vcpu_load(vcpu);
return 0;
 }
 
@@ -2197,10 +2203,21 @@ static long kvm_vcpu_ioctl(struct file *filp,
return kvm_arch_vcpu_ioctl(filp, ioctl, arg);
 #endif
 
+   if (!mutex_trylock(vcpu-mutex)) {
+   /*
+* Before a potentially long sleep, check if we'd exit anyway.
+* The common case is for the mutex not to be contended, when
+* this does not add overhead.
+*/
+   if (unlikely(_IOC_TYPE(ioctl) != KVMIO))
+   return -EINVAL;
+
+   if (mutex_lock_killable(vcpu-mutex))
+   return -EINTR;
+   }
+
+   __vcpu_load(vcpu);
 
-   r = vcpu_load(vcpu);
-   if (r)
-   return r;
switch (ioctl) {
case KVM_RUN:
r = -EINVAL;
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] kvm: don't take vcpu mutex for obviously invalid vcpu ioctls

2014-09-22 Thread Christian Borntraeger
On 09/22/2014 04:31 PM, Paolo Bonzini wrote:
 Il 22/09/2014 15:45, Christian Borntraeger ha scritto:
 We now have an extra condition check for every valid ioctl, to make an error 
 case go faster.
 I know, the extra check is just a 1 or 2 cycles if branch prediction is 
 right, but still.
 
 I applied the patch because the delay could be substantial,

I know, but only for seriously misbehaving userspace, no? See my comment is 
really a minor one - lets say 2 more cycles for something that exited to 
userspace - nobody would even notice. I am just disturbed by the fact that we 
care about something that is not slow-path but broken beyond repair (why does 
userspace call a non-KVM ioctl on a fd of a vcpu from a different thread 
(otherwise the mutex would be free)?

Please, can we have an explanation, e.g. something like
while using trinity to fuzz KVM, we noticed long stalls on invalid ioctls. 
Lets bail out early on invalid ioctls. or similar?


 depending on what the other VCPU is doing.
 Perhaps something like this would be
 better?
 
 (Untested, but Tested-by/Reviewed-bys are welcome).

Given that it makes sense to improve a misbehaving userspace, I prefer Davids 
variant as the patch is smaller and easier to get right. No need to revert, but 
a better explanation for the use case would be appeciated.

Christian 
 
 Paolo
 
 diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
 index 84e24b210273..ed31760d79fe 100644
 --- a/virt/kvm/kvm_main.c
 +++ b/virt/kvm/kvm_main.c
 @@ -117,12 +117,10 @@ bool kvm_is_mmio_pfn(pfn_t pfn)
  /*
   * Switches to specified vcpu, until a matching vcpu_put()
   */
 -int vcpu_load(struct kvm_vcpu *vcpu)
 +static void __vcpu_load(struct kvm_vcpu *vcpu)
  {
   int cpu;
 
 - if (mutex_lock_killable(vcpu-mutex))
 - return -EINTR;
   if (unlikely(vcpu-pid != current-pids[PIDTYPE_PID].pid)) {
   /* The thread running this VCPU changed. */
   struct pid *oldpid = vcpu-pid;
 @@ -136,6 +134,14 @@ int vcpu_load(struct kvm_vcpu *vcpu)
   preempt_notifier_register(vcpu-preempt_notifier);
   kvm_arch_vcpu_load(vcpu, cpu);
   put_cpu();
 +}
 +
 +int vcpu_load(struct kvm_vcpu *vcpu)
 +{
 + if (mutex_lock_killable(vcpu-mutex))
 + return -EINTR;
 +
 + __vcpu_load(vcpu);
   return 0;
  }
 
 @@ -1989,9 +1995,6 @@ static long kvm_vcpu_ioctl(struct file *filp,
   if (vcpu-kvm-mm != current-mm)
   return -EIO;
 
 - if (unlikely(_IOC_TYPE(ioctl) != KVMIO))
 - return -EINVAL;
 -
  #if defined(CONFIG_S390) || defined(CONFIG_PPC) || defined(CONFIG_MIPS)
   /*
* Special cases: vcpu ioctls that are asynchronous to vcpu execution,
 @@ -2001,8 +2004,21 @@ static long kvm_vcpu_ioctl(struct file *filp,
   return kvm_arch_vcpu_ioctl(filp, ioctl, arg);
  #endif
 
 + if (!mutex_trylock(vcpu-mutex))) {
 + /*
 +  * Before a potentially long sleep, check if we'd exit anyway.
 +  * The common case is for the mutex not to be contended, when
 +  * this does not add overhead.
 +  */
 + if (unlikely(_IOC_TYPE(ioctl) != KVMIO))
 + return -EINVAL;
 +
 + if (mutex_lock_killable(vcpu-mutex))
 + return -EINTR;
 + }
 +
 
 - r = vcpu_load(vcpu);
 + r = __vcpu_load(vcpu);
   if (r)
   return r;
   switch (ioctl) {
 

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] kvm: don't take vcpu mutex for obviously invalid vcpu ioctls

2014-09-22 Thread Paolo Bonzini
Il 22/09/2014 21:20, Christian Borntraeger ha scritto:
 while using trinity to fuzz KVM, we noticed long stalls on invalid ioctls. 
 Lets bail out early on invalid ioctls. or similar?

Okay.  David, can you explain how you found it so that I can make up my
mind?

Gleb and Marcelo, a fourth and fifth opinion? :)

Paolo
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] kvm: don't take vcpu mutex for obviously invalid vcpu ioctls

2014-09-22 Thread David Matlack
On 09/22, Christian Borntraeger wrote:
 On 09/22/2014 04:31 PM, Paolo Bonzini wrote:
  Il 22/09/2014 15:45, Christian Borntraeger ha scritto:
  We now have an extra condition check for every valid ioctl, to make an 
  error case go faster.
  I know, the extra check is just a 1 or 2 cycles if branch prediction is 
  right, but still.
  
  I applied the patch because the delay could be substantial,
 
 I know, but only for seriously misbehaving userspace, no? See my comment is 
 really a minor one - lets say 2 more cycles for something that exited to 
 userspace - nobody would even notice. I am just disturbed by the fact that we 
 care about something that is not slow-path but broken beyond repair (why does 
 userspace call a non-KVM ioctl on a fd of a vcpu from a different thread 
 (otherwise the mutex would be free)?
 
 Please, can we have an explanation, e.g. something like
 while using trinity to fuzz KVM, we noticed long stalls on invalid ioctls. 
 Lets bail out early on invalid ioctls. or similar?

We noticed this while using code that inspects the open file descriptors
of a process. One of the things it did was check if each file descriptor
was a tty (isatty() calls ioctl(TCGETS)).

 
 
  depending on what the other VCPU is doing.
  Perhaps something like this would be
  better?
  
  (Untested, but Tested-by/Reviewed-bys are welcome).
 
 Given that it makes sense to improve a misbehaving userspace, I prefer Davids 
 variant as the patch is smaller and easier to get right. No need to revert, 
 but a better explanation for the use case would be appeciated.
 
 Christian 
  
  Paolo
  
  diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
  index 84e24b210273..ed31760d79fe 100644
  --- a/virt/kvm/kvm_main.c
  +++ b/virt/kvm/kvm_main.c
  @@ -117,12 +117,10 @@ bool kvm_is_mmio_pfn(pfn_t pfn)
   /*
* Switches to specified vcpu, until a matching vcpu_put()
*/
  -int vcpu_load(struct kvm_vcpu *vcpu)
  +static void __vcpu_load(struct kvm_vcpu *vcpu)
   {
  int cpu;
  
  -   if (mutex_lock_killable(vcpu-mutex))
  -   return -EINTR;
  if (unlikely(vcpu-pid != current-pids[PIDTYPE_PID].pid)) {
  /* The thread running this VCPU changed. */
  struct pid *oldpid = vcpu-pid;
  @@ -136,6 +134,14 @@ int vcpu_load(struct kvm_vcpu *vcpu)
  preempt_notifier_register(vcpu-preempt_notifier);
  kvm_arch_vcpu_load(vcpu, cpu);
  put_cpu();
  +}
  +
  +int vcpu_load(struct kvm_vcpu *vcpu)
  +{
  +   if (mutex_lock_killable(vcpu-mutex))
  +   return -EINTR;
  +
  +   __vcpu_load(vcpu);
  return 0;
   }
  
  @@ -1989,9 +1995,6 @@ static long kvm_vcpu_ioctl(struct file *filp,
  if (vcpu-kvm-mm != current-mm)
  return -EIO;
  
  -   if (unlikely(_IOC_TYPE(ioctl) != KVMIO))
  -   return -EINVAL;
  -
   #if defined(CONFIG_S390) || defined(CONFIG_PPC) || defined(CONFIG_MIPS)
  /*
   * Special cases: vcpu ioctls that are asynchronous to vcpu execution,
  @@ -2001,8 +2004,21 @@ static long kvm_vcpu_ioctl(struct file *filp,
  return kvm_arch_vcpu_ioctl(filp, ioctl, arg);
   #endif
  
  +   if (!mutex_trylock(vcpu-mutex))) {
  +   /*
  +* Before a potentially long sleep, check if we'd exit anyway.
  +* The common case is for the mutex not to be contended, when
  +* this does not add overhead.
  +*/
  +   if (unlikely(_IOC_TYPE(ioctl) != KVMIO))
  +   return -EINVAL;
  +
  +   if (mutex_lock_killable(vcpu-mutex))
  +   return -EINTR;
  +   }
  +
  
  -   r = vcpu_load(vcpu);
  +   r = __vcpu_load(vcpu);
  if (r)
  return r;
  switch (ioctl) {
  
 
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] kvm: don't take vcpu mutex for obviously invalid vcpu ioctls

2014-09-22 Thread Marcelo Tosatti
On Fri, Sep 19, 2014 at 04:03:25PM -0700, David Matlack wrote:
 vcpu ioctls can hang the calling thread if issued while a vcpu is
 running. 

There is a mutex per-vcpu, so thats expected, OK...

 If we know ioctl is going to be rejected as invalid anyway,
 we can fail before trying to take the vcpu mutex.

Consider a valid ioctl that takes the vcpu mutex. If you need immediate
access for that valid ioctl, it is necessary to interrupt thread
which KVM_RUN ioctl executes. 

So knowledge of whether KVM_RUN is being executed is expected in
userspace (either
that or ask the KVM_RUN thread to run the ioctl for you, as qemu does).

Can't see why having different behaviour for valid/invalid ioctls
is a good thing.

 This patch does not change functionality, it just makes invalid ioctls
 fail faster.

Should not be executing vcpu ioctls without interrupt KVM_RUN in the
first place.


--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] kvm: don't take vcpu mutex for obviously invalid vcpu ioctls

2014-09-22 Thread Paolo Bonzini
Il 22/09/2014 22:08, Marcelo Tosatti ha scritto:
  This patch does not change functionality, it just makes invalid ioctls
  fail faster.
 
 Should not be executing vcpu ioctls without interrupt KVM_RUN in the
 first place.

This is not entirely true, there are a couple of asynchronous ioctls
though they are not used on x86 (commit 2122ff5eab8f, KVM: move vcpu
locking to dispatcher for generic vcpu ioctls, 2010-05-13).

Paolo
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] kvm: don't take vcpu mutex for obviously invalid vcpu ioctls

2014-09-22 Thread David Matlack
On 09/22, Marcelo Tosatti wrote:
 On Fri, Sep 19, 2014 at 04:03:25PM -0700, David Matlack wrote:
  vcpu ioctls can hang the calling thread if issued while a vcpu is
  running. 
 
 There is a mutex per-vcpu, so thats expected, OK...
 
  If we know ioctl is going to be rejected as invalid anyway,
  we can fail before trying to take the vcpu mutex.
 
 Consider a valid ioctl that takes the vcpu mutex. If you need immediate
 access for that valid ioctl, it is necessary to interrupt thread
 which KVM_RUN ioctl executes. 
 
 So knowledge of whether KVM_RUN is being executed is expected in
 userspace (either
 that or ask the KVM_RUN thread to run the ioctl for you, as qemu does).
 
 Can't see why having different behaviour for valid/invalid ioctls
 is a good thing.
 
  This patch does not change functionality, it just makes invalid ioctls
  fail faster.
 
 Should not be executing vcpu ioctls without interrupt KVM_RUN in the
 first place.

This patch is trying to be nice to code that isn't aware it's
probing kvm file descriptors. We saw long hangs with some generic
process inspection code that was probing all open file descriptors.
There's no reason non-kvm ioctls should have to wait for the vcpu
mutex to become available just to fail.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] kvm: don't take vcpu mutex for obviously invalid vcpu ioctls

2014-09-22 Thread Marcelo Tosatti
On Mon, Sep 22, 2014 at 11:29:16PM +0200, Paolo Bonzini wrote:
 Il 22/09/2014 22:08, Marcelo Tosatti ha scritto:
   This patch does not change functionality, it just makes invalid ioctls
   fail faster.
  
  Should not be executing vcpu ioctls without interrupt KVM_RUN in the
  first place.
 
 This is not entirely true, there are a couple of asynchronous ioctls
 though they are not used on x86 (commit 2122ff5eab8f, KVM: move vcpu
 locking to dispatcher for generic vcpu ioctls, 2010-05-13).
 
 Paolo

Alright. 

s/Should not be executing vcpu ioctls/Should not be executing vcpu
ioctls which take vcpu mutex/


--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] kvm: don't take vcpu mutex for obviously invalid vcpu ioctls

2014-09-22 Thread Marcelo Tosatti
On Mon, Sep 22, 2014 at 03:58:16PM -0700, David Matlack wrote:
  Should not be executing vcpu ioctls without interrupt KVM_RUN in the
  first place.
 
 This patch is trying to be nice to code that isn't aware it's
 probing kvm file descriptors. We saw long hangs with some generic
 process inspection code that was probing all open file descriptors.
 There's no reason non-kvm ioctls should have to wait for the vcpu
 mutex to become available just to fail.

OK then, please add the usecase to the changelog.

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html