Patchwork [v4,3/6] KVM: Initialize irqfd from kvm_init().

login
register
mail settings
Submitter Cornelia Huck
Date Feb. 28, 2013, 9:22 a.m.
Message ID <1362043352-56889-4-git-send-email-cornelia.huck@de.ibm.com>
Download mbox | patch
Permalink /patch/223847/
State New
Headers show

Comments

Cornelia Huck - Feb. 28, 2013, 9:22 a.m.
Currently, eventfd introduces module_init/module_exit functions
to initialize/cleanup the irqfd workqueue. This only works, however,
if no other module_init/module_exit functions are built into the
same module.

Let's just move the initialization and cleanup to kvm_init and kvm_exit.
This way, it is also clearer where kvm startup may fail.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 include/linux/kvm_host.h | 13 +++++++++++++
 virt/kvm/eventfd.c       |  7 ++-----
 virt/kvm/kvm_main.c      |  6 ++++++
 3 files changed, 21 insertions(+), 5 deletions(-)
Sasha Levin - April 3, 2013, 12:40 a.m.
On 02/28/2013 04:22 AM, Cornelia Huck wrote:
> Currently, eventfd introduces module_init/module_exit functions
> to initialize/cleanup the irqfd workqueue. This only works, however,
> if no other module_init/module_exit functions are built into the
> same module.
> 
> Let's just move the initialization and cleanup to kvm_init and kvm_exit.
> This way, it is also clearer where kvm startup may fail.
> 
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

I'm seeing this during boot:

[    6.763302] ------------[ cut here ]------------
[    6.763763] WARNING: at kernel/workqueue.c:4204 destroy_workqueue+0x1df/0x3d0()
[    6.764507] Modules linked in:
[    6.764792] Pid: 1, comm: swapper/0 Tainted: G        W    3.9.0-rc5-next-20130402-sasha-00015-g3522ec5 #324
[    6.765654] Call Trace:
[    6.765875]  [<ffffffff811074fb>] warn_slowpath_common+0x8b/0xc0
[    6.766436]  [<ffffffff81107545>] warn_slowpath_null+0x15/0x20
[    6.766947]  [<ffffffff8112ca7f>] destroy_workqueue+0x1df/0x3d0
[    6.768631]  [<ffffffff8100d880>] kvm_irqfd_exit+0x10/0x20
[    6.770000]  [<ffffffff81004dbb>] kvm_init+0x2ab/0x310
[    6.770607]  [<ffffffff86183dc0>] ? cpu_has_kvm_support+0x4d/0x4d
[    6.771241]  [<ffffffff86183fb4>] vmx_init+0x1f4/0x437
[    6.771709]  [<ffffffff86183dc0>] ? cpu_has_kvm_support+0x4d/0x4d
[    6.772266]  [<ffffffff810020f2>] do_one_initcall+0xb2/0x1b0
[    6.772995]  [<ffffffff86180021>] kernel_init_freeable+0x15d/0x1ef
[    6.773857]  [<ffffffff8617f801>] ? loglevel+0x31/0x31
[    6.774609]  [<ffffffff83d51230>] ? rest_init+0x140/0x140
[    6.775551]  [<ffffffff83d51239>] kernel_init+0x9/0xf0
[    6.776162]  [<ffffffff83dbf37c>] ret_from_fork+0x7c/0xb0
[    6.776662]  [<ffffffff83d51230>] ? rest_init+0x140/0x140
[    6.777241] ---[ end trace 10bba684ced4346a ]---

And I think it has something to do with this patch.


Thanks,
Sasha
Cornelia Huck - April 3, 2013, 11:32 a.m.
On Tue, 02 Apr 2013 20:40:53 -0400
Sasha Levin <sasha.levin@oracle.com> wrote:

