Messages in this thread | | | Date | Mon, 05 May 2014 16:26:30 +0200 | From | Paolo Bonzini <> | Subject | Re: [PATCH v4] kvm/irqchip: Speed up KVM_SET_GSI_ROUTING |
| |
Il 05/05/2014 16:21, Christian Borntraeger ha scritto: > On 28/04/14 18:39, Paolo Bonzini wrote: >> From: Christian Borntraeger <borntraeger@de.ibm.com> > > Given all your work, What about From: Paolo Bonzini <pbonzini@redhat.com> > 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); >> >
| |