Absolutely agree, there is a race in code lines 317-320, and it's
possible that two threads will enter first if clause(line 317) and
make suspend_request negative, so suspended thread will never get
notified.
317 if(thread->suspend_request > 0) {
318 if (thread->safepoint_callback && thread->suspend_request
< 2) return;
319 apr_atomic_dec32((apr_uint32_t *)&(thread->suspend_request));
320 if(thread->suspend_request == 0) {
321 // Notify the thread that it may wake up now
322 hysem_post(thread->resume_event);
323 TRACE(("TM: resume one thread: %p request count:
%d",thread , thread->suspend_request));
324 thread->state &= ~TM_THREAD_STATE_SUSPENDED;
The code sample you've provided is exactly how this code chunk should
look like.
I'd like to comment only what's the second if clause means(line 318):
if safepoint callback(the function to be called on safe_point
entrance) set to non null value then we have suspend_request_count
value increased by one and additional resume will be send by
callback_processing mechanism after callback is processed.
I'll put your code into JIRA. Evgueni Brevnov, recently filled a
H-2648 issue against safepoint_callback mechanism, so this on will be
very good addendum to it.
Nik.
On 12/12/06, Weldon Washburn <[EMAIL PROTECTED]> wrote:
- A question on hythread_resume()
- Line 318: " if (thread->safepoint_callback &&
thread->suspend_request < 2) return"
- is there ever the situation where "thread->suspend_request == 0 "
??
-
- is there a race condition in the use of
"thread->suspend_request" in lines 318, 319, 320?
- In specific, does it make sense to replace these lines
of code with something like:
int current_suspend_request_count;
while (1) {
current_suspend_request_count = thread->suspend_request;
if (thread->safepoint_callback && current_suspend_request_count < 2) return;
int status = apr_atomic_casptr (current_suspend_request_count,
current_suspend_request--, ((apr_uint32_t *)&(thread->suspend_request);
if(/*original thread->suspend_request*/ status == 1) { /* thus the
new value has to be zero
// Notify the thread that it may wake up now
hysem_post(thread->resume_event);
TRACE(("TM: resume one thread: %p request count: %d",thread ,
thread->suspend_request));
thread->state &= ~TM_THREAD_STATE_SUSPENDED;
break;
--
Weldon Washburn
Intel Enterprise Solutions Software Division
