Re: Bug #27457 (“Threads, mutexes, and critical sections”)

[ludo@gnu.org (Ludovic Courtès)]

Hi Neil,

Neil Jerram <neil@ossau.uklinux.net> writes:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>> I should have mentioned this one:
>> http://thread.gmane.org/gmane.comp.programming.garbage-collection.boehmgc/3185 .
>>
>> This is normally somewhat fixed in current BDW-GC CVS, but Guile itself
>> may have troubles of its own dealing with cancellation.
>
> Thanks.  I thought already about cancellation, but I couldn't see
> anything in the test program that would do that.  Do you know of
> anything in that program, or in BDW-GC, that does a pthread_cancel?

Hmm no, sorry for the confusion.  The srfi-18 test this was taken from
did use cancellation, though.

> I've cherry-picked the following branch_release-1-8 fixes.
>
> commit 499c43b03225abb8d3af9087b7630d523b74e13a
> Author: Neil Jerram <neil@ossau.uklinux.net>
> Date:   Thu Mar 5 20:03:33 2009 +0000
>
>     Avoid throw from critical section, given invalid sigaction call
>
> (This is for a different critical section problem, but which still
> occurs (prior to this commit) in master.)

Good that you noticed it was missing from ‘master’.  (BTW ‘signals.test’
should be changed to GPLv3+ and have a ‘define-module’ clause.)

> commit 3009b5d557e1ebfd828fd0de9b1da5abb2f6ec9a
> Author: Neil Jerram <neil@ossau.uklinux.net>
> Date:   Tue Mar 10 23:55:31 2009 +0000
>
>     Fix spurious `throw from within critical section' errors
>
> (This is the commit that I hope will fix the errors that you're seeing.)
>
> Can you see how the test runs now?

The abort () is apparently never reached with this patch.

It took me some time to understand this:

-  if (scm_i_critical_section_level)
+  if (thread->critical_section_level)
     {
       fprintf (stderr, "continuation invoked from within critical section.\n");
       abort ();

Critical sections concern all the threads in guile mode.  Thus I would
think that the question “are the threads in a critical section?” cannot
be answered by looking at a per-thread ‘critical_section_level’.

However, it really seems that the intent was really to forbid ‘throw’s
when the *current thread* is holding the critical section mutex, so the
patch is correct but I find the name ‘critical_section_level’ is
slightly misleading.

What do you think?

> Also, can I check my thinking on one other fix, and how it
> interacts with BDW-GC?
>
> commit 2bfcaf2605f8366d8c708c148bde5313b88497e0
> Author: Neil Jerram <neil@ossau.uklinux.net>
> Date:   Wed Mar 4 23:45:11 2009 +0000
>
>     Lock ordering: don't allocate when in critical section (scm_sigaction_for_thread)
>
> With BDW-GC I believe that allocation in a critical section is no longer
> a problem, specifically because
>
> - the stop-the-world mechanism uses signals and sigsuspend - whereas
>   Guile GC used mutexes - and hence there are no concerns about lock
>   ordering
>
> - it doesn't matter if there are threads waiting for the critical
>   section when a GC happens, because the signal mechanism will still
>   interrupt them.
>
> Is that right?

Yes.

BTW, rest assured: thanks to libgc we won’t get any Helgrind report!
:-)

Thanks,
Ludo’.