WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] [GIT] Re: [PATCH] xenbus: do not hold transaction_mutex when

To: "jeremy@xxxxxxxx" <jeremy@xxxxxxxx>
Subject: [Xen-devel] [GIT] Re: [PATCH] xenbus: do not hold transaction_mutex when returning to userspace
From: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
Date: Wed, 20 May 2009 15:48:31 +0100
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Wed, 20 May 2009 07:48:58 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1237916662-17792-1-git-send-email-Ian.Campbell@xxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <1237916662-17792-1-git-send-email-Ian.Campbell@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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/0xa0

I'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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>
  • [Xen-devel] [GIT] Re: [PATCH] xenbus: do not hold transaction_mutex when returning to userspace, Ian Campbell <=