[PATCH/RFC 0/2] KVM: s390: enable kvm_vpcu_kick/wake_up

[PATCH/RFC 0/2] KVM: s390: enable kvm_vpcu_kick/wake_up

[Christian Borntraeger]

David, I have certainly forgotten something, but I think
this should be good enough for all non-special cases that require
specific CPUs outside of the guest context and only need to handle
a request fast.

Christian Borntraeger (2):
  s390/smp: export smp_send_reschedule
  KVM: enable kvm_vcpu_kick/wake_up for s390

 arch/s390/kernel/smp.c   |  1 +
 arch/s390/kvm/kvm-s390.c | 12 ++++++++++--
 virt/kvm/kvm_main.c      |  2 --
 3 files changed, 11 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH/RFC 1/2] s390/smp: export smp_send_reschedule

[Christian Borntraeger]

needed by KVM common code.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kernel/smp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
index 4c9ebb3..a4d7124 100644
--- a/arch/s390/kernel/smp.c
+++ b/arch/s390/kernel/smp.c
@@ -513,6 +513,7 @@ void smp_send_reschedule(int cpu)
 {
 	pcpu_ec_call(pcpu_devices + cpu, ec_schedule);
 }
+EXPORT_SYMBOL_GPL(smp_send_reschedule);
 
 /*
  * parameter area for the set/clear control bit callbacks
-- 
2.7.4

[PATCH] KVM: add kvm_arch_cpu_kick

[Radim Krčmář]

2017-02-17 14:10+0100, Christian Borntraeger:
> needed by KVM common code.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kernel/smp.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
> index 4c9ebb3..a4d7124 100644
> --- a/arch/s390/kernel/smp.c
> +++ b/arch/s390/kernel/smp.c
> @@ -513,6 +513,7 @@ void smp_send_reschedule(int cpu)
>  {
>  	pcpu_ec_call(pcpu_devices + cpu, ec_schedule);
>  }
> +EXPORT_SYMBOL_GPL(smp_send_reschedule);

s390 doesn't want to use smp_send_reschedule() so I think the patch at
the bottom is going in a better direction.

Btw. I think that we shouldn't be using smp_send_reschedule() for
forcing VCPUs out of guest mode anyway -- the task we want to run is
already scheduled and we don't want the schduler to do anything.

We could use smp_call_function() with an empty function instead, but
that also has high overhead ... allocating a new IPI seems best.

But optimizing the current code is premature. :)

---8<---
s390 has a different mechanism from bringing the VCPU out of guest mode.
Make the kick arch-specific.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/arm/kvm/arm.c         | 5 +++++
 arch/mips/kvm/mips.c       | 5 +++++
 arch/powerpc/kvm/powerpc.c | 5 +++++
 arch/s390/kvm/kvm-s390.c   | 6 ++++++
 arch/x86/kvm/x86.c         | 5 +++++
 include/linux/kvm_host.h   | 1 +
 virt/kvm/kvm_main.c        | 2 +-
 7 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 21c493a9e5c9..a52b0399fa43 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -97,6 +97,11 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
 	return kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE;
 }
 
+void kvm_arch_cpu_kick(int cpu)
+{
+	smp_send_reschedule(cpu);
+}
+
 int kvm_arch_hardware_setup(void)
 {
 	return 0;
diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
index 31ee5ee0010b..8ed510d7f8c5 100644
--- a/arch/mips/kvm/mips.c
+++ b/arch/mips/kvm/mips.c
@@ -78,6 +78,11 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+void kvm_arch_cpu_kick(int cpu)
+{
+	smp_send_reschedule(cpu);
+}
+
 int kvm_arch_hardware_enable(void)
 {
 	return 0;
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index b094d9c1e1ef..b31149e63817 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -60,6 +60,11 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+void kvm_arch_cpu_kick(int cpu)
+{
+	smp_send_reschedule(cpu);
+}
+
 /*
  * Common checks before entering the guest world.  Call with interrupts
  * disabled.
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 8e36734fa5b6..f67ec9796f5e 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2131,6 +2131,12 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
+void kvm_arch_cpu_kick(int cpu)
+{
+	/* TODO: implement kicking with SIE */
+	BUG();
+}
+
 static int kvm_arch_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu,
 					   struct kvm_one_reg *reg)
 {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 19c853f87dd4..d6f40f20c0b5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8405,6 +8405,11 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
 	return kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE;
 }
 
+void kvm_arch_cpu_kick(int cpu)
+{
+	smp_send_reschedule(cpu);
+}
+
 int kvm_arch_interrupt_allowed(struct kvm_vcpu *vcpu)
 {
 	return kvm_x86_ops->interrupt_allowed(vcpu);
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 5438548fec10..9872bd7713f1 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -766,6 +766,7 @@ void kvm_arch_hardware_unsetup(void);
 void kvm_arch_check_processor_compat(void *rtn);
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu);
 int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu);
+void kvm_arch_cpu_kick(int cpu);
 
 void *kvm_kvzalloc(unsigned long size);
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 8d2ef0661ea8..4f4d250e1f53 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2240,7 +2240,7 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
 	me = get_cpu();
 	if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
 		if (kvm_arch_vcpu_should_kick(vcpu))
-			smp_send_reschedule(cpu);
+			kvm_arch_cpu_kick(cpu);
 	put_cpu();
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_kick);
-- 
2.11.1

Re: [PATCH] KVM: add kvm_arch_cpu_kick

[Christian Borntraeger]

On 02/17/2017 04:12 PM, Radim Krčmář wrote:
> 2017-02-17 14:10+0100, Christian Borntraeger:
>> needed by KVM common code.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  arch/s390/kernel/smp.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
>> index 4c9ebb3..a4d7124 100644
>> --- a/arch/s390/kernel/smp.c
>> +++ b/arch/s390/kernel/smp.c
>> @@ -513,6 +513,7 @@ void smp_send_reschedule(int cpu)
>>  {
>>  	pcpu_ec_call(pcpu_devices + cpu, ec_schedule);
>>  }
>> +EXPORT_SYMBOL_GPL(smp_send_reschedule);
> 
> s390 doesn't want to use smp_send_reschedule() so I think the patch at
> the bottom is going in a better direction.
> 
> Btw. I think that we shouldn't be using smp_send_reschedule() for
> forcing VCPUs out of guest mode anyway -- the task we want to run is
> already scheduled and we don't want the schduler to do anything.
> 
> We could use smp_call_function() with an empty function instead, but
> that also has high overhead ... allocating a new IPI seems best.
> 
> But optimizing the current code is premature. :)
> 
> ---8<---
> s390 has a different mechanism from bringing the VCPU out of guest mode.
> Make the kick arch-specific.

Looks good. The kick does not have to be synchronous and its ok if we
reenter the guest as long as we execute the request in a timely manner,
correct?

e.g. 
- kick vcpu
- vcpu enters SIE
- vcpu exits SIE immediately
- vcpu handles request
- vcpu enters SIE

would be perfectly fine?

In that case the s390 implementation could be pretty simple.


> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/arm/kvm/arm.c         | 5 +++++
>  arch/mips/kvm/mips.c       | 5 +++++
>  arch/powerpc/kvm/powerpc.c | 5 +++++
>  arch/s390/kvm/kvm-s390.c   | 6 ++++++
>  arch/x86/kvm/x86.c         | 5 +++++
>  include/linux/kvm_host.h   | 1 +
>  virt/kvm/kvm_main.c        | 2 +-
>  7 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 21c493a9e5c9..a52b0399fa43 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -97,6 +97,11 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
>  	return kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE;
>  }
> 
> +void kvm_arch_cpu_kick(int cpu)
> +{
> +	smp_send_reschedule(cpu);
> +}
> +
>  int kvm_arch_hardware_setup(void)
>  {
>  	return 0;
> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
> index 31ee5ee0010b..8ed510d7f8c5 100644
> --- a/arch/mips/kvm/mips.c
> +++ b/arch/mips/kvm/mips.c
> @@ -78,6 +78,11 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
>  	return 1;
>  }
> 
> +void kvm_arch_cpu_kick(int cpu)
> +{
> +	smp_send_reschedule(cpu);
> +}
> +
>  int kvm_arch_hardware_enable(void)
>  {
>  	return 0;
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index b094d9c1e1ef..b31149e63817 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -60,6 +60,11 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
>  	return 1;
>  }
> 
> +void kvm_arch_cpu_kick(int cpu)
> +{
> +	smp_send_reschedule(cpu);
> +}
> +
>  /*
>   * Common checks before entering the guest world.  Call with interrupts
>   * disabled.
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 8e36734fa5b6..f67ec9796f5e 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2131,6 +2131,12 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
> 
> +void kvm_arch_cpu_kick(int cpu)
> +{
> +	/* TODO: implement kicking with SIE */
> +	BUG();
> +}
> +
>  static int kvm_arch_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu,
>  					   struct kvm_one_reg *reg)
>  {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 19c853f87dd4..d6f40f20c0b5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8405,6 +8405,11 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
>  	return kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE;
>  }
> 
> +void kvm_arch_cpu_kick(int cpu)
> +{
> +	smp_send_reschedule(cpu);
> +}
> +
>  int kvm_arch_interrupt_allowed(struct kvm_vcpu *vcpu)
>  {
>  	return kvm_x86_ops->interrupt_allowed(vcpu);
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 5438548fec10..9872bd7713f1 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -766,6 +766,7 @@ void kvm_arch_hardware_unsetup(void);
>  void kvm_arch_check_processor_compat(void *rtn);
>  int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu);
>  int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu);
> +void kvm_arch_cpu_kick(int cpu);
> 
>  void *kvm_kvzalloc(unsigned long size);
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 8d2ef0661ea8..4f4d250e1f53 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2240,7 +2240,7 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
>  	me = get_cpu();
>  	if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
>  		if (kvm_arch_vcpu_should_kick(vcpu))
> -			smp_send_reschedule(cpu);
> +			kvm_arch_cpu_kick(cpu);
>  	put_cpu();
>  }
>  EXPORT_SYMBOL_GPL(kvm_vcpu_kick);
> 

Re: [PATCH] KVM: add kvm_arch_cpu_kick

[Paolo Bonzini]

On 17/02/2017 16:46, Christian Borntraeger wrote:
> Looks good. The kick does not have to be synchronous and its ok if we
> reenter the guest as long as we execute the request in a timely manner,
> correct?
> 
> e.g. 
> - kick vcpu
> - vcpu enters SIE
> - vcpu exits SIE immediately
> - vcpu handles request
> - vcpu enters SIE
> 
> would be perfectly fine?

Yes, it would.  There's some parallel with QEMU's qemu_cpu_kick, where
the signal would be processed immediately after entering KVM_RUN.

Paolo

Re: [PATCH] KVM: add kvm_arch_cpu_kick

[Christian Borntraeger]

On 02/17/2017 05:23 PM, Paolo Bonzini wrote:
> 
> 
> On 17/02/2017 16:46, Christian Borntraeger wrote:
>> Looks good. The kick does not have to be synchronous and its ok if we
>> reenter the guest as long as we execute the request in a timely manner,
>> correct?
>>
>> e.g. 
>> - kick vcpu
>> - vcpu enters SIE
>> - vcpu exits SIE immediately
>> - vcpu handles request
>> - vcpu enters SIE
>>
>> would be perfectly fine?
> 
> Yes, it would.  There's some parallel with QEMU's qemu_cpu_kick, where
> the signal would be processed immediately after entering KVM_RUN.

Something like 

---snip-----
        struct kvm_s390_sie_block *scb = READ_ONCE(vcpu->arch.vsie_block);

	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
        if (scb)
		atomic_or(CPUSTAT_STOP_INT, &scb->cpuflags);
---snip-----

or 
---snip-----
	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
	kvm_s390_vsie_kick(vcpu);
---snip-----

should be enough then. The code will either delete that stop request when
processing interrupts, but then the requests will be handled afterwards,
or we enter the guest once, exit and process then the requests in the next
loop iteration.

As I am on my way into the weekend this needs double checking.

Re: [PATCH] KVM: add kvm_arch_cpu_kick

[David Hildenbrand]

>> Yes, it would.  There's some parallel with QEMU's qemu_cpu_kick, where
>> the signal would be processed immediately after entering KVM_RUN.
> 
> Something like 
> 
> ---snip-----
>         struct kvm_s390_sie_block *scb = READ_ONCE(vcpu->arch.vsie_block);
> 
> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>         if (scb)
> 		atomic_or(CPUSTAT_STOP_INT, &scb->cpuflags);
> ---snip-----
> 
> or 
> ---snip-----
> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
> 	kvm_s390_vsie_kick(vcpu);
> ---snip-----

I'd go for the latter one. Keep the vsie stuff isolated. Please note
that the vsie loop currently doesn't look for vcpu->requests yet. (I
left it out because its just racy either way and all current requests
don't require to be processes immediately - and it doesn't harm).

> 
> should be enough then. The code will either delete that stop request when
> processing interrupts, but then the requests will be handled afterwards,
> or we enter the guest once, exit and process then the requests in the next
> loop iteration.

-- 
Thanks,

David

Re: [PATCH] KVM: add kvm_arch_cpu_kick

[Christian Borntraeger]

On 02/17/2017 06:10 PM, David Hildenbrand wrote:
> 
>>> Yes, it would.  There's some parallel with QEMU's qemu_cpu_kick, where
>>> the signal would be processed immediately after entering KVM_RUN.
>>
>> Something like 
>>
>> ---snip-----
>>         struct kvm_s390_sie_block *scb = READ_ONCE(vcpu->arch.vsie_block);
>>
>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>         if (scb)
>> 		atomic_or(CPUSTAT_STOP_INT, &scb->cpuflags);
>> ---snip-----
>>
>> or 
>> ---snip-----
>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>> 	kvm_s390_vsie_kick(vcpu);
>> ---snip-----
> 
> I'd go for the latter one. Keep the vsie stuff isolated. Please note

Yes makes sense.

Radim, if you go with this patch something like this can be used as the
s390 variant of kvm_arch_cpu_kick:

---snip---
	/*
	 * The stop indication is reset in the interrupt code. As the CPU
	 * loop handles requests after interrupts, we will
	 * a: miss the request handler and enter the guest, but then the
	 * stop request will exit the CPU and handle the request in the next
	 * round or
	 * b: handle the request directly before entering the guest
	 */
	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
	kvm_s390_vsie_kick(vcpu);

---snip---
feel free to add that to your patch. I can also send a fixup patch later
on if you prefer that.

Christian

Re: [PATCH] KVM: add kvm_arch_cpu_kick

[David Hildenbrand]

