xen/pciback: Use mutexes when working with Xenbus state transitions.

Quoted 1 line

Quoted 1 line · Konrad Rzeszutek Wilk

The caller that orchestrates the state changes is xenwatch_thread and it takes a mutex. In our processing of Xenbus states we can take the luxery of going to sleep on a mutex, so lets do that and

also fix this bug:

BUG: sleeping function called from invalid context at 

/linux/kernel/mutex.c:271
in_atomic(): 1, irqs_disabled(): 0, pid: 32, name: xenwatch 2 locks held by xenwatch/32:
#0: (xenwatch_mutex){......}, at: [<ffffffff813856ab>]

xenwatch_thread+0x4b/0x180
 #1:  (&(&pdev->dev_lock)->rlock){......}, at: [<ffffffff8138f05b>] 
xen_pcibk_disconnect+0x1b/0x80
Pid: 32, comm: xenwatch Not tainted 3.1.0-rc6-00015-g3ce340d #2
Call Trace:
 [<ffffffff810892b2>] __might_sleep+0x102/0x130
 [<ffffffff8163b90f>] mutex_lock_nested+0x2f/0x50
 [<ffffffff81382c1c>] unbind_from_irq+0x2c/0x1b0
 [<ffffffff8110da66>] ? free_irq+0x56/0xb0
 [<ffffffff81382dbc>] unbind_from_irqhandler+0x1c/0x30
 [<ffffffff8138f06b>] xen_pcibk_disconnect+0x2b/0x80
 [<ffffffff81390348>] xen_pcibk_frontend_changed+0xe8/0x140
 [<ffffffff81387ac2>] xenbus_otherend_changed+0xd2/0x150
 [<ffffffff810895c1>] ? get_parent_ip+0x11/0x50
 [<ffffffff81387de0>] frontend_changed+0x10/0x20
 [<ffffffff81385712>] xenwatch_thread+0xb2/0x180

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

---
 drivers/xen/xen-pciback/pciback.h |    2 +-
 drivers/xen/xen-pciback/xenbus.c  |   16 +++++-----------
 2 files changed, 6 insertions(+), 12 deletions(-)

drivers/xen/xen-pciback/pciback.h +1 -1

