Re: [PATCH 4/4] KVM: x86: remove notifiers from PIT discard policy

[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.

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@xxxxxxxxxx>

> Signed-off-by: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> ---
>  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;
>  }
>  
>