[PATCH 0/3] fixes for __airqs_kick_single_vcpu()

[PATCH 0/3] fixes for __airqs_kick_single_vcpu()

[Halil Pasic]

The three fixes aren't closely related. The first one is the
most imporant one. They can be picked separately. I deciced to send them
out together so that if reviewers see: hey there is more broken, they
can also see the fixes near by.

Halil Pasic (3):
  KVM: s390: clear kicked_mask before sleeping again
  KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu
  KVM: s390: clear kicked_mask if not idle after set

 arch/s390/kvm/interrupt.c | 12 +++++++++---
 arch/s390/kvm/kvm-s390.c  |  3 +--
 2 files changed, 10 insertions(+), 5 deletions(-)


base-commit: 519d81956ee277b4419c723adfb154603c2565ba
-- 
2.25.1

[PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again

[Halil Pasic]

The idea behind kicked mask is that we should not re-kick a vcpu that
is already in the "kick" process, i.e. that was kicked and is
is about to be dispatched if certain conditions are met.

The problem with the current implementation is, that it assumes the
kicked vcpu is going to enter SIE shortly. But under certain
circumstances, the vcpu we just kicked will be deemed non-runnable and
will remain in wait state. This can happen, if the interrupt(s) this
vcpu got kicked to deal with got already cleared (because the interrupts
got delivered to another vcpu). In this case kvm_arch_vcpu_runnable()
would return false, and the vcpu would remain in kvm_vcpu_block(),
but this time with its kicked_mask bit set. So next time around we
wouldn't kick the vcpu form __airqs_kick_single_vcpu(), but would assume
that we just kicked it.

Let us make sure the kicked_mask is cleared before we give up on
re-dispatching the vcpu.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Reported-by: Matthew Rosato <mjrosato@linux.ibm.com>
Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
---
 arch/s390/kvm/kvm-s390.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 6a6dd5e1daf6..1c97493d21e1 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3363,6 +3363,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
 {
+	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
 	return kvm_s390_vcpu_has_irq(vcpu, 0);
 }
 
-- 
2.25.1

[PATCH 2/3] KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu

[Halil Pasic]

Changing the deliverable mask in __airqs_kick_single_vcpu() is a bug. If
one idle vcpu can't take the interrupts we want to deliver, we should
look for another vcpu that can, instead of saying that we don't want
to deliver these interrupts by clearing the bits from the
deliverable_mask.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
---
 arch/s390/kvm/interrupt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 10722455fd02..2245f4b8d362 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -3053,13 +3053,14 @@ static void __airqs_kick_single_vcpu(struct kvm *kvm, u8 deliverable_mask)
 	int vcpu_idx, online_vcpus = atomic_read(&kvm->online_vcpus);
 	struct kvm_s390_gisa_interrupt *gi = &kvm->arch.gisa_int;
 	struct kvm_vcpu *vcpu;
+	u8 vcpu_isc_mask;
 
 	for_each_set_bit(vcpu_idx, kvm->arch.idle_mask, online_vcpus) {
 		vcpu = kvm_get_vcpu(kvm, vcpu_idx);
 		if (psw_ioint_disabled(vcpu))
 			continue;
-		deliverable_mask &= (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
-		if (deliverable_mask) {
+		vcpu_isc_mask = (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
+		if (deliverable_mask & vcpu_isc_mask) {
 			/* lately kicked but not yet running */
 			if (test_and_set_bit(vcpu_idx, gi->kicked_mask))
 				return;
-- 
2.25.1

[PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set

[Halil Pasic]

The idea behind kicked mask is that we should not re-kick a vcpu
from __airqs_kick_single_vcpu() that is already in the middle of
being kicked by the same function.

If however the vcpu that was idle before when the idle_mask was
examined, is not idle any more after the kicked_mask is set, that
means that we don't need to kick, and that we need to clear the
bit we just set because we may be beyond the point where it would
get cleared in the wake-up process. Since the time window is short,
this is probably more a theoretical than a practical thing: the race
window is small.

To get things harmonized let us also move the clear from vcpu_pre_run()
to __unset_cpu_idle().

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
---
 arch/s390/kvm/interrupt.c | 7 ++++++-
 arch/s390/kvm/kvm-s390.c  | 2 --
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 2245f4b8d362..3c80a2237ef5 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -426,6 +426,7 @@ static void __unset_cpu_idle(struct kvm_vcpu *vcpu)
 {
 	kvm_s390_clear_cpuflags(vcpu, CPUSTAT_WAIT);
 	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.idle_mask);
+	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
 }
 
 static void __reset_intercept_indicators(struct kvm_vcpu *vcpu)
@@ -3064,7 +3065,11 @@ static void __airqs_kick_single_vcpu(struct kvm *kvm, u8 deliverable_mask)
 			/* lately kicked but not yet running */
 			if (test_and_set_bit(vcpu_idx, gi->kicked_mask))
 				return;
-			kvm_s390_vcpu_wakeup(vcpu);
+			/* if meanwhile not idle: clear  and don't kick */
+			if (test_bit(vcpu_idx, kvm->arch.idle_mask))
+				kvm_s390_vcpu_wakeup(vcpu);
+			else
+				clear_bit(vcpu_idx, gi->kicked_mask);
 			return;
 		}
 	}
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 1c97493d21e1..6b779ef9f5fb 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -4067,8 +4067,6 @@ static int vcpu_pre_run(struct kvm_vcpu *vcpu)
 		kvm_s390_patch_guest_per_regs(vcpu);
 	}
 
-	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
-
 	vcpu->arch.sie_block->icptcode = 0;
 	cpuflags = atomic_read(&vcpu->arch.sie_block->cpuflags);
 	VCPU_EVENT(vcpu, 6, "entering sie flags %x", cpuflags);
-- 
2.25.1

Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again

[Christian Borntraeger]

Am 19.10.21 um 19:53 schrieb Halil Pasic:
> The idea behind kicked mask is that we should not re-kick a vcpu that
> is already in the "kick" process, i.e. that was kicked and is
> is about to be dispatched if certain conditions are met.
> 
> The problem with the current implementation is, that it assumes the
> kicked vcpu is going to enter SIE shortly. But under certain
> circumstances, the vcpu we just kicked will be deemed non-runnable and
> will remain in wait state. This can happen, if the interrupt(s) this
> vcpu got kicked to deal with got already cleared (because the interrupts
> got delivered to another vcpu). In this case kvm_arch_vcpu_runnable()
> would return false, and the vcpu would remain in kvm_vcpu_block(),
> but this time with its kicked_mask bit set. So next time around we
> wouldn't kick the vcpu form __airqs_kick_single_vcpu(), but would assume
> that we just kicked it.
> 
> Let us make sure the kicked_mask is cleared before we give up on
> re-dispatching the vcpu.
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Reported-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

> ---
>   arch/s390/kvm/kvm-s390.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 6a6dd5e1daf6..1c97493d21e1 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3363,6 +3363,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>   
>   int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
>   {
> +	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
>   	return kvm_s390_vcpu_has_irq(vcpu, 0);
>   }
>   
> 

Re: [PATCH 2/3] KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu

[Christian Borntraeger]

Am 19.10.21 um 19:54 schrieb Halil Pasic:
> Changing the deliverable mask in __airqs_kick_single_vcpu() is a bug. If
> one idle vcpu can't take the interrupts we want to deliver, we should
> look for another vcpu that can, instead of saying that we don't want
> to deliver these interrupts by clearing the bits from the
> deliverable_mask.
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

> ---
>   arch/s390/kvm/interrupt.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 10722455fd02..2245f4b8d362 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -3053,13 +3053,14 @@ static void __airqs_kick_single_vcpu(struct kvm *kvm, u8 deliverable_mask)
>   	int vcpu_idx, online_vcpus = atomic_read(&kvm->online_vcpus);
>   	struct kvm_s390_gisa_interrupt *gi = &kvm->arch.gisa_int;
>   	struct kvm_vcpu *vcpu;
> +	u8 vcpu_isc_mask;
>   
>   	for_each_set_bit(vcpu_idx, kvm->arch.idle_mask, online_vcpus) {
>   		vcpu = kvm_get_vcpu(kvm, vcpu_idx);
>   		if (psw_ioint_disabled(vcpu))
>   			continue;
> -		deliverable_mask &= (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
> -		if (deliverable_mask) {
> +		vcpu_isc_mask = (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
> +		if (deliverable_mask & vcpu_isc_mask) {
>   			/* lately kicked but not yet running */
>   			if (test_and_set_bit(vcpu_idx, gi->kicked_mask))
>   				return;
> 

Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set

[Christian Borntraeger]

Am 19.10.21 um 19:54 schrieb Halil Pasic:
> The idea behind kicked mask is that we should not re-kick a vcpu
> from __airqs_kick_single_vcpu() that is already in the middle of
> being kicked by the same function.
> 
> If however the vcpu that was idle before when the idle_mask was
> examined, is not idle any more after the kicked_mask is set, that
> means that we don't need to kick, and that we need to clear the
> bit we just set because we may be beyond the point where it would
> get cleared in the wake-up process. Since the time window is short,
> this is probably more a theoretical than a practical thing: the race
> window is small.
> 
> To get things harmonized let us also move the clear from vcpu_pre_run()
> to __unset_cpu_idle().

this part makes sense.
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
> ---
>   arch/s390/kvm/interrupt.c | 7 ++++++-
>   arch/s390/kvm/kvm-s390.c  | 2 --
>   2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 2245f4b8d362..3c80a2237ef5 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -426,6 +426,7 @@ static void __unset_cpu_idle(struct kvm_vcpu *vcpu)
>   {
>   	kvm_s390_clear_cpuflags(vcpu, CPUSTAT_WAIT);
>   	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.idle_mask);
> +	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
>   }
>   
>   static void __reset_intercept_indicators(struct kvm_vcpu *vcpu)
> @@ -3064,7 +3065,11 @@ static void __airqs_kick_single_vcpu(struct kvm *kvm, u8 deliverable_mask)
>   			/* lately kicked but not yet running */
>   			if (test_and_set_bit(vcpu_idx, gi->kicked_mask))
>   				return;
> -			kvm_s390_vcpu_wakeup(vcpu);
> +			/* if meanwhile not idle: clear  and don't kick */
> +			if (test_bit(vcpu_idx, kvm->arch.idle_mask))
> +				kvm_s390_vcpu_wakeup(vcpu);
> +			else
> +				clear_bit(vcpu_idx, gi->kicked_mask);

I think this is now a bug. We should not return but continue in that case, no?

I think it might be safer to also clear kicked_mask in __set_cpu_idle
 From a CPUs perspective: We have been running and are on our way to become idle.
There is no way that someone kicked us for a wakeup. In other words as long as we
are running, there is no point in kicking us but when going idle we should get rid
of old kick_mask bit.
Doesnt this cover your scenario?


>   			return;
>   		}
>   	}
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 1c97493d21e1..6b779ef9f5fb 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4067,8 +4067,6 @@ static int vcpu_pre_run(struct kvm_vcpu *vcpu)
>   		kvm_s390_patch_guest_per_regs(vcpu);
>   	}
>   
> -	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
> -
>   	vcpu->arch.sie_block->icptcode = 0;
>   	cpuflags = atomic_read(&vcpu->arch.sie_block->cpuflags);
>   	VCPU_EVENT(vcpu, 6, "entering sie flags %x", cpuflags);
> 

Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set

[Christian Borntraeger]

Am 19.10.21 um 23:35 schrieb Christian Borntraeger:
> 
> 
> Am 19.10.21 um 19:54 schrieb Halil Pasic:
>> The idea behind kicked mask is that we should not re-kick a vcpu
>> from __airqs_kick_single_vcpu() that is already in the middle of
>> being kicked by the same function.
>>
>> If however the vcpu that was idle before when the idle_mask was
>> examined, is not idle any more after the kicked_mask is set, that
>> means that we don't need to kick, and that we need to clear the
>> bit we just set because we may be beyond the point where it would
>> get cleared in the wake-up process. Since the time window is short,
>> this is probably more a theoretical than a practical thing: the race
>> window is small.
>>
>> To get things harmonized let us also move the clear from vcpu_pre_run()
>> to __unset_cpu_idle().
> 
> this part makes sense.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
>> Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
>> ---
>>   arch/s390/kvm/interrupt.c | 7 ++++++-
>>   arch/s390/kvm/kvm-s390.c  | 2 --
>>   2 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index 2245f4b8d362..3c80a2237ef5 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -426,6 +426,7 @@ static void __unset_cpu_idle(struct kvm_vcpu *vcpu)
>>   {
>>       kvm_s390_clear_cpuflags(vcpu, CPUSTAT_WAIT);
>>       clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.idle_mask);
>> +    clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
>>   }
>>   static void __reset_intercept_indicators(struct kvm_vcpu *vcpu)
>> @@ -3064,7 +3065,11 @@ static void __airqs_kick_single_vcpu(struct kvm *kvm, u8 deliverable_mask)
>>               /* lately kicked but not yet running */
>>               if (test_and_set_bit(vcpu_idx, gi->kicked_mask))
>>                   return;
>> -            kvm_s390_vcpu_wakeup(vcpu);
>> +            /* if meanwhile not idle: clear  and don't kick */
>> +            if (test_bit(vcpu_idx, kvm->arch.idle_mask))
>> +                kvm_s390_vcpu_wakeup(vcpu);
>> +            else
>> +                clear_bit(vcpu_idx, gi->kicked_mask);
> 
> I think this is now a bug. We should not return but continue in that case, no?

Thinking again about this, it might be ok. If we went from idle to non-idle we
likely were in SIE and the interrupt should have been delivered. But I would rather
wake up too often than too less.
> 
> I think it might be safer to also clear kicked_mask in __set_cpu_idle
>  From a CPUs perspective: We have been running and are on our way to become idle.
> There is no way that someone kicked us for a wakeup. In other words as long as we
> are running, there is no point in kicking us but when going idle we should get rid
> of old kick_mask bit.
> Doesnt this cover your scenario?
> 
> 
>>               return;
>>           }
>>       }
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 1c97493d21e1..6b779ef9f5fb 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -4067,8 +4067,6 @@ static int vcpu_pre_run(struct kvm_vcpu *vcpu)
>>           kvm_s390_patch_guest_per_regs(vcpu);
>>       }
>> -    clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
>> -
>>       vcpu->arch.sie_block->icptcode = 0;
>>       cpuflags = atomic_read(&vcpu->arch.sie_block->cpuflags);
>>       VCPU_EVENT(vcpu, 6, "entering sie flags %x", cpuflags);
>>

Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again

