* [PATCH] xen/pciback: Use mutexes when working with Xenbus state transitions.
@ 2011-09-16 19:06 Konrad Rzeszutek Wilk
2011-09-19 10:43 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Konrad Rzeszutek Wilk @ 2011-09-16 19:06 UTC (permalink / raw)
To: xen-devel, JBeulich; +Cc: linux-kernel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] xen/pciback: Use mutexes when working with Xenbus state transitions.
2011-09-16 19:06 [PATCH] xen/pciback: Use mutexes when working with Xenbus state transitions Konrad Rzeszutek Wilk
@ 2011-09-19 10:43 ` Jan Beulich
2011-09-19 11:01 ` [Xen-devel] " Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2011-09-19 10:43 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: xen-devel, linux-kernel
>>> 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
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;
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Xen-devel] Re: [PATCH] xen/pciback: Use mutexes when working with Xenbus state transitions.
2011-09-19 10:43 ` Jan Beulich
@ 2011-09-19 11:01 ` Jan Beulich
2011-09-21 21:08 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2011-09-19 11:01 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: xen-devel, linux-kernel
>>> 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?
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Xen-devel] Re: [PATCH] xen/pciback: Use mutexes when working with Xenbus state transitions.
2011-09-19 11:01 ` [Xen-devel] " Jan Beulich
@ 2011-09-21 21:08 ` Konrad Rzeszutek Wilk
2011-09-21 21:12 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 5+ messages in thread
From: Konrad Rzeszutek Wilk @ 2011-09-21 21:08 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, linux-kernel
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);
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Xen-devel] Re: [PATCH] xen/pciback: Use mutexes when working with Xenbus state transitions.
2011-09-21 21:08 ` Konrad Rzeszutek Wilk
@ 2011-09-21 21:12 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 5+ messages in thread
From: Konrad Rzeszutek Wilk @ 2011-09-21 21:12 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, linux-kernel
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-09-21 21:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-16 19:06 [PATCH] xen/pciback: Use mutexes when working with Xenbus state transitions Konrad Rzeszutek Wilk
2011-09-19 10:43 ` Jan Beulich
2011-09-19 11:01 ` [Xen-devel] " Jan Beulich
2011-09-21 21:08 ` Konrad Rzeszutek Wilk
2011-09-21 21:12 ` Konrad Rzeszutek Wilk
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox