| Message ID | 1454516585-28491-5-git-send-email-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/02/2016 17:23, Radim Kr?má? wrote: > Discard policy doesn't rely on information from notifiers, so we don't > need to register notifiers unconditionally. > > Use of ps->lock doesn't make sense, but isn't any worse than before. Oh, it's perfectly okay. Too fine-grained locks are bad, and lock contention on ps->lock is a non-issue. Can you however add a patch that says what fields of kvm_kpit_state are protected by which locks? Then this patch will just add /* Protected by kvm_kpit_state lock. */ above the reinject field. Otherwise Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com> > --- > arch/x86/kvm/i8254.c | 44 ++++++++++++++++++++++++++++++-------------- > arch/x86/kvm/i8254.h | 1 + > arch/x86/kvm/x86.c | 6 +++--- > 3 files changed, 34 insertions(+), 17 deletions(-) > > diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c > index fc554fbf71a7..f1dfc250bbe0 100644 > --- a/arch/x86/kvm/i8254.c > +++ b/arch/x86/kvm/i8254.c > @@ -237,9 +237,6 @@ static void kvm_pit_ack_irq(struct kvm_irq_ack_notifier *kian) > struct kvm_kpit_state *ps = container_of(kian, struct kvm_kpit_state, > irq_ack_notifier); > > - if (!ps->reinject) > - return; > - > atomic_set(&ps->irq_ack, 1); > if (atomic_add_unless(&ps->pending, -1, 0)) > /* in this case, we had multiple outstanding pit interrupts > @@ -708,14 +705,13 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags) > 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; > mutex_unlock(&pit->pit_state.lock); > > - 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_pit_reset(pit); > > kvm_iodevice_init(&pit->dev, &pit_dev_ops); > ret = kvm_io_bus_register_dev(kvm, KVM_PIO_BUS, KVM_PIT_BASE_ADDRESS, > @@ -738,14 +734,37 @@ 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); > return NULL; > } > > +void kvm_pit_set_reinject(struct kvm_pit *pit, bool reinject) > +{ > + /* Implicit BUG on NULL dereference. */ > + struct kvm_kpit_state *ps = &pit->pit_state; > + struct kvm *kvm = pit->kvm; > + > + mutex_lock(&ps->lock); > + > + if (ps->reinject == reinject) > + goto out; > + > + ps->reinject = reinject; > + if (reinject) { > + 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); > + } > +out: > + mutex_unlock(&ps->lock); > +} > + > void kvm_free_pit(struct kvm *kvm) > { > struct hrtimer *timer; > @@ -754,10 +773,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); > mutex_lock(&kvm->arch.vpit->pit_state.lock); > timer = &kvm->arch.vpit->pit_state.timer; > hrtimer_cancel(timer); > diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h > index f8cf4b84f435..7ac94bc0799f 100644 > --- a/arch/x86/kvm/i8254.h > +++ b/arch/x86/kvm/i8254.h > @@ -60,5 +60,6 @@ void kvm_pit_load_count(struct kvm *kvm, int channel, u32 val, int hpet_legacy_s > struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags); > void kvm_free_pit(struct kvm *kvm); > void kvm_pit_reset(struct kvm_pit *pit); > +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 5b937fdebc66..6bc03141e809 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3652,9 +3652,9 @@ static int kvm_vm_ioctl_reinject(struct kvm *kvm, > { > if (!kvm->arch.vpit) > return -ENXIO; > - mutex_lock(&kvm->arch.vpit->pit_state.lock); > - kvm->arch.vpit->pit_state.reinject = control->pit_reinject; > - mutex_unlock(&kvm->arch.vpit->pit_state.lock); > + > + kvm_pit_set_reinject(kvm->arch.vpit, control->pit_reinject); > + > return 0; > } > > -- 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-03 17:51+0100, Paolo Bonzini: > On 03/02/2016 17:23, Radim Kr?má? wrote: >> Discard policy doesn't rely on information from notifiers, so we don't >> need to register notifiers unconditionally. >> >> Use of ps->lock doesn't make sense, but isn't any worse than before. Oops, it is worse than before ... toggling KVM_REINJECT_CONTROL when the guest is running and reading reinject without locking is now far more complex. This patch should have also ignored KVM_REINJECT_CONTROL when PIT has been started. > Oh, it's perfectly okay. Too fine-grained locks are bad, and lock > contention on ps->lock is a non-issue. > > Can you however add a patch that says what fields of kvm_kpit_state are > protected by which locks? Ok. (I'll be careful to not rewrite the whole PIT while at it. :]) > Then this patch will just add > > /* Protected by kvm_kpit_state lock. */ > > above the reinject field. There was no need to lock reinject in the past and v2 will hopefully achieve it again. > Otherwise > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Thanks. (Might not be applicable to v2, though; sorry.) -- 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 fc554fbf71a7..f1dfc250bbe0 100644 --- a/arch/x86/kvm/i8254.c +++ b/arch/x86/kvm/i8254.c @@ -237,9 +237,6 @@ static void kvm_pit_ack_irq(struct kvm_irq_ack_notifier *kian) struct kvm_kpit_state *ps = container_of(kian, struct kvm_kpit_state, irq_ack_notifier); - if (!ps->reinject) - return; - atomic_set(&ps->irq_ack, 1); if (atomic_add_unless(&ps->pending, -1, 0)) /* in this case, we had multiple outstanding pit interrupts @@ -708,14 +705,13 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags) 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; mutex_unlock(&pit->pit_state.lock); - 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_pit_reset(pit); kvm_iodevice_init(&pit->dev, &pit_dev_ops); ret = kvm_io_bus_register_dev(kvm, KVM_PIO_BUS, KVM_PIT_BASE_ADDRESS, @@ -738,14 +734,37 @@ 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); return NULL; } +void kvm_pit_set_reinject(struct kvm_pit *pit, bool reinject) +{ + /* Implicit BUG on NULL dereference. */ + struct kvm_kpit_state *ps = &pit->pit_state; + struct kvm *kvm = pit->kvm; + + mutex_lock(&ps->lock); + + if (ps->reinject == reinject) + goto out; + + ps->reinject = reinject; + if (reinject) { + 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); + } +out: + mutex_unlock(&ps->lock); +} + void kvm_free_pit(struct kvm *kvm) { struct hrtimer *timer; @@ -754,10 +773,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); mutex_lock(&kvm->arch.vpit->pit_state.lock); timer = &kvm->arch.vpit->pit_state.timer; hrtimer_cancel(timer); diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h index f8cf4b84f435..7ac94bc0799f 100644 --- a/arch/x86/kvm/i8254.h +++ b/arch/x86/kvm/i8254.h @@ -60,5 +60,6 @@ void kvm_pit_load_count(struct kvm *kvm, int channel, u32 val, int hpet_legacy_s struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags); void kvm_free_pit(struct kvm *kvm); void kvm_pit_reset(struct kvm_pit *pit); +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 5b937fdebc66..6bc03141e809 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3652,9 +3652,9 @@ static int kvm_vm_ioctl_reinject(struct kvm *kvm, { if (!kvm->arch.vpit) return -ENXIO; - mutex_lock(&kvm->arch.vpit->pit_state.lock); - kvm->arch.vpit->pit_state.reinject = control->pit_reinject; - mutex_unlock(&kvm->arch.vpit->pit_state.lock); + + kvm_pit_set_reinject(kvm->arch.vpit, control->pit_reinject); + return 0; }
Discard policy doesn't rely on information from notifiers, so we don't need to register notifiers unconditionally. Use of ps->lock doesn't make sense, but isn't any worse than before. Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com> --- arch/x86/kvm/i8254.c | 44 ++++++++++++++++++++++++++++++-------------- arch/x86/kvm/i8254.h | 1 + arch/x86/kvm/x86.c | 6 +++--- 3 files changed, 34 insertions(+), 17 deletions(-)