Re: srfi-18 requirements

[Neil Jerram]

"Julian Graham" <address@hidden> writes:

> Hi Neil,
>
>> > Of course.
>>
>> Good, thanks.
>
> Find attached a patch against HEAD that includes only the bug fix
> stuff (2 deadlocks and use of thread-specific admin mutex) from the
> original patch, modified to change make_jmpbuf instead of the signal
> delivery code.

Thanks.  Just a couple further points...

>  static void
>  scm_enter_guile (scm_t_guile_ticket ticket)
>  {
>    scm_i_thread *t = (scm_i_thread *)ticket;
>    if (t)
>      {
> +      /* The admin mutex must be locked here to prevent the thread from
> +      entering guile-mode after scm_thread_go_to_sleep has been set to 1 in
> +      scm_i_thread_go_to_sleep. */
> +
> +      scm_i_pthread_mutex_lock (&thread_admin_mutex);
>        scm_i_pthread_mutex_lock (&t->heap_mutex);
>        resume (t);
> +      scm_i_pthread_mutex_unlock (&thread_admin_mutex);
>      }
>  }

1. Embarassingly - given that I already said "Nice fix" to this - I'm
afraid I can't now see exactly why this is needed.

I understand that if the line to lock thread_admin_mutex is absent, it
is possible for this thread (A, say) to lock its heap_mutex, and call
resume(), and return from scm_enter_guile(), even though
scm_i_thread_go_to_sleep is already 1.

I'm not sure why this is a problem, though.  It seems to me that what
will then ensue is that

- scm_i_thread_put_to_sleep() will block when it tries to lock thread
  A's heap_mutex

- after a short while, thread A will call SCM_TICK, which will call
  SCM_THREAD_SWITCHING_CODE, which will notice that
  scm_i_thread_go_to_sleep is 1 and so call
  scm_i_thread_sleep_for_gc()

- scm_i_thread_sleep_for_gc() will do the cond_wait, releasing the
  heap_mutex, and so allowing the GC thread to lock it.

So the worst that happens is that thread A manages to do a little more
guile mode execution before it goes to sleep.

Is that right?  I think you suggested in one of your previous emails
that it might be possible for thread A to enter and leave guile mode
multiple times, but I don't see how that is possible.

2. Should admin_mutex be locked in scm_c_thread_exited_p()?  I think
it should.  (This was equally wrong when using thread_admin_mutex, of
course; your patch doesn't make anything worse, but it's worth fixing
this in passing if you agree.)

Regards,
        Neil