fixes for __airqs_kick_single_vcpu()
cover fixes for __airqs_kick_single_vcpu()
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
Signature · 1 line
2.25.1
PATCH 1/3 KVM: s390: clear kicked_mask before sleeping again
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.
--- arch/s390/kvm/kvm-s390.c | 1 + 1 file changed, 1 insertion(+)
arch/s390/kvm/kvm-s390.c +1 -0
@@ -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 ); }
Signature · 1 line
2.25.1
Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again
Am 19.10.21 um 19:53 schrieb Halil Pasic:
Quoted 15 lines
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 inkvm_vcpu_block(), but this time with itskicked_maskbit set. So next time around we wouldn't kick the vcpu form__airqs_kick_single_vcpu(), but would assume that we just kicked it.
Quoted 6 lines
Let us make sure the
kicked_maskis cleared before we give up on re-dispatching the vcpu.Signed-off-by Halil Pasic <[email protected]>Reported-by Matthew Rosato <[email protected]>Fixes 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
Quoted 17 lines
--- arch/s390/kvm/kvm-s390.c | 1 + 1 file changed, 1 insertion(+)arch/s390/kvm/kvm-s390.c +1
-0@@ -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 ); }
Claudio Imbrenda
maintainerRe: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again
In reply to Halil PasicQuoted 30 lines · On Tue, 19 Oct 2021 19:53:59 +0200 Halil Pasic <[email protected]> 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 inkvm_vcpu_block(), but this time with itskicked_maskbit 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_maskis cleared before we give up on re-dispatching the vcpu.Signed-off-by Halil Pasic <[email protected]>Reported-by Matthew Rosato <[email protected]>Fixes 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")--- arch/s390/kvm/kvm-s390.c | 1 + 1 file changed, 1 insertion(+)arch/s390/kvm/kvm-s390.c +0
-0
arch/s390/kvm/kvm-s390.c int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) · line 3366
Quoted 4 lines
arch/s390/kvm/kvm-s390.c +1
-0@@ -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?
Quoted 3 lines
arch/s390/kvm/kvm-s390.c +0
-0return kvm_s390_vcpu_has_irq (vcpu,0 ); }
Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again
Am 20.10.21 um 07:35 schrieb Claudio Imbrenda:
Quoted 41 lines
Quoted 30 lines · On Tue, 19 Oct 2021 19:53:59 +0200 Halil Pasic <[email protected]> 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 inkvm_vcpu_block(), but this time with itskicked_maskbit 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_maskis cleared before we give up on re-dispatching the vcpu.Signed-off-by Halil Pasic <[email protected]>Reported-by Matthew Rosato <[email protected]>Fixes 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")--- arch/s390/kvm/kvm-s390.c | 1 + 1 file changed, 1 insertion(+)arch/s390/kvm/kvm-s390.c +0
-0arch/s390/kvm/kvm-s390.c int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) · line 3366
Quoted 4 lines
arch/s390/kvm/kvm-s390.c +1
-0@@ -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?
Quoted 5 lines
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.
Quoted 5 lines
Quoted 4 lines
arch/s390/kvm/kvm-s390.c +0
-0return kvm_s390_vcpu_has_irq (vcpu,0 ); }
Claudio Imbrenda
maintainerRe: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again
Quoted 48 lines · On Wed, 20 Oct 2021 08:03:40 +0200 Christian Borntraeger…
Am 20.10.21 um 07:35 schrieb Claudio Imbrenda:
Quoted 41 lines
Quoted 30 lines · On Tue, 19 Oct 2021 19:53:59 +0200 Halil Pasic <[email protected]> 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 inkvm_vcpu_block(), but this time with itskicked_maskbit 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_maskis cleared before we give up on re-dispatching the vcpu.Signed-off-by Halil Pasic <[email protected]>Reported-by Matthew Rosato <[email protected]>Fixes 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")--- arch/s390/kvm/kvm-s390.c | 1 + 1 file changed, 1 insertion(+)arch/s390/kvm/kvm-s390.c +0
-0arch/s390/kvm/kvm-s390.c int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) · line 3366
Quoted 4 lines
arch/s390/kvm/kvm-s390.c +1
-0@@ -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?
Quoted 5 lines
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?
Quoted 6 lines
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
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
arch/s390/kvm/kvm-s390.c +0
-0return kvm_s390_vcpu_has_irq (vcpu,0 ); }
Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again
Quoted 4 lines · On Wed, 20 Oct 2021 08:08:16 +0200 Claudio Imbrenda <[email protected]>…
Quoted 4 lines
Quoted 4 lines
arch/s390/kvm/kvm-s390.c int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu) · line 3366
Quoted 1 line
arch/s390/kvm/kvm-s390.c +1
-0+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.
Quoted 14 lines
Quoted 7 lines
Quoted 6 lines
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?
Quoted 6 lines
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.
Quoted 2 lines
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
Claudio Imbrenda
maintainerRe: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again
On Wed, 20 Oct 2021 10:14:50 +0200
Halil Pasic <[email protected]> wrote:
[...]
Quoted 1 line
running, so I see no correctness issues there.
fair enough :)
Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again
In reply to Halil PasicQuoted 15 lines · 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 inkvm_vcpu_block(), but this time with itskicked_maskbit set. So next time around we wouldn't kick the vcpu form__airqs_kick_single_vcpu(), but would assume that we just kicked it.
Quoted 6 lines
Let us make sure the
kicked_maskis cleared before we give up on re-dispatching the vcpu.Signed-off-by Halil Pasic <[email protected]>Reported-by Matthew Rosato <[email protected]>Fixes 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
Quoted 17 lines
--- arch/s390/kvm/kvm-s390.c | 1 + 1 file changed, 1 insertion(+)arch/s390/kvm/kvm-s390.c +1
-0@@ -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 1/3] KVM: s390: clear kicked_mask before sleeping again
In reply to Halil PasicQuoted 15 lines · On Tue, 19 Oct 2021 19:53:59 +0200 Halil Pasic <[email protected]> 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 inkvm_vcpu_block(), but this time with itskicked_maskbit set. So next time around we wouldn't kick the vcpu form__airqs_kick_single_vcpu(), but would assume that we just kicked it.
Quoted 6 lines
Let us make sure the
kicked_maskis cleared before we give up on re-dispatching the vcpu.Signed-off-by Halil Pasic <[email protected]>Reported-by Matthew Rosato <[email protected]>Fixes 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
Quoted 16 lines
--- arch/s390/kvm/kvm-s390.c | 1 + 1 file changed, 1 insertion(+)arch/s390/kvm/kvm-s390.c +1
-0@@ -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 ); }
PATCH 2/3 KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu
In reply to Halil PasicChanging 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.
--- arch/s390/kvm/interrupt.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
arch/s390/kvm/interrupt.c +3 -2
@@ -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 ;
Signature · 1 line
2.25.1
Re: [PATCH 2/3] KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu
Am 19.10.21 um 19:54 schrieb Halil Pasic:
Quoted 8 lines
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 thedeliverable_mask.Signed-off-by Halil Pasic <[email protected]>Fixes 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
Quoted 26 lines
--- arch/s390/kvm/interrupt.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)arch/s390/kvm/interrupt.c +3
-2@@ -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 2/3] KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu
In reply to Halil PasicQuoted 8 lines · On Tue, 19 Oct 2021 19:54:00 +0200 Halil Pasic <[email protected]> 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 thedeliverable_mask.Signed-off-by Halil Pasic <[email protected]>Fixes 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
Quoted 25 lines
--- arch/s390/kvm/interrupt.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)arch/s390/kvm/interrupt.c +3
-2@@ -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 2/3] KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu
In reply to Halil PasicQuoted 8 lines · 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 thedeliverable_mask.Signed-off-by Halil Pasic <[email protected]>Fixes 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
Quoted 26 lines
--- arch/s390/kvm/interrupt.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)arch/s390/kvm/interrupt.c +3
-2@@ -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 ;
PATCH 3/3 KVM: s390: clear kicked_mask if not idle after set
In reply to Halil PasicThe 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().
--- arch/s390/kvm/interrupt.c | 7 ++++++- arch/s390/kvm/kvm-s390.c | 2 -- 2 files changed, 6 insertions(+), 3 deletions(-)
arch/s390/kvm/interrupt.c +6 -1
@@ -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 ; } }
arch/s390/kvm/kvm-s390.c +0 -2
@@ -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);
Signature · 1 line
2.25.1
Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set
Am 19.10.21 um 19:54 schrieb Halil Pasic:
Quoted 12 lines
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_maskwas examined, is not idle any more after thekicked_maskis 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.
Quoted 2 lines
To get things harmonized let us also move the clear from
vcpu_pre_run()to__unset_cpu_idle().
this part makes sense.
Quoted 21 lines
Signed-off-by Halil Pasic <[email protected]>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(-)arch/s390/kvm/interrupt.c +1
-0@@ -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)
arch/s390/kvm/interrupt.c static void __airqs_kick_single_vcpu(struct kvm *kvm, u8 deliverable_mask) · line 3072
Quoted 9 lines
arch/s390/kvm/interrupt.c +5
-1@@ -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?
Quoted 17 lines
arch/s390/kvm/interrupt.c +0
-0return ; } }arch/s390/kvm/kvm-s390.c +0
-2@@ -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
Am 19.10.21 um 23:35 schrieb Christian Borntraeger:
Quoted 42 lines
Am 19.10.21 um 19:54 schrieb Halil Pasic:
Quoted 12 lines
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_maskwas examined, is not idle any more after thekicked_maskis 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.Quoted 2 lines
To get things harmonized let us also move the clear from
vcpu_pre_run()to__unset_cpu_idle().this part makes sense.
Quoted 23 lines
Signed-off-by Halil Pasic <[email protected]>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(-)arch/s390/kvm/interrupt.c +1
-0@@ -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;
Quoted 8 lines
arch/s390/kvm/interrupt.c static void __airqs_kick_single_vcpu(struct kvm *kvm, u8 deliverable_mask) · line 3072
Quoted 6 lines
arch/s390/kvm/interrupt.c +5
-1- 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.
Quoted 25 lines
I think it might be safer to also clear
kicked_maskin__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 oldkick_maskbit.
Doesnt this cover your scenario?Quoted 16 lines
arch/s390/kvm/interrupt.c +0
-0return; } }arch/s390/kvm/kvm-s390.c +0
-2@@ -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
In reply to Christian BorntraegerQuoted 5 lines · On Tue, 19 Oct 2021 23:35:25 +0200 Christian Borntraeger…
arch/s390/kvm/interrupt.c static void __unset_cpu_idle(struct kvm_vcpu *vcpu) · line 429
Quoted 5 lines
arch/s390/kvm/interrupt.c +1
-0@@ -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.
Quoted 16 lines
arch/s390/kvm/interrupt.c static void __airqs_kick_single_vcpu(struct kvm *kvm, u8 deliverable_mask) · line 3072
Quoted 13 lines
arch/s390/kvm/interrupt.c +5
-1}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().
Quoted 2 lines
I think it might be safer to also clear
kicked_maskin__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();
} Quoted 5 lines
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 oldkick_maskbit.
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 3/3] KVM: s390: clear kicked_mask if not idle after set
Am 20.10.21 um 09:52 schrieb Halil Pasic:
Quoted 13 lines
Quoted 5 lines · On Tue, 19 Oct 2021 23:35:25 +0200 Christian Borntraeger…
arch/s390/kvm/interrupt.c static void __unset_cpu_idle(struct kvm_vcpu *vcpu) · line 429
Quoted 5 lines
arch/s390/kvm/interrupt.c +1
-0@@ -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 thekicked_maskbit 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.
Quoted 5 lines
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.
Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set
In reply to Halil PasicQuoted 12 lines · 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_maskwas examined, is not idle any more after thekicked_maskis 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.
Quoted 5 lines
To get things harmonized let us also move the clear from
vcpu_pre_run()to__unset_cpu_idle().Signed-off-by Halil Pasic <[email protected]>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.
Quoted 44 lines
--- arch/s390/kvm/interrupt.c | 7 ++++++- arch/s390/kvm/kvm-s390.c | 2 -- 2 files changed, 6 insertions(+), 3 deletions(-)arch/s390/kvm/interrupt.c +6
-1@@ -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 ; } }arch/s390/kvm/kvm-s390.c +0
-2@@ -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
Am 20.10.21 um 11:48 schrieb Michael Mueller:
Quoted 21 lines · Halil Pasic
Quoted 12 lines
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_maskwas examined, is not idle any more after thekicked_maskis 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.Quoted 5 lines
To get things harmonized let us also move the clear from
vcpu_pre_run()to__unset_cpu_idle().Signed-off-by Halil Pasic <[email protected]>Fixes 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
Quoted 6 lines
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_masklogic 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?
Quoted 44 lines
A similar run can be done with this code.
Quoted 42 lines
---
arch/s390/kvm/interrupt.c| 7 ++++++-
arch/s390/kvm/kvm-s390.c | 2 --2 files changed, 6 insertions(+), 3 deletions(-)arch/s390/kvm/interrupt.c +6
-1@@ -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; } }arch/s390/kvm/kvm-s390.c +0
-2@@ -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
Quoted 9 lines · On Wed, 20 Oct 2021 12:31:19 +0200 Christian Borntraeger…
Quoted 6 lines
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_masklogic 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
Am 20.10.21 um 12:45 schrieb Halil Pasic:
Quoted 10 lines
Quoted 7 lines · On Wed, 20 Oct 2021 12:31:19 +0200 Christian Borntraeger…
Quoted 6 lines
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_masklogic 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.
Quoted 7 lines
Quoted 2 lines
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 3/3] KVM: s390: clear kicked_mask if not idle after set
In reply to Christian BorntraegerQuoted 29 lines · Christian Borntraeger
Am 20.10.21 um 11:48 schrieb Michael Mueller:
Quoted 21 lines · Halil Pasic
Quoted 12 lines
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_maskwas examined, is not idle any more after thekicked_maskis 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.Quoted 5 lines
To get things harmonized let us also move the clear from
vcpu_pre_run()to__unset_cpu_idle().Signed-off-by Halil Pasic <[email protected]>Fixes 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")Quoted 6 lines
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_masklogic 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.
Quoted 2 lines
But I think patch 1 and 2 can go immediately as they measurably or testable fix things. Correct?
Yes
Quoted 46 lines
Quoted 46 lines
A similar run can be done with this code.
Quoted 43 lines
---
arch/s390/kvm/interrupt.c| 7 ++++++-
arch/s390/kvm/kvm-s390.c | 2 --2 files changed, 6 insertions(+), 3 deletions(-)arch/s390/kvm/interrupt.c +6
-1@@ -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; }}
arch/s390/kvm/kvm-s390.c +0
-2@@ -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);
Christian Borntraeger
AppliedAm 19.10.21 um 19:53 schrieb Halil Pasic:
Quoted 10 lines
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: clearkicked_maskbefore sleeping again
KVM: s390: preservedeliverable_maskin__airqs_kick_single_vcpu
KVM: s390: clearkicked_maskif not idle after set
Quoted 6 lines
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.
Quoted 16 lines · On Wed, 20 Oct 2021 13:04:05 +0200 Christian Borntraeger…
Am 19.10.21 um 19:53 schrieb Halil Pasic:
Quoted 15 lines
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: clearkicked_maskbefore sleeping again
KVM: s390: preservedeliverable_maskin__airqs_kick_single_vcpu
KVM: s390: clearkicked_maskif not idle after setarch/s390/kvm/interrupt.c | 12 +++++++++--- arch/s390/kvm/kvm-s390.c | 3 +-- 2 files changed, 10 insertions(+), 5 deletions(-)
Quoted 4 lines
Quoted 1 line
base-commit:
519d81956ee277b4419c723adfb154603c2565baI 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