* [PATCH 0/3] fixes for __airqs_kick_single_vcpu()
@ 2021-10-19 17:53 Halil Pasic
2021-10-19 17:53 ` [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again Halil Pasic
` (3 more replies)
0 siblings, 4 replies; 26+ messages in thread
From: Halil Pasic @ 2021-10-19 17:53 UTC (permalink / raw)
To: Christian Borntraeger, Janosch Frank, Michael Mueller,
linux-s390, linux-kernel
Cc: Halil Pasic, David Hildenbrand, Claudio Imbrenda, Heiko Carstens,
Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak,
Matthew Rosato, Niklas Schnelle, farman, kvm
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
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again 2021-10-19 17:53 [PATCH 0/3] fixes for __airqs_kick_single_vcpu() Halil Pasic @ 2021-10-19 17:53 ` Halil Pasic 2021-10-19 21:22 ` Christian Borntraeger ` (3 more replies) 2021-10-19 17:54 ` [PATCH 2/3] KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu Halil Pasic ` (2 subsequent siblings) 3 siblings, 4 replies; 26+ messages in thread From: Halil Pasic @ 2021-10-19 17:53 UTC (permalink / raw) To: Christian Borntraeger, Janosch Frank, Michael Mueller, linux-s390, linux-kernel Cc: Halil Pasic, Matthew Rosato, David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Niklas Schnelle, farman, kvm 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again 2021-10-19 17:53 ` [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again Halil Pasic @ 2021-10-19 21:22 ` Christian Borntraeger 2021-10-20 5:35 ` Claudio Imbrenda ` (2 subsequent siblings) 3 siblings, 0 replies; 26+ messages in thread From: Christian Borntraeger @ 2021-10-19 21:22 UTC (permalink / raw) To: Halil Pasic, Janosch Frank, Michael Mueller, linux-s390, linux-kernel Cc: Matthew Rosato, David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Niklas Schnelle, farman, kvm 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); > } > > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again 2021-10-19 17:53 ` [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again Halil Pasic 2021-10-19 21:22 ` Christian Borntraeger @ 2021-10-20 5:35 ` Claudio Imbrenda 2021-10-20 6:03 ` Christian Borntraeger 2021-10-20 8:06 ` Michael Mueller 2021-10-20 10:44 ` Claudio Imbrenda 3 siblings, 1 reply; 26+ messages in thread From: Claudio Imbrenda @ 2021-10-20 5:35 UTC (permalink / raw) To: Halil Pasic Cc: Christian Borntraeger, Janosch Frank, Michael Mueller, linux-s390, linux-kernel, Matthew Rosato, David Hildenbrand, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Niklas Schnelle, farman, kvm 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); > } > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again 2021-10-20 5:35 ` Claudio Imbrenda @ 2021-10-20 6:03 ` Christian Borntraeger 2021-10-20 6:08 ` Claudio Imbrenda 0 siblings, 1 reply; 26+ messages in thread From: Christian Borntraeger @ 2021-10-20 6:03 UTC (permalink / raw) To: Claudio Imbrenda, Halil Pasic Cc: Janosch Frank, Michael Mueller, linux-s390, linux-kernel, Matthew Rosato, David Hildenbrand, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Niklas Schnelle, farman, kvm 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); >> } >> > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again 2021-10-20 6:03 ` Christian Borntraeger @ 2021-10-20 6:08 ` Claudio Imbrenda 2021-10-20 8:14 ` Halil Pasic 0 siblings, 1 reply; 26+ messages in thread From: Claudio Imbrenda @ 2021-10-20 6:08 UTC (permalink / raw) To: Christian Borntraeger Cc: Halil Pasic, Janosch Frank, Michael Mueller, linux-s390, linux-kernel, Matthew Rosato, David Hildenbrand, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Niklas Schnelle, farman, kvm 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); > >> } > >> > > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again 2021-10-20 6:08 ` Claudio Imbrenda @ 2021-10-20 8:14 ` Halil Pasic 2021-10-20 10:42 ` Claudio Imbrenda 0 siblings, 1 reply; 26+ messages in thread From: Halil Pasic @ 2021-10-20 8:14 UTC (permalink / raw) To: Claudio Imbrenda Cc: Christian Borntraeger, Janosch Frank, Michael Mueller, linux-s390, linux-kernel, Matthew Rosato, David Hildenbrand, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Niklas Schnelle, farman, kvm, 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again 2021-10-20 8:14 ` Halil Pasic @ 2021-10-20 10:42 ` Claudio Imbrenda 0 siblings, 0 replies; 26+ messages in thread From: Claudio Imbrenda @ 2021-10-20 10:42 UTC (permalink / raw) To: Halil Pasic Cc: Christian Borntraeger, Janosch Frank, Michael Mueller, linux-s390, linux-kernel, Matthew Rosato, David Hildenbrand, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Niklas Schnelle, farman, kvm 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 :) ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again 2021-10-19 17:53 ` [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again Halil Pasic 2021-10-19 21:22 ` Christian Borntraeger 2021-10-20 5:35 ` Claudio Imbrenda @ 2021-10-20 8:06 ` Michael Mueller 2021-10-20 10:44 ` Claudio Imbrenda 3 siblings, 0 replies; 26+ messages in thread From: Michael Mueller @ 2021-10-20 8:06 UTC (permalink / raw) To: Halil Pasic, Christian Borntraeger, Janosch Frank, linux-s390, linux-kernel Cc: Matthew Rosato, David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Niklas Schnelle, farman, kvm 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); > } > > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again 2021-10-19 17:53 ` [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again Halil Pasic ` (2 preceding siblings ...) 2021-10-20 8:06 ` Michael Mueller @ 2021-10-20 10:44 ` Claudio Imbrenda 3 siblings, 0 replies; 26+ messages in thread From: Claudio Imbrenda @ 2021-10-20 10:44 UTC (permalink / raw) To: Halil Pasic Cc: Christian Borntraeger, Janosch Frank, Michael Mueller, linux-s390, linux-kernel, Matthew Rosato, David Hildenbrand, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Niklas Schnelle, farman, kvm 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); > } > ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/3] KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu 2021-10-19 17:53 [PATCH 0/3] fixes for __airqs_kick_single_vcpu() Halil Pasic 2021-10-19 17:53 ` [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again Halil Pasic @ 2021-10-19 17:54 ` Halil Pasic 2021-10-19 21:24 ` Christian Borntraeger ` (2 more replies) 2021-10-19 17:54 ` [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set Halil Pasic 2021-10-20 11:04 ` [PATCH 0/3] fixes for __airqs_kick_single_vcpu() Christian Borntraeger 3 siblings, 3 replies; 26+ messages in thread From: Halil Pasic @ 2021-10-19 17:54 UTC (permalink / raw) To: Christian Borntraeger, Janosch Frank, Michael Mueller, linux-s390, linux-kernel Cc: Halil Pasic, David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Matthew Rosato, Niklas Schnelle, farman, kvm 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 2/3] KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu 2021-10-19 17:54 ` [PATCH 2/3] KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu Halil Pasic @ 2021-10-19 21:24 ` Christian Borntraeger 2021-10-20 5:39 ` Claudio Imbrenda 2021-10-20 8:08 ` Michael Mueller 2 siblings, 0 replies; 26+ messages in thread From: Christian Borntraeger @ 2021-10-19 21:24 UTC (permalink / raw) To: Halil Pasic, Janosch Frank, Michael Mueller, linux-s390, linux-kernel Cc: David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Matthew Rosato, Niklas Schnelle, farman, kvm 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; > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/3] KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu 2021-10-19 17:54 ` [PATCH 2/3] KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu Halil Pasic 2021-10-19 21:24 ` Christian Borntraeger @ 2021-10-20 5:39 ` Claudio Imbrenda 2021-10-20 8:08 ` Michael Mueller 2 siblings, 0 replies; 26+ messages in thread From: Claudio Imbrenda @ 2021-10-20 5:39 UTC (permalink / raw) To: Halil Pasic Cc: Christian Borntraeger, Janosch Frank, Michael Mueller, linux-s390, linux-kernel, David Hildenbrand, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Matthew Rosato, Niklas Schnelle, farman, kvm 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; ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/3] KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu 2021-10-19 17:54 ` [PATCH 2/3] KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu Halil Pasic 2021-10-19 21:24 ` Christian Borntraeger 2021-10-20 5:39 ` Claudio Imbrenda @ 2021-10-20 8:08 ` Michael Mueller 2 siblings, 0 replies; 26+ messages in thread From: Michael Mueller @ 2021-10-20 8:08 UTC (permalink / raw) To: Halil Pasic, Christian Borntraeger, Janosch Frank, linux-s390, linux-kernel Cc: David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Matthew Rosato, Niklas Schnelle, farman, kvm 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; > ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set 2021-10-19 17:53 [PATCH 0/3] fixes for __airqs_kick_single_vcpu() Halil Pasic 2021-10-19 17:53 ` [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again Halil Pasic 2021-10-19 17:54 ` [PATCH 2/3] KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu Halil Pasic @ 2021-10-19 17:54 ` Halil Pasic 2021-10-19 21:35 ` Christian Borntraeger 2021-10-20 9:48 ` Michael Mueller 2021-10-20 11:04 ` [PATCH 0/3] fixes for __airqs_kick_single_vcpu() Christian Borntraeger 3 siblings, 2 replies; 26+ messages in thread From: Halil Pasic @ 2021-10-19 17:54 UTC (permalink / raw) To: Christian Borntraeger, Janosch Frank, Michael Mueller, linux-s390, linux-kernel Cc: Halil Pasic, David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Matthew Rosato, Niklas Schnelle, farman, kvm 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set 2021-10-19 17:54 ` [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set Halil Pasic @ 2021-10-19 21:35 ` Christian Borntraeger 2021-10-20 5:14 ` Christian Borntraeger 2021-10-20 7:52 ` Halil Pasic 2021-10-20 9:48 ` Michael Mueller 1 sibling, 2 replies; 26+ messages in thread From: Christian Borntraeger @ 2021-10-19 21:35 UTC (permalink / raw) To: Halil Pasic, Janosch Frank, Michael Mueller, linux-s390, linux-kernel Cc: David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Matthew Rosato, Niklas Schnelle, farman, kvm 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); > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set 2021-10-19 21:35 ` Christian Borntraeger @ 2021-10-20 5:14 ` Christian Borntraeger 2021-10-20 7:52 ` Halil Pasic 1 sibling, 0 replies; 26+ messages in thread From: Christian Borntraeger @ 2021-10-20 5:14 UTC (permalink / raw) To: Halil Pasic, Janosch Frank, Michael Mueller, linux-s390, linux-kernel Cc: David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Matthew Rosato, Niklas Schnelle, farman, kvm 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); >> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set 2021-10-19 21:35 ` Christian Borntraeger 2021-10-20 5:14 ` Christian Borntraeger @ 2021-10-20 7:52 ` Halil Pasic 2021-10-26 8:52 ` Christian Borntraeger 1 sibling, 1 reply; 26+ messages in thread From: Halil Pasic @ 2021-10-20 7:52 UTC (permalink / raw) To: Christian Borntraeger Cc: Janosch Frank, Michael Mueller, linux-s390, linux-kernel, David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Matthew Rosato, Niklas Schnelle, farman, kvm, 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set 2021-10-20 7:52 ` Halil Pasic @ 2021-10-26 8:52 ` Christian Borntraeger 0 siblings, 0 replies; 26+ messages in thread From: Christian Borntraeger @ 2021-10-26 8:52 UTC (permalink / raw) To: Halil Pasic Cc: Janosch Frank, Michael Mueller, linux-s390, linux-kernel, David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Matthew Rosato, Niklas Schnelle, farman, kvm 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. > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set 2021-10-19 17:54 ` [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set Halil Pasic 2021-10-19 21:35 ` Christian Borntraeger @ 2021-10-20 9:48 ` Michael Mueller 2021-10-20 10:31 ` Christian Borntraeger 1 sibling, 1 reply; 26+ messages in thread From: Michael Mueller @ 2021-10-20 9:48 UTC (permalink / raw) To: Halil Pasic, Christian Borntraeger, Janosch Frank, linux-s390, linux-kernel Cc: David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Matthew Rosato, Niklas Schnelle, farman, kvm 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); > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set 2021-10-20 9:48 ` Michael Mueller @ 2021-10-20 10:31 ` Christian Borntraeger 2021-10-20 10:45 ` Halil Pasic 2021-10-20 10:51 ` Michael Mueller 0 siblings, 2 replies; 26+ messages in thread From: Christian Borntraeger @ 2021-10-20 10:31 UTC (permalink / raw) To: Michael Mueller, Halil Pasic, Janosch Frank, linux-s390, linux-kernel Cc: David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Matthew Rosato, Niklas Schnelle, farman, kvm 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); >> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set 2021-10-20 10:31 ` Christian Borntraeger @ 2021-10-20 10:45 ` Halil Pasic 2021-10-20 10:52 ` Christian Borntraeger 2021-10-20 10:51 ` Michael Mueller 1 sibling, 1 reply; 26+ messages in thread From: Halil Pasic @ 2021-10-20 10:45 UTC (permalink / raw) To: Christian Borntraeger Cc: Michael Mueller, Janosch Frank, linux-s390, linux-kernel, David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Matthew Rosato, Niklas Schnelle, farman, kvm, 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set 2021-10-20 10:45 ` Halil Pasic @ 2021-10-20 10:52 ` Christian Borntraeger 0 siblings, 0 replies; 26+ messages in thread From: Christian Borntraeger @ 2021-10-20 10:52 UTC (permalink / raw) To: Halil Pasic Cc: Michael Mueller, Janosch Frank, linux-s390, linux-kernel, David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Matthew Rosato, Niklas Schnelle, farman, kvm 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set 2021-10-20 10:31 ` Christian Borntraeger 2021-10-20 10:45 ` Halil Pasic @ 2021-10-20 10:51 ` Michael Mueller 1 sibling, 0 replies; 26+ messages in thread From: Michael Mueller @ 2021-10-20 10:51 UTC (permalink / raw) To: Christian Borntraeger, Halil Pasic, Janosch Frank, linux-s390, linux-kernel Cc: David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Matthew Rosato, Niklas Schnelle, farman, kvm 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); >>> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] fixes for __airqs_kick_single_vcpu() 2021-10-19 17:53 [PATCH 0/3] fixes for __airqs_kick_single_vcpu() Halil Pasic ` (2 preceding siblings ...) 2021-10-19 17:54 ` [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set Halil Pasic @ 2021-10-20 11:04 ` Christian Borntraeger 2021-10-20 12:12 ` Halil Pasic 3 siblings, 1 reply; 26+ messages in thread From: Christian Borntraeger @ 2021-10-20 11:04 UTC (permalink / raw) To: Halil Pasic, Janosch Frank, Michael Mueller, linux-s390, linux-kernel Cc: David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Matthew Rosato, Niklas Schnelle, farman, kvm 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] fixes for __airqs_kick_single_vcpu() 2021-10-20 11:04 ` [PATCH 0/3] fixes for __airqs_kick_single_vcpu() Christian Borntraeger @ 2021-10-20 12:12 ` Halil Pasic 0 siblings, 0 replies; 26+ messages in thread From: Halil Pasic @ 2021-10-20 12:12 UTC (permalink / raw) To: Christian Borntraeger Cc: Janosch Frank, Michael Mueller, linux-s390, linux-kernel, David Hildenbrand, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Pierre Morel, Tony Krowiak, Matthew Rosato, Niklas Schnelle, farman, kvm, 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2021-10-26 8:52 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-19 17:53 [PATCH 0/3] fixes for __airqs_kick_single_vcpu() Halil Pasic 2021-10-19 17:53 ` [PATCH 1/3] KVM: s390: clear kicked_mask before sleeping again Halil Pasic 2021-10-19 21:22 ` Christian Borntraeger 2021-10-20 5:35 ` Claudio Imbrenda 2021-10-20 6:03 ` Christian Borntraeger 2021-10-20 6:08 ` Claudio Imbrenda 2021-10-20 8:14 ` Halil Pasic 2021-10-20 10:42 ` Claudio Imbrenda 2021-10-20 8:06 ` Michael Mueller 2021-10-20 10:44 ` Claudio Imbrenda 2021-10-19 17:54 ` [PATCH 2/3] KVM: s390: preserve deliverable_mask in __airqs_kick_single_vcpu Halil Pasic 2021-10-19 21:24 ` Christian Borntraeger 2021-10-20 5:39 ` Claudio Imbrenda 2021-10-20 8:08 ` Michael Mueller 2021-10-19 17:54 ` [PATCH 3/3] KVM: s390: clear kicked_mask if not idle after set Halil Pasic 2021-10-19 21:35 ` Christian Borntraeger 2021-10-20 5:14 ` Christian Borntraeger 2021-10-20 7:52 ` Halil Pasic 2021-10-26 8:52 ` Christian Borntraeger 2021-10-20 9:48 ` Michael Mueller 2021-10-20 10:31 ` Christian Borntraeger 2021-10-20 10:45 ` Halil Pasic 2021-10-20 10:52 ` Christian Borntraeger 2021-10-20 10:51 ` Michael Mueller 2021-10-20 11:04 ` [PATCH 0/3] fixes for __airqs_kick_single_vcpu() Christian Borntraeger 2021-10-20 12:12 ` Halil Pasic
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).