Am 20.02.2017 um 12:12 schrieb Christian Borntraeger:
> On 02/17/2017 06:10 PM, David Hildenbrand wrote:
>>
>>>> Yes, it would.  There's some parallel with QEMU's qemu_cpu_kick, where
>>>> the signal would be processed immediately after entering KVM_RUN.
>>>
>>> Something like 
>>>
>>> ---snip-----
>>>         struct kvm_s390_sie_block *scb = READ_ONCE(vcpu->arch.vsie_block);
>>>
>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>>         if (scb)
>>> 		atomic_or(CPUSTAT_STOP_INT, &scb->cpuflags);
>>> ---snip-----
>>>
>>> or 
>>> ---snip-----
>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>> 	kvm_s390_vsie_kick(vcpu);
>>> ---snip-----
>>
>> I'd go for the latter one. Keep the vsie stuff isolated. Please note
> 
> Yes makes sense.
> 
> Radim, if you go with this patch something like this can be used as the
> s390 variant of kvm_arch_cpu_kick:
> 
> ---snip---
> 	/*
> 	 * The stop indication is reset in the interrupt code. As the CPU
> 	 * loop handles requests after interrupts, we will
> 	 * a: miss the request handler and enter the guest, but then the
> 	 * stop request will exit the CPU and handle the request in the next
> 	 * round or
> 	 * b: handle the request directly before entering the guest
> 	 */
> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
> 	kvm_s390_vsie_kick(vcpu);
> 
> ---snip---
> feel free to add that to your patch. I can also send a fixup patch later
> on if you prefer that.

kvm_arch_vcpu_should_kick() then also has to be changed to return 1 for now.

An interesting thing to note is how vcpu->cpu is used.

Again, as s390x can preempt just before entering the guest, vcpu_kick()
might see vcpu->cpu = -1. Therefore, kvm_arch_vcpu_should_kick() won't
even be called. So our cpu might go into guest mode and stay there
longer than expected (as we won't kick it).

On x86, it is the following way:

If vcpu->cpu is -1, no need to kick the VCPU. It will check for requests
when preemption is disabled, therefore when rescheduled.

If vcpu->cpu is set, kvm_arch_vcpu_should_kick() remembers if the VCPU
has already been kicked while in the critical section. It will get
kicked by smp resched as soon as entering guest mode.

So here, disabled preemption + checks in the section with disabled
preemption (for requests and EXITING_GUEST_MODE) make sure that the
guest will leave guest mode and process requests in a timely fashion.

On s390x, this is not 100% true. vcpu->cpu cannot be used as an
indicator whether a kick is necessary. Either that is ok for now, or the
vcpu->cpu != -1 check has to be disabled for s390x, e.g. by moving the
check into kvm_arch_vcpu_should_kick().

-- 
Thanks,

David

Re: [PATCH] KVM: add kvm_arch_cpu_kick

[Radim Krčmář]

2017-02-20 12:35+0100, David Hildenbrand:
> Am 20.02.2017 um 12:12 schrieb Christian Borntraeger:
>> On 02/17/2017 06:10 PM, David Hildenbrand wrote:
>>>
>>>>> Yes, it would.  There's some parallel with QEMU's qemu_cpu_kick, where
>>>>> the signal would be processed immediately after entering KVM_RUN.
>>>>
>>>> Something like 
>>>>
>>>> ---snip-----
>>>>         struct kvm_s390_sie_block *scb = READ_ONCE(vcpu->arch.vsie_block);
>>>>
>>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>>>         if (scb)
>>>> 		atomic_or(CPUSTAT_STOP_INT, &scb->cpuflags);
>>>> ---snip-----
>>>>
>>>> or 
>>>> ---snip-----
>>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>>> 	kvm_s390_vsie_kick(vcpu);
>>>> ---snip-----
>>>
>>> I'd go for the latter one. Keep the vsie stuff isolated. Please note
>> 
>> Yes makes sense.
>> 
>> Radim, if you go with this patch something like this can be used as the
>> s390 variant of kvm_arch_cpu_kick:
>> 
>> ---snip---
>> 	/*
>> 	 * The stop indication is reset in the interrupt code. As the CPU
>> 	 * loop handles requests after interrupts, we will
>> 	 * a: miss the request handler and enter the guest, but then the
>> 	 * stop request will exit the CPU and handle the request in the next
>> 	 * round or
>> 	 * b: handle the request directly before entering the guest
>> 	 */
>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>> 	kvm_s390_vsie_kick(vcpu);
>> 
>> ---snip---
>> feel free to add that to your patch. I can also send a fixup patch later
>> on if you prefer that.
> 
> kvm_arch_vcpu_should_kick() then also has to be changed to return 1 for now.
> 
> An interesting thing to note is how vcpu->cpu is used.
> 
> Again, as s390x can preempt just before entering the guest, vcpu_kick()
> might see vcpu->cpu = -1. Therefore, kvm_arch_vcpu_should_kick() won't
> even be called. So our cpu might go into guest mode and stay there
> longer than expected (as we won't kick it).
> 
> On x86, it is the following way:
> 
> If vcpu->cpu is -1, no need to kick the VCPU. It will check for requests
> when preemption is disabled, therefore when rescheduled.
> 
> If vcpu->cpu is set, kvm_arch_vcpu_should_kick() remembers if the VCPU
> has already been kicked while in the critical section. It will get
> kicked by smp resched as soon as entering guest mode.
> 
> So here, disabled preemption + checks in the section with disabled
> preemption (for requests and EXITING_GUEST_MODE) make sure that the
> guest will leave guest mode and process requests in a timely fashion.
> 
> On s390x, this is not 100% true. vcpu->cpu cannot be used as an
> indicator whether a kick is necessary. Either that is ok for now, or the
> vcpu->cpu != -1 check has to be disabled for s390x, e.g. by moving the
> check into kvm_arch_vcpu_should_kick().

Good point.

So s390 doesn't need vcpu->cpu and only sets it because other arches do?

And do I understand it correctly that the s390 SIE block operations have
no side-effect, apart from changed memory, when outside of guest-mode?
(We have cpu->mode mostly because interrupts are expensive. :])

In the end, I'd like to use kvm_vcpu_kick() for kvm_s390_vcpu_wakeup().
s390 sets vcpu->preempted to get a performance boost, which makes
touching it less than desirable ...
On s390, vcpu->preempted is only used in __diag_time_slice_end(), which
seems to be a type of spinning-on-a-taken-lock hypercall -- any reason
why that optimization shouldn't work on other architectures?

Thanks.

Re: [PATCH] KVM: add kvm_arch_cpu_kick

[Christian Borntraeger]

On 02/20/2017 10:45 PM, Radim Krčmář wrote:
> 2017-02-20 12:35+0100, David Hildenbrand:
>> Am 20.02.2017 um 12:12 schrieb Christian Borntraeger:
>>> On 02/17/2017 06:10 PM, David Hildenbrand wrote:
>>>>
>>>>>> Yes, it would.  There's some parallel with QEMU's qemu_cpu_kick, where
>>>>>> the signal would be processed immediately after entering KVM_RUN.
>>>>>
>>>>> Something like 
>>>>>
>>>>> ---snip-----
>>>>>         struct kvm_s390_sie_block *scb = READ_ONCE(vcpu->arch.vsie_block);
>>>>>
>>>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>>>>         if (scb)
>>>>> 		atomic_or(CPUSTAT_STOP_INT, &scb->cpuflags);
>>>>> ---snip-----
>>>>>
>>>>> or 
>>>>> ---snip-----
>>>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>>>> 	kvm_s390_vsie_kick(vcpu);
>>>>> ---snip-----
>>>>
>>>> I'd go for the latter one. Keep the vsie stuff isolated. Please note
>>>
>>> Yes makes sense.
>>>
>>> Radim, if you go with this patch something like this can be used as the
>>> s390 variant of kvm_arch_cpu_kick:
>>>
>>> ---snip---
>>> 	/*
>>> 	 * The stop indication is reset in the interrupt code. As the CPU
>>> 	 * loop handles requests after interrupts, we will
>>> 	 * a: miss the request handler and enter the guest, but then the
>>> 	 * stop request will exit the CPU and handle the request in the next
>>> 	 * round or
>>> 	 * b: handle the request directly before entering the guest
>>> 	 */
>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>> 	kvm_s390_vsie_kick(vcpu);
>>>
>>> ---snip---
>>> feel free to add that to your patch. I can also send a fixup patch later
>>> on if you prefer that.
>>
>> kvm_arch_vcpu_should_kick() then also has to be changed to return 1 for now.
>>
>> An interesting thing to note is how vcpu->cpu is used.
>>
>> Again, as s390x can preempt just before entering the guest, vcpu_kick()
>> might see vcpu->cpu = -1. Therefore, kvm_arch_vcpu_should_kick() won't
>> even be called. So our cpu might go into guest mode and stay there
>> longer than expected (as we won't kick it).
>>
>> On x86, it is the following way:
>>
>> If vcpu->cpu is -1, no need to kick the VCPU. It will check for requests
>> when preemption is disabled, therefore when rescheduled.
>>
>> If vcpu->cpu is set, kvm_arch_vcpu_should_kick() remembers if the VCPU
>> has already been kicked while in the critical section. It will get
>> kicked by smp resched as soon as entering guest mode.
>>
>> So here, disabled preemption + checks in the section with disabled
>> preemption (for requests and EXITING_GUEST_MODE) make sure that the
>> guest will leave guest mode and process requests in a timely fashion.
>>
>> On s390x, this is not 100% true. vcpu->cpu cannot be used as an
>> indicator whether a kick is necessary. Either that is ok for now, or the
>> vcpu->cpu != -1 check has to be disabled for s390x, e.g. by moving the
>> check into kvm_arch_vcpu_should_kick().
> 
> Good point.
> 
> So s390 doesn't need vcpu->cpu and only sets it because other arches do?

David added it as a sanity check for cpu time accounting while in host. But
we do not need it, yes.


> And do I understand it correctly that the s390 SIE block operations have
> no side-effect, apart from changed memory, when outside of guest-mode?

You mean accesses to the sie control block vcpu->arch.sie_block? Yes its just
changed memory as long as the VCPU that is backed by this sie control block
is not running.


> (We have cpu->mode mostly because interrupts are expensive. :])
> 
> In the end, I'd like to use kvm_vcpu_kick() for kvm_s390_vcpu_wakeup().

something like this?

--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -1067,15 +1067,7 @@ void kvm_s390_vcpu_wakeup(struct kvm_vcpu *vcpu)
         * in kvm_vcpu_block without having the waitqueue set (polling)
         */
        vcpu->valid_wakeup = true;
-       if (swait_active(&vcpu->wq)) {
-               /*
-                * The vcpu gave up the cpu voluntarily, mark it as a good
-                * yield-candidate.
-                */
-               vcpu->preempted = true;
-               swake_up(&vcpu->wq);
-               vcpu->stat.halt_wakeup++;
-       }
+       kvm_vcpu_wake_up(vcpu);
        /*
         * The VCPU might not be sleeping but is executing the VSIE. Let's
         * kick it, so it leaves the SIE to process the request.
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2213,6 +2213,7 @@ void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
        wqp = kvm_arch_vcpu_wq(vcpu);
        if (swait_active(wqp)) {
                swake_up(wqp);
+               vcpu->preempted = true;
                ++vcpu->stat.halt_wakeup;
        }
 


> s390 sets vcpu->preempted to get a performance boost, which makes
> touching it less than desirable ...
> On s390, vcpu->preempted is only used in __diag_time_slice_end(), which
> seems to be a type of spinning-on-a-taken-lock hypercall -- any reason
> why that optimization shouldn't work on other architectures?

We set preempted in kvm_s390_vcpu_wakeup because otherwise a cpu that sleeps
(halted) would not be considered as a good candidate in kvm_vcpu_on_spin,
even if we just decided to wakeup that CPU for an interrupt.

Yes, it certainly makes sense to do that in kvm_vcpu_wake_up as well.

Re: [PATCH] KVM: add kvm_arch_cpu_kick

[Radim Krčmář]

2017-02-21 09:59+0100, Christian Borntraeger:
> On 02/20/2017 10:45 PM, Radim Krčmář wrote:
>> 2017-02-20 12:35+0100, David Hildenbrand:
>>> Am 20.02.2017 um 12:12 schrieb Christian Borntraeger:
>>>> On 02/17/2017 06:10 PM, David Hildenbrand wrote:
>>>>>
>>>>>>> Yes, it would.  There's some parallel with QEMU's qemu_cpu_kick, where
>>>>>>> the signal would be processed immediately after entering KVM_RUN.
>>>>>>
>>>>>> Something like 
>>>>>>
>>>>>> ---snip-----
>>>>>>         struct kvm_s390_sie_block *scb = READ_ONCE(vcpu->arch.vsie_block);
>>>>>>
>>>>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>>>>>         if (scb)
>>>>>> 		atomic_or(CPUSTAT_STOP_INT, &scb->cpuflags);
>>>>>> ---snip-----
>>>>>>
>>>>>> or 
>>>>>> ---snip-----
>>>>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>>>>> 	kvm_s390_vsie_kick(vcpu);
>>>>>> ---snip-----
>>>>>
>>>>> I'd go for the latter one. Keep the vsie stuff isolated. Please note
>>>>
>>>> Yes makes sense.
>>>>
>>>> Radim, if you go with this patch something like this can be used as the
>>>> s390 variant of kvm_arch_cpu_kick:
>>>>
>>>> ---snip---
>>>> 	/*
>>>> 	 * The stop indication is reset in the interrupt code. As the CPU
>>>> 	 * loop handles requests after interrupts, we will
>>>> 	 * a: miss the request handler and enter the guest, but then the
>>>> 	 * stop request will exit the CPU and handle the request in the next
>>>> 	 * round or
>>>> 	 * b: handle the request directly before entering the guest
>>>> 	 */
>>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>>> 	kvm_s390_vsie_kick(vcpu);
>>>>
>>>> ---snip---
>>>> feel free to add that to your patch. I can also send a fixup patch later
>>>> on if you prefer that.
>>>
>>> kvm_arch_vcpu_should_kick() then also has to be changed to return 1 for now.
>>>
>>> An interesting thing to note is how vcpu->cpu is used.
>>>
>>> Again, as s390x can preempt just before entering the guest, vcpu_kick()
>>> might see vcpu->cpu = -1. Therefore, kvm_arch_vcpu_should_kick() won't
>>> even be called. So our cpu might go into guest mode and stay there
>>> longer than expected (as we won't kick it).
>>>
>>> On x86, it is the following way:
>>>
>>> If vcpu->cpu is -1, no need to kick the VCPU. It will check for requests
>>> when preemption is disabled, therefore when rescheduled.
>>>
>>> If vcpu->cpu is set, kvm_arch_vcpu_should_kick() remembers if the VCPU
>>> has already been kicked while in the critical section. It will get
>>> kicked by smp resched as soon as entering guest mode.
>>>
>>> So here, disabled preemption + checks in the section with disabled
>>> preemption (for requests and EXITING_GUEST_MODE) make sure that the
>>> guest will leave guest mode and process requests in a timely fashion.
>>>
>>> On s390x, this is not 100% true. vcpu->cpu cannot be used as an
>>> indicator whether a kick is necessary. Either that is ok for now, or the
>>> vcpu->cpu != -1 check has to be disabled for s390x, e.g. by moving the
>>> check into kvm_arch_vcpu_should_kick().
>> 
>> Good point.
>> 
>> So s390 doesn't need vcpu->cpu and only sets it because other arches do?
> 
> David added it as a sanity check for cpu time accounting while in host. But
> we do not need it, yes.
> 
>> And do I understand it correctly that the s390 SIE block operations have
>> no side-effect, apart from changed memory, when outside of guest-mode?
> 
> You mean accesses to the sie control block vcpu->arch.sie_block? Yes its just
> changed memory as long as the VCPU that is backed by this sie control block
> is not running.

Great, thanks.

>> (We have cpu->mode mostly because interrupts are expensive. :])
>> 
>> In the end, I'd like to use kvm_vcpu_kick() for kvm_s390_vcpu_wakeup().
> 
> something like this?
> 
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -1067,15 +1067,7 @@ void kvm_s390_vcpu_wakeup(struct kvm_vcpu *vcpu)
>          * in kvm_vcpu_block without having the waitqueue set (polling)
>          */
>         vcpu->valid_wakeup = true;
> -       if (swait_active(&vcpu->wq)) {
> -               /*
> -                * The vcpu gave up the cpu voluntarily, mark it as a good
> -                * yield-candidate.
> -                */
> -               vcpu->preempted = true;
> -               swake_up(&vcpu->wq);
> -               vcpu->stat.halt_wakeup++;
> -       }
> +       kvm_vcpu_wake_up(vcpu);
>         /*
>          * The VCPU might not be sleeping but is executing the VSIE. Let's
>          * kick it, so it leaves the SIE to process the request.

Yes, and ideally also covering the SIE kick, so the result would be

  {
  	vcpu->valid_wakeup = true;
 +	kvm_vcpu_kick(vcpu);
  }

> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2213,6 +2213,7 @@ void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
>         wqp = kvm_arch_vcpu_wq(vcpu);
>         if (swait_active(wqp)) {
>                 swake_up(wqp);
> +               vcpu->preempted = true;
> 
>                 ++vcpu->stat.halt_wakeup;
>         }
> 
>> s390 sets vcpu->preempted to get a performance boost, which makes
>> touching it less than desirable ...
>> On s390, vcpu->preempted is only used in __diag_time_slice_end(), which
>> seems to be a type of spinning-on-a-taken-lock hypercall -- any reason
>> why that optimization shouldn't work on other architectures?
> 
> We set preempted in kvm_s390_vcpu_wakeup because otherwise a cpu that sleeps
> (halted) would not be considered as a good candidate in kvm_vcpu_on_spin,
> even if we just decided to wakeup that CPU for an interrupt.
> 
> Yes, it certainly makes sense to do that in kvm_vcpu_wake_up as well.

I assume that s390 doesn't go to sleep while holding a spinlock, so it
would mean that we need to update kvm_vcpu_on_spin().
(Ignoring a formerly sleeping VCPU as a candidate made sense: the VCPU
 shouldn't have been holding a lock that is blocking the spinning VCPU.)

Boosting a VCPU that is likely not going to use the contended spinlock
seems like a good thing to try.  I think we don't need vcpu->preempted
in its current form then.  After the change, it it would be false only
in two cases:
 1) the VCPU task is currently scheduled on some CPU
 2) the VCPU is sleeping

There might be some other way know (1) (or we can adapt vcpu->preempted
to vcpu->scheduled) and (2) is done with swait_active().

Sadly, any change of kvm_vcpu_on_spin() is going to need many days of
testing ...

Re: [PATCH] KVM: add kvm_arch_cpu_kick

[Christian Borntraeger]

On 02/21/2017 06:15 PM, Radim Krčmář wrote:
> 2017-02-21 09:59+0100, Christian Borntraeger:
>> On 02/20/2017 10:45 PM, Radim Krčmář wrote:
>>> 2017-02-20 12:35+0100, David Hildenbrand:
>>>> Am 20.02.2017 um 12:12 schrieb Christian Borntraeger:
>>>>> On 02/17/2017 06:10 PM, David Hildenbrand wrote:
>>>>>>
>>>>>>>> Yes, it would.  There's some parallel with QEMU's qemu_cpu_kick, where
>>>>>>>> the signal would be processed immediately after entering KVM_RUN.
>>>>>>>
>>>>>>> Something like 
>>>>>>>
>>>>>>> ---snip-----
>>>>>>>         struct kvm_s390_sie_block *scb = READ_ONCE(vcpu->arch.vsie_block);
>>>>>>>
>>>>>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>>>>>>         if (scb)
>>>>>>> 		atomic_or(CPUSTAT_STOP_INT, &scb->cpuflags);
>>>>>>> ---snip-----
>>>>>>>
>>>>>>> or 
>>>>>>> ---snip-----
>>>>>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>>>>>> 	kvm_s390_vsie_kick(vcpu);
>>>>>>> ---snip-----
>>>>>>
>>>>>> I'd go for the latter one. Keep the vsie stuff isolated. Please note
>>>>>
>>>>> Yes makes sense.
>>>>>
>>>>> Radim, if you go with this patch something like this can be used as the
>>>>> s390 variant of kvm_arch_cpu_kick:
>>>>>
>>>>> ---snip---
>>>>> 	/*
>>>>> 	 * The stop indication is reset in the interrupt code. As the CPU
>>>>> 	 * loop handles requests after interrupts, we will
>>>>> 	 * a: miss the request handler and enter the guest, but then the
>>>>> 	 * stop request will exit the CPU and handle the request in the next
>>>>> 	 * round or
>>>>> 	 * b: handle the request directly before entering the guest
>>>>> 	 */
>>>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>>>> 	kvm_s390_vsie_kick(vcpu);
>>>>>
>>>>> ---snip---
>>>>> feel free to add that to your patch. I can also send a fixup patch later
>>>>> on if you prefer that.
>>>>
>>>> kvm_arch_vcpu_should_kick() then also has to be changed to return 1 for now.
>>>>
>>>> An interesting thing to note is how vcpu->cpu is used.
>>>>
>>>> Again, as s390x can preempt just before entering the guest, vcpu_kick()
>>>> might see vcpu->cpu = -1. Therefore, kvm_arch_vcpu_should_kick() won't
>>>> even be called. So our cpu might go into guest mode and stay there
>>>> longer than expected (as we won't kick it).
>>>>
>>>> On x86, it is the following way:
>>>>
>>>> If vcpu->cpu is -1, no need to kick the VCPU. It will check for requests
>>>> when preemption is disabled, therefore when rescheduled.
>>>>
>>>> If vcpu->cpu is set, kvm_arch_vcpu_should_kick() remembers if the VCPU
>>>> has already been kicked while in the critical section. It will get
>>>> kicked by smp resched as soon as entering guest mode.
>>>>
>>>> So here, disabled preemption + checks in the section with disabled
>>>> preemption (for requests and EXITING_GUEST_MODE) make sure that the
>>>> guest will leave guest mode and process requests in a timely fashion.
>>>>
>>>> On s390x, this is not 100% true. vcpu->cpu cannot be used as an
>>>> indicator whether a kick is necessary. Either that is ok for now, or the
>>>> vcpu->cpu != -1 check has to be disabled for s390x, e.g. by moving the
>>>> check into kvm_arch_vcpu_should_kick().
>>>
>>> Good point.
>>>
>>> So s390 doesn't need vcpu->cpu and only sets it because other arches do?
>>
>> David added it as a sanity check for cpu time accounting while in host. But
>> we do not need it, yes.
>>
>>> And do I understand it correctly that the s390 SIE block operations have
>>> no side-effect, apart from changed memory, when outside of guest-mode?
>>
>> You mean accesses to the sie control block vcpu->arch.sie_block? Yes its just
>> changed memory as long as the VCPU that is backed by this sie control block
>> is not running.
> 
> Great, thanks.
> 
>>> (We have cpu->mode mostly because interrupts are expensive. :])
>>>
>>> In the end, I'd like to use kvm_vcpu_kick() for kvm_s390_vcpu_wakeup().
>>
>> something like this?
>>
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -1067,15 +1067,7 @@ void kvm_s390_vcpu_wakeup(struct kvm_vcpu *vcpu)
>>          * in kvm_vcpu_block without having the waitqueue set (polling)
>>          */
>>         vcpu->valid_wakeup = true;
>> -       if (swait_active(&vcpu->wq)) {
>> -               /*
>> -                * The vcpu gave up the cpu voluntarily, mark it as a good
>> -                * yield-candidate.
>> -                */
>> -               vcpu->preempted = true;
>> -               swake_up(&vcpu->wq);
>> -               vcpu->stat.halt_wakeup++;
>> -       }
>> +       kvm_vcpu_wake_up(vcpu);
>>         /*
>>          * The VCPU might not be sleeping but is executing the VSIE. Let's
>>          * kick it, so it leaves the SIE to process the request.
> 
> Yes, and ideally also covering the SIE kick, so the result would be
> 
>   {
>   	vcpu->valid_wakeup = true;
>  +	kvm_vcpu_kick(vcpu);
>   }
> 

No we do not want to replace kvm_s390_vcpu_wakeup with the generic SIE kick. the
stop exit will exit the guest unconditionally. As kvm_s390_vcpu_wakeup is also called 
after queuing interrupts we want to use the special exits for the different interrupt
types that wait with the guest exits until that interrupt is enabled by the guest. (e.g.
not when running in local_irq_disable)
So I think it does not make sense to replace this with the generic kvm_vcpu_wake_up
if we need the kick.


>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -2213,6 +2213,7 @@ void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
>>         wqp = kvm_arch_vcpu_wq(vcpu);
>>         if (swait_active(wqp)) {
>>                 swake_up(wqp);
>> +               vcpu->preempted = true;
>>
>>                 ++vcpu->stat.halt_wakeup;
>>         }
>>
>>> s390 sets vcpu->preempted to get a performance boost, which makes
>>> touching it less than desirable ...
>>> On s390, vcpu->preempted is only used in __diag_time_slice_end(), which
>>> seems to be a type of spinning-on-a-taken-lock hypercall -- any reason
>>> why that optimization shouldn't work on other architectures?
>>
>> We set preempted in kvm_s390_vcpu_wakeup because otherwise a cpu that sleeps
>> (halted) would not be considered as a good candidate in kvm_vcpu_on_spin,
>> even if we just decided to wakeup that CPU for an interrupt.
>>
>> Yes, it certainly makes sense to do that in kvm_vcpu_wake_up as well.
> 
> I assume that s390 doesn't go to sleep while holding a spinlock, so it

Yes x86 and s390 do not sleep in atomic contexts. Both spin for a while 
and exit after some time into kvm_vcpu_on_on (*) We try to find a good 
candidate for yielding and then sleep due to the yield. But a normal 
spinlock does not cause a halt (or a wait PSW).

(*) all modern s390 Linuxes now use the directed yield hypercall for
spinlocks (see diag 9c) so the diag 44 is only relevant for stop machine
run.

> would mean that we need to update kvm_vcpu_on_spin().
> (Ignoring a formerly sleeping VCPU as a candidate made sense: the VCPU
>  shouldn't have been holding a lock that is blocking the spinning VCPU.)

Yes, if the yielding wants to boost the lock holder, indeed ignoring
a sleeping cpu sounds like a good idea. In reality things are more complicated
and lock holder preemption is especially nasty with many VCPUs. In that case
we want to also boost these CPUs that are delivering interrupts.
We had several performance issues with some workloads that were fixed by this
in commit 9cac38dd5dc41c943d711b96f9755a29c8b854ea
    KVM/s390: Set preempted flag during vcpu wakeup and interrupt delivery

so it turned out to be better this way for a semop intense workload back then.

 
> Boosting a VCPU that is likely not going to use the contended spinlock
> seems like a good thing to try.  I think we don't need vcpu->preempted
> in its current form then.  After the change, it it would be false only
> in two cases:
>  1) the VCPU task is currently scheduled on some CPU
>  2) the VCPU is sleeping
> 
> There might be some other way know (1) (or we can adapt vcpu->preempted
> to vcpu->scheduled) and (2) is done with swait_active().
> 
> Sadly, any change of kvm_vcpu_on_spin() is going to need many days of
> testing ...

Yes, this requires a lot of tuning. Maybe we should just stick with the
minimal changes then?

Re: [PATCH] KVM: add kvm_arch_cpu_kick

[Radim Krčmář]

2017-02-21 20:08+0100, Christian Borntraeger:
> On 02/21/2017 06:15 PM, Radim Krčmář wrote:
>> 2017-02-21 09:59+0100, Christian Borntraeger:
>>> On 02/20/2017 10:45 PM, Radim Krčmář wrote:
>>>> 2017-02-20 12:35+0100, David Hildenbrand:
>>>>> Am 20.02.2017 um 12:12 schrieb Christian Borntraeger:
>>>>>> On 02/17/2017 06:10 PM, David Hildenbrand wrote:
>>>>>>>
>>>>>>>>> Yes, it would.  There's some parallel with QEMU's qemu_cpu_kick, where
>>>>>>>>> the signal would be processed immediately after entering KVM_RUN.
>>>>>>>>
>>>>>>>> Something like 
>>>>>>>>
>>>>>>>> ---snip-----
>>>>>>>>         struct kvm_s390_sie_block *scb = READ_ONCE(vcpu->arch.vsie_block);
>>>>>>>>
>>>>>>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>>>>>>>         if (scb)
>>>>>>>> 		atomic_or(CPUSTAT_STOP_INT, &scb->cpuflags);
>>>>>>>> ---snip-----
>>>>>>>>
>>>>>>>> or 
>>>>>>>> ---snip-----
>>>>>>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>>>>>>> 	kvm_s390_vsie_kick(vcpu);
>>>>>>>> ---snip-----
>>>>>>>
>>>>>>> I'd go for the latter one. Keep the vsie stuff isolated. Please note
>>>>>>
>>>>>> Yes makes sense.
>>>>>>
>>>>>> Radim, if you go with this patch something like this can be used as the
>>>>>> s390 variant of kvm_arch_cpu_kick:
>>>>>>
>>>>>> ---snip---
>>>>>> 	/*
>>>>>> 	 * The stop indication is reset in the interrupt code. As the CPU
>>>>>> 	 * loop handles requests after interrupts, we will
>>>>>> 	 * a: miss the request handler and enter the guest, but then the
>>>>>> 	 * stop request will exit the CPU and handle the request in the next
>>>>>> 	 * round or
>>>>>> 	 * b: handle the request directly before entering the guest
>>>>>> 	 */
>>>>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>>>>> 	kvm_s390_vsie_kick(vcpu);
>>>>>>
>>>>>> ---snip---
>>>>>> feel free to add that to your patch. I can also send a fixup patch later
>>>>>> on if you prefer that.
>>>>>
>>>>> kvm_arch_vcpu_should_kick() then also has to be changed to return 1 for now.
>>>>>
>>>>> An interesting thing to note is how vcpu->cpu is used.
>>>>>
>>>>> Again, as s390x can preempt just before entering the guest, vcpu_kick()
>>>>> might see vcpu->cpu = -1. Therefore, kvm_arch_vcpu_should_kick() won't
>>>>> even be called. So our cpu might go into guest mode and stay there
>>>>> longer than expected (as we won't kick it).
>>>>>
>>>>> On x86, it is the following way:
>>>>>
>>>>> If vcpu->cpu is -1, no need to kick the VCPU. It will check for requests
>>>>> when preemption is disabled, therefore when rescheduled.
>>>>>
>>>>> If vcpu->cpu is set, kvm_arch_vcpu_should_kick() remembers if the VCPU
>>>>> has already been kicked while in the critical section. It will get
>>>>> kicked by smp resched as soon as entering guest mode.
>>>>>
>>>>> So here, disabled preemption + checks in the section with disabled
>>>>> preemption (for requests and EXITING_GUEST_MODE) make sure that the
>>>>> guest will leave guest mode and process requests in a timely fashion.
>>>>>
>>>>> On s390x, this is not 100% true. vcpu->cpu cannot be used as an
>>>>> indicator whether a kick is necessary. Either that is ok for now, or the
>>>>> vcpu->cpu != -1 check has to be disabled for s390x, e.g. by moving the
>>>>> check into kvm_arch_vcpu_should_kick().
>>>>
>>>> Good point.
>>>>
>>>> So s390 doesn't need vcpu->cpu and only sets it because other arches do?
>>>
>>> David added it as a sanity check for cpu time accounting while in host. But
>>> we do not need it, yes.
>>>
>>>> And do I understand it correctly that the s390 SIE block operations have
>>>> no side-effect, apart from changed memory, when outside of guest-mode?
>>>
>>> You mean accesses to the sie control block vcpu->arch.sie_block? Yes its just
>>> changed memory as long as the VCPU that is backed by this sie control block
>>> is not running.
>> 
>> Great, thanks.
>> 
>>>> (We have cpu->mode mostly because interrupts are expensive. :])
>>>>
>>>> In the end, I'd like to use kvm_vcpu_kick() for kvm_s390_vcpu_wakeup().
>>>
>>> something like this?
>>>
>>> --- a/arch/s390/kvm/interrupt.c
>>> +++ b/arch/s390/kvm/interrupt.c
>>> @@ -1067,15 +1067,7 @@ void kvm_s390_vcpu_wakeup(struct kvm_vcpu *vcpu)
>>>          * in kvm_vcpu_block without having the waitqueue set (polling)
>>>          */
>>>         vcpu->valid_wakeup = true;
>>> -       if (swait_active(&vcpu->wq)) {
>>> -               /*
>>> -                * The vcpu gave up the cpu voluntarily, mark it as a good
>>> -                * yield-candidate.
>>> -                */
>>> -               vcpu->preempted = true;
>>> -               swake_up(&vcpu->wq);
>>> -               vcpu->stat.halt_wakeup++;
>>> -       }
>>> +       kvm_vcpu_wake_up(vcpu);
>>>         /*
>>>          * The VCPU might not be sleeping but is executing the VSIE. Let's
>>>          * kick it, so it leaves the SIE to process the request.
>> 
>> Yes, and ideally also covering the SIE kick, so the result would be
>> 
>>   {
>>   	vcpu->valid_wakeup = true;
>>  +	kvm_vcpu_kick(vcpu);
>>   }
>> 
> 
> No we do not want to replace kvm_s390_vcpu_wakeup with the generic SIE kick. the
> stop exit will exit the guest unconditionally. As kvm_s390_vcpu_wakeup is also called 
> after queuing interrupts we want to use the special exits for the different interrupt
> types that wait with the guest exits until that interrupt is enabled by the guest. (e.g.
> not when running in local_irq_disable)
> So I think it does not make sense to replace this with the generic kvm_vcpu_wake_up
> if we need the kick.

Makes sense, thanks.  I was misunderstanding the vsie kick and its
users.

>>> --- a/virt/kvm/kvm_main.c
>>> +++ b/virt/kvm/kvm_main.c
>>> @@ -2213,6 +2213,7 @@ void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
>>>         wqp = kvm_arch_vcpu_wq(vcpu);
>>>         if (swait_active(wqp)) {
>>>                 swake_up(wqp);
>>> +               vcpu->preempted = true;
>>>
>>>                 ++vcpu->stat.halt_wakeup;
>>>         }
>>>
>>>> s390 sets vcpu->preempted to get a performance boost, which makes
>>>> touching it less than desirable ...
>>>> On s390, vcpu->preempted is only used in __diag_time_slice_end(), which
>>>> seems to be a type of spinning-on-a-taken-lock hypercall -- any reason
>>>> why that optimization shouldn't work on other architectures?
>>>
>>> We set preempted in kvm_s390_vcpu_wakeup because otherwise a cpu that sleeps
>>> (halted) would not be considered as a good candidate in kvm_vcpu_on_spin,
>>> even if we just decided to wakeup that CPU for an interrupt.
>>>
>>> Yes, it certainly makes sense to do that in kvm_vcpu_wake_up as well.
>> 
>> I assume that s390 doesn't go to sleep while holding a spinlock, so it
> 
> Yes x86 and s390 do not sleep in atomic contexts. Both spin for a while 
> and exit after some time into kvm_vcpu_on_on (*) We try to find a good 
> candidate for yielding and then sleep due to the yield. But a normal 
> spinlock does not cause a halt (or a wait PSW).
> 
> (*) all modern s390 Linuxes now use the directed yield hypercall for
> spinlocks (see diag 9c) so the diag 44 is only relevant for stop machine
> run.
> 
>> would mean that we need to update kvm_vcpu_on_spin().
>> (Ignoring a formerly sleeping VCPU as a candidate made sense: the VCPU
>>  shouldn't have been holding a lock that is blocking the spinning VCPU.)
> 
> Yes, if the yielding wants to boost the lock holder, indeed ignoring
> a sleeping cpu sounds like a good idea. In reality things are more complicated
> and lock holder preemption is especially nasty with many VCPUs. In that case
> we want to also boost these CPUs that are delivering interrupts.
> We had several performance issues with some workloads that were fixed by this
> in commit 9cac38dd5dc41c943d711b96f9755a29c8b854ea
>     KVM/s390: Set preempted flag during vcpu wakeup and interrupt delivery
> 
> so it turned out to be better this way for a semop intense workload back then.
> 
>  
>> Boosting a VCPU that is likely not going to use the contended spinlock
>> seems like a good thing to try.  I think we don't need vcpu->preempted
>> in its current form then.  After the change, it it would be false only
>> in two cases:
>>  1) the VCPU task is currently scheduled on some CPU
>>  2) the VCPU is sleeping
>> 
>> There might be some other way know (1) (or we can adapt vcpu->preempted
>> to vcpu->scheduled) and (2) is done with swait_active().
>> 
>> Sadly, any change of kvm_vcpu_on_spin() is going to need many days of
>> testing ...
> 
> Yes, this requires a lot of tuning. Maybe we should just stick with the
> minimal changes then?

Any changes would need the same testing ... I could start with no
changes :)

  if (kvm_vcpu_wake_up(vcpu))
  	vcpu->preempted = true;

Finding this has made the rewrite useful much earlier than I hoped,
though you mentioned that s390 doesn't use it all that much and x86 is
also using directed kicks for blocked locks.

Re: [PATCH] KVM: add kvm_arch_cpu_kick

[Radim Krčmář]

2017-02-20 12:12+0100, Christian Borntraeger:
> On 02/17/2017 06:10 PM, David Hildenbrand wrote:
>> 
>>>> Yes, it would.  There's some parallel with QEMU's qemu_cpu_kick, where
>>>> the signal would be processed immediately after entering KVM_RUN.
>>>
>>> Something like 
>>>
>>> ---snip-----
>>>         struct kvm_s390_sie_block *scb = READ_ONCE(vcpu->arch.vsie_block);
>>>
>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>>         if (scb)
>>> 		atomic_or(CPUSTAT_STOP_INT, &scb->cpuflags);
>>> ---snip-----
>>>
>>> or 
>>> ---snip-----
>>> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
>>> 	kvm_s390_vsie_kick(vcpu);
>>> ---snip-----
>> 
>> I'd go for the latter one. Keep the vsie stuff isolated. Please note
> 
> Yes makes sense.
> 
> Radim, if you go with this patch something like this can be used as the
> s390 variant of kvm_arch_cpu_kick:
> 
> ---snip---
> 	/*
> 	 * The stop indication is reset in the interrupt code. As the CPU
> 	 * loop handles requests after interrupts, we will
> 	 * a: miss the request handler and enter the guest, but then the
> 	 * stop request will exit the CPU and handle the request in the next
> 	 * round or
> 	 * b: handle the request directly before entering the guest
> 	 */
> 	atomic_or(CPUSTAT_STOP_INT, &vcpu->arch.sie_block->cpuflags);
> 	kvm_s390_vsie_kick(vcpu);
> 
> ---snip---
> feel free to add that to your patch. I can also send a fixup patch later
> on if you prefer that.

I'll probably use it.  Just looking at the hunk made me realize that the
interface will need to pass the vcpu as well, so I'd be best to have the
whole picture.

Thanks.

Re: [PATCH] KVM: add kvm_arch_cpu_kick

[David Hildenbrand]

Am 17.02.2017 um 16:12 schrieb Radim Krčmář:
> 2017-02-17 14:10+0100, Christian Borntraeger:
>> needed by KVM common code.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  arch/s390/kernel/smp.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
>> index 4c9ebb3..a4d7124 100644
>> --- a/arch/s390/kernel/smp.c
>> +++ b/arch/s390/kernel/smp.c
>> @@ -513,6 +513,7 @@ void smp_send_reschedule(int cpu)
>>  {
>>  	pcpu_ec_call(pcpu_devices + cpu, ec_schedule);
>>  }
>> +EXPORT_SYMBOL_GPL(smp_send_reschedule);
> 
> s390 doesn't want to use smp_send_reschedule() so I think the patch at
> the bottom is going in a better direction.
> 
> Btw. I think that we shouldn't be using smp_send_reschedule() for
> forcing VCPUs out of guest mode anyway -- the task we want to run is
> already scheduled and we don't want the schduler to do anything.
> 
> We could use smp_call_function() with an empty function instead, but
> that also has high overhead ... allocating a new IPI seems best.
> 
> But optimizing the current code is premature. :)
> 
> ---8<---
> s390 has a different mechanism from bringing the VCPU out of guest mode.
> Make the kick arch-specific.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/arm/kvm/arm.c         | 5 +++++
>  arch/mips/kvm/mips.c       | 5 +++++
>  arch/powerpc/kvm/powerpc.c | 5 +++++
>  arch/s390/kvm/kvm-s390.c   | 6 ++++++
>  arch/x86/kvm/x86.c         | 5 +++++
>  include/linux/kvm_host.h   | 1 +
>  virt/kvm/kvm_main.c        | 2 +-
>  7 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 21c493a9e5c9..a52b0399fa43 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -97,6 +97,11 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
>  	return kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE;
>  }
>  

That's what I had in mind. Looks good.

-- 
Thanks,

David

[PATCH/RFC 2/2] KVM: enable kvm_vcpu_kick/wake_up for s390

[Christian Borntraeger]

while we still need to provide our private version of kick and wakeup
to synchronize with some special cases, we can still enable the common
code variant.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 12 ++++++++++--
 virt/kvm/kvm_main.c      |  2 --
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 7cfd0dd..fa20686 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2485,8 +2485,14 @@ static void kvm_gmap_notifier(struct gmap *gmap, unsigned long start,
 
 int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
 {
-	/* kvm common code refers to this, but never calls it */
-	BUG();
+	/*
+	 * STOP indication is resetted when delivering interrupts. This
+	 * is done before we handle requests, so we only "loose" this
+	 * when we are still going to handle requests. In that case
+	 * we no longer need that STOP indication.
+	 */
+	__set_cpuflag(vcpu, CPUSTAT_STOP_INT);
+	kvm_s390_vsie_kick(vcpu);
 	return 0;
 }
 
@@ -3064,6 +3070,7 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 	 */
 	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
 
+	vcpu->mode = IN_GUEST_MODE;
 	do {
 		rc = vcpu_pre_run(vcpu);
 		if (rc)
@@ -3088,6 +3095,7 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 
 		rc = vcpu_post_run(vcpu, exit_reason);
 	} while (!signal_pending(current) && !guestdbg_exit_pending(vcpu) && !rc);
+	vcpu->mode = OUTSIDE_GUEST_MODE;
 
 	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
 	return rc;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 482612b..f80e08a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2205,7 +2205,6 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_block);
 