[Claudio Imbrenda]

On Tue, 19 Oct 2021 19:53:59 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> The idea behind kicked mask is that we should not re-kick a vcpu that
> is already in the "kick" process, i.e. that was kicked and is
> is about to be dispatched if certain conditions are met.
> 
> The problem with the current implementation is, that it assumes the
> kicked vcpu is going to enter SIE shortly. But under certain
> circumstances, the vcpu we just kicked will be deemed non-runnable and
> will remain in wait state. This can happen, if the interrupt(s) this
> vcpu got kicked to deal with got already cleared (because the interrupts
> got delivered to another vcpu). In this case kvm_arch_vcpu_runnable()
> would return false, and the vcpu would remain in kvm_vcpu_block(),
> but this time with its kicked_mask bit set. So next time around we
> wouldn't kick the vcpu form __airqs_kick_single_vcpu(), but would assume
> that we just kicked it.
> 
> Let us make sure the kicked_mask is cleared before we give up on
> re-dispatching the vcpu.
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Reported-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
> ---
>  arch/s390/kvm/kvm-s390.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 6a6dd5e1daf6..1c97493d21e1 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3363,6 +3363,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  
>  int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
>  {
> +	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);

so, you unconditionally clear the flag, before knowing if the vCPU is
runnable?

from your description I would have expected to only clear the bit if
the vCPU is not runnable.

