Messages in this thread Patch in this message | | | Date | Thu, 18 Nov 2010 15:03:37 +0200 | From | "Michael S. Tsirkin" <> | Subject | Re: [PATCH RFC] kvm: fast-path msi injection with irqfd |
| |
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
| |