-#ifndef CONFIG_S390
 void kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
 {
 	struct swait_queue_head *wqp;
@@ -2235,7 +2234,6 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
 	put_cpu();
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_kick);
-#endif /* !CONFIG_S390 */
 
 int kvm_vcpu_yield_to(struct kvm_vcpu *target)
 {
-- 
2.7.4

Re: [PATCH/RFC 2/2] KVM: enable kvm_vcpu_kick/wake_up for s390

[Radim Krčmář]

2017-02-17 14:10+0100, Christian Borntraeger:
> while we still need to provide our private version of kick and wakeup
> to synchronize with some special cases, we can still enable the common
> code variant.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 12 ++++++++++--
>  virt/kvm/kvm_main.c      |  2 --
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 7cfd0dd..fa20686 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2485,8 +2485,14 @@ static void kvm_gmap_notifier(struct gmap *gmap, unsigned long start,
>  
>  int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
>  {
> -	/* kvm common code refers to this, but never calls it */
> -	BUG();
> +	/*
> +	 * STOP indication is resetted when delivering interrupts. This
> +	 * is done before we handle requests, so we only "loose" this
> +	 * when we are still going to handle requests. In that case
> +	 * we no longer need that STOP indication.
> +	 */
> +	__set_cpuflag(vcpu, CPUSTAT_STOP_INT);
> +	kvm_s390_vsie_kick(vcpu);

I wouldn't put the kick into that function.  On other architectures, the
function decides whether we should call an operation that actually
forces the exit.  e.g. x86 and arm do

  return kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE

(which also contains an optimization, because they will not kick a VCPU
 twice or do a kick for a VCPU outside the guest mode.)

There little point in having vcpu->mode if we do not use it here.
And we'd also like to use kvm_arch_vcpu_should_kick() for the same
decision in kvm_make_all_cpus_request(); basic idea:

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4f4d250e1f53..676c0636d73c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -163,10 +163,6 @@ void vcpu_put(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(vcpu_put);
 
-static void ack_flush(void *_completed)
-{
-}
-
 bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
 {
 	int i, cpu, me;
@@ -194,13 +190,13 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
 		smp_mb__after_atomic();
 
 		if (cpus != NULL && cpu != -1 && cpu != me &&
-		      kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE)
+		    kvm_arch_vcpu_should_kick(vcpu))
 			cpumask_set_cpu(cpu, cpus);
 	}
 	if (unlikely(cpus == NULL))
-		smp_call_function_many(cpu_online_mask, ack_flush, NULL, 1);
+		kvm_arch_cpu_kick_many(cpu_online_mask);
 	else if (!cpumask_empty(cpus))
-		smp_call_function_many(cpus, ack_flush, NULL, 1);
+		kvm_arch_cpu_kick_many(cpus);
 	else
 		called = false;
 	put_cpu();