Re: [PATCH] KVM: race-free exit from KVM_RUN without POSIX signals

[Paolo Bonzini]

On 16/02/2017 20:26, David Hildenbrand wrote:
> As mentioned already on IRC, maybe something like "block_vcpu_run" would
> fit better now.

Hmm, the purpose of the flag is cause an immediate exit and it does do
so...  Surely incorrect (or just uncommon) usage will prevent a VCPU
from running, but that is just a side effect of the semantics, not the
intended usage.

Paolo

> But this is also ok and looks good to me.
> 
> Reviewed-by: David Hildenbrand 

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: race-free exit from KVM_RUN without POSIX signals

[David Hildenbrand]

>   post_kvm_run_save(vcpu);
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 7964b970b9ad..f51d5082a377 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -218,7 +218,8 @@ struct kvm_hyperv_exit {
>  struct kvm_run {
>   /* in */
>   __u8 request_interrupt_window;
> - __u8 padding1[7];
> + __u8 immediate_exit;

As mentioned already on IRC, maybe something like "block_vcpu_run" would
fit better now.

But this is also ok and looks good to me.

Reviewed-by: David Hildenbrand 

-- 
Thanks,

David
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: race-free exit from KVM_RUN without POSIX signals

[Radim Krčmář]

2017-02-15 15:43+0100, Paolo Bonzini:
> The purpose of the KVM_SET_SIGNAL_MASK API is to let userspace "kick"
> a VCPU out of KVM_RUN through a POSIX signal.  A signal is attached
> to a dummy signal handler; by blocking the signal outside KVM_RUN and
> unblocking it inside, this possible race is closed:
> 
>   VCPU thread service thread
>--
> check flag
>   set flag
>   raise signal
> (signal handler does nothing)
> KVM_RUN
> 
> However, one issue with KVM_SET_SIGNAL_MASK is that it has to take
> tsk->sighand->siglock on every KVM_RUN.  This lock is often on a
> remote NUMA node, because it is on the node of a thread's creator.
> Taking this lock can be very expensive if there are many userspace
> exits (as is the case for SMP Windows VMs without Hyper-V reference
> time counter).
> 
> As an alternative, we can put the flag directly in kvm_run so that
> KVM can see it:
> 
>   VCPU thread service thread
>--
>   raise signal
> signal handler
>   set run->immediate_exit
> KVM_RUN
>   check run->immediate_exit
> 
> Signed-off-by: Paolo Bonzini 
> ---

The old immediate exit with signal did more work, but none of it should
affect user-space, so it looks like another minor optimization,

Reviewed-by: Radim Krčmář 

>   change from RFC:
>   - implement in each architecture to ensure MMIO is completed
> [Radim]
>   - do not clear the flag [David Hildenbrand, offlist]
> 
>  Documentation/virtual/kvm/api.txt | 13 -
>  arch/arm/kvm/arm.c|  4 
>  arch/mips/kvm/mips.c  |  7 ++-
>  arch/powerpc/kvm/powerpc.c|  6 +-
>  arch/s390/kvm/kvm-s390.c  |  4 
>  arch/x86/kvm/x86.c|  6 +-
>  include/uapi/linux/kvm.h  |  4 +++-
>  7 files changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt 
> b/Documentation/virtual/kvm/api.txt
> index e4f2cdcf78eb..925b1b6be073 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -3389,7 +3389,18 @@ struct kvm_run {
>  Request that KVM_RUN return when it becomes possible to inject external
>  interrupts into the guest.  Useful in conjunction with KVM_INTERRUPT.
>  
> - __u8 padding1[7];
> + __u8 immediate_exit;
> +
> +This field is polled once when KVM_RUN starts; if non-zero, KVM_RUN
> +exits immediately, returning -EINTR.  In the common scenario where a
> +signal is used to "kick" a VCPU out of KVM_RUN, this field can be used
> +to avoid usage of KVM_SET_SIGNAL_MASK, which has worse scalability.
> +Rather than blocking the signal outside KVM_RUN, userspace can set up
> +a signal handler that sets run->immediate_exit to a non-zero value.
> +
> +This field is ignored if KVM_CAP_IMMEDIATE_EXIT is not available.
> +
> + __u8 padding1[6];
>  
>   /* out */
>   __u32 exit_reason;
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 21c493a9e5c9..c9a2103faeb9 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -206,6 +206,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long 
> ext)
>   case KVM_CAP_ARM_PSCI_0_2:
>   case KVM_CAP_READONLY_MEM:
>   case KVM_CAP_MP_STATE:
> + case KVM_CAP_IMMEDIATE_EXIT:
>   r = 1;
>   break;
>   case KVM_CAP_COALESCED_MMIO:
> @@ -604,6 +605,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct 
> kvm_run *run)
>   return ret;
>   }
>  
> + if (run->immediate_exit)
> + return -EINTR;
> +
>   if (vcpu->sigset_active)
>   sigprocmask(SIG_SETMASK, >sigset, );
>  
> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
> index 31ee5ee0010b..ed81e5ac1426 100644
> --- a/arch/mips/kvm/mips.c
> +++ b/arch/mips/kvm/mips.c
> @@ -397,7 +397,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu 
> *vcpu,
>  
>  int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  {
> - int r = 0;
> + int r = -EINTR;
>   sigset_t sigsaved;
>  
>   if (vcpu->sigset_active)
> @@ -409,6 +409,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct 
> kvm_run *run)
>   vcpu->mmio_needed = 0;
>   }
>  
> + if (run->immediate_exit)
> + goto out;
> +
>   lose_fpu(1);
>  
>   local_irq_disable();
> @@ -429,6 +432,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct 
> kvm_run *run)
>   guest_exit_irqoff();
>   local_irq_enable();
>  
> +out:
>   if (vcpu->sigset_active)
>   sigprocmask(SIG_SETMASK, , NULL);
>  
> @@ -1021,6 

Re: [PATCH] KVM: race-free exit from KVM_RUN without POSIX signals

[Paolo Bonzini]

On 15/02/2017 16:24, Christian Borntraeger wrote:
> On 02/15/2017 03:43 PM, Paolo Bonzini wrote:
>> The purpose of the KVM_SET_SIGNAL_MASK API is to let userspace "kick"
>> a VCPU out of KVM_RUN through a POSIX signal.  A signal is attached
>> to a dummy signal handler; by blocking the signal outside KVM_RUN and
>> unblocking it inside, this possible race is closed:
>>
>>   VCPU thread service thread
>>--
>> check flag
>>   set flag
>>   raise signal
>> (signal handler does nothing)
>> KVM_RUN
>>
>> However, one issue with KVM_SET_SIGNAL_MASK is that it has to take
>> tsk->sighand->siglock on every KVM_RUN.  This lock is often on a
>> remote NUMA node, because it is on the node of a thread's creator.
>> Taking this lock can be very expensive if there are many userspace
>> exits (as is the case for SMP Windows VMs without Hyper-V reference
>> time counter).
>>
>> As an alternative, we can put the flag directly in kvm_run so that
>> KVM can see it:
>>
>>   VCPU thread service thread
>>--
>>   raise signal
>> signal handler
>>   set run->immediate_exit
>> KVM_RUN
>>   check run->immediate_exit
>>
>> Signed-off-by: Paolo Bonzini 
> 
> 
> Generic parts, the concept and the s390 parts looks good. (not tested yet, 
> though)

Note that this series doesn't work (due to David's suggestion) with the
patches I posted last week.

Paolo
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm