Hi, I think you missed a cleanup_srcu_struct(&kvm->irq_srcu) in kvm_destroy_vm(). On Mon, May 5, 2014 at 10:26 PM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote: > 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/ -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html