would things break if we were to try to kick the vCPU again after
clearing the bit, but before dispatching it?

>  	return kvm_s390_vcpu_has_irq(vcpu, 0);
>  }
>  

Re: [PATCH 2/3] KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu

[Claudio Imbrenda]

On Tue, 19 Oct 2021 19:54:00 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> Changing the deliverable mask in __airqs_kick_single_vcpu() is a bug. If
> one idle vcpu can't take the interrupts we want to deliver, we should
> look for another vcpu that can, instead of saying that we don't want
> to deliver these interrupts by clearing the bits from the
> deliverable_mask.
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  arch/s390/kvm/interrupt.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 10722455fd02..2245f4b8d362 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -3053,13 +3053,14 @@ static void __airqs_kick_single_vcpu(struct kvm *kvm, u8 deliverable_mask)
>  	int vcpu_idx, online_vcpus = atomic_read(&kvm->online_vcpus);
>  	struct kvm_s390_gisa_interrupt *gi = &kvm->arch.gisa_int;
>  	struct kvm_vcpu *vcpu;
> +	u8 vcpu_isc_mask;
>  
>  	for_each_set_bit(vcpu_idx, kvm->arch.idle_mask, online_vcpus) {
>  		vcpu = kvm_get_vcpu(kvm, vcpu_idx);
>  		if (psw_ioint_disabled(vcpu))
>  			continue;
> -		deliverable_mask &= (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
> -		if (deliverable_mask) {
> +		vcpu_isc_mask = (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
> +		if (deliverable_mask & vcpu_isc_mask) {
>  			/* lately kicked but not yet running */
>  			if (test_and_set_bit(vcpu_idx, gi->kicked_mask))
>  				return;

Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again

[Christian Borntraeger]

Am 20.10.21 um 07:35 schrieb Claudio Imbrenda:
> On Tue, 19 Oct 2021 19:53:59 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> The idea behind kicked mask is that we should not re-kick a vcpu that
>> is already in the "kick" process, i.e. that was kicked and is
>> is about to be dispatched if certain conditions are met.
>>
>> The problem with the current implementation is, that it assumes the
>> kicked vcpu is going to enter SIE shortly. But under certain
>> circumstances, the vcpu we just kicked will be deemed non-runnable and
>> will remain in wait state. This can happen, if the interrupt(s) this
>> vcpu got kicked to deal with got already cleared (because the interrupts
>> got delivered to another vcpu). In this case kvm_arch_vcpu_runnable()
>> would return false, and the vcpu would remain in kvm_vcpu_block(),
>> but this time with its kicked_mask bit set. So next time around we
>> wouldn't kick the vcpu form __airqs_kick_single_vcpu(), but would assume
>> that we just kicked it.
>>
>> Let us make sure the kicked_mask is cleared before we give up on
>> re-dispatching the vcpu.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
>> Reported-by: Matthew Rosato <mjrosato@linux.ibm.com>
>> Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
>> ---
>>   arch/s390/kvm/kvm-s390.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 6a6dd5e1daf6..1c97493d21e1 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -3363,6 +3363,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>>   
>>   int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
>>   {
>> +	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
> 
> so, you unconditionally clear the flag, before knowing if the vCPU is
> runnable?
> 
> from your description I would have expected to only clear the bit if
> the vCPU is not runnable.
> 
> would things break if we were to try to kick the vCPU again after
> clearing the bit, but before dispatching it?

The whole logic is just an optimization to avoid unnecessary wakeups.
When the bit is set a wakup might be omitted.
I prefer to do an unneeded wakeup over not doing a wakeup so I think
over-clearing is safer.
In fact, getting rid of this micro-optimization would be a valid
alternative.
> 
>>   	return kvm_s390_vcpu_has_irq(vcpu, 0);
>>   }
>>   
> 

Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again

[Claudio Imbrenda]

On Wed, 20 Oct 2021 08:03:40 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Am 20.10.21 um 07:35 schrieb Claudio Imbrenda:
> > On Tue, 19 Oct 2021 19:53:59 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> >> The idea behind kicked mask is that we should not re-kick a vcpu that
> >> is already in the "kick" process, i.e. that was kicked and is
> >> is about to be dispatched if certain conditions are met.
> >>
> >> The problem with the current implementation is, that it assumes the
> >> kicked vcpu is going to enter SIE shortly. But under certain
> >> circumstances, the vcpu we just kicked will be deemed non-runnable and
> >> will remain in wait state. This can happen, if the interrupt(s) this
> >> vcpu got kicked to deal with got already cleared (because the interrupts
> >> got delivered to another vcpu). In this case kvm_arch_vcpu_runnable()
> >> would return false, and the vcpu would remain in kvm_vcpu_block(),
> >> but this time with its kicked_mask bit set. So next time around we
> >> wouldn't kick the vcpu form __airqs_kick_single_vcpu(), but would assume
> >> that we just kicked it.
> >>
> >> Let us make sure the kicked_mask is cleared before we give up on
> >> re-dispatching the vcpu.
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> >> Reported-by: Matthew Rosato <mjrosato@linux.ibm.com>
> >> Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
> >> ---
> >>   arch/s390/kvm/kvm-s390.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> >> index 6a6dd5e1daf6..1c97493d21e1 100644
> >> --- a/arch/s390/kvm/kvm-s390.c
> >> +++ b/arch/s390/kvm/kvm-s390.c
> >> @@ -3363,6 +3363,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> >>   
> >>   int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
> >>   {
> >> +	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);  
> > 
> > so, you unconditionally clear the flag, before knowing if the vCPU is
> > runnable?
> > 
> > from your description I would have expected to only clear the bit if
> > the vCPU is not runnable.
> > 
> > would things break if we were to try to kick the vCPU again after
> > clearing the bit, but before dispatching it?  
> 
> The whole logic is just an optimization to avoid unnecessary wakeups.
> When the bit is set a wakup might be omitted.
> I prefer to do an unneeded wakeup over not doing a wakeup so I think
> over-clearing is safer.
> In fact, getting rid of this micro-optimization would be a valid
> alternative.

my only concern was if things would break in case we kick the vCPU
again after clearing the bit; it seems nothing breaks, so I'm ok with it

> >   
> >>   	return kvm_s390_vcpu_has_irq(vcpu, 0);
> >>   }
> >>     
> >   

Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set

[Halil Pasic]

On Tue, 19 Oct 2021 23:35:25 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> > @@ -426,6 +426,7 @@ static void __unset_cpu_idle(struct kvm_vcpu *vcpu)
> >   {
> >   	kvm_s390_clear_cpuflags(vcpu, CPUSTAT_WAIT);
> >   	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.idle_mask);
> > +	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);

BTW, do you know are bit-ops garanteed to be serialized as seen by
another cpu even when acting on a different byte? I mean
could the kick_single_vcpu() set the clear of the kicked_mask bit but
not see the clear of the idle mask?

If that is not true we may need some barriers, or possibly merging the
two bitmasks like idle bit, kick bit alterating to ensure there
absolutely ain't no race.


> >   }
> >   
> >   static void __reset_intercept_indicators(struct kvm_vcpu *vcpu)
> > @@ -3064,7 +3065,11 @@ static void __airqs_kick_single_vcpu(struct kvm *kvm, u8 deliverable_mask)
> >   			/* lately kicked but not yet running */
> >   			if (test_and_set_bit(vcpu_idx, gi->kicked_mask))
> >   				return;
> > -			kvm_s390_vcpu_wakeup(vcpu);
> > +			/* if meanwhile not idle: clear  and don't kick */
> > +			if (test_bit(vcpu_idx, kvm->arch.idle_mask))
> > +				kvm_s390_vcpu_wakeup(vcpu);
> > +			else
> > +				clear_bit(vcpu_idx, gi->kicked_mask);  
> 
> I think this is now a bug. We should not return but continue in that case, no?
> 

I don't think so. The purpose of this function is to kick a *single* vcpu
that can handle *some* of the I/O interrupts indicated by the
deliverable_mask. The deliverable mask predates the check of the idle_mask.
I believe if we selected a suitable vcpu, that was idle and before we
actually do a wakeup on it we see that it isn't idle any more, I believe
it is as good if not better as performing the wakeup (and a new wakeup()
call is pointless: this vcpu either already got the the irqs it can get,
or is about to enter SIE soon to do so. We just saved a pointless call
to wakeup().

> 
> I think it might be safer to also clear kicked_mask in __set_cpu_idle

It would not hurt, but my guess is that kvm_arch_vcpu_runnable() before
we really decide to go to sleep:

void kvm_vcpu_block(struct kvm_vcpu *vcpu)                                      
{ 
[..]
        for (;;) {                                                              
                set_current_state(TASK_INTERRUPTIBLE);                          
                                                                                
                if (kvm_vcpu_check_block(vcpu) < 0)     <=== calls runnable()                        
                        break;                                                  
                                                                                
                waited = true;                                                  
                schedule();                                                     
        } 

>  From a CPUs perspective: We have been running and are on our way to become idle.
> There is no way that someone kicked us for a wakeup. In other words as long as we
> are running, there is no point in kicking us but when going idle we should get rid
> of old kick_mask bit.
> Doesnt this cover your scenario?

In practice probably yes, in theory I don't think so. I hope this is
more of a theoretical problem than a practical one anyway. But let me
discuss the theory anyway.

Under the assumption that an arbitrary amount of time can pass between 
1) for_each_set_bit finds the vcpus bit in the idle mask set
and
2) test_and_set_bit(kicked_mask) that returns a false (bit was not set,
and we did set it)
then if we choose an absurdly large amount of time, it is possible that
we are past a whole cycle: an __unset_cpu_ilde() and an __set_cpu_idle()
but we didn't reach set_current_state(TASK_INTERRUPTIBLE). If
we set the bit at a suitable place there we theoretically may end up
in a situation where the wakeup is ineffective because the state didn't
change yet, but the bit gets set. So we end up in a stable sleeps and
does not want to get woken up state. If the clear in
kvm_arch_vcpu_runnable() does not save us... It could be that, that
clear alone is sufficient. Because, before we really go to sleep we kind
of attempt to wake up, and this guy clears on every attempted wakeup. So
the clear in kvm_arch_vcpu_runnable() may be the only clear we need. Or?

Anyway the scenario I described is very very far fetched I guess, but I
prefer solutions that are theoretically race free over solutions that
are practically race free if performance does not suffer.

Regards,
Halil

Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again

[Michael Mueller]

On 19.10.21 19:53, Halil Pasic wrote:
> The idea behind kicked mask is that we should not re-kick a vcpu that
> is already in the "kick" process, i.e. that was kicked and is
> is about to be dispatched if certain conditions are met.
> 
> The problem with the current implementation is, that it assumes the
> kicked vcpu is going to enter SIE shortly. But under certain
> circumstances, the vcpu we just kicked will be deemed non-runnable and
> will remain in wait state. This can happen, if the interrupt(s) this
> vcpu got kicked to deal with got already cleared (because the interrupts
> got delivered to another vcpu). In this case kvm_arch_vcpu_runnable()
> would return false, and the vcpu would remain in kvm_vcpu_block(),
> but this time with its kicked_mask bit set. So next time around we
> wouldn't kick the vcpu form __airqs_kick_single_vcpu(), but would assume
> that we just kicked it.
> 
> Let us make sure the kicked_mask is cleared before we give up on
> re-dispatching the vcpu.
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Reported-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")

Reviewed-by: Michael Mueller <mimu@linux.ibm.com>

> ---
>   arch/s390/kvm/kvm-s390.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 6a6dd5e1daf6..1c97493d21e1 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3363,6 +3363,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>   
>   int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
>   {
> +	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
>   	return kvm_s390_vcpu_has_irq(vcpu, 0);
>   }
>   
> 

Re: [PATCH 2/3] KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu

[Michael Mueller]

On 19.10.21 19:54, Halil Pasic wrote:
> Changing the deliverable mask in __airqs_kick_single_vcpu() is a bug. If
> one idle vcpu can't take the interrupts we want to deliver, we should
> look for another vcpu that can, instead of saying that we don't want
> to deliver these interrupts by clearing the bits from the
> deliverable_mask.
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")

Reviewed-by: Michael Mueller <mimu@linux.ibm.com>

> ---
>   arch/s390/kvm/interrupt.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 10722455fd02..2245f4b8d362 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -3053,13 +3053,14 @@ static void __airqs_kick_single_vcpu(struct kvm *kvm, u8 deliverable_mask)
>   	int vcpu_idx, online_vcpus = atomic_read(&kvm->online_vcpus);
>   	struct kvm_s390_gisa_interrupt *gi = &kvm->arch.gisa_int;
>   	struct kvm_vcpu *vcpu;
> +	u8 vcpu_isc_mask;
>   
>   	for_each_set_bit(vcpu_idx, kvm->arch.idle_mask, online_vcpus) {
>   		vcpu = kvm_get_vcpu(kvm, vcpu_idx);
>   		if (psw_ioint_disabled(vcpu))
>   			continue;
> -		deliverable_mask &= (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
> -		if (deliverable_mask) {
> +		vcpu_isc_mask = (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
> +		if (deliverable_mask & vcpu_isc_mask) {
>   			/* lately kicked but not yet running */
>   			if (test_and_set_bit(vcpu_idx, gi->kicked_mask))
>   				return;
> 

Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again

[Halil Pasic]

On Wed, 20 Oct 2021 08:08:16 +0200
Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:

> > >> +	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);    
> > > 
> > > so, you unconditionally clear the flag, before knowing if the vCPU is
> > > runnable?

Right. I talked about this with  Mimu. It would extend the section
guarded by the bit, and than may be a good thing. Maybe we should
measure that alternative as well.

> > > 
> > > from your description I would have expected to only clear the bit if
> > > the vCPU is not runnable.
> > > 
> > > would things break if we were to try to kick the vCPU again after
> > > clearing the bit, but before dispatching it?    
> > 
> > The whole logic is just an optimization to avoid unnecessary wakeups.
> > When the bit is set a wakup might be omitted.
> > I prefer to do an unneeded wakeup over not doing a wakeup so I think
> > over-clearing is safer.
> > In fact, getting rid of this micro-optimization would be a valid
> > alternative.  
> 
> my only concern was if things would break in case we kick the vCPU
> again after clearing the bit; it seems nothing breaks, so I'm ok with it

I'm not sure about the exact impact of over-waking.
kvm_s390_vcpu_wakeup() sets vcpu->valid_wakeup which is I believe used
for some halt poll heuristics. We unset that in
kvm_arch_vcpu_block_finish(). If we cleared only conditionally the
protection would extend for that as well. Which would be a good thing.
The statistics stuff in kvm_vcpu_wake_up() does account for already
running, so I see no correctness issues there.

Regards,
Halil

Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set

[Michael Mueller]

On 19.10.21 19:54, Halil Pasic wrote:
> The idea behind kicked mask is that we should not re-kick a vcpu
> from __airqs_kick_single_vcpu() that is already in the middle of
> being kicked by the same function.
> 
> If however the vcpu that was idle before when the idle_mask was
> examined, is not idle any more after the kicked_mask is set, that
> means that we don't need to kick, and that we need to clear the
> bit we just set because we may be beyond the point where it would
> get cleared in the wake-up process. Since the time window is short,
> this is probably more a theoretical than a practical thing: the race
> window is small.
> 
> To get things harmonized let us also move the clear from vcpu_pre_run()
> to __unset_cpu_idle().
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")

Before releasing something like this, where none of us is sure if
it really saves cpu cost, I'd prefer to run some measurement with
the whole kicked_mask logic removed and to compare the number of
vcpu wake ups with the number of interrupts to be processed by
the gib alert mechanism in a slightly over committed host while
driving with Matthews test load.

A similar run can be done with this code.

> ---
>   arch/s390/kvm/interrupt.c | 7 ++++++-
>   arch/s390/kvm/kvm-s390.c  | 2 --
>   2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 2245f4b8d362..3c80a2237ef5 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -426,6 +426,7 @@ static void __unset_cpu_idle(struct kvm_vcpu *vcpu)
>   {
>   	kvm_s390_clear_cpuflags(vcpu, CPUSTAT_WAIT);
>   	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.idle_mask);
> +	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
>   }
>   
>   static void __reset_intercept_indicators(struct kvm_vcpu *vcpu)
> @@ -3064,7 +3065,11 @@ static void __airqs_kick_single_vcpu(struct kvm *kvm, u8 deliverable_mask)
>   			/* lately kicked but not yet running */
>   			if (test_and_set_bit(vcpu_idx, gi->kicked_mask))
>   				return;
> -			kvm_s390_vcpu_wakeup(vcpu);
> +			/* if meanwhile not idle: clear  and don't kick */
> +			if (test_bit(vcpu_idx, kvm->arch.idle_mask))
> +				kvm_s390_vcpu_wakeup(vcpu);
> +			else
> +				clear_bit(vcpu_idx, gi->kicked_mask);
>   			return;
>   		}
>   	}
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 1c97493d21e1..6b779ef9f5fb 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4067,8 +4067,6 @@ static int vcpu_pre_run(struct kvm_vcpu *vcpu)
>   		kvm_s390_patch_guest_per_regs(vcpu);
>   	}
>   
> -	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
> -
>   	vcpu->arch.sie_block->icptcode = 0;
>   	cpuflags = atomic_read(&vcpu->arch.sie_block->cpuflags);
>   	VCPU_EVENT(vcpu, 6, "entering sie flags %x", cpuflags);
> 

Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set

[Christian Borntraeger]

Am 20.10.21 um 11:48 schrieb Michael Mueller:
> 
> 
> On 19.10.21 19:54, Halil Pasic wrote:
>> The idea behind kicked mask is that we should not re-kick a vcpu
>> from __airqs_kick_single_vcpu() that is already in the middle of
>> being kicked by the same function.
>>
>> If however the vcpu that was idle before when the idle_mask was
>> examined, is not idle any more after the kicked_mask is set, that
>> means that we don't need to kick, and that we need to clear the
>> bit we just set because we may be beyond the point where it would
>> get cleared in the wake-up process. Since the time window is short,
>> this is probably more a theoretical than a practical thing: the race
>> window is small.
>>
>> To get things harmonized let us also move the clear from vcpu_pre_run()
>> to __unset_cpu_idle().
>>
>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
>> Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
> 
> Before releasing something like this, where none of us is sure if
> it really saves cpu cost, I'd prefer to run some measurement with
> the whole kicked_mask logic removed and to compare the number of
> vcpu wake ups with the number of interrupts to be processed by
> the gib alert mechanism in a slightly over committed host while
> driving with Matthews test load.

But I think patch 1 and 2 can go immediately as they measurably or
testable fix things. Correct?

> A similar run can be done with this code.
> 
>> ---
>>   arch/s390/kvm/interrupt.c | 7 ++++++-
>>   arch/s390/kvm/kvm-s390.c  | 2 --
>>   2 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index 2245f4b8d362..3c80a2237ef5 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -426,6 +426,7 @@ static void __unset_cpu_idle(struct kvm_vcpu *vcpu)
>>   {
>>       kvm_s390_clear_cpuflags(vcpu, CPUSTAT_WAIT);
>>       clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.idle_mask);
>> +    clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
>>   }
>>   static void __reset_intercept_indicators(struct kvm_vcpu *vcpu)
>> @@ -3064,7 +3065,11 @@ static void __airqs_kick_single_vcpu(struct kvm *kvm, u8 deliverable_mask)
>>               /* lately kicked but not yet running */
>>               if (test_and_set_bit(vcpu_idx, gi->kicked_mask))
>>                   return;
>> -            kvm_s390_vcpu_wakeup(vcpu);
>> +            /* if meanwhile not idle: clear  and don't kick */
>> +            if (test_bit(vcpu_idx, kvm->arch.idle_mask))
>> +                kvm_s390_vcpu_wakeup(vcpu);
>> +            else
>> +                clear_bit(vcpu_idx, gi->kicked_mask);
>>               return;
>>           }
>>       }
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 1c97493d21e1..6b779ef9f5fb 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -4067,8 +4067,6 @@ static int vcpu_pre_run(struct kvm_vcpu *vcpu)
>>           kvm_s390_patch_guest_per_regs(vcpu);
>>       }
>> -    clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
>> -
>>       vcpu->arch.sie_block->icptcode = 0;
>>       cpuflags = atomic_read(&vcpu->arch.sie_block->cpuflags);
>>       VCPU_EVENT(vcpu, 6, "entering sie flags %x", cpuflags);
>>

Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again

