Re: [PATCH 1/4] Move irq routing data structure to rcu locking

[Gleb Natapov <gleb@redhat.com>]

On Mon, Jul 13, 2009 at 01:42:13PM -0300, Marcelo Tosatti wrote:
> On Mon, Jul 13, 2009 at 07:33:30PM +0300, Gleb Natapov wrote:
> > On Mon, Jul 13, 2009 at 01:27:38PM -0300, Marcelo Tosatti wrote:
> > > On Mon, Jul 13, 2009 at 07:24:53PM +0300, Gleb Natapov wrote:
> > > > On Mon, Jul 13, 2009 at 12:55:31PM -0300, Marcelo Tosatti wrote:
> > > > > On Mon, Jul 13, 2009 at 04:15:34PM +0300, Gleb Natapov wrote:
> > > > > > On Mon, Jul 13, 2009 at 09:01:33AM -0400, Gregory Haskins wrote:
> > > > > > > Gleb Natapov wrote:
> > > > > > > > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > > > > > > > ---
> > > > > > > >  include/linux/kvm_host.h |    2 +-
> > > > > > > >  virt/kvm/irq_comm.c      |   55 +++++++++++++++++++++-------------------------
> > > > > > > >  virt/kvm/kvm_main.c      |    1 -
> > > > > > > >  3 files changed, 26 insertions(+), 32 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > > > > > > > index f54a0d3..6756b3e 100644
> > > > > > > > --- a/include/linux/kvm_host.h
> > > > > > > > +++ b/include/linux/kvm_host.h
> > > > > > > > @@ -161,7 +161,7 @@ struct kvm {
> > > > > > > >  
> > > > > > > >  	struct mutex irq_lock;
> > > > > > > >  #ifdef CONFIG_HAVE_KVM_IRQCHIP
> > > > > > > > -	struct list_head irq_routing; /* of kvm_kernel_irq_routing_entry */
> > > > > > > > +	struct kvm_kernel_irq_routing_entry *irq_routing;
> > > > > > > >  	struct hlist_head mask_notifier_list;
> > > > > > > >  #endif
> > > > > > > >  
> > > > > > > > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > > > > > > > index 7af18b8..b2fa3f6 100644
> > > > > > > > --- a/virt/kvm/irq_comm.c
> > > > > > > > +++ b/virt/kvm/irq_comm.c
> > > > > > > > @@ -148,7 +148,8 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level)
> > > > > > > >  	 * IOAPIC.  So set the bit in both. The guest will ignore
> > > > > > > >  	 * writes to the unused one.
> > > > > > > >  	 */
> > > > > > > > -	list_for_each_entry(e, &kvm->irq_routing, link)
> > > > > > > > +	rcu_read_lock();
> > > > > > > > +	for (e = rcu_dereference(kvm->irq_routing); e && e->set; e++) {
> > > > > > > >   
> > > > > > > 
> > > > > > > Hi Gleb,
> > > > > > >   I haven't had a chance to fully digest and review these patches, but
> > > > > > > one thing I did notice is that you seem to be converting from a list to
> > > > > > > an open-coded structure.  I am just curious why you made this design
> > > > > > > decision instead of using the RCU variant of list?
> > > > > > > 
> > > > > > It is not scary "open-coded structure" it's just an array :) As I responded
> > > > > > to Michael the idea is to move msis out of irq_routing, make the array
> > > > > > much smaller and either use gsi as an index in the array or use hash table
> > > > > > instead looping over all entries. For now I can justify array as more
> > > > > > cache friendly data structure as we scan it linearly.
> > > > > 
> > > > > I think its more important to convert to faster search mechanism (the
> > > > > list walk shows up high in profiling), then convert to RCU?
> > > > Why in this order? I am working on faster search mechanism now (on top
> > > > of the series).
> > > 
> > > Because as Michael mentioned we can use slots_lock (should be renamed
> > > to global_lock) instead of RCU on the write-side.
> > > 
> > I don't get it. The point for RCU is to get rid of reader's lock. If
> > I'll have to take slot_lock on each EOI I achieved nothing.
> 
> You already take slots_lock for read on every exit.
> 
We should fix that, not add even more users. Shouldn't we?

> > > than RCU and you stop the spread of locks. Needs to be discussed...
> > > 
> > I much prefer to have many well defined locks with well understood
> > scope, then a small number of globals locks that are split ad-hoc when
> > deadlock is discovered (lock->irq_lock).
> 
> OK.
> 

--
			Gleb.