From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH 4/4] KVM: x86: remove notifiers from PIT discard policy Date: Wed, 3 Feb 2016 17:51:21 +0100 Message-ID: <56B23009.1020109@redhat.com> References: <1454516585-28491-1-git-send-email-rkrcmar@redhat.com> <1454516585-28491-5-git-send-email-rkrcmar@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org, Yuki Shibuya To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , linux-kernel@vger.kernel.org Return-path: Received: from mail-wm0-f68.google.com ([74.125.82.68]:35275 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758015AbcBCQvY (ORCPT ); Wed, 3 Feb 2016 11:51:24 -0500 In-Reply-To: <1454516585-28491-5-git-send-email-rkrcmar@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 03/02/2016 17:23, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: > Discard policy doesn't rely on information from notifiers, so we don'= t > need to register notifiers unconditionally. >=20 > 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 > Signed-off-by: Radim Kr=C4=8Dm=C3=A1=C5=99 > --- > 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(-) >=20 > 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_no= tifier *kian) > struct kvm_kpit_state *ps =3D container_of(kian, struct kvm_kpit_st= ate, > irq_ack_notifier); > =20 > - 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 =3D 0; > pit_state->irq_ack_notifier.irq_acked =3D kvm_pit_ack_irq; > - kvm_register_irq_ack_notifier(kvm, &pit_state->irq_ack_notifier); > - pit_state->reinject =3D true; > mutex_unlock(&pit->pit_state.lock); > =20 > - kvm_pit_reset(pit); > - > pit->mask_notifier.func =3D pit_mask_notifer; > - kvm_register_irq_mask_notifier(kvm, 0, &pit->mask_notifier); > + > + kvm_pit_set_reinject(pit, true); > + > + kvm_pit_reset(pit); > =20 > kvm_iodevice_init(&pit->dev, &pit_dev_ops); > ret =3D kvm_io_bus_register_dev(kvm, KVM_PIO_BUS, KVM_PIT_BASE_ADDR= ESS, > @@ -738,14 +734,37 @@ fail_unregister: > kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &pit->dev); > =20 > 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; > } > =20 > +void kvm_pit_set_reinject(struct kvm_pit *pit, bool reinject) > +{ > + /* Implicit BUG on NULL dereference. */ > + struct kvm_kpit_state *ps =3D &pit->pit_state; > + struct kvm *kvm =3D pit->kvm; > + > + mutex_lock(&ps->lock); > + > + if (ps->reinject =3D=3D reinject) > + goto out; > + > + ps->reinject =3D 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 =3D &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 channe= l, 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); > =20 > #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 *kv= m, > { > if (!kvm->arch.vpit) > return -ENXIO; > - mutex_lock(&kvm->arch.vpit->pit_state.lock); > - kvm->arch.vpit->pit_state.reinject =3D control->pit_reinject; > - mutex_unlock(&kvm->arch.vpit->pit_state.lock); > + > + kvm_pit_set_reinject(kvm->arch.vpit, control->pit_reinject); > + > return 0; > } > =20 >=20