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
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 <[email protected]>
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
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 <[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(-)
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
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 <[email protected]>
> Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
Reviewed-by: Christian Borntraeger <[email protected]>
> ---
> 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;
>
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 <[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(-)
>
> 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);
>
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 <[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(-)
>>
>> 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);
>>
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 the
> deliverable_mask.
>
> Signed-off-by: Halil Pasic <[email protected]>
> Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
Reviewed-by: Claudio Imbrenda <[email protected]>
> ---
> 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;
On Tue, 19 Oct 2021 23:35:25 +0200
Christian Borntraeger <[email protected]> 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
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 <[email protected]>
> Fixes: 9f30f6216378 ("KVM: s390: add gib_alert_irq_handler()")
Reviewed-by: Michael Mueller <[email protected]>
> ---
> 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;
>
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 <[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.
> ---
> 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);
>
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 <[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.
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);
>>
On Wed, 20 Oct 2021 12:31:19 +0200
Christian Borntraeger <[email protected]> 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
Am 20.10.21 um 12:45 schrieb Halil Pasic:
> On Wed, 20 Oct 2021 12:31:19 +0200
> Christian Borntraeger <[email protected]> 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.
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 <[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.
>
> 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);
>>>
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.
On Wed, 20 Oct 2021 13:04:05 +0200
Christian Borntraeger <[email protected]> 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
Am 20.10.21 um 09:52 schrieb Halil Pasic:
> On Tue, 19 Oct 2021 23:35:25 +0200
> Christian Borntraeger <[email protected]> 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.
>