[Claudio Imbrenda]

On Wed, 20 Oct 2021 10:14:50 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

[...]

> running, so I see no correctness issues there.

fair enough :)

Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again

[Claudio Imbrenda]

On Tue, 19 Oct 2021 19:53:59 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> The idea behind kicked mask is that we should not re-kick a vcpu that
> is already in the "kick" process, i.e. that was kicked and is
> is about to be dispatched if certain conditions are met.
> 
> The problem with the current implementation is, that it assumes the
> kicked vcpu is going to enter SIE shortly. But under certain
> circumstances, the vcpu we just kicked will be deemed non-runnable and
> will remain in wait state. This can happen, if the interrupt(s) this
> vcpu got kicked to deal with got already cleared (because the interrupts
> got delivered to another vcpu). In this case kvm_arch_vcpu_runnable()
> would return false, and the vcpu would remain in kvm_vcpu_block(),
> but this time with its kicked_mask bit set. So next time around we
> wouldn't kick the vcpu form __airqs_kick_single_vcpu(), but would assume
> that we just kicked it.
> 
> Let us make sure the kicked_mask is cleared before we give up on
> re-dispatching the vcpu.
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Reported-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  arch/s390/kvm/kvm-s390.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 6a6dd5e1daf6..1c97493d21e1 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3363,6 +3363,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  
>  int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
>  {
> +	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
>  	return kvm_s390_vcpu_has_irq(vcpu, 0);
>  }
>  

Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set

