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

Re: [Xen-devel] [PATCH, v2] xenstore: Fix deadlock in xs_read_watch

To: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH, v2] xenstore: Fix deadlock in xs_read_watch
From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Date: Mon, 30 Aug 2010 14:52:50 +0100
Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>
Delivery-date: Mon, 30 Aug 2010 06:53:20 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4C7BB3E6.1080406@xxxxxxxxxxxxx>
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>
References: <4C76D9C3.5020600@xxxxxxxxxxxxx> <alpine.DEB.2.00.1008301413080.2545@kaball-desktop> <4C7BB3E6.1080406@xxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Alpine 2.00 (DEB 1167 2008-08-23)
On Mon, 30 Aug 2010, Daniel De Graaf wrote:
> On 08/30/2010 09:16 AM, Stefano Stabellini wrote:
> > On Thu, 26 Aug 2010, Daniel De Graaf wrote:
> >> When read_message returns -1 while a read is pending, an attempt is made
> >> to lock h->request_mutex which is already locked by the reader. Change
> >> the read thread to only exit on thread cancellation, where
> >> read_thr_exists will return 0.
> >>
> >> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> >>
> >> diff -r 6bebaf40e925 tools/xenstore/xs.c
> >> --- a/tools/xenstore/xs.c  Tue Jul 20 13:42:17 2010 +0100
> >> +++ b/tools/xenstore/xs.c  Thu Aug 26 17:08:55 2010 -0400
> >> @@ -271,6 +271,7 @@
> >>  {
> >>  #ifdef USE_PTHREAD
> >>    if (h->read_thr_exists) {
> >> +          h->read_thr_exists = 0;
> >>            pthread_cancel(h->read_thr);
> >>            pthread_join(h->read_thr, NULL);
> >>    }
> >> @@ -667,7 +668,7 @@
> >>    mutex_lock(&h->watch_mutex);
> >>  
> >>    /* Wait on the condition variable for a watch to fire. */
> >> -  while (list_empty(&h->watch_list) && read_thread_exists(h))
> >> +  while (list_empty(&h->watch_list) && (!read_from_thread || 
> >> read_thread_exists(h)))
> >>            condvar_wait(&h->watch_condvar, &h->watch_mutex, h);
> >>    if (!read_thread_exists(h)) {
> >>            mutex_unlock(&h->watch_mutex);
> > 
> > read_from_thread is not declared in this function
> > 
> 
> Sorry about that, looks like I picked the wrong patch to send. Corrected
> patch attached.
> 

According to the description you gave, this patch fixes a problem
occurring when read_message returns -1 while a read is pending in
read_thread that is relevant only when USE_PTHREAD.
Why are you changing a code path that is only compiled when
!USE_PTHREAD?
Also shouldn't you be checking the return value of read_message anyway?
Considering that h->read_thr_exists is set to zero only on
xs_daemon_close, and in that case pthread_cancel is called right after,
I doubt that any of the operation run in read_thread after the loop will
be executed.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel