Patch: PR 16748 (Re: hash synchronization on linux)

[Bryce McKinlay <mckinlay at redhat dot com>]

The attached patch should correct this problem, using Hans' first 
suggestion below. I'm going to check this in to HEAD and the GCC 3.4 
branch, and maybe GCC 3.3 branch as well.

Thanks to you both for tracking this down.

Regards

Bryce


Boehm, Hans wrote:

> The problem is in thread 8.  I doubt it has been fixed,
> but I didn't check.
>
> Basically we have
>
> _Jv_MonitorEnter
> 	acquires lock bit on lightweight lock entry
> 	notices it needs to allocate heavyweight lock
> 	calls eventually alloc_heavy.
> alloc_heavy call GC_local_gcj_malloc
> GC_local_gcj_malloc is out of free list entries AND
> 	is first allocation to run since GC, which
> 	results in a call to
> FinalizerThread.finalizerReady, which is currently
> 	unrestricted Java code.  It then calls
> _Jv_MonitorEnter
> 	which tries to acquire the same lock table
> 	entry it already owns ==>
>
> deadlock
>
> Is it hard to make FinalizerThread.finalizerReady a small piece
> of C++ code, which does not
> acquire Java locks, e.g. by just doing a pthread_cond_signal
> equivalent?
>  
>
> The alternative would be to avoid allocation calls with the
> lock bit on a lock table entry set.  (I think that registering
> a finalizer is OK.  That allocates internally, but I don't
> think it can invoke finalizer notification.)  But that looks
> a bit messy and more expensive:  You'd have to preallocate the
> heavyweight object before acquiring the lock bit, and then drop
> it if you didn't need it.
>
> Or I guess we could add a GC allocation call which didn't do the
> finalizer notification check.
>
> I'm also not sure that running Java code during finalizer
> notification is otherwise safe.  This can happen during arbitrary
> allocations, with locks held, etc.
>
> Hans
>  
>
> > -----Original Message-----
> > From: java-owner@gcc.gnu.org 
> > [mailto:java-owner@gcc.gnu.org]On Behalf Of
> > Jacob Gladish
> > Sent: Thursday, July 08, 2004 2:32 PM
> > To: java@gcc.gnu.org
> > Subject: hash synchronization on linux
> >
> >
> > I've suspected a problem with the hash synchronization for 
> > some time now
> > and decided that I might be able to reproduce the problem with a unit
> > test, and think that I have stumbled onto something. 
> > I've attached two classes that can be run against gcc-3.1 and very
> > quickly deadlock. To make this happen you must rebuild with a changed
> > hash_entry table size of 2. I suspected a race with bucket 
> > access (heavy
> > lock chain) so reducing the size increased the likelihood that I would
> > be able to reproduce this easily. This is being run on Linux 
> > SMP 2.4.18
> > (machine has two procs).
> > I've also included the backtrace of all the threads in this state. No
> > threads ever return from _Jv_MonitorEnter. I know the 3.1 release is
> > pretty old at this point, but I'm in the middle of building 3.4.1 with
> > the hash_entry table size shrunk to 2 and am going to see if 
> > it behaves
> > the same. I was also under the impression that the 
> > hash_synchronization
> > code has basically been untouched for a while.
> > Thanks
> > Jake
> >
> >    

2004-07-11  Bryce McKinlay  <mckinlay@redhat.com>

	PR libgcj/16748	
	* prims.cc (_Jv_CreateJavaVM): Fix comment.
	* gnu/gcj/runtime/FinalizerThread.java (init): New. Native.
	(finalizerReady): Now native.
	(run): Likewise.
	(runFinalizers): Removed.
	* gnu/gcj/runtime/natFinalizerThread.cc (run): Implement here. Use
	a primitive lock, and don't hold it while running the finalizers.
	(runFinalizers): Implement. Don't aquire any Java lock.
	(finalizerReady): Use lock primitives to signal finalizer thread.

Index: prims.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/prims.cc,v
retrieving revision 1.90
diff -u -r1.90 prims.cc
--- prims.cc	4 Jul 2004 15:27:04 -0000	1.90
+++ prims.cc	11 Jul 2004 21:05:19 -0000
@@ -1008,8 +1008,7 @@
   _Jv_GCInitializeFinalizers (&::gnu::gcj::runtime::FinalizerThread::finalizerReady);
 
   // Start the GC finalizer thread.  A VirtualMachineError can be