> On 02/28/2013 04:22 AM, Cornelia Huck wrote:
> > Currently, eventfd introduces module_init/module_exit functions
> > to initialize/cleanup the irqfd workqueue. This only works, however,
> > if no other module_init/module_exit functions are built into the
> > same module.
> > 
> > Let's just move the initialization and cleanup to kvm_init and kvm_exit.
> > This way, it is also clearer where kvm startup may fail.
> > 
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> I'm seeing this during boot:
> 
> [    6.763302] ------------[ cut here ]------------
> [    6.763763] WARNING: at kernel/workqueue.c:4204 destroy_workqueue+0x1df/0x3d0()
> [    6.764507] Modules linked in:
> [    6.764792] Pid: 1, comm: swapper/0 Tainted: G        W    3.9.0-rc5-next-20130402-sasha-00015-g3522ec5 #324
> [    6.765654] Call Trace:
> [    6.765875]  [<ffffffff811074fb>] warn_slowpath_common+0x8b/0xc0
> [    6.766436]  [<ffffffff81107545>] warn_slowpath_null+0x15/0x20
> [    6.766947]  [<ffffffff8112ca7f>] destroy_workqueue+0x1df/0x3d0
> [    6.768631]  [<ffffffff8100d880>] kvm_irqfd_exit+0x10/0x20
> [    6.770000]  [<ffffffff81004dbb>] kvm_init+0x2ab/0x310
> [    6.770607]  [<ffffffff86183dc0>] ? cpu_has_kvm_support+0x4d/0x4d
> [    6.771241]  [<ffffffff86183fb4>] vmx_init+0x1f4/0x437
> [    6.771709]  [<ffffffff86183dc0>] ? cpu_has_kvm_support+0x4d/0x4d
> [    6.772266]  [<ffffffff810020f2>] do_one_initcall+0xb2/0x1b0
> [    6.772995]  [<ffffffff86180021>] kernel_init_freeable+0x15d/0x1ef
> [    6.773857]  [<ffffffff8617f801>] ? loglevel+0x31/0x31
> [    6.774609]  [<ffffffff83d51230>] ? rest_init+0x140/0x140
> [    6.775551]  [<ffffffff83d51239>] kernel_init+0x9/0xf0
> [    6.776162]  [<ffffffff83dbf37c>] ret_from_fork+0x7c/0xb0
> [    6.776662]  [<ffffffff83d51230>] ? rest_init+0x140/0x140
> [    6.777241] ---[ end trace 10bba684ced4346a ]---
> 
> And I think it has something to do with this patch.

What this patch does is move initialization of the workqueue - the code
called should be indentical.

The workqueue created/destroyed should not have been touched (no irqfds
yet), but the new workqueue code seems to encounter something
unexpected.

I'd expect destroy_workqueue() to be callable after a successful
create_singlethread_workqueue(). Tejun?

> 
> 
> Thanks,
> Sasha
>

Patch

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 722cae7..3b768ef 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -423,6 +423,19 @@  void kvm_vcpu_uninit(struct kvm_vcpu *vcpu);
 int __must_check vcpu_load(struct kvm_vcpu *vcpu);
 void vcpu_put(struct kvm_vcpu *vcpu);
 
+#ifdef __KVM_HAVE_IOAPIC
+int kvm_irqfd_init(void);
+void kvm_irqfd_exit(void);
+#else
+static inline int kvm_irqfd_init(void)
+{
+	return 0;
+}
+
+static inline void kvm_irqfd_exit(void)
+{
+}
+#endif
 int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
 		  struct module *module);
 void kvm_exit(void);
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index b6eea5c..f0ced1a 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -544,7 +544,7 @@  void kvm_irq_routing_update(struct kvm *kvm,
  * aggregated from all vm* instances. We need our own isolated single-thread
  * queue to prevent deadlock against flushing the normal work-queue.
  */
-static int __init irqfd_module_init(void)
+int kvm_irqfd_init(void)
 {
 	irqfd_cleanup_wq = create_singlethread_workqueue("kvm-irqfd-cleanup");
 	if (!irqfd_cleanup_wq)
@@ -553,13 +553,10 @@  static int __init irqfd_module_init(void)
 	return 0;
 }
 
-static void __exit irqfd_module_exit(void)
+void kvm_irqfd_exit(void)
 {
 	destroy_workqueue(irqfd_cleanup_wq);
 }
-
-module_init(irqfd_module_init);
-module_exit(irqfd_module_exit);
 #endif
 
 /*
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index adc68fe..7c188a3 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2920,6 +2920,9 @@  int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
 	int r;
 	int cpu;
 
+	r = kvm_irqfd_init();
+	if (r)
+		goto out_irqfd;
 	r = kvm_arch_init(opaque);
 	if (r)
 		goto out_fail;
@@ -3000,6 +3003,8 @@  out_free_0a:
 out_free_0:
 	kvm_arch_exit();
 out_fail:
+	kvm_irqfd_exit();
+out_irqfd:
 	return r;
 }
 EXPORT_SYMBOL_GPL(kvm_init);
@@ -3016,6 +3021,7 @@  void kvm_exit(void)
 	on_each_cpu(hardware_disable_nolock, NULL, 1);
 	kvm_arch_hardware_unsetup();
 	kvm_arch_exit();
+	kvm_irqfd_exit();
 	free_cpumask_var(cpus_hardware_enabled);
 }
 EXPORT_SYMBOL_GPL(kvm_exit);