Konrad Rzeszutek Wilk
2011-Sep-16 19:06 UTC
[Xen-devel] [PATCH] xen/pciback: Use mutexes when working with Xenbus state transitions.
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 <konrad.wilk@oracle.com>
---
drivers/xen/xen-pciback/pciback.h | 2 +-
drivers/xen/xen-pciback/xenbus.c | 16 +++++-----------
2 files changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/xen/xen-pciback/pciback.h
b/drivers/xen/xen-pciback/pciback.h
index a0e131a..c3af628 100644
--- a/drivers/xen/xen-pciback/pciback.h
+++ b/drivers/xen/xen-pciback/pciback.h
@@ -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;
diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index 978d2c6..c057d67 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -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;
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)
@@ -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(
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;
}
--
1.7.4.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Jan Beulich
2011-Sep-19 10:43 UTC
[Xen-devel] Re: [PATCH] xen/pciback: Use mutexes when working with Xenbus state transitions.
>>> On 16.09.11 at 21:06, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:> 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 andThis 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> 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 <konrad.wilk@oracle.com> > --- > drivers/xen/xen-pciback/pciback.h | 2 +- > drivers/xen/xen-pciback/xenbus.c | 16 +++++----------- > 2 files changed, 6 insertions(+), 12 deletions(-) > > diff --git a/drivers/xen/xen-pciback/pciback.h > b/drivers/xen/xen-pciback/pciback.h > index a0e131a..c3af628 100644 > --- a/drivers/xen/xen-pciback/pciback.h > +++ b/drivers/xen/xen-pciback/pciback.h > @@ -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; > diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c > index 978d2c6..c057d67 100644 > --- a/drivers/xen/xen-pciback/xenbus.c > +++ b/drivers/xen/xen-pciback/xenbus.c > @@ -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; > 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) > @@ -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( > 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; > } >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Sep-19 11:01 UTC
[Xen-devel] Re: [PATCH] xen/pciback: Use mutexes when working with Xenbus state transitions.
>>> On 19.09.11 at 12:43, "Jan Beulich" <JBeulich@suse.com> wrote: >>>> On 16.09.11 at 21:06, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > >> 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()), andActually, 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? Jan> those should now get re-added imo. > > Jan > >> 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 <konrad.wilk@oracle.com> >> --- >> drivers/xen/xen-pciback/pciback.h | 2 +- >> drivers/xen/xen-pciback/xenbus.c | 16 +++++----------- >> 2 files changed, 6 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/xen/xen-pciback/pciback.h >> b/drivers/xen/xen-pciback/pciback.h >> index a0e131a..c3af628 100644 >> --- a/drivers/xen/xen-pciback/pciback.h >> +++ b/drivers/xen/xen-pciback/pciback.h >> @@ -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; >> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c >> index 978d2c6..c057d67 100644 >> --- a/drivers/xen/xen-pciback/xenbus.c >> +++ b/drivers/xen/xen-pciback/xenbus.c >> @@ -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; >> 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) >> @@ -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( >> 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; >> } >> > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Sep-21 21:08 UTC
Re: [Xen-devel] Re: [PATCH] xen/pciback: Use mutexes when working with Xenbus state transitions.
On Mon, Sep 19, 2011 at 12:01:56PM +0100, Jan Beulich wrote:> >>> On 19.09.11 at 12:43, "Jan Beulich" <JBeulich@suse.com> wrote: > >>>> On 16.09.11 at 21:06, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > > > >> 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?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. I think the better choice will be to restore the call-sites of these spinlocks but use mutex instead. So let me post two patches - one for xenbus.c and one for vpci.c to complement "xen/pciback: use mutex rather than spinlock in passthrough backend" you posted and I queued up. commit 3a8d1841ae2dd32452b79284da03eda596f30827 Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Date: Fri Jul 23 14:35:47 2010 -0400 xen/pciback: Redo spinlock usage. We were using coarse spinlocks that could end up with a deadlock. This patch fixes that and makes the spinlocks much more fine-grained. We also drop be->watchding state spinlocks as they are already guarded by the xenwatch_thread against multiple customers. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> diff --git a/drivers/xen/pciback/xenbus.c b/drivers/xen/pciback/xenbus.c index d448bf5..993b659 100644 --- a/drivers/xen/pciback/xenbus.c +++ b/drivers/xen/pciback/xenbus.c @@ -54,23 +54,29 @@ static void pciback_disconnect(struct pciback_device *pdev) 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 */ + + /* Note, the workqueue does not use spinlocks at all.*/ flush_workqueue(pciback_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); + } static void free_pdev(struct pciback_device *pdev) { - if (pdev->be_watching) + if (pdev->be_watching) { unregister_xenbus_watch(&pdev->be_watch); + pdev->be_watching = 0; + } pciback_disconnect(pdev); @@ -98,7 +104,10 @@ static int pciback_do_attach(struct pciback_device *pdev, int gnt_ref, "Error mapping other domain page in ours."); goto out; } + + spin_lock(&pdev->dev_lock); pdev->sh_info = vaddr; + spin_unlock(&pdev->dev_lock); err = bind_interdomain_evtchn_to_irqhandler( pdev->xdev->otherend_id, remote_evtchn, pciback_handle_event, @@ -108,7 +117,10 @@ static int pciback_do_attach(struct pciback_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"); @@ -122,7 +134,6 @@ static int pciback_attach(struct pciback_device *pdev) int gnt_ref, remote_evtchn; char *magic = NULL; - spin_lock(&pdev->dev_lock); /* Make sure we only do this setup once */ if (xenbus_read_driver_state(pdev->xdev->nodename) !@@ -168,7 +179,6 @@ static int pciback_attach(struct pciback_device *pdev) dev_dbg(&pdev->xdev->dev, "Connected? %d\n", err); out: - spin_unlock(&pdev->dev_lock); kfree(magic); @@ -340,7 +350,6 @@ static int pciback_reconfigure(struct pciback_device *pdev) char state_str[64]; char dev_str[64]; - spin_lock(&pdev->dev_lock); dev_dbg(&pdev->xdev->dev, "Reconfiguring device ...\n"); @@ -481,8 +490,6 @@ static int pciback_reconfigure(struct pciback_device *pdev) } out: - spin_unlock(&pdev->dev_lock); - return 0; } @@ -539,8 +546,6 @@ static int pciback_setup_backend(struct pciback_device *pdev) char dev_str[64]; char state_str[64]; - spin_lock(&pdev->dev_lock); - /* It''s possible we could get the call to setup twice, so make sure * we''re not already connected. */ @@ -621,8 +626,6 @@ static int pciback_setup_backend(struct pciback_device *pdev) "Error switching to initialised state!"); out: - spin_unlock(&pdev->dev_lock); - if (!err) /* see if pcifront is already configured (if not, we''ll wait) */ pciback_attach(pdev); @@ -669,6 +672,7 @@ static int pciback_xenbus_probe(struct xenbus_device *dev, pciback_be_watch); if (err) goto out; + pdev->be_watching = 1; /* We need to force a call to our callback here in case @@ -708,8 +712,8 @@ int __init pciback_xenbus_register(void) { pciback_wq = create_workqueue("pciback_workqueue"); if (!pciback_wq) { - printk(KERN_ERR "pciback_xenbus_register: create" - "pciback_workqueue failed\n"); + printk(KERN_ERR "%s: create" + "pciback_workqueue failed\n",__FUNCTION__); return -EFAULT; } return xenbus_register_backend(&xenbus_pciback_driver); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-Sep-21 21:12 UTC
Re: [Xen-devel] Re: [PATCH] xen/pciback: Use mutexes when working with Xenbus state transitions.
On Wed, Sep 21, 2011 at 05:08:38PM -0400, Konrad Rzeszutek Wilk wrote:> On Mon, Sep 19, 2011 at 12:01:56PM +0100, Jan Beulich wrote: > > >>> On 19.09.11 at 12:43, "Jan Beulich" <JBeulich@suse.com> wrote: > > >>>> On 16.09.11 at 21:06, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > > > > > >> 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? > > 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.You know what.. I think the issue was that I was trying to fix the "sleeping on a spinlock" issue and was moving the spinlocks around to fix it. .. Without realizing I could have just used a mutex. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel