Re: [gold patch] Fix thread cancellation problem

Re: [gold patch] Fix thread cancellation problem

[Ian Lance Taylor]

Cary Coutant <ccoutant@google.com> writes:

> 	* workqueue-internal.h (Workqueue_threader::should_cancel_thread):
> 	Add thread_number parameter.
> 	(Workqueue_threader_threadpool::should_cancel_thread): Likewise.
> 	* workqueue-threads.cc
> 	(Workqueue_threader_threadpool::should_cancel_thread): Cancel
> 	current thread if its thread number is greater than desired thread
> 	count.
> 	* workqueue.cc (Workqueue_threader_single::should_cancel_thread):
> 	Add thread_number parameter.
> 	(Workqueue::should_cancel_thread): Likewise.
> 	(Workqueue::find_runnable_or_wait): Pass thread_number to
> 	should_cancel_thread.
> 	* workqueue.h (Workqueue::should_cancel_thread): Add thread_number
> 	parameter.

This is OK.

Thanks.

Ian

[gold patch] Fix thread cancellation problem

[Cary Coutant]

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

If the number of threads requested for --thread-count-middle or
--thread-count-final is smaller than that of an earlier pass, gold
will arrange for the n excess threads to cancel themselves. The first
n threads that happen to check for this condition will terminate.
Unfortunately, if one of those n threads happens to be thread #0,
control returns to the end of main(), and gold exits normally without
completing the link.

This patch fixes this problem by having only the threads whose thread
numbers are larger than the desired thread count terminate.

Tested on x86_64 with a build of libffmpeg.

OK?

-cary

	* workqueue-internal.h (Workqueue_threader::should_cancel_thread):
	Add thread_number parameter.
	(Workqueue_threader_threadpool::should_cancel_thread): Likewise.
	* workqueue-threads.cc
	(Workqueue_threader_threadpool::should_cancel_thread): Cancel
	current thread if its thread number is greater than desired thread
	count.
	* workqueue.cc (Workqueue_threader_single::should_cancel_thread):
	Add thread_number parameter.
	(Workqueue::should_cancel_thread): Likewise.
	(Workqueue::find_runnable_or_wait): Pass thread_number to
	should_cancel_thread.
	* workqueue.h (Workqueue::should_cancel_thread): Add thread_number
	parameter.

[-- Attachment #2: gold-thread-cancel-patch.txt --]
[-- Type: text/plain, Size: 3853 bytes --]

2011-07-28  Cary Coutant  <ccoutant@google.com>

	* workqueue-internal.h (Workqueue_threader::should_cancel_thread):
	Add thread_number parameter.
	(Workqueue_threader_threadpool::should_cancel_thread): Likewise.
	* workqueue-threads.cc
	(Workqueue_threader_threadpool::should_cancel_thread): Cancel
	current thread if its thread number is greater than desired thread
	count.
	* workqueue.cc (Workqueue_threader_single::should_cancel_thread):
	Add thread_number parameter.
	(Workqueue::should_cancel_thread): Likewise.
	(Workqueue::find_runnable_or_wait): Pass thread_number to
	should_cancel_thread.
	* workqueue.h (Workqueue::should_cancel_thread): Add thread_number
	parameter.


commit f380593e3f87ae9ba2e3ce6018bb5ee32797bfb8
Author: Cary Coutant <ccoutant@google.com>
Date:   Thu Jul 28 14:15:48 2011 -0700

    Fix race in thread cancellation.

diff --git a/gold/workqueue-internal.h b/gold/workqueue-internal.h
index 684c65b..764dc91 100644
--- a/gold/workqueue-internal.h
+++ b/gold/workqueue-internal.h
@@ -56,7 +56,7 @@ class Workqueue_threader
 
   // Return whether to cancel the current thread.
   virtual bool
-  should_cancel_thread() = 0;
+  should_cancel_thread(int thread_number) = 0;
 
  protected:
   // Get the Workqueue.
@@ -84,7 +84,7 @@ class Workqueue_threader_threadpool : public Workqueue_threader
 
   // Return whether to cancel a thread.
   bool
-  should_cancel_thread();
+  should_cancel_thread(int thread_number);
 
   // Process all tasks.  This keeps running until told to cancel.
   void
diff --git a/gold/workqueue-threads.cc b/gold/workqueue-threads.cc
index 60d4adc..de2ce5b 100644
--- a/gold/workqueue-threads.cc
+++ b/gold/workqueue-threads.cc
@@ -174,7 +174,7 @@ Workqueue_threader_threadpool::set_thread_count(int thread_count)
 // Return whether the current thread should be cancelled.
 
 bool
-Workqueue_threader_threadpool::should_cancel_thread()
+Workqueue_threader_threadpool::should_cancel_thread(int thread_number)
 {
   // Fast exit without taking a lock.
   if (!this->check_thread_count_)
@@ -182,12 +182,13 @@ Workqueue_threader_threadpool::should_cancel_thread()
 
   {
     Hold_lock hl(this->lock_);
-    if (this->threads_ > this->desired_thread_count_)
+    if (thread_number > this->desired_thread_count_)
       {
 	--this->threads_;
+	if (this->threads_ <= this->desired_thread_count_)
+	  this->check_thread_count_ = 0;
 	return true;
       }
-    this->check_thread_count_ = 0;
   }
 
   return false;
diff --git a/gold/workqueue.cc b/gold/workqueue.cc
index 6449bba..e78e86b 100644
--- a/gold/workqueue.cc
+++ b/gold/workqueue.cc
@@ -110,7 +110,7 @@ class Workqueue_threader_single : public Workqueue_threader
   { gold_assert(thread_count > 0); }
 
   bool
-  should_cancel_thread()
+  should_cancel_thread(int)
   { return false; }
 };
 
@@ -202,9 +202,9 @@ Workqueue::queue_next(Task* t)
 // Return whether to cancel the current thread.
 
 inline bool
-Workqueue::should_cancel_thread()
+Workqueue::should_cancel_thread(int thread_number)
 {
-  return this->threader_->should_cancel_thread();
+  return this->threader_->should_cancel_thread(thread_number);
 }
 
 // Find a runnable task in TASKS.  Return NULL if none could be found.
@@ -264,7 +264,7 @@ Workqueue::find_runnable_or_wait(int thread_number)
 	  return NULL;
 	}
 
-      if (this->should_cancel_thread())
+      if (this->should_cancel_thread(thread_number))
 	return NULL;
 
       gold_debug(DEBUG_TASK, "%3d sleeping", thread_number);
diff --git a/gold/workqueue.h b/gold/workqueue.h
index 9121e10..424b5e7 100644
--- a/gold/workqueue.h
+++ b/gold/workqueue.h
@@ -268,7 +268,7 @@ class Workqueue
 
   // Return whether to cancel this thread.
   bool
-  should_cancel_thread();
+  should_cancel_thread(int thread_number);
 
   // Master Workqueue lock.  This controls access to the following
   // member variables.