[PATCH] kernel: Check the Semaphore before queuing the interrupt

While porting Phexample I noticed that the interrupt was queued
long after the Delay has stopped and the process continued. It
was interrupted at the wrong point. Check the Semaphore for signals
before queuing the interrupt.

I think the race-free operation should make sure that the process
can't wake up if it is still sleeping, then check sem and then
schedule the interrupt and resume the process (if needed).

2014-01-20  Holger Hans Peter Freyther  <[hidden email]>

        * kernel/Delay.st: Check the Semaphore before queuing the
        interrupt for the target process.
---
 ChangeLog       | 5 +++++
 kernel/Delay.st | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 46a0bc5..ed928cd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2014-01-20  Holger Hans Peter Freyther  <[hidden email]>
+
+ * kernel/Delay.st: Check the Semaphore before queuing the
+ interrupt for the target process.
+
 2014-01-19  Gwenael Casaccio  <[hidden email]>
 
  * kernel/MethodDict.st: Introduce >>#select: and
diff --git a/kernel/Delay.st b/kernel/Delay.st
index 47f40bb..df4417a 100644
--- a/kernel/Delay.st
+++ b/kernel/Delay.st
@@ -308,7 +308,8 @@ created.'>
 
                 "Wait and see if it is timed out. If so send a signal."
                 (self timedWaitOn: sem) ifTrue: [
-                   proc signalInterrupt: (Kernel.TimeoutNotification on: self).
+    sem signals = 0 ifTrue: [
+                        proc signalInterrupt: (Kernel.TimeoutNotification on: self)].
                 ].
             ] fork.
 
--
1.8.5.2


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Mon, Jan 20, 2014 at 05:13:56PM +0100, Holger Hans Peter Freyther wrote:

Dear Paolo,

> I think the race-free operation should make sure that the process
> can't wake up if it is still sleeping, then check sem and then
> schedule the interrupt and resume the process (if needed).

do you have an idea/comment of the above?

kind regards
        holger

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

The patch looks good.

Alternatively, what about this?  It is a bit clearer to me, but the
logic is pretty much the same.

Paolo

diff --git a/kernel/Delay.st b/kernel/Delay.st
index 47f40bb..ceeedfd 100644
--- a/kernel/Delay.st
+++ b/kernel/Delay.st
@@ -295,7 +295,7 @@ created.'>
          evaluate aTimeoutBlock."
         <category: 'timeout'>
         | value timeout |
-        timeout := false.
+        timeout := 0.
         value := [ | sem proc |
             "Use the semaphore to signal that we executed everything"
             sem := Semaphore new.
@@ -308,19 +308,20 @@ created.'>
 
                 "Wait and see if it is timed out. If so send a signal."
                 (self timedWaitOn: sem) ifTrue: [
-                   proc signalInterrupt: (Kernel.TimeoutNotification on: self).
+                   timeout = 0 ifTrue: [
+                       proc signalInterrupt: (Kernel.TimeoutNotification on: self)].
                 ].
             ] fork.
 
-            aBlock ensure: [sem signal].
+            aBlock ensure: [timeout := timeout bitOr: 1. sem signal].
         ] on: Kernel.TimeoutNotification do: [:e |
             e delay = self
-                ifTrue:  [timeout := true]
+                ifTrue:  [timeout := timeout bitOr: 2]
                 ifFalse: [e pass].
         ].
 
         "Make sure we call the #ensure:/#ifCurtailed: blocks first."
-        ^ timeout
+        ^ (timeout bitAnd: 2) = 2
             ifTrue:  [aTimeoutBlock value]
             ifFalse: [value].
     ]

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Mon, Jan 20, 2014 at 05:30:52PM +0100, Paolo Bonzini wrote:
> The patch looks good.
>
> Alternatively, what about this?  It is a bit clearer to me, but the
> logic is pretty much the same.

Thanks!

>                  (self timedWaitOn: sem) ifTrue: [
> -                   proc signalInterrupt: (Kernel.TimeoutNotification on: self).
> +                   timeout = 0 ifTrue: [
> +                       proc signalInterrupt: (Kernel.TimeoutNotification on: self)].
>                  ].

If we ever get the execution of multiple native processes the timeout
code and the aBlock ensure:[] could run at the same time. How would we
make that mutual exclusive? add a lock/token then?


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Il 20/01/2014 20:30, Holger Hans Peter Freyther ha scritto:
We probably would need syntax for atomic compare and exchange of OOPs.

Paolo


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Mon, Jan 20, 2014 at 05:30:52PM +0100, Paolo Bonzini wrote:
> The patch looks good.
>
> Alternatively, what about this?  It is a bit clearer to me, but the
> logic is pretty much the same.

Okay, we will go with this version. I thought I saw it failing once
but I might have still used an older kernel.

The "hammer" would be to have another Semaphore that is signalled
by the forked process and waited on by the outer one. This way we
are sure that the two code paths don't execute in parallel?


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk