Re: srfi-18 requirements

[Neil Jerram]

"Julian Graham" <address@hidden> writes:

> Hi Neil,

Hi Julian; sorry for taking so long to follow up on this!

>> 2. If two threads are using pthread_cond_wait and pthread_cond_signal
>>    to communicate, and using the cond_var itself as a state
>>    indication, they have to be certain that the pthread_cond_wait
>>    starts before the pthread_cond_signal, otherwise it won't work.
>
> Right -- holding the right mutexes when you signal / broadcast is
> pretty important.

I don't think holding a mutex when you signal/broadcast is enough.
It's still possible for the signal/broadcast to happen before the
other thread starts waiting.

> I've come up with something similar that seems to work decently and
> seems a bit simple.  See what you think (apologies for the
> formatting):
>
> static scm_i_pthread_cond_t wake_up_cond;
> static scm_i_pthread_mutex_t wake_up_mutex;
> static int wake_up_flag = 0;
> int scm_i_thread_go_to_sleep;
>
> void
> scm_i_thread_put_to_sleep ()
> {
>   if (threads_initialized_p)
>     {
>       scm_i_thread *t;
>
>       scm_leave_guile ();
>       scm_i_pthread_mutex_lock (&thread_admin_mutex);
>
>       wake_up_flag = 0;
>       scm_i_thread_go_to_sleep = 1;
>       for (t = all_threads; t; t = t->next_thread)

NB I have an orthogonal concern here (i.e. possibly yet another issue
with the current code!): If a thread that was running in Guile mode
called scm_leave_guile() to do non-guile stuff for a while, and is
still outside Guile mode, shouldn't it have been removed from the
all_threads list when it called scm_leave_guile()?

(But please feel free to ignore this one for now.  We already have
enough loose ends in the air!)

>         {
>          scm_i_pthread_mutex_lock (&t->heap_mutex);
>         }
>       scm_i_thread_go_to_sleep = 0;
>     }
> }
>
> void
> scm_i_thread_wake_up ()
> {
>   if (threads_initialized_p)
>     {
>       scm_i_thread *t;
>
[B]
>       scm_i_pthread_mutex_lock (&wake_up_mutex);
>       wake_up_flag = 1;
>       scm_i_pthread_cond_broadcast (&wake_up_cond);
>       scm_i_pthread_mutex_unlock (&wake_up_mutex);
>       for (t = all_threads; t; t = t->next_thread)
>         {
>            scm_i_pthread_mutex_unlock (&t->heap_mutex);
>         }
>       scm_i_pthread_mutex_unlock (&thread_admin_mutex);
>       scm_enter_guile ((scm_t_guile_ticket) SCM_I_CURRENT_THREAD);
>     }
> }
>
> void
> scm_i_thread_sleep_for_gc ()
> {
>   scm_i_thread *t = suspend ();
>
>   scm_i_pthread_cleanup_push ((void (*)(void *)) scm_i_pthread_mutex_unlock,
>                             &wake_up_mutex);
>   scm_i_pthread_mutex_lock (&wake_up_mutex);
>   scm_i_pthread_mutex_unlock (&t->heap_mutex);
>   do

I think you need to check !wake_up_flag at the start of the loop.  I
think it is possible that when the loop starts, the GC thread has
already set wake_up_flag to 1 and signalled wake_up_cond.

>     {
>       scm_i_pthread_cond_wait (&wake_up_cond, &wake_up_mutex);
>     }
>   while (!wake_up_flag);
[A]
>   scm_i_pthread_mutex_lock (&t->heap_mutex);
>   scm_i_pthread_mutex_unlock (&wake_up_mutex);

Do the locks of t->heap_mutex and wake_up_mutex really need to overlap
like this?  I think this could lead to a deadlock: at [A] above, the
non-GC thread holds wake_up_mutex and tries to lock t->heap_mutex,
whereas at [B] above, the GC thread holds t->heap_mutex (for every
thread) and tries to lock wake_up_mutex.

If there isn't a hard reason for the overlapping, the two lock/unlock
pairs just above can be swapped, and that eliminates the deadlock
possibility.

Otherwise I think your code looks good.

> Of course -- all I meant by this was that in the existing thread tests
> (and in much of the SRFI-18 test code I wrote) the lifespans of
> threads besides the main thread (and the signal delivery thread) are
> usually short enough that they don't end up participating in this
> whole co-op GC process.  Maybe we need some test code for
> longer-running, guile-mode threads.  (Perhaps developers with
> multi-threaded Guile application development under their belts would
> care to chime in here?)

Yes, some longer-running threads would be good.  Does anyone have some
multi-threaded code that we could use for testing?

> I'm not totally sure -- I'll have to add some more logs and get back
> to you.  I think are definitely some places where an extra SCM_TICK
> might do some good (in fat_cond_timedwait, e.g.).

OK, no worries.  Let's handle this when we need to.

Regards,
      Neil