Re: [PATCH 1/3] KVM: Fold assigned interrupt work into IRQ handler

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 6308 bytes --]

Am 01.11.2010 15:08, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> The complete work handler runs with assigned_dev_lock acquired and
> interrupts disabled, so there is nothing to gain pushing this work out
> of the actually IRQ handler. Fold them together.

Err, forget it. kvm_set_irq pulls in the famous pic lock, and that one
is not prepared to be called from IRQ context (lockdep just complained).

I will try to clean this up via a threaded IRQ handler, maybe using the
slow-path only for INTx-type IRQs and pushing MSIs directly from the
hard handler.

Jan

> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  include/linux/kvm_host.h |    1 -
>  virt/kvm/assigned-dev.c  |   71 +++++++++++++++-------------------------------
>  2 files changed, 23 insertions(+), 49 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 462b982..df5497f 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -465,7 +465,6 @@ struct kvm_guest_msix_entry {
>  
>  struct kvm_assigned_dev_kernel {
>  	struct kvm_irq_ack_notifier ack_notifier;
> -	struct work_struct interrupt_work;
>  	struct list_head list;
>  	int assigned_dev_id;
>  	int host_segnr;
> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> index 7c98928..5c1b56a 100644
> --- a/virt/kvm/assigned-dev.c
> +++ b/virt/kvm/assigned-dev.c
> @@ -55,18 +55,24 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernel
>  	return index;
>  }
>  
> -static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
> +static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
>  {
> -	struct kvm_assigned_dev_kernel *assigned_dev;
> -	int i;
> -
> -	assigned_dev = container_of(work, struct kvm_assigned_dev_kernel,
> -				    interrupt_work);
> +	struct kvm_assigned_dev_kernel *assigned_dev =
> +		(struct kvm_assigned_dev_kernel *) dev_id;
> +	unsigned long flags;
>  
> -	spin_lock_irq(&assigned_dev->assigned_dev_lock);
> +	spin_lock_irqsave(&assigned_dev->assigned_dev_lock, flags);
>  	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) {
>  		struct kvm_guest_msix_entry *guest_entries =
>  			assigned_dev->guest_msix_entries;
> +		int index = find_index_from_host_irq(assigned_dev, irq);
> +		int i;
> +
> +		if (index < 0)
> +			goto out;
> +
> +		guest_entries[index].flags |= KVM_ASSIGNED_MSIX_PENDING;
> +
>  		for (i = 0; i < assigned_dev->entries_nr; i++) {
>  			if (!(guest_entries[i].flags &
>  					KVM_ASSIGNED_MSIX_PENDING))
> @@ -76,33 +82,15 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
>  				    assigned_dev->irq_source_id,
>  				    guest_entries[i].vector, 1);
>  		}
> -	} else
> +	} else {
>  		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
>  			    assigned_dev->guest_irq, 1);
>  
> -	spin_unlock_irq(&assigned_dev->assigned_dev_lock);
> -}
> -
> -static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
> -{
> -	unsigned long flags;
> -	struct kvm_assigned_dev_kernel *assigned_dev =
> -		(struct kvm_assigned_dev_kernel *) dev_id;
> -
> -	spin_lock_irqsave(&assigned_dev->assigned_dev_lock, flags);
> -	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) {
> -		int index = find_index_from_host_irq(assigned_dev, irq);
> -		if (index < 0)
> -			goto out;
> -		assigned_dev->guest_msix_entries[index].flags |=
> -			KVM_ASSIGNED_MSIX_PENDING;
> -	}
> -
> -	schedule_work(&assigned_dev->interrupt_work);
> -
> -	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_GUEST_INTX) {
> -		disable_irq_nosync(irq);
> -		assigned_dev->host_irq_disabled = true;
> +		if (assigned_dev->irq_requested_type &
> +				KVM_DEV_IRQ_GUEST_INTX) {
> +			disable_irq_nosync(irq);
> +			assigned_dev->host_irq_disabled = true;
> +		}
>  	}
>  
>  out:
> @@ -147,33 +135,23 @@ static void deassign_guest_irq(struct kvm *kvm,
>  	assigned_dev->irq_requested_type &= ~(KVM_DEV_IRQ_GUEST_MASK);
>  }
>  
> -/* The function implicit hold kvm->lock mutex due to cancel_work_sync() */
>  static void deassign_host_irq(struct kvm *kvm,
>  			      struct kvm_assigned_dev_kernel *assigned_dev)
>  {
>  	/*
> -	 * In kvm_free_device_irq, cancel_work_sync return true if:
> -	 * 1. work is scheduled, and then cancelled.
> -	 * 2. work callback is executed.
> -	 *
> -	 * The first one ensured that the irq is disabled and no more events
> -	 * would happen. But for the second one, the irq may be enabled (e.g.
> -	 * for MSI). So we disable irq here to prevent further events.
> +	 * We disable irq here to prevent further events.
>  	 *
>  	 * Notice this maybe result in nested disable if the interrupt type is
>  	 * INTx, but it's OK for we are going to free it.
>  	 *
>  	 * If this function is a part of VM destroy, please ensure that till
>  	 * now, the kvm state is still legal for probably we also have to wait
> -	 * interrupt_work done.
> +	 * on a currently running IRQ handler.
>  	 */
>  	if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) {
>  		int i;
>  		for (i = 0; i < assigned_dev->entries_nr; i++)
> -			disable_irq_nosync(assigned_dev->
> -					   host_msix_entries[i].vector);
> -
> -		cancel_work_sync(&assigned_dev->interrupt_work);
> +			disable_irq(assigned_dev->host_msix_entries[i].vector);
>  
>  		for (i = 0; i < assigned_dev->entries_nr; i++)
>  			free_irq(assigned_dev->host_msix_entries[i].vector,
> @@ -185,8 +163,7 @@ static void deassign_host_irq(struct kvm *kvm,
>  		pci_disable_msix(assigned_dev->dev);
>  	} else {
>  		/* Deal with MSI and INTx */
> -		disable_irq_nosync(assigned_dev->host_irq);
> -		cancel_work_sync(&assigned_dev->interrupt_work);
> +		disable_irq(assigned_dev->host_irq);
>  
>  		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
>  
> @@ -558,8 +535,6 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
>  	match->irq_source_id = -1;
>  	match->kvm = kvm;
>  	match->ack_notifier.irq_acked = kvm_assigned_dev_ack_irq;
> -	INIT_WORK(&match->interrupt_work,
> -		  kvm_assigned_dev_interrupt_work_handler);
>  
>  	list_add(&match->list, &kvm->arch.assigned_dev_head);
>  



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]