<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2014-04-23 11:34 GMT+02:00 Nicolas Cellier <span dir="ltr"><<a href="mailto:nicolas.cellier.aka.nice@gmail.com" target="_blank">nicolas.cellier.aka.nice@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2014-04-23 2:27 GMT+02:00 Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Apr 22, 2014 at 5:23 PM, Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Apr 22, 2014 at 4:43 PM, John McIntosh <span dir="ltr"><<a href="mailto:johnmci@smalltalkconsulting.com" target="_blank">johnmci@smalltalkconsulting.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br><div dir="ltr"><br><div class="gmail_extra"><br><br>
<div class="gmail_quote"><div><div>
On Tue, Apr 22, 2014 at 4:10 PM, Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br><div dir="ltr"><br><div class="gmail_extra"><br><br>
<div class="gmail_quote">
On Tue, Apr 22, 2014 at 3:51 PM, John McIntosh <span dir="ltr"><<a href="mailto:johnmci@smalltalkconsulting.com" target="_blank">johnmci@smalltalkconsulting.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br><div dir="ltr"><br><div class="gmail_extra"><br><br>
<div class="gmail_quote">
On Tue, Apr 22, 2014 at 3:21 PM, Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br><div dir="ltr"><br><div class="gmail_extra"><br><br>
<div class="gmail_quote"><div>
On Tue, Apr 22, 2014 at 3:18 PM, Nicolas Cellier <span dir="ltr"><<a href="mailto:nicolas.cellier.aka.nice@gmail.com" target="_blank">nicolas.cellier.aka.nice@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br><div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">
2014-04-23 0:12 GMT+02:00 Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br><div dir="ltr">Hi Esteban,<br><div class="gmail_extra">
<br><br><div class="gmail_quote">On Tue, Apr 22, 2014 at 2:40 PM, Esteban Lorenzano <span dir="ltr"><<a href="mailto:estebanlm@gmail.com" target="_blank">estebanlm@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br><div style="word-wrap:break-word"><br><div><div>On 22 Apr 2014, at 23:34, Nicolas Cellier <<a href="mailto:nicolas.cellier.aka.nice@gmail.com" target="_blank">nicolas.cellier.aka.nice@gmail.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">2014-04-22 12:47 GMT+02:00 Esteban Lorenzano <span dir="ltr"><<a href="mailto:estebanlm@gmail.com" target="_blank">estebanlm@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br><div style="word-wrap:break-word">thanks Nicolas for your words. <div>
now… let’s go back to our common passion: to help this community on having the best vm possible :)</div>
<div><br></div><div>Esteban</div><div><br></div></div></blockquote><div><br></div><div>Nah, concerning the best vm I feel disqualified, I let Eliot care of it ;)<br></div></div></div></div></blockquote><blockquote type="cite">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>We just want a better VM with a few hacks, corrections and support for some specific Pharo libraries/extensions.<br>
</div><div>Eliot, do you agree that the<span> sqCompareAndSwapRes provided #ifdef</span><span> TARGET_OS_IS_IPHONE is buggish (see below)?<br></span></div></div></div></div></blockquote></div></div></blockquote><div><br>
</div><div>To be honest I don't know. I don't have an iPhone development library handy. John McIntosh write the code and he's more than competent. I'd rather leave this to others with experience of the platform to check. I /think/ the desired semantics are clear enough. If they're not I can fix that. But I want to leave writing the correct code for the iPhone to those that are actually using the platform. Forgive me.</div>
<div> </div></div></div></div></blockquote><div><br></div><div>OK, thanks Eliot, then I'll trust what the comments say.<br>If John is able to confirm that'll be better, but I think I get a good picture of the problem now.<br>
I cannot build either, but once Esteban can test it and if it works OK, then we'll propose a fix for integration in your svn branch, sounds right?<br></div></div></div></div></blockquote><div><br></div></div><div>perfect, thanks !</div>
</div></div></div></blockquote><div><br></div><div>Well this is all very interesting, since the original version I sent to Eliot dated from July 2010, and it was</div><div><br></div><div><p style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(120,73,42)">
# define sqCompareAndSwapRes(var,old,new,res) res = var; OSAtomicCompareAndSwap32(old, new, &var) </p></div></div></div></div></blockquote><div><br></div><div>which I changed to read</div><div><div><br></div>
<div># define sqCompareAndSwap(var,old,new) OSAtomicCompareAndSwap32(old, new, &var)</div>
<div>/* N.B. This is not atomic in fetching var's old value :( */</div><div># define sqCompareAndSwapRes(var,old,new,res) do { res = var; if (OSAtomicCompareAndSwap32(old, new, &var)) res = new; } while (0)</div>
<div><br></div></div><div>because e.g.</div><div><br></div><div> if (expr) sqCompareAndSwap(foo,bar,baz)</div><div><br></div><div>will /not/ work as intended if written as</div><div><br></div><div># define sqCompareAndSwapRes(var,old,new,res) res = var; if (OSAtomicCompareAndSwap32(old, new, &var))<br>
</div><div><br></div><div>But this does seem broken. How is res atomically set to var's previous value? There's a suspension point between "res = var" and OSAtomicCompareAndSwap32, so res could end up being assigned a stale version of var's previous value.</div>
<div><br></div></div></div></div></blockquote><div><br></div></div></div><div>? <span style="color:rgb(0,132,0);font-family:Menlo;font-size:11px">in any case, res</span></div><div><div><p style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(120,73,42)">
</p><p style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(0,132,0)"> * is set to the previous value of var.</p><div><br></div></div></div><div>Er so yes res would get the previous value of var. Assuming it's not stale, do you mean it could be stale where two processes hit the semaphore and res is not atomically set correctly? </div>
</div></div></div></blockquote><div><br></div><div>Yes. Here's the timeline:</div><div><br></div><div>Var: 0</div><div><span style="white-space:pre-wrap"> </span>P1<span style="white-space:pre-wrap"> </span><span style="white-space:pre-wrap"> </span><span style="white-space:pre-wrap"> </span><span style="white-space:pre-wrap"> P2</span></div>
<div>res := var (res = 0)</div><div><span style="white-space:pre-wrap"> (suspended)</span></div><div><span style="white-space:pre-wrap"> </span><span style="white-space:pre-wrap"> </span><span style="white-space:pre-wrap"> </span><span style="white-space:pre-wrap"> </span><span style="white-space:pre-wrap"> res := var (now res = 0)</span></div>
<div><span style="white-space:pre-wrap"> </span><span style="white-space:pre-wrap"> </span><span style="white-space:pre-wrap"> </span><span style="white-space:pre-wrap"> </span><span style="white-space:pre-wrap"> </span>OSAtomicCompareAndSwap32(0,2,&var) (now var = 2)</div>
<div>OSAtomicCompareAndSwap32(0,2,&var)<br></div><div>(still var = 2)</div><div>res = 0, but should = 2</div><div><br></div><div>And of course there are lots of other possible interleavings.</div><div><br></div><div>
<br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>Or does the intel assembler match the story?</div></div></div></div></blockquote><div><br></div><div><blush>no</blush>. It suffers exactly the same flaw.</div></div></div></div></blockquote><div><br></div>
<div>This should be redefined to answer true if it succeeded, false if it didn't and not load the previous value. The intel instruction can be made to do that. Of course if the intrinsics are available we can simply use them.</div>
<div> </div></div></div></div></blockquote><div><br></div><div>Ah, yes, I would prefer that.<br></div><div>That's what I woud have written for an OS specific lockSignalQueue<br></div><div><div><br>static inline void lockSignalQueue()<br>
{<br> /* spin to obtain a lock */<br> do {<br> sqLowLevelMFence();<br></div> } while (! OSAtomicCompareInt32( 0, 1, &sigLock));<br>}<br> <br></div><div>So if we redefine the OS agnostic atomic operation <span><span>sqCompareAndSwap(var,old,new)</span></span> to just return a bool (= 0 if fail, != 0 if succeeded) then we're done.<br>
<br></div><div><div>static inline void lockSignalQueue()<br>{<br> /* spin to obtain a lock */<br> do {<br> sqLowLevelMFence();<br></div> } while (! <span><span>sqCompareAndSwap(sigLock,0,1)</span></span>);<br>
}</div>
<div> <br></div><br></div></div></div></blockquote></div><br></div><div class="gmail_extra">And if we go with this definition to answer true on success, the gcc >= 4.1 intrinsic used by Tim for arm is just fine:<br><br>
</div><div class="gmail_extra">I came up with these solutions:<br><br>#if defined TARGET_OS_IS_IPHONE<br># define sqCompareAndSwap(var,old,new) OSAtomicCompareAndSwap32(old, new, &var) <br><br>#elif defined _MSC_VER<br>
# include <intrin.h><br># define sqCompareAndSwap(var,old,new) (_InterlockedCompareExchange(ptr, newval, oldval)==oldval)<br><br>#elif defined(__GNUC__) && ( __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 1) )<br>
/* use the gcc inbuilt functions detailed in <a href="http://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Atomic-Builtins.html" target="_blank">http://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Atomic-Builtins.html</a> */<br># define sqCompareAndSwap(var,old,new) __sync_bool_compare_and_swap(&(var), (old), (new))<br>
<br>#else<br>/* Dear implementor, you have choices. Google atomic compare and swap and you will<br> * find a number of implementations for other architectures.<br> */<br># error atomic compare/swap of 32-bit variables not yet defined for this platfom<br>
#endif<br><br></div><div class="gmail_extra">I cannot write the asm( ... ) inclusion if ever gcc < 4.1,<br>this gcc asm macro syntax is above my skills, but maybe Eliot can do it<br></div><div class="gmail_extra">(it's just a setz al; xor ah,ah; to be added after the lock cmpxchg)<br>
<br></div><div class="gmail_extra">Then, starting at svn rev 2847, I modified the sqCompareAndSwap/sqCompareAndSwapRes send sites.<br></div><div class="gmail_extra">Could you check?<br></div><div class="gmail_extra"><br>
===================================================================<br>--- unix/vm/sqUnixITimerTickerHeartbeat.c (revision 2847)<br>
+++ unix/vm/sqUnixITimerTickerHeartbeat.c (working copy)<br>@@ -483,14 +483,8 @@<br> }<br> <br> #if !defined(SA_NODEFER)<br>- { int zero = 0;<br>- int previouslyHandlingHeartbeat;<br>- sqCompareAndSwapRes(handling_heartbeat,zero,1,previouslyHandlingHeartbeat);<br>
- if (previouslyHandlingHeartbeat)<br>+ if( ! sqCompareAndSwapRes(handling_heartbeat,0,1))<br> return;<br>- }<br>-<br>- handling_heartbeat = 1;<br> #endif<br> <br> heartbeat();<br>
===================================================================<br>--- unix/vm/sqUnixITimerHeartbeat.c (revision 2847)<br>+++ unix/vm/sqUnixITimerHeartbeat.c (working copy)<br>@@ -355,14 +355,8 @@<br> heartbeat_handler(int sig, struct siginfo *sig_info, void *context)<br>
{<br> #if !defined(SA_NODEFER)<br>- { int zero = 0;<br>- int previouslyHandlingHeartbeat;<br>- sqCompareAndSwapRes(handling_heartbeat,zero,1,previouslyHandlingHeartbeat);<br>- if (previouslyHandlingHeartbeat)<br>
+ if (! sqCompareAndSwap(handling_heartbeat,0,1))<br> return;<br>- }<br>-<br>- handling_heartbeat = 1;<br> #endif<br> <br> heartbeat();<br>===================================================================<br>
--- unix/misc/threadValidate/sqTicker.c (revision 2847)<br>+++ unix/misc/threadValidate/sqTicker.c (working copy)<br>@@ -213,9 +213,7 @@<br> if (async[i].tickee<br> && !async[i].inProgress<br>
&& utcMicrosecondClock >= async[i].tickeeDeadlineUsecs) {<br>- int previousInProgress;<br>- sqCompareAndSwapRes(async[i].inProgress,0,1,previousInProgress);<br>
- if (previousInProgress == 0) {<br>+ if( sqCompareAndSwap(async[i].inProgress,0,1) ) {<br> assert(async[i].inProgress);<br> async[i].tickeeDeadlineUsecs += async[i].tickeePeriodUsecs;<br>
async[i].tickee();<br>===================================================================<br>--- unix/misc/threadValidate/sqUnixHeartbeat.c (revision 2847)<br>+++ unix/misc/threadValidate/sqUnixHeartbeat.c (working copy)<br>
@@ -412,13 +412,8 @@<br> }<br> <br> #if !defined(SA_NODEFER)<br>- { int zeroAndPreviousHandlingHeartbeat = 0;<br>- sqCompareAndSwap(handling_heartbeat,zeroAndPreviousHandlingHeartbeat,1);<br>- if (zeroAndPreviousHandlingHeartbeat)<br>
+ if( ! sqCompareAndSwap(handling_heartbeat,0,1))<br> return;<br>- }<br>-<br>- handling_heartbeat = 1;<br> #endif<br> <br> heartbeat();<br>===================================================================<br>
--- Cross/vm/sqTicker.c (revision 2847)<br>+++ Cross/vm/sqTicker.c (working copy)<br>@@ -246,9 +246,7 @@<br> if (async[i].tickee<br> && !async[i].inProgress<br> && utcMicrosecondClock >= async[i].tickeeDeadlineUsecs) {<br>
- int previousInProgress;<br>- sqCompareAndSwapRes(async[i].inProgress,0,1,previousInProgress);<br>- if (previousInProgress == 0) {<br>+ if( sqCompareAndSwap(async[i].inProgress,0,1) {<br>
assert(async[i].inProgress);<br> if (async[i].tickeeDeadlineUsecs + HiccupThreshold<br> < utcMicrosecondClock)<br>===================================================================<br>
--- Cross/vm/sq.h (revision 2847)<br>+++ Cross/vm/sq.h (working copy)<br>@@ -276,10 +276,9 @@<br> * threads after myindex is obtained but before asprintf completes we can get<br> * two threads using the same entry. But this is good enough for now.<br>
*/<br>-#define THRLOG(...) do { int myidx, oldidx; \<br>+#define THRLOG(...) do { int myidx; \<br> do { myidx = thrlogidx; \<br>- sqCompareAndSwapRes(thrlogidx,myidx,(myidx+1)&(THRLOGSZ-1),oldidx); \<br>
- } while (myidx != oldidx); \<br>+ } while( ! sqCompareAndSwap(thrlogidx,myidx,(myidx+1)&(THRLOGSZ-1)i)); \<br> if (thrlog[myidx]) free(thrlog[myidx]); \<br> asprintf(thrlog + myidx, __VA_ARGS__); \<br>
} while (0)<br><br></div></div>