Re: [PATCH RFC] kvm: fast-path msi injection with irqfd

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Nov 18, 2010 at 02:29:11PM +0200, Avi Kivity wrote:
> On 11/18/2010 01:10 PM, Michael S. Tsirkin wrote:
> >>  I guess I should create an empty Documentation/kvm/locking.txt and
> >>  force everyone else to update it.
> >
> >Comments near the relevant fields not better?
> >
> 
> Not an either/or.  You can't understand the system from random
> source comments.
> 
> >diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> >index a055742..d13ced3 100644
> >--- a/include/linux/kvm_host.h
> >+++ b/include/linux/kvm_host.h
> >@@ -16,6 +16,7 @@
> >  #include<linux/mm.h>
> >  #include<linux/preempt.h>
> >  #include<linux/msi.h>
> >+#include<linux/rcupdate.h>
> >  #include<asm/signal.h>
> >
> >  #include<linux/kvm.h>
> >@@ -206,6 +207,8 @@ struct kvm {
> >
> >  	struct mutex irq_lock;
> >  #ifdef CONFIG_HAVE_KVM_IRQCHIP
> >+	/* Update side is protected by irq_lock and,
> >+	 * if configured, irqfds.lock. */
> 
> /*
>  * kernel style comment
>  * here and elsewhere
>  */
> 
> 
> 
> >  	struct kvm_irq_routing_table __rcu *irq_routing;
> >  	struct hlist_head mask_notifier_list;
> >  	struct hlist_head irq_ack_notifier_list;
> >@@ -462,6 +465,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
> >  				   unsigned long *deliver_bitmask);
> >  #endif
> >  int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);
> >+int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
> >+		int irq_source_id, int level);
> 
> No point in the level argument for an msi specific function.

This is an existing function I made non-static.
We have per-gsi callbacks so level is required there to match.
I could add a wrapper I guess:

int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
		int irq_source_id, int level)
{
	if (!level)
		return -1;
	return kvm_send_msi(irq_entry, kvm, irq_source_id);
}

This results in less code for irqfd but more code for ioctl injection
... is it worth it?

> >
> >  #else
> >@@ -614,6 +620,12 @@ static inline int kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
> >  }
> >
> >  static inline void kvm_irqfd_release(struct kvm *kvm) {}
> 
> blank line
> 

There's no line before kvm_eventfd_init either ...
I added one.

> >+static inline void kvm_irq_routing_update(struct kvm *kvm,
> >+					  struct kvm_irq_routing_table *irq_rt)
> >+{
> >+	rcu_assign_pointer(kvm->irq_routing, irq_rt);
> >+}
> >+
> >  static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
> >  {
> >  	return -ENOSYS;
> 
> Apart from these minor issues, looks good.


Something we should consider improving is the loop over all VCPUs that
kvm_irq_delivery_to_apic invokes.  I think that (for non-broadcast
interrupts) it should be possible to precompute an store the CPU
in question as part of the routing entry.

Something for a separate patch ... comments?

> -- 
> error compiling committee.c: too many arguments to function