| Message ID | 1455736496-374-9-git-send-email-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17/02/2016 20:14, Radim Kr?má? wrote: > Discard policy doesn't rely on information from notifiers, so we don't > need to register notifiers unconditionally. We kept correct counts in > case userspace switched between policies during runtime, but that can be > avoided by reseting the state. Ah okay so the WARN_ON could actually be triggered. But otherwise I think my suggestion should work. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17/02/2016 20:14, Radim Kr?má? wrote: > + /* pit->pit_state.lock was overloaded to prevent userspace from getting > + * an inconsistent state after running multiple KVM_REINJECT_CONTROL > + * ioctls in parallel. Use a separate lock if that ioctl isn't rare. > + */ > + mutex_lock(&pit->pit_state.lock); > + kvm_pit_set_reinject(pit, control->pit_reinject); > + mutex_unlock(&pit->pit_state.lock); ... so in patch 7 concurrent _writes_ of reinject are protected by the lock, but reads are done outside it (in pit_timer_fn). WDYT about making reinject an atomic_t? Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-02-18 19:08+0100, Paolo Bonzini: > On 17/02/2016 20:14, Radim Kr?má? wrote: >> + /* pit->pit_state.lock was overloaded to prevent userspace from getting >> + * an inconsistent state after running multiple KVM_REINJECT_CONTROL >> + * ioctls in parallel. Use a separate lock if that ioctl isn't rare. >> + */ >> + mutex_lock(&pit->pit_state.lock); >> + kvm_pit_set_reinject(pit, control->pit_reinject); >> + mutex_unlock(&pit->pit_state.lock); > > ... so in patch 7 concurrent _writes_ of reinject are protected by the > lock, but reads are done outside it (in pit_timer_fn). WDYT about > making reinject an atomic_t? There was/is no harm in having reinject non-atomic. This patch added notifiers, which is the reason for re-introducing a mutex. Userspace can (and SHOULDN'T) call this function multiple times, concurrently, so the mutex prevents a situations where, e.g. only one notifier is registered in the end. I thought about really stupid stuff when doing this series ... -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19/02/2016 16:04, Radim Kr?má? wrote: >> > >> > ... so in patch 7 concurrent _writes_ of reinject are protected by the >> > lock, but reads are done outside it (in pit_timer_fn). WDYT about >> > making reinject an atomic_t? > There was/is no harm in having reinject non-atomic. This patch added > notifiers, which is the reason for re-introducing a mutex. > > Userspace can (and SHOULDN'T) call this function multiple times, > concurrently, so the mutex prevents a situations where, e.g. only one > notifier is registered in the end. Yes, I understand why you added the mutex here; good catch indeed. The atomic_t is just to show that it's okay to read it outside the lock. It's just for a bit of extra documentation. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-02-19 16:06+0100, Paolo Bonzini: > On 19/02/2016 16:04, Radim Kr?má? wrote: >>>> ... so in patch 7 concurrent _writes_ of reinject are protected by the >>>> lock, but reads are done outside it (in pit_timer_fn). WDYT about >>>> making reinject an atomic_t? >> There was/is no harm in having reinject non-atomic. This patch added >> notifiers, which is the reason for re-introducing a mutex. >> >> Userspace can (and SHOULDN'T) call this function multiple times, >> concurrently, so the mutex prevents a situations where, e.g. only one >> notifier is registered in the end. > > Yes, I understand why you added the mutex here; good catch indeed. The > atomic_t is just to show that it's okay to read it outside the lock. > It's just for a bit of extra documentation. Hm, good point. I will add it. Thanks. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c index 01f9c70ebc21..e514f7fa0093 100644 --- a/arch/x86/kvm/i8254.c +++ b/arch/x86/kvm/i8254.c @@ -225,7 +225,7 @@ static void kvm_pit_ack_irq(struct kvm_irq_ack_notifier *kian) * inc(pending) in pit_timer_fn and xchg(irq_ack, 0) in pit_do_work. */ smp_mb(); - if (atomic_dec_if_positive(&ps->pending) > 0 && ps->reinject) + if (atomic_dec_if_positive(&ps->pending) > 0) queue_kthread_work(&ps->pit->worker, &ps->pit->expired); } @@ -301,6 +301,27 @@ static inline void kvm_pit_reset_reinject(struct kvm_pit *pit) atomic_set(&pit->pit_state.irq_ack, 1); } +void kvm_pit_set_reinject(struct kvm_pit *pit, bool reinject) +{ + struct kvm_kpit_state *ps = &pit->pit_state; + struct kvm *kvm = pit->kvm; + + if (ps->reinject == reinject) + return; + + if (reinject) { + /* The initial state is preserved while ps->reinject == 0. */ + kvm_pit_reset_reinject(pit); + kvm_register_irq_ack_notifier(kvm, &ps->irq_ack_notifier); + kvm_register_irq_mask_notifier(kvm, 0, &pit->mask_notifier); + } else { + kvm_unregister_irq_ack_notifier(kvm, &ps->irq_ack_notifier); + kvm_unregister_irq_mask_notifier(kvm, 0, &pit->mask_notifier); + } + + ps->reinject = reinject; +} + static void create_pit_timer(struct kvm_pit *pit, u32 val, int is_period) { struct kvm_kpit_state *ps = &pit->pit_state; @@ -681,15 +702,14 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags) pit_state = &pit->pit_state; pit_state->pit = pit; hrtimer_init(&pit_state->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS); + pit_state->irq_ack_notifier.gsi = 0; pit_state->irq_ack_notifier.irq_acked = kvm_pit_ack_irq; - kvm_register_irq_ack_notifier(kvm, &pit_state->irq_ack_notifier); - pit_state->reinject = true; + pit->mask_notifier.func = pit_mask_notifer; kvm_pit_reset(pit); - pit->mask_notifier.func = pit_mask_notifer; - kvm_register_irq_mask_notifier(kvm, 0, &pit->mask_notifier); + kvm_pit_set_reinject(pit, true); kvm_iodevice_init(&pit->dev, &pit_dev_ops); ret = kvm_io_bus_register_dev(kvm, KVM_PIO_BUS, KVM_PIT_BASE_ADDRESS, @@ -712,8 +732,7 @@ fail_unregister: kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &pit->dev); fail: - kvm_unregister_irq_mask_notifier(kvm, 0, &pit->mask_notifier); - kvm_unregister_irq_ack_notifier(kvm, &pit_state->irq_ack_notifier); + kvm_pit_set_reinject(pit, false); kvm_free_irq_source_id(kvm, pit->irq_source_id); kthread_stop(pit->worker_task); kfree(pit); @@ -728,10 +747,7 @@ void kvm_free_pit(struct kvm *kvm) kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &kvm->arch.vpit->dev); kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &kvm->arch.vpit->speaker_dev); - kvm_unregister_irq_mask_notifier(kvm, 0, - &kvm->arch.vpit->mask_notifier); - kvm_unregister_irq_ack_notifier(kvm, - &kvm->arch.vpit->pit_state.irq_ack_notifier); + kvm_pit_set_reinject(kvm->arch.vpit, false); timer = &kvm->arch.vpit->pit_state.timer; hrtimer_cancel(timer); flush_kthread_work(&kvm->arch.vpit->expired); diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h index 568d87ec3698..47a96e803aab 100644 --- a/arch/x86/kvm/i8254.h +++ b/arch/x86/kvm/i8254.h @@ -63,5 +63,6 @@ void kvm_free_pit(struct kvm *kvm); void kvm_pit_load_count(struct kvm_pit *pit, int channel, u32 val, int hpet_legacy_start); +void kvm_pit_set_reinject(struct kvm_pit *pit, bool reinject); #endif diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index f2f7ba74da6b..785cc781673d 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3683,10 +3683,18 @@ static int kvm_vm_ioctl_set_pit2(struct kvm *kvm, struct kvm_pit_state2 *ps) static int kvm_vm_ioctl_reinject(struct kvm *kvm, struct kvm_reinject_control *control) { - if (!kvm->arch.vpit) + struct kvm_pit *pit = kvm->arch.vpit; + + if (!pit) return -ENXIO; - kvm->arch.vpit->pit_state.reinject = control->pit_reinject; + /* pit->pit_state.lock was overloaded to prevent userspace from getting + * an inconsistent state after running multiple KVM_REINJECT_CONTROL + * ioctls in parallel. Use a separate lock if that ioctl isn't rare. + */ + mutex_lock(&pit->pit_state.lock); + kvm_pit_set_reinject(pit, control->pit_reinject); + mutex_unlock(&pit->pit_state.lock); return 0; }
Discard policy doesn't rely on information from notifiers, so we don't need to register notifiers unconditionally. We kept correct counts in case userspace switched between policies during runtime, but that can be avoided by reseting the state. Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com> --- v2: * lock the ioctl * rebase arch/x86/kvm/i8254.c | 38 +++++++++++++++++++++++++++----------- arch/x86/kvm/i8254.h | 1 + arch/x86/kvm/x86.c | 12 ++++++++++-- 3 files changed, 38 insertions(+), 13 deletions(-)