17055 - Add support for enabling/disbaling interrupts

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P3)

Description

[srinivas]

It will be useful to functions to enable and disable interrupts.

Comment 1

[srinivas]

Two new interfaces, PR_BlockInterrupt and PR_UnblockInterrupt are added to NSPR.
When a thread blocks interrupt, the interrupt is kept pending until it is
unblocked.

Files modified:

include/prthread.h, rev 3.5
include/private/primpl.h, rev 3.25
src/pthreads/ptio.c, rev 3.33
src/pthreads/ptsynch.c, rev 3.10
src/pthreads/ptthread.c, rev 3.27
src/threads/prcthr.c, rev 3.8
tests/intrupt.c, rev 3.4

Comment 2

[Wan-Teh Chang]

The current implementation will generate a spurious
wakeup if a thread in PR_WaitCondVar is interrupted
while interrupt is blocked, because PR_Interrupt
does not check whether interrupt is being blocked.

For example, in ptthread.c, PR_Interrupt, we have:
    PRCondVar *cv;
    PR_ASSERT(NULL != thred);
    if (NULL == thred) return PR_FAILURE;

    thred->state |= PT_THREAD_ABORTED;

    cv = thred->waiting;
    if (NULL != cv)
    {
        PRIntn rv = pthread_cond_broadcast(&cv->cv);
        PR_ASSERT(0 == rv);
    }
    return PR_SUCCESS;

So pthread_cond_broadcast is always called
regardless of the value of thred->interrupt_blocked.
PR_WaitCondVar does check thred->interrupt_blocked
and would return PR_SUCCESS (as opposed to PR_FAILURE
with PR_PENDING_INTERRUPT_ERROR), resulting in a
spurious wakeup.

Since our clients are supposed to deal with spurious
wakeups, this won't break any correct code.  But ideally
PR_Interrupt should check thred->interrupt_blocked
to avoid generating a spurious wakeup. However, to
do that check safely would require a lock on 'thred',
right?

Comment 3

[srinivas]

Yes, a lock on thred is required. But, if thred->interrupt_blocked is checked
without a lock, most spurious interrupts are likely to be avoided because a
thread can block/unblock it's own interrupts. Simultaneous thread interrupts and
interrupt blocks don't happen frequently.

Comment 4

[srinivas]

The following patch for reducing spurious notifications needs to be reviewed:

RCS file: /cvsroot/mozilla/nsprpub/pr/src/pthreads/ptthread.c,v
retrieving revision 3.28
diff -u -r3.28 ptthread.c
--- ptthread.c  1999/11/16 23:44:34     3.28
+++ ptthread.c  1999/12/22 22:34:59
@@ -657,7 +657,7 @@
     thred->state |= PT_THREAD_ABORTED;

     cv = thred->waiting;
-    if (NULL != cv)
+    if ((NULL != cv) && !thred->interrupt_blocked)
     {
         PRIntn rv = pthread_cond_broadcast(&cv->cv);
         PR_ASSERT(0 == rv);
Index: src/threads/prcthr.c
===================================================================
RCS file: /cvsroot/mozilla/nsprpub/pr/src/threads/prcthr.c,v
retrieving revision 3.11
diff -u -r3.11 prcthr.c
--- prcthr.c    1999/12/22 20:51:26     3.11
+++ prcthr.c    1999/12/22 22:34:59
@@ -171,7 +171,7 @@
     thread->flags |= _PR_INTERRUPT;
     victim = thread->wait.cvar;
     _PR_THREAD_UNLOCK(thread);
-    if (NULL != victim) {
+    if ((NULL != victim) && !thread->interrupt_blocked) {
         int haveLock = (victim->lock->owner == _PR_MD_CURRENT_THREAD());

         if (!haveLock) PR_Lock(victim->lock);
@@ -194,7 +194,8 @@
                          * call is made with thread locked;
                          * on return lock is released
                          */
-                        _PR_NotifyLockedThread(thread);
+                                               if (!thread->interrupt_blocked)
+                               _PR_NotifyLockedThread(thread);
                         break;
                 case _PR_IO_WAIT:
                         /*
@@ -203,7 +204,8 @@
                          * released.
                          */
 #if defined(XP_UNIX) || defined(WINNT) || defined(WIN16)
-                        _PR_Unblock_IO_Wait(thread);
+                                               if (!thread->interrupt_blocked)
+                               _PR_Unblock_IO_Wait(thread);
 #else
                         _PR_THREAD_UNLOCK(thread);
 #endif

Comment 5

[Wan-Teh Chang]

I reviewed the patch for reducing spurious notifications.
The patch itself is good, although our thread interrupt
code still has race conditions (not introduced by this
patch).

The problem is that we are notifying the cv (thred->waiting
in the pthreads version or thread->wait.cvar in the global
threads only version) without locking the thread.  It is
possible for the target thread to finish waiting on that
cv at that instant of time.  Then, the cv could be deleted,
and we would be notifying a destroyed cv.  Or the
target thread could go on to wait on a different cv, and
notifying the old cv would not wake up the thread.

It seems that solving this problem requires that we
notify the cv while locking the thread.  Since this
problem has always been there and is not introduced
or made worse by this patch, I think it is okay to
check in this patch while we think about how to completely
solve the race condition problem.

Comment 6

[srinivas]

Files modified:

/cvsroot/mozilla/nsprpub/pr/src/pthreads/ptthread.c,v  <--  ptthread.c
new revision: 3.29; previous revision: 3.28
/cvsroot/mozilla/nsprpub/pr/src/threads/prcthr.c,v  <--  prcthr.c
new revision: 3.12; previous revision: 3.11

Comment 7

[srinivas]

The patch to prcthr.c was incorrect; the _PR_INTERRUPT_BLOCKED field of
thread->flags should be used for classic version of NSPR and the
thread->interrupt_blocked field should be used for the pthreads version.

===================================================================
RCS file: /cvsroot/mozilla/nsprpub/pr/src/threads/prcthr.c,v
retrieving revision 3.13
diff -u -r3.13 prcthr.c
--- prcthr.c    1999/12/24 02:07:15     3.13
+++ prcthr.c    1999/12/29 16:49:19
@@ -171,7 +171,7 @@
     thread->flags |= _PR_INTERRUPT;
     victim = thread->wait.cvar;
     _PR_THREAD_UNLOCK(thread);
-    if (NULL != victim) {
+    if ((NULL != victim) && (!(thread->flags & _PR_INTERRUPT_BLOCKED))) {
         int haveLock = (victim->lock->owner == _PR_MD_CURRENT_THREAD());

         if (!haveLock) PR_Lock(victim->lock);
@@ -194,7 +194,8 @@
                          * call is made with thread locked;
                          * on return lock is released
                          */
-                        _PR_NotifyLockedThread(thread);
+                                               if (!(thread->flags & _PR_INTERR
UPT_BLOCKED))
+                               _PR_NotifyLockedThread(thread);
                         break;
                 case _PR_IO_WAIT:
                         /*
@@ -203,7 +204,8 @@
                          * released.
                          */
 #if defined(XP_UNIX) || defined(WINNT) || defined(WIN16)
-                        _PR_Unblock_IO_Wait(thread);
+                                               if (!(thread->flags & _PR_INTERR
UPT_BLOCKED))
+                               _PR_Unblock_IO_Wait(thread);
 #else
                         _PR_THREAD_UNLOCK(thread);
 #endif