Re: [PATCH v4] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING

[Paolo Bonzini]

Il 05/05/2014 16:21, Christian Borntraeger ha scritto:
> On 28/04/14 18:39, Paolo Bonzini wrote:
> > From: Christian Borntraeger <borntraeger@xxxxxxxxxx>
>
> Given all your work, What about From: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> plus "Based on an inital patch from Christian Borntraeger"

No big deal, I don't care about authorship that much.

> > @@ -221,17 +225,18 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
> >  	unsigned long flags = (unsigned long)key;
> >  	struct kvm_kernel_irq_routing_entry *irq;
> >  	struct kvm *kvm = irqfd->kvm;
> > +	int idx;
> >
> >  	if (flags & POLLIN) {
> > -		rcu_read_lock();
> > -		irq = rcu_dereference(irqfd->irq_entry);
> > +		idx = srcu_read_lock(&kvm->irq_srcu);
> > +		irq = srcu_dereference(irqfd->irq_entry, &kvm->irq_srcu);
> >  		/* An event has been signaled, inject an interrupt */
> >  		if (irq)
> >  			kvm_set_msi(irq, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1,
> >  					false);
> >  		else
> >  			schedule_work(&irqfd->inject);
> > -		rcu_read_unlock();
> > +		srcu_read_unlock(&kvm->irq_srcu, idx);
> >  	}
> >
> >  	if (flags & POLLHUP) {
> > @@ -363,7 +368,7 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
> >  		}
> >
> >  		list_add_rcu(&irqfd->resampler_link, &irqfd->resampler->list);
> > -		synchronize_rcu();
> > +		synchronize_srcu(&kvm->irq_srcu);
>
> No idea what resampler is, can this become time critical as well - iow do we need expedited here?

It's for level-triggered interrupts.  I decided that if synchronize_rcu 
was good enough before, synchronize_srcu will do after the patch.

> > @@ -85,7 +86,7 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm,
> >  	mutex_lock(&kvm->irq_lock);
> >  	hlist_del_init_rcu(&kian->link);
> >  	mutex_unlock(&kvm->irq_lock);
> > -	synchronize_rcu();
> > +	synchronize_srcu_expedited(&kvm->irq_srcu);
>
> Hmm, looks like all callers are slow path (shutdown, deregister assigned dev). Couldnt
> we use the non expedited variant?

... but I have screwed up this one.  Thanks, I'll change it.

> >  	r = kvm_arch_init_vm(kvm, type);
> >  	if (r)
> > -		goto out_err_nodisable;
> > +		goto out_err_no_disable;
> >
> >  	r = hardware_enable_all();
> >  	if (r)
> > -		goto out_err_nodisable;
> > +		goto out_err_no_disable;
> >
> >  #ifdef CONFIG_HAVE_KVM_IRQCHIP
> >  	INIT_HLIST_HEAD(&kvm->mask_notifier_list);
> > @@ -473,10 +473,12 @@ static struct kvm *kvm_create_vm(unsigned long type)
> >  	r = -ENOMEM;
> >  	kvm->memslots = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL);
> >  	if (!kvm->memslots)
> > -		goto out_err_nosrcu;
> > +		goto out_err_no_srcu;
> >  	kvm_init_memslots_id(kvm);
> >  	if (init_srcu_struct(&kvm->srcu))
> > -		goto out_err_nosrcu;
> > +		goto out_err_no_srcu;
> > +	if (init_srcu_struct(&kvm->irq_srcu))
> > +		goto out_err_no_irq_srcu;
> >  	for (i = 0; i < KVM_NR_BUSES; i++) {
> >  		kvm->buses[i] = kzalloc(sizeof(struct kvm_io_bus),
> >  					GFP_KERNEL);
> > @@ -505,10 +507,12 @@ static struct kvm *kvm_create_vm(unsigned long type)
> >  	return kvm;
> >
> >  out_err:
> > +	cleanup_srcu_struct(&kvm->irq_srcu);
> > +out_err_no_irq_srcu:
> >  	cleanup_srcu_struct(&kvm->srcu);
> > -out_err_nosrcu:
> > +out_err_no_srcu:
> >  	hardware_disable_all();
> > -out_err_nodisable:
> > +out_err_no_disable:
>
>
> the patch would be smaller without this change, but it makes the naming more consistent, so ok.

Yeah, out_err_noirq_srcu or out_err_noirqsrcu are both very ugly.

Thanks for the review, I'm making the small change to remove expedited 
and applying to kvm/queue.

Paolo

> >  	for (i = 0; i < KVM_NR_BUSES; i++)
> >  		kfree(kvm->buses[i]);
> >  	kfree(kvm->memslots);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/