-  // thrown by the runtime if, say, threads aren't available.  In this
-  // case finalizers simply won't run.
+  // thrown by the runtime if, say, threads aren't available.
   try
     {
       using namespace gnu::gcj::runtime;
Index: gnu/gcj/runtime/FinalizerThread.java
===================================================================
RCS file: /cvs/gcc/gcc/libjava/gnu/gcj/runtime/FinalizerThread.java,v
retrieving revision 1.1
diff -u -r1.1 FinalizerThread.java
--- gnu/gcj/runtime/FinalizerThread.java	10 Oct 2001 22:25:43 -0000	1.1
+++ gnu/gcj/runtime/FinalizerThread.java	11 Jul 2004 21:05:19 -0000
@@ -1,6 +1,6 @@
 // FinalizerThread.java -- Thread in which finalizers are run.
 
-/* Copyright (C) 2001  Free Software Foundation
+/* Copyright (C) 2001, 2004  Free Software Foundation
 
    This file is part of libgcj.
 
@@ -16,58 +16,17 @@
  */
 public final class FinalizerThread extends Thread
 {
-  // Finalizers must be run in a thread with no Java-visible locks
-  // held.  This qualifies because we don't make the lock visible.
-  private static final Object lock = new Object ();
-
-  // This is true if the finalizer thread started successfully.  It
-  // might be false if, for instance, there are no threads on the
-  // current platform.  In this situation we run finalizers in the
-  // caller's thread.
-  private static boolean thread_started = false;
+  private static boolean finalizer_ready;
 
   public FinalizerThread ()
   {
     super ("LibgcjInternalFinalizerThread");
     setDaemon (true);
+    finalizer_ready = false;
+    init();
   }
 
-  // This is called by the runtime when a finalizer is ready to be
-  // run.  It simply wakes up the finalizer thread.
-  public static void finalizerReady ()
-  {
-    synchronized (lock)
-      {
-	if (! thread_started)
-	  runFinalizers ();
-	else
-	  lock.notify ();
-      }
-  }
-
-  // Actually run the finalizers.
-  private static native void runFinalizers ();
-
-  public void run ()
-  {
-    // Wait on a lock.  Whenever we wake up, try to invoke the
-    // finalizers.
-    synchronized (lock)
-      {
-	thread_started = true;
-	while (true)
-	  {
-	    try
-	      {
-		lock.wait ();
-	      }
-	    catch (InterruptedException _)
-	      {
-		// Just ignore it.  It doesn't hurt to run finalizers
-		// when none are pending.
-	      }
-	    runFinalizers ();
-	  }
-      }
-  }
+  private native void init();
+  static native void finalizerReady();
+  public native void run();
 }
Index: gnu/gcj/runtime/natFinalizerThread.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/gnu/gcj/runtime/natFinalizerThread.cc,v
retrieving revision 1.1
diff -u -r1.1 natFinalizerThread.cc
--- gnu/gcj/runtime/natFinalizerThread.cc	10 Oct 2001 22:25:43 -0000	1.1
+++ gnu/gcj/runtime/natFinalizerThread.cc	11 Jul 2004 21:05:19 -0000
@@ -1,6 +1,6 @@
 // natFinalizerThread.cc - Implementation of FinalizerThread native methods.
 
-/* Copyright (C) 2001  Free Software Foundation
+/* Copyright (C) 2001, 2004  Free Software Foundation
 
    This file is part of libgcj.
 
@@ -15,8 +15,48 @@
 
 #include <gnu/gcj/runtime/FinalizerThread.h>
 
+#include <java-threads.h>
+
+static _Jv_Mutex_t mutex;
+static _Jv_ConditionVariable_t condition;
+
+// Initialize lock & condition variable.
+void
+gnu::gcj::runtime::FinalizerThread::init ()
+{
+  _Jv_MutexInit (&mutex);
+  _Jv_CondInit (&condition);
+}
+
+// This is called by the GC when a finalizer is ready to be
+// run.  It sets a flag and wakes up the finalizer thread. Note
+// that this MUST NOT aquire any Java lock, as this could result in 
+// the hash synchronization code being re-entered: the synchronization
+// code itself might need to allocate. See PR 16478.
 void
-gnu::gcj::runtime::FinalizerThread::runFinalizers ()
+gnu::gcj::runtime::FinalizerThread::finalizerReady ()
 {
+#ifdef __JV_NO_THREADS__
   _Jv_RunFinalizers ();
+#else
+  _Jv_MutexLock (&mutex);
+  finalizer_ready = true;
+  _Jv_CondNotify (&condition, &mutex);
+  _Jv_MutexUnlock (&mutex);
+#endif
+}
+
+// Main loop for the finalizer thread. 
+void
+gnu::gcj::runtime::FinalizerThread::run ()
+{
+  while (true)
+    {
+      _Jv_MutexLock (&mutex);
+      if (! finalizer_ready)
+	_Jv_CondWait (&condition, &mutex, 0, 0);
+      finalizer_ready = false;
+      _Jv_MutexUnlock (&mutex);
+      _Jv_RunFinalizers ();
+    }
 }