[Halil Pasic]

On Wed, 20 Oct 2021 12:31:19 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> > Before releasing something like this, where none of us is sure if
> > it really saves cpu cost, I'd prefer to run some measurement with
> > the whole kicked_mask logic removed and to compare the number of
> > vcpu wake ups with the number of interrupts to be processed by
> > the gib alert mechanism in a slightly over committed host while
> > driving with Matthews test load.  
> 
> But I think patch 1 and 2 can go immediately as they measurably or
> testable fix things. Correct?

I think so as well. And if patch 3 is going to be dropped, I would
really like to keep the unconditional clear in kvm_arch_vcpu_runnable(),
as my analysis in the discussion points out: I think it can save us
form trouble this patch is trying to address.

Regards,
Halil

Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set

[Michael Mueller]

On 20.10.21 12:31, Christian Borntraeger wrote:
> Am 20.10.21 um 11:48 schrieb Michael Mueller:
>>
>>
>> On 19.10.21 19:54, Halil Pasic wrote:
>>> The idea behind kicked mask is that we should not re-kick a vcpu
>>> from __airqs_kick_single_vcpu() that is already in the middle of
>>> being kicked by the same function.
>>>
>>> If however the vcpu that was idle before when the idle_mask was
>>> examined, is not idle any more after the kicked_mask is set, that
>>> means that we don't need to kick, and that we need to clear the
>>> bit we just set because we may be beyond the point where it would
>>> get cleared in the wake-up process. Since the time window is short,
>>> this is probably more a theoretical than a practical thing: the race
>>> window is small.
>>>
>>> To get things harmonized let us also move the clear from vcpu_pre_run()
>>> to __unset_cpu_idle().
>>>
>>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
>>> Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
>>
>> Before releasing something like this, where none of us is sure if
>> it really saves cpu cost, I'd prefer to run some measurement with
>> the whole kicked_mask logic removed and to compare the number of
>> vcpu wake ups with the number of interrupts to be processed by
>> the gib alert mechanism in a slightly over committed host while
>> driving with Matthews test load.
>
> But I think patch 1 and 2 can go immediately as they measurably or
> testable fix things. Correct?

Yes

>
>> A similar run can be done with this code.
>>
>>> ---
>>>   arch/s390/kvm/interrupt.c | 7 ++++++-
>>>   arch/s390/kvm/kvm-s390.c  | 2 --
>>>   2 files changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>> index 2245f4b8d362..3c80a2237ef5 100644
>>> --- a/arch/s390/kvm/interrupt.c
>>> +++ b/arch/s390/kvm/interrupt.c
>>> @@ -426,6 +426,7 @@ static void __unset_cpu_idle(struct kvm_vcpu *vcpu)
>>>   {
>>>       kvm_s390_clear_cpuflags(vcpu, CPUSTAT_WAIT);
>>>       clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.idle_mask);
>>> +    clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
>>>   }
>>>   static void __reset_intercept_indicators(struct kvm_vcpu *vcpu)
>>> @@ -3064,7 +3065,11 @@ static void __airqs_kick_single_vcpu(struct 
>>> kvm *kvm, u8 deliverable_mask)
>>>               /* lately kicked but not yet running */
>>>               if (test_and_set_bit(vcpu_idx, gi->kicked_mask))
>>>                   return;
>>> -            kvm_s390_vcpu_wakeup(vcpu);
>>> +            /* if meanwhile not idle: clear  and don't kick */
>>> +            if (test_bit(vcpu_idx, kvm->arch.idle_mask))
>>> +                kvm_s390_vcpu_wakeup(vcpu);
>>> +            else
>>> +                clear_bit(vcpu_idx, gi->kicked_mask);
>>>               return;
>>>           }
>>>       }
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index 1c97493d21e1..6b779ef9f5fb 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -4067,8 +4067,6 @@ static int vcpu_pre_run(struct kvm_vcpu *vcpu)
>>>           kvm_s390_patch_guest_per_regs(vcpu);
>>>       }
>>> -    clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
>>> -
>>>       vcpu->arch.sie_block->icptcode = 0;
>>>       cpuflags = atomic_read(&vcpu->arch.sie_block->cpuflags);
>>>       VCPU_EVENT(vcpu, 6, "entering sie flags %x", cpuflags);
>>>

Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set

