Ian Campbell
2009-Mar-24 17:44 UTC
[Xen-devel] [PATCH] xenbus: do not hold transaction_mutex when returning to userspace
=============================================== [ BUG: lock held when returning
to user space! ]
------------------------------------------------
xenstore-list/3522 is leaving the kernel with locks still held!
1 lock held by xenstore-list/3522:
#0: (&xs_state.transaction_mutex){......}, at: [<c026dc6f>]
xenbus_dev_request_and_reply+0x8f/0xa0
The canonical fix for this type of issue appears to be to maintain a
count manually rather than using an rwsem so do that here.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
drivers/xen/xenbus/xenbus_xs.c | 57 +++++++++++++++++++++++++++++++++-------
1 files changed, 47 insertions(+), 10 deletions(-)
diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
index eab33f1..6f91e8c 100644
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -76,6 +76,14 @@ struct xs_handle {
/*
* Mutex ordering: transaction_mutex -> watch_mutex -> request_mutex.
* response_mutex is never taken simultaneously with the other three.
+ *
+ * transaction_mutex must be held before incrementing
+ * transaction_count. The mutex is held when a suspend is in
+ * progress to prevent new transactions starting.
+ *
+ * When decrementing transaction_count to zero the wait queue
+ * should be woken up, the suspend code waits for count to
+ * reach zero.
*/
/* One request at a time. */
@@ -85,7 +93,9 @@ struct xs_handle {
struct mutex response_mutex;
/* Protect transactions against save/restore. */
- struct rw_semaphore transaction_mutex;
+ struct mutex transaction_mutex;
+ atomic_t transaction_count;
+ wait_queue_head_t transaction_wq;
/* Protect watch (de)register against save/restore. */
struct rw_semaphore watch_mutex;
@@ -157,6 +167,31 @@ static void *read_reply(enum xsd_sockmsg_type *type,
unsigned int *len)
return body;
}
+static void transaction_start(void)
+{
+ mutex_lock(&xs_state.transaction_mutex);
+ atomic_inc(&xs_state.transaction_count);
+ mutex_unlock(&xs_state.transaction_mutex);
+}
+
+static void transaction_end(void)
+{
+ if (atomic_dec_and_test(&xs_state.transaction_count))
+ wake_up(&xs_state.transaction_wq);
+}
+
+static void transaction_suspend(void)
+{
+ mutex_lock(&xs_state.transaction_mutex);
+ wait_event(xs_state.transaction_wq,
+ atomic_read(&xs_state.transaction_count) == 0);
+}
+
+static void transaction_resume(void)
+{
+ mutex_unlock(&xs_state.transaction_mutex);
+}
+
void *xenbus_dev_request_and_reply(struct xsd_sockmsg *msg)
{
void *ret;
@@ -164,7 +199,7 @@ void *xenbus_dev_request_and_reply(struct xsd_sockmsg *msg)
int err;
if (req_msg.type == XS_TRANSACTION_START)
- down_read(&xs_state.transaction_mutex);
+ transaction_start();
mutex_lock(&xs_state.request_mutex);
@@ -180,7 +215,7 @@ void *xenbus_dev_request_and_reply(struct xsd_sockmsg *msg)
if ((msg->type == XS_TRANSACTION_END) ||
((req_msg.type == XS_TRANSACTION_START) &&
(msg->type == XS_ERROR)))
- up_read(&xs_state.transaction_mutex);
+ transaction_end();
return ret;
}
@@ -432,11 +467,11 @@ int xenbus_transaction_start(struct xenbus_transaction *t)
{
char *id_str;
- down_read(&xs_state.transaction_mutex);
+ transaction_start();
id_str = xs_single(XBT_NIL, XS_TRANSACTION_START, "", NULL);
if (IS_ERR(id_str)) {
- up_read(&xs_state.transaction_mutex);
+ transaction_end();
return PTR_ERR(id_str);
}
@@ -461,7 +496,7 @@ int xenbus_transaction_end(struct xenbus_transaction t, int
abort)
err = xs_error(xs_single(t, XS_TRANSACTION_END, abortstr, NULL));
- up_read(&xs_state.transaction_mutex);
+ transaction_end();
return err;
}
@@ -662,7 +697,7 @@ EXPORT_SYMBOL_GPL(unregister_xenbus_watch);
void xs_suspend(void)
{
- down_write(&xs_state.transaction_mutex);
+ transaction_suspend();
down_write(&xs_state.watch_mutex);
mutex_lock(&xs_state.request_mutex);
mutex_lock(&xs_state.response_mutex);
@@ -677,7 +712,7 @@ void xs_resume(void)
mutex_unlock(&xs_state.response_mutex);
mutex_unlock(&xs_state.request_mutex);
- up_write(&xs_state.transaction_mutex);
+ transaction_resume();
/* No need for watches_lock: the watch_mutex is sufficient. */
list_for_each_entry(watch, &watches, list) {
@@ -693,7 +728,7 @@ void xs_suspend_cancel(void)
mutex_unlock(&xs_state.response_mutex);
mutex_unlock(&xs_state.request_mutex);
up_write(&xs_state.watch_mutex);
- up_write(&xs_state.transaction_mutex);
+ mutex_unlock(&xs_state.transaction_mutex);
}
static int xenwatch_thread(void *unused)
@@ -843,8 +878,10 @@ int xs_init(void)
mutex_init(&xs_state.request_mutex);
mutex_init(&xs_state.response_mutex);
- init_rwsem(&xs_state.transaction_mutex);
+ mutex_init(&xs_state.transaction_mutex);
init_rwsem(&xs_state.watch_mutex);
+ atomic_set(&xs_state.transaction_count, 0);
+ init_waitqueue_head(&xs_state.transaction_wq);
/* Initialize the shared memory rings to talk to xenstored */
err = xb_init_comms();
--
1.5.6.5
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2009-May-20 14:48 UTC
[Xen-devel] [GIT] Re: [PATCH] xenbus: do not hold transaction_mutex when returning to userspace
On Tue, 2009-03-24 at 13:44 -0400, Ian Campbell wrote:> ===============================================> [ BUG: lock held when returning to user space! ] > ------------------------------------------------ > xenstore-list/3522 is leaving the kernel with locks still held! > 1 lock held by xenstore-list/3522: > #0: (&xs_state.transaction_mutex){......}, at: [<c026dc6f>] xenbus_dev_request_and_reply+0x8f/0xa0I''m still seeing these with xen-tip/master, I guess the patch got dropped somewhere along the way? The following changes since commit 4c2f7369e095df4c0a9b5c4cd05926f68cc701dd: Jeremy Fitzhardinge (1): Merge branch ''xen-tip/core'' into xen-tip/master are available in the git repository at: git://xenbits.xensource.com/people/ianc/linux-2.6.git for-jeremy/xenbus Ian Campbell (1): xenbus: do not hold transaction_mutex when returning to userspace drivers/xen/xenbus/xenbus_xs.c | 57 +++++++++++++++++++++++++++++++++------- 1 files changed, 47 insertions(+), 10 deletions(-)> > The canonical fix for this type of issue appears to be to maintain a > count manually rather than using an rwsem so do that here. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > --- > drivers/xen/xenbus/xenbus_xs.c | 57 +++++++++++++++++++++++++++++++++------- > 1 files changed, 47 insertions(+), 10 deletions(-) > > diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c > index eab33f1..6f91e8c 100644 > --- a/drivers/xen/xenbus/xenbus_xs.c > +++ b/drivers/xen/xenbus/xenbus_xs.c > @@ -76,6 +76,14 @@ struct xs_handle { > /* > * Mutex ordering: transaction_mutex -> watch_mutex -> request_mutex. > * response_mutex is never taken simultaneously with the other three. > + * > + * transaction_mutex must be held before incrementing > + * transaction_count. The mutex is held when a suspend is in > + * progress to prevent new transactions starting. > + * > + * When decrementing transaction_count to zero the wait queue > + * should be woken up, the suspend code waits for count to > + * reach zero. > */ > > /* One request at a time. */ > @@ -85,7 +93,9 @@ struct xs_handle { > struct mutex response_mutex; > > /* Protect transactions against save/restore. */ > - struct rw_semaphore transaction_mutex; > + struct mutex transaction_mutex; > + atomic_t transaction_count; > + wait_queue_head_t transaction_wq; > > /* Protect watch (de)register against save/restore. */ > struct rw_semaphore watch_mutex; > @@ -157,6 +167,31 @@ static void *read_reply(enum xsd_sockmsg_type *type, unsigned int *len) > return body; > } > > +static void transaction_start(void) > +{ > + mutex_lock(&xs_state.transaction_mutex); > + atomic_inc(&xs_state.transaction_count); > + mutex_unlock(&xs_state.transaction_mutex); > +} > + > +static void transaction_end(void) > +{ > + if (atomic_dec_and_test(&xs_state.transaction_count)) > + wake_up(&xs_state.transaction_wq); > +} > + > +static void transaction_suspend(void) > +{ > + mutex_lock(&xs_state.transaction_mutex); > + wait_event(xs_state.transaction_wq, > + atomic_read(&xs_state.transaction_count) == 0); > +} > + > +static void transaction_resume(void) > +{ > + mutex_unlock(&xs_state.transaction_mutex); > +} > + > void *xenbus_dev_request_and_reply(struct xsd_sockmsg *msg) > { > void *ret; > @@ -164,7 +199,7 @@ void *xenbus_dev_request_and_reply(struct xsd_sockmsg *msg) > int err; > > if (req_msg.type == XS_TRANSACTION_START) > - down_read(&xs_state.transaction_mutex); > + transaction_start(); > > mutex_lock(&xs_state.request_mutex); > > @@ -180,7 +215,7 @@ void *xenbus_dev_request_and_reply(struct xsd_sockmsg *msg) > if ((msg->type == XS_TRANSACTION_END) || > ((req_msg.type == XS_TRANSACTION_START) && > (msg->type == XS_ERROR))) > - up_read(&xs_state.transaction_mutex); > + transaction_end(); > > return ret; > } > @@ -432,11 +467,11 @@ int xenbus_transaction_start(struct xenbus_transaction *t) > { > char *id_str; > > - down_read(&xs_state.transaction_mutex); > + transaction_start(); > > id_str = xs_single(XBT_NIL, XS_TRANSACTION_START, "", NULL); > if (IS_ERR(id_str)) { > - up_read(&xs_state.transaction_mutex); > + transaction_end(); > return PTR_ERR(id_str); > } > > @@ -461,7 +496,7 @@ int xenbus_transaction_end(struct xenbus_transaction t, int abort) > > err = xs_error(xs_single(t, XS_TRANSACTION_END, abortstr, NULL)); > > - up_read(&xs_state.transaction_mutex); > + transaction_end(); > > return err; > } > @@ -662,7 +697,7 @@ EXPORT_SYMBOL_GPL(unregister_xenbus_watch); > > void xs_suspend(void) > { > - down_write(&xs_state.transaction_mutex); > + transaction_suspend(); > down_write(&xs_state.watch_mutex); > mutex_lock(&xs_state.request_mutex); > mutex_lock(&xs_state.response_mutex); > @@ -677,7 +712,7 @@ void xs_resume(void) > > mutex_unlock(&xs_state.response_mutex); > mutex_unlock(&xs_state.request_mutex); > - up_write(&xs_state.transaction_mutex); > + transaction_resume(); > > /* No need for watches_lock: the watch_mutex is sufficient. */ > list_for_each_entry(watch, &watches, list) { > @@ -693,7 +728,7 @@ void xs_suspend_cancel(void) > mutex_unlock(&xs_state.response_mutex); > mutex_unlock(&xs_state.request_mutex); > up_write(&xs_state.watch_mutex); > - up_write(&xs_state.transaction_mutex); > + mutex_unlock(&xs_state.transaction_mutex); > } > > static int xenwatch_thread(void *unused) > @@ -843,8 +878,10 @@ int xs_init(void) > > mutex_init(&xs_state.request_mutex); > mutex_init(&xs_state.response_mutex); > - init_rwsem(&xs_state.transaction_mutex); > + mutex_init(&xs_state.transaction_mutex); > init_rwsem(&xs_state.watch_mutex); > + atomic_set(&xs_state.transaction_count, 0); > + init_waitqueue_head(&xs_state.transaction_wq); > > /* Initialize the shared memory rings to talk to xenstored */ > err = xb_init_comms();_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel