From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758040AbcBCQv2 (ORCPT ); Wed, 3 Feb 2016 11:51:28 -0500 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 Subject: Re: [PATCH 4/4] KVM: x86: remove notifiers from PIT discard policy To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , linux-kernel@vger.kernel.org References: <1454516585-28491-1-git-send-email-rkrcmar@redhat.com> <1454516585-28491-5-git-send-email-rkrcmar@redhat.com> Cc: kvm@vger.kernel.org, Yuki Shibuya From: Paolo Bonzini Message-ID: <56B23009.1020109@redhat.com> Date: Wed, 3 Feb 2016 17:51:21 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: <1454516585-28491-5-git-send-email-rkrcmar@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Signed-off-by: Radim Krčmář > --- > 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; > } > >