[Christian Borntraeger]

Am 20.10.21 um 12:45 schrieb Halil Pasic:
> On Wed, 20 Oct 2021 12:31:19 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>>> Before releasing something like this, where none of us is sure if
>>> it really saves cpu cost, I'd prefer to run some measurement with
>>> the whole kicked_mask logic removed and to compare the number of
>>> vcpu wake ups with the number of interrupts to be processed by
>>> the gib alert mechanism in a slightly over committed host while
>>> driving with Matthews test load.
>>
>> But I think patch 1 and 2 can go immediately as they measurably or
>> testable fix things. Correct?
> 
> I think so as well. And if patch 3 is going to be dropped, I would
> really like to keep the unconditional clear in kvm_arch_vcpu_runnable(),
> as my analysis in the discussion points out: I think it can save us
> form trouble this patch is trying to address.

Yes, lets keep patch 1 and 2 as us and let us look deeper into this patch.
I will apply and queue 1 and 2 soon.

Re: [PATCH 0/3] fixes for __airqs_kick_single_vcpu()

[Christian Borntraeger]

Am 19.10.21 um 19:53 schrieb Halil Pasic:
> The three fixes aren't closely related. The first one is the
> most imporant one. They can be picked separately. I deciced to send them
> out together so that if reviewers see: hey there is more broken, they
> can also see the fixes near by.
> 
> Halil Pasic (3):
>    KVM: s390: clear kicked_mask before sleeping again
>    KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu
>    KVM: s390: clear kicked_mask if not idle after set
> 
>   arch/s390/kvm/interrupt.c | 12 +++++++++---
>   arch/s390/kvm/kvm-s390.c  |  3 +--
>   2 files changed, 10 insertions(+), 5 deletions(-)
> 
> 
> base-commit: 519d81956ee277b4419c723adfb154603c2565ba

I picked and queued patches 1 and 2. Thanks a lot for fixing.
I will need some time to dig through the code to decide about patch3.

Re: [PATCH 0/3] fixes for __airqs_kick_single_vcpu()

[Halil Pasic]

On Wed, 20 Oct 2021 13:04:05 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Am 19.10.21 um 19:53 schrieb Halil Pasic:
> > The three fixes aren't closely related. The first one is the
> > most imporant one. They can be picked separately. I deciced to send them
> > out together so that if reviewers see: hey there is more broken, they
> > can also see the fixes near by.
> > 
> > Halil Pasic (3):
> >    KVM: s390: clear kicked_mask before sleeping again
> >    KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu
> >    KVM: s390: clear kicked_mask if not idle after set
> > 
> >   arch/s390/kvm/interrupt.c | 12 +++++++++---
> >   arch/s390/kvm/kvm-s390.c  |  3 +--
> >   2 files changed, 10 insertions(+), 5 deletions(-)
> > 
> > 
> > base-commit: 519d81956ee277b4419c723adfb154603c2565ba  
> 
> I picked and queued patches 1 and 2. Thanks a lot for fixing.
> I will need some time to dig through the code to decide about patch3.

Sure. In the meantime I think we can do slightly better for patch 3
by making the clear introduced by patch 1 conditional (in patch 3
as patch 3 makes that a safe move).

Regards,
Halil

Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set

[Christian Borntraeger]

Am 20.10.21 um 09:52 schrieb Halil Pasic:
> On Tue, 19 Oct 2021 23:35:25 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>>> @@ -426,6 +426,7 @@ static void __unset_cpu_idle(struct kvm_vcpu *vcpu)
>>>    {
>>>    	kvm_s390_clear_cpuflags(vcpu, CPUSTAT_WAIT);
>>>    	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.idle_mask);
>>> +	clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.gisa_int.kicked_mask);
> 
> BTW, do you know are bit-ops garanteed to be serialized as seen by
> another cpu even when acting on a different byte? I mean
> could the kick_single_vcpu() set the clear of the kicked_mask bit but
> not see the clear of the idle mask?

clear_bit explicitely says.
  * This is a relaxed atomic operation (no implied memory barriers).

so if we really need the ordering, then we need to add a barrier.

> 
> If that is not true we may need some barriers, or possibly merging the
> two bitmasks like idle bit, kick bit alterating to ensure there
> absolutely ain't no race.
>