index a0e131a..c3af628 100644
@@ -27,7 +27,7 @@ struct pci_dev_entry {
 
 struct xen_pcibk_device {
 	void *pci_dev_data;
-	spinlock_t dev_lock;
+	struct mutex dev_lock;
 	struct xenbus_device *xdev;
 	struct xenbus_watch be_watch;
 	u8 be_watching;

drivers/xen/xen-pciback/xenbus.c +1 -1

index 978d2c6..c057d67 100644
@@ -44,7 +44,7 @@ static struct xen_pcibk_device *alloc_pdev(struct 
 xenbus_device *xdev)
 	pdev->xdev = xdev;
 	dev_set_drvdata(&xdev->dev, pdev);
 
-	spin_lock_init(&pdev->dev_lock);
+	mutex_init(&pdev->dev_lock);
 
 	pdev->sh_info = NULL;

drivers/xen/xen-pciback/xenbus.c +2 -5

 	pdev->evtchn_irq = INVALID_EVTCHN_IRQ;

@@ -62,14 +62,13 @@ out:
 
 static void xen_pcibk_disconnect(struct xen_pcibk_device *pdev)
 {
-	spin_lock(&pdev->dev_lock);
+	mutex_lock(&pdev->dev_lock);
 
 	/* Ensure the guest can't trigger our handler before removing devices */
 	if (pdev->evtchn_irq != INVALID_EVTCHN_IRQ) {
 		unbind_from_irqhandler(pdev->evtchn_irq, pdev);
 		pdev->evtchn_irq = INVALID_EVTCHN_IRQ;
 	}
-	spin_unlock(&pdev->dev_lock);
 
 	/* If the driver domain started an op, make sure we complete it
 	 * before releasing the shared memory */

@@ -77,13 +76,11 @@ static void xen_pcibk_disconnect(struct xen_pcibk_device 
 *pdev)
 	/* Note, the workqueue does not use spinlocks at all.*/
 	flush_workqueue(xen_pcibk_wq);
 
-	spin_lock(&pdev->dev_lock);
 	if (pdev->sh_info != NULL) {
 		xenbus_unmap_ring_vfree(pdev->xdev, pdev->sh_info);
 		pdev->sh_info = NULL;
 	}
-	spin_unlock(&pdev->dev_lock);
-
+	mutex_unlock(&pdev->dev_lock);
 }
 

static void free_pdev(struct xen_pcibk_device *pdev)

drivers/xen/xen-pciback/xenbus.c +1 -2

@@ -120,9 +117,8 @@ static int xen_pcibk_do_attach(struct xen_pcibk_device 
 *pdev, int gnt_ref,
 		goto out;
 	}
 
-	spin_lock(&pdev->dev_lock);
+	mutex_lock(&pdev->dev_lock);
 	pdev->sh_info = vaddr;
-	spin_unlock(&pdev->dev_lock);
 
 	err = bind_interdomain_evtchn_to_irqhandler(

drivers/xen/xen-pciback/xenbus.c +1 -3

 		pdev->xdev->otherend_id, remote_evtchn, xen_pcibk_handle_event,

@@ -132,14 +128,12 @@ static int xen_pcibk_do_attach(struct xen_pcibk_device 
 *pdev, int gnt_ref,
 				 "Error binding event channel to IRQ");
 		goto out;
 	}
-
-	spin_lock(&pdev->dev_lock);
 	pdev->evtchn_irq = err;
-	spin_unlock(&pdev->dev_lock);
 	err = 0;
 
 	dev_dbg(&pdev->xdev->dev, "Attached!\n");
 out:
+	mutex_unlock(&pdev->dev_lock);
 	return err;
 }

Quoted 6 lines

Quoted 2 lines · Jan Beulich

Quoted 1 line · Konrad Rzeszutek Wilk

The caller that orchestrates the state changes is xenwatch_thread and it takes a mutex. In our processing of Xenbus states we can take the luxery of going to sleep on a mutex, so lets do that and

This is only the direct conversion of existing spinlock accesses in xenbus.c. However, in the course of converting from the legacy implementation you stripped a couple more (in xen_pcibk_attach(), xen_pcibk_reconfigure(), and xen_pcibk_setup_backend()), and

those should now get re-added imo.

Jan

Quoted 109 lines

also fix this bug:

BUG: sleeping function called from invalid context at 

/linux/kernel/mutex.c:271
in_atomic(): 1, irqs_disabled(): 0, pid: 32, name: xenwatch 2 locks held by xenwatch/32:
#0: (xenwatch_mutex){......}, at: [<ffffffff813856ab>]

xenwatch_thread+0x4b/0x180
 #1:  (&(&pdev->dev_lock)->rlock){......}, at: [<ffffffff8138f05b>] 
xen_pcibk_disconnect+0x1b/0x80
Pid: 32, comm: xenwatch Not tainted 3.1.0-rc6-00015-g3ce340d #2
Call Trace:
 [<ffffffff810892b2>] __might_sleep+0x102/0x130
 [<ffffffff8163b90f>] mutex_lock_nested+0x2f/0x50
 [<ffffffff81382c1c>] unbind_from_irq+0x2c/0x1b0
 [<ffffffff8110da66>] ? free_irq+0x56/0xb0
 [<ffffffff81382dbc>] unbind_from_irqhandler+0x1c/0x30
 [<ffffffff8138f06b>] xen_pcibk_disconnect+0x2b/0x80
 [<ffffffff81390348>] xen_pcibk_frontend_changed+0xe8/0x140
 [<ffffffff81387ac2>] xenbus_otherend_changed+0xd2/0x150
 [<ffffffff810895c1>] ? get_parent_ip+0x11/0x50
 [<ffffffff81387de0>] frontend_changed+0x10/0x20
 [<ffffffff81385712>] xenwatch_thread+0xb2/0x180

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

---
 drivers/xen/xen-pciback/pciback.h |    2 +-
 drivers/xen/xen-pciback/xenbus.c  |   16 +++++-----------
 2 files changed, 6 insertions(+), 12 deletions(-)

drivers/xen/xen-pciback/pciback.h +1 -1

index a0e131a..c3af628 100644
@@ -27,7 +27,7 @@ struct pci_dev_entry {
 
 struct xen_pcibk_device {
 	void *pci_dev_data;
-	spinlock_t dev_lock;
+	struct mutex dev_lock;
 	struct xenbus_device *xdev;
 	struct xenbus_watch be_watch;
 	u8 be_watching;

drivers/xen/xen-pciback/xenbus.c +1 -1

index 978d2c6..c057d67 100644
@@ -44,7 +44,7 @@ static struct xen_pcibk_device *alloc_pdev(struct 
 xenbus_device *xdev)
 	pdev->xdev = xdev;
 	dev_set_drvdata(&xdev->dev, pdev);
 
-	spin_lock_init(&pdev->dev_lock);
+	mutex_init(&pdev->dev_lock);
 
 	pdev->sh_info = NULL;

drivers/xen/xen-pciback/xenbus.c +2 -5

 	pdev->evtchn_irq = INVALID_EVTCHN_IRQ;

@@ -62,14 +62,13 @@ out:
 
 static void xen_pcibk_disconnect(struct xen_pcibk_device *pdev)
 {
-	spin_lock(&pdev->dev_lock);
+	mutex_lock(&pdev->dev_lock);
 
 	/* Ensure the guest can't trigger our handler before removing devices */
 	if (pdev->evtchn_irq != INVALID_EVTCHN_IRQ) {
 		unbind_from_irqhandler(pdev->evtchn_irq, pdev);
 		pdev->evtchn_irq = INVALID_EVTCHN_IRQ;
 	}
-	spin_unlock(&pdev->dev_lock);
 
 	/* If the driver domain started an op, make sure we complete it
 	 * before releasing the shared memory */

@@ -77,13 +76,11 @@ static void xen_pcibk_disconnect(struct xen_pcibk_device 
 *pdev)
 	/* Note, the workqueue does not use spinlocks at all.*/
 	flush_workqueue(xen_pcibk_wq);
 
-	spin_lock(&pdev->dev_lock);
 	if (pdev->sh_info != NULL) {
 		xenbus_unmap_ring_vfree(pdev->xdev, pdev->sh_info);
 		pdev->sh_info = NULL;
 	}
-	spin_unlock(&pdev->dev_lock);
-
+	mutex_unlock(&pdev->dev_lock);
 }
 

static void free_pdev(struct xen_pcibk_device *pdev)

drivers/xen/xen-pciback/xenbus.c +1 -2

@@ -120,9 +117,8 @@ static int xen_pcibk_do_attach(struct xen_pcibk_device 
 *pdev, int gnt_ref,
 		goto out;
 	}
 
-	spin_lock(&pdev->dev_lock);
+	mutex_lock(&pdev->dev_lock);
 	pdev->sh_info = vaddr;
-	spin_unlock(&pdev->dev_lock);
 
 	err = bind_interdomain_evtchn_to_irqhandler(

drivers/xen/xen-pciback/xenbus.c +0 -1

 		pdev->xdev->otherend_id, remote_evtchn, xen_pcibk_handle_event,

@@ -132,14 +128,12 @@ static int xen_pcibk_do_attach(struct xen_pcibk_device 
 *pdev, int gnt_ref,
 				 "Error binding event channel to IRQ");
 		goto out;
 	}
-

drivers/xen/xen-pciback/xenbus.c static int xen_pcibk_do_attach(struct xen_pcibk_device · line 139

Quoted 10 lines

drivers/xen/xen-pciback/xenbus.c +1 -2

-	spin_lock(&pdev->dev_lock);
 	pdev->evtchn_irq = err;
-	spin_unlock(&pdev->dev_lock);
 	err = 0;
 
 	dev_dbg(&pdev->xdev->dev, "Attached!\n");
 out:
+	mutex_unlock(&pdev->dev_lock);
 	return err;
 }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Quoted 11 lines

Quoted 6 lines

Quoted 2 lines · Jan Beulich

Quoted 1 line · Konrad Rzeszutek Wilk

The caller that orchestrates the state changes is xenwatch_thread and it takes a mutex. In our processing of Xenbus states we can take the luxery of going to sleep on a mutex, so lets do that and

This is only the direct conversion of existing spinlock accesses in xenbus.c. However, in the course of converting from the legacy implementation you stripped a couple more (in xen_pcibk_attach(), xen_pcibk_reconfigure(), and xen_pcibk_setup_backend()), and

Actually, xen_pcibk_attach() has its lock taken in xen_pcibk_do_attach(), so no change needed there.

In xen_pcibk_reconfigure() and xen_pcibk_setup_backend() the locking may be redundant with the one in passthrough.c/vpci.c - is that the basis upon which you removed the locks taken there?

Quoted 15 lines · Jan Beulich

Quoted 11 lines

Quoted 6 lines

Quoted 2 lines · Jan Beulich

Quoted 1 line · Konrad Rzeszutek Wilk

The caller that orchestrates the state changes is xenwatch_thread and it takes a mutex. In our processing of Xenbus states we can take the luxery of going to sleep on a mutex, so lets do that and

This is only the direct conversion of existing spinlock accesses in xenbus.c. However, in the course of converting from the legacy implementation you stripped a couple more (in xen_pcibk_attach(), xen_pcibk_reconfigure(), and xen_pcibk_setup_backend()), and

Actually, xen_pcibk_attach() has its lock taken in xen_pcibk_do_attach(), so no change needed there.

Quoted 3 lines

In xen_pcibk_reconfigure() and xen_pcibk_setup_backend() the locking may be redundant with the one in passthrough.c/vpci.c - is that the basis upon which you removed the locks taken there?

No. I believe the reason was much simpler.. it was b/c of this patch (see below). But for the life of me I don't recall what deadlock we could hit.