lkml.org 
[lkml]   [2012]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH RFC] kvm: deliver msix interrupts from irq handler
On Thu, Jan 19, 2012 at 09:21:23AM +0200, Gleb Natapov wrote:
> > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > index ba892df..68cd127 100644
> > --- a/virt/kvm/irq_comm.c
> > +++ b/virt/kvm/irq_comm.c
> > @@ -201,6 +201,58 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level,
> > return ret;
> > }
> >
> > +static inline struct kvm_kernel_irq_routing_entry *
> > +kvm_get_entry(struct kvm *kvm, struct kvm_irq_routing_table *irq_rq, u32 irq)
> > +{
> > + struct kvm_kernel_irq_routing_entry *e;
> > + if (likely(irq < irq_rt->nr_rt_entries))
> > + hlist_for_each_entry(e, n, &irq_rt->map[irq], link)
> > + if (e->type == KVM_IRQ_ROUTING_MSI)
> > + return e;
> > + else
> > + return ERR_PTR(-EWOULDBLOCK);
> > + return ERR_PTR(-EINVAL);
> > +}
> Unused?

Yes, leftovers from an attempt to reuse kvm_set_irq as you suggested
below. It didn't work out - we get much more code this way,
so this needs to be removed.

> > +
> > +/*
> > + * Deliver an IRQ in an atomic context if we can, or return a failure,
> > + * user can retry in a process context.
> > + * Return value:
> > + * -EWOULDBLOCK Can't deliver in atomic context
> > + * < 0 Interrupt was ignored (masked or not delivered for other reasons)
> > + * = 0 Interrupt was coalesced (previous irq is still pending)
> > + * > 0 Number of CPUs interrupt was delivered to
> > + */
> > +int kvm_set_irq_inatomic(struct kvm *kvm, int irq_source_id, u32 irq, int level,
> > + int host_irq)
> > +{
> > + struct kvm_kernel_irq_routing_entry *e;
> > + int ret = -EINVAL;
> > + struct kvm_irq_routing_table *irq_rt;
> > + struct hlist_node *n;
> > +
> > + trace_kvm_set_irq(irq, level, irq_source_id);
> > +
> > + /*
> > + * We know MSI are safe in interrupt context. They are also
> > + * easy as there's a single routing entry for these GSIs.
> > + * So only handle MSI in an atomic context, for now.
> > + */
> > + rcu_read_lock_bh();
> _bh?
>
> /**
> * rcu_read_lock_bh() - mark the beginning of an RCU-bh critical section
> *
> * This is equivalent of rcu_read_lock(), but to be used when updates
> * are being done using call_rcu_bh() or synchronize_rcu_bh().
> ....
>
> Since updates to irq routing table are not done using _bh variant I
> doubt rcu_read_lock_bh() is justified here.

Right. Thanks for ppointing this out, I was confused.

> > + irq_rt = rcu_dereference(kvm->irq_routing);
> > + if (irq < irq_rt->nr_rt_entries)
> > + hlist_for_each_entry(e, n, &irq_rt->map[irq], link) {
> > + if (ei->type == KVM_IRQ_ROUTING_MSI)
> > + ret = kvm_set_msi(e, kvm, irq_source_id, level,
> > + host_irq);
> > + else
> > + ret = -EWOULDBLOCK;
> > + break;
> > + }
> > + rcu_read_unlock_bh();
> > + return ret;
> > +}
> > +
> Share implementation with kvm_set_irq().

I considered this. There are several reasons not to do it:
- Amount of common code is very small
- As it's separate, it's more obvious that it can't block (kvm_set_irq can block)
We can even tag kvm_set_irq with might_sleep.
- This is way simpler and faster as we can do operations directly,
instead of copying the irq out, and as it's datapath
an optimization is I think justified.

> > void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin)
> > {
> > struct kvm_irq_ack_notifier *kian;
> > --
> > 1.7.8.2.325.g247f9
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
>
> --
> Gleb.


\
 
 \ /
  Last update: 2012-01-19 14:49    [W:0.061 / U:0.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site