| Message ID | 1411167805-2458-1-git-send-email-dmatlack@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <dmatlack@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <dmatlack@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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 <dmatlack@google.com> --- virt/kvm/kvm_main.c | 4 ++++ 1 file changed, 4 insertions(+)