[PATCH]: deadlock in make_struct()

["Linas Vepstas" <linasvepstas@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2652 bytes --]

Hi,

Here's a deadlock patch. When committing this patch, please copy
the text below into the source code commit message, as it provides
a record of what the patch does, and why it was made.

--linas


The patch below fixes a deadlock in the multi-threading code.
It fixes this by simply removing the CRITICAL_SECTION lock.
There does not seem to be any need for a lock on this section,
since all variables accessed are local, and all other literals
are compile-time constants.

A typical deadlock that was witnessed was:

thread 7 -- holding critical section lock, sleeping on &scm_i_sweep_mutex
            held in scm_make_struct() at struct.c:463,
            sleeping in increase_mtrigger() at  gc-malloc.c:234
thread 5 -- holding heap_mutex, sleeping on critical section
            held because thread is in guile mode
            sleeping in scm_c_catch() at throw.c:187
thread 12 -- holding &scm_i_sweep_mutex, sleeping on heap_mutex
             held in increase_mtrigger() at gc-malloc.c:234
             sleeping in scm_i_thread_put_to_sleep() at threads.c:1621

Signed-off-by: Linas Vepstas <linasvepstas@gmail.com>

---
 libguile/struct.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Index: guile-1.8.5/libguile/struct.c
===================================================================
--- guile-1.8.5.orig/libguile/struct.c	2008-11-16 10:58:31.000000000 -0600
+++ guile-1.8.5/libguile/struct.c	2008-11-16 12:17:05.000000000 -0600
@@ -450,7 +450,14 @@ SCM_DEFINE (scm_make_struct, "make-struc
         goto bad_tail;
     }

-  SCM_CRITICAL_SECTION_START;
+  /* In guile 1.8.5 and earlier, everything below was covered by a
+     CRITICAL_SECTION lock. This leads to deadlocks in garbage collection,
+     since other threads might be holding the heap_mutex, while sleeping
+     on the CRITICAL_SECTION lock. There does not seem to be any need
+     for a lock on the section below, as it does not access or update
+     any globals. vtable, basic_size, tail_elts are all local variables,
+     scm_tc3_struct and scm_struct_i_* are all compile-time consts.
+     So the lock has been removed. */
   if (SCM_STRUCT_DATA (vtable)[scm_struct_i_flags] & SCM_STRUCTF_ENTITY)
     {
       data = scm_alloc_struct (basic_size + tail_elts,
@@ -466,7 +473,6 @@ SCM_DEFINE (scm_make_struct, "make-struc
   handle = scm_double_cell ((((scm_t_bits) SCM_STRUCT_DATA (vtable))
 			     + scm_tc3_struct),
 			    (scm_t_bits) data, 0, 0);
-  SCM_CRITICAL_SECTION_END;

   /* In guile 1.8.1 and earlier, the SCM_CRITICAL_SECTION_END above covered
      also the following scm_struct_init.  But that meant if scm_struct_init

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: deadlock-struct.patch --]
[-- Type: text/x-diff; name=deadlock-struct.patch, Size: 2746 bytes --]

Subject: [PATCH]: deadlock in make_struct()
Date: 16 Nov 2008
To: bug-guile@gnu.org

Hi, 

Here's a deadlock patch. When committing this patch, please copy
the text below into the source code commit message, as it provides
a record of what the patch does, and why it was made. 

--linas


The patch below fixes a deadlock in the multi-threading code.
It fixes this by simply removing the CRITICAL_SECTION lock.
There does not seem to be any need for a lock on this section,
since all variables accessed are local, and all other literals
are compile-time constants.

A typical deadlock that was witnessed was:

thread 7 -- holding critical section lock, sleeping on &scm_i_sweep_mutex
            held in scm_make_struct() at struct.c:463, 
            sleeping in increase_mtrigger() at  gc-malloc.c:234
thread 5 -- holding heap_mutex, sleeping on critical section
            held because thread is in guile mode
            sleeping in scm_c_catch() at throw.c:187
thread 12 -- holding &scm_i_sweep_mutex, sleeping on heap_mutex
             held in increase_mtrigger() at gc-malloc.c:234
             sleeping in scm_i_thread_put_to_sleep() at threads.c:1621

Signed-off-by: Linas Vepstas <linasvepstas@gmail.com>

---
 libguile/struct.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Index: guile-1.8.5/libguile/struct.c
===================================================================
--- guile-1.8.5.orig/libguile/struct.c	2008-11-16 10:58:31.000000000 -0600
+++ guile-1.8.5/libguile/struct.c	2008-11-16 12:17:05.000000000 -0600
@@ -450,7 +450,14 @@ SCM_DEFINE (scm_make_struct, "make-struc
         goto bad_tail;
     }
     
-  SCM_CRITICAL_SECTION_START;
+  /* In guile 1.8.5 and earlier, everything below was covered by a
+     CRITICAL_SECTION lock. This leads to deadlocks in garbage collection,
+     since other threads might be holding the heap_mutex, while sleeping
+     on the CRITICAL_SECTION lock. There does not seem to be any need
+     for a lock on the section below, as it does not access or update
+     any globals. vtable, basic_size, tail_elts are all local variables,
+     scm_tc3_struct and scm_struct_i_* are all compile-time consts.
+     So the lock has been removed. */
   if (SCM_STRUCT_DATA (vtable)[scm_struct_i_flags] & SCM_STRUCTF_ENTITY)
     {
       data = scm_alloc_struct (basic_size + tail_elts,
@@ -466,7 +473,6 @@ SCM_DEFINE (scm_make_struct, "make-struc
   handle = scm_double_cell ((((scm_t_bits) SCM_STRUCT_DATA (vtable))
 			     + scm_tc3_struct),
 			    (scm_t_bits) data, 0, 0);
-  SCM_CRITICAL_SECTION_END;
 
   /* In guile 1.8.1 and earlier, the SCM_CRITICAL_SECTION_END above covered
      also the following scm_struct_init.  But that meant if scm_struct_init