[Vm-dev] Why isn't signalSemaphoreWithIndex() thread-safe?

[Joshua Gargus]

Eliot Miranda wrote:
>  
>
> ------------------------------------------------------------------------
>
>
>
> On Sun, Sep 20, 2009 at 1:00 PM, Eliot Miranda
> <eliot.miranda at gmail.com <mailto:eliot.miranda at gmail.com>> wrote:
>
>     Hi Josh,
>
>     On Sun, Sep 20, 2009 at 12:43 PM, Joshua Gargus <schwa at fastmail.us
>     <mailto:schwa at fastmail.us>> wrote:
>
>
>         Hi,
>
>         I've always assumed that signalSemaphoreWithIndex() must be
>         thread-safe;
>         after all, it's the "official" mechanism for notifying the
>         image that an
>         asynchronous event has occurred.  However, a pang of paranoia
>         prompted
>         me to actually look at the code, and it seems clearly unsafe.
>          This is
>         bad, because I've been using it to signal events from separate
>         native
>         threads.
>
>         What should we do about this?  It seems to me that it should
>         be wrapped
>         in a critical section, using the appropriate platform-specific
>         synchronization primitives.
>
>      
>     There's no need for this heavyweight approach because the VM can
>     only respond to these signals synchronously.
>
>
> On second reading I think what I wrote below is exactly what you
> implied.  Sorry.

Nope, you're always giving me too much credit ;-)  I wasn't thinking
about any particular implementation of thread-safety, only raising the
issue that (in my understanding) we don't have it.

>
>  
>
>      Instead we can use three variables per index to implement an
>     exclusion-free solution, thusly:
>
>     typedef struct { int lock; // accessed via e.g. XCHG to protect
>     signalRequest
>                            int requests;
>                            int responses;
>                } ExternalSignalRequest;
>
>     ExternalSignalRequest *externalSignalRequests;
>     int checkExternalSignalRequests;
>
>     void
>     requestSignal(int i)
>     {
>            while (!lock(&externalSignalRequests[i].lock))
>                  usleep(1);
>
>            ++externalSignalRequests[i].requests;
>
>             unlock(&externalSignalRequests[i].lock);
>
>             checkExternalSignalRequests = 1;
>             forceInterruptCheck();  // set a flag to cause the VM to
>     check for interrupts; in the stack VM this is stackLimit :=
>     (usqInt)-1;
>     }
>                            
>
>     The VM responds in checkEvents:
>
>         if (checkExternalSignalRequests)
>             for (i = 0; i < numExternalSemaphores; i++)
>                 while (externalSignalRequests[i]. responses
>     != externalSignalRequests[i]. requests) {
>                     signalSemaphoreWithIndex(i);
>                     externalSignalRequests[i]. responses;
>                 }
>


Am I correct that ExternalSignalRequest.reponses is never reset to zero,
instead eventually wrapping around?  That worried me for a minute, but I
see that it doesn't matter.  Neeto.

Your solution looks good; it's certainly nicer than a heavyweight
critical section, especially since contention will be infrequent.

Cheers,
Josh




>
>
>         Thanks,
>         Josh
>
>
>