harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Nikolay Kuznetsov" <nikolay.kuznet...@gmail.com>
Subject Re: [drlvm][threading] -- some questions on thread_native_suspend.c::hythread_resume()
Date Tue, 12 Dec 2006 18:29:21 GMT
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 <weldonwjw@gmail.com> 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
>
>

Mime
View raw message