[M3devel] cvsup fix refinements
Tony Hosking
hosking at cs.purdue.edu
Fri Mar 19 19:47:00 CET 2010
I think the atfork support should go into Unix.i3. It's not necessary for Windows, since Unix.fork is not available there.
Also, we should make user and system threading behave the same way. So, for user threads we would destroy all the other threads in the child. This implies that we should have an m3core process fork mechanism. Anyone wanting to fork can then rely on the same behaviour.
> Index: src/m3core.h
> ===================================================================
> RCS file: /usr/cvs/cm3/m3-libs/m3core/src/m3core.h,v
> retrieving revision 1.8
> diff -u -r1.8 m3core.h
> --- src/m3core.h 19 Mar 2010 17:07:58 -0000 1.8
> +++ src/m3core.h 19 Mar 2010 17:08:41 -0000
> @@ -404,6 +404,13 @@
> UINT32 __cdecl Uin__htonl(UINT32 x);
> UINT16 __cdecl Uin__htons(UINT16 x);
>
> +typedef void (*PThreadForkHandler)(void);
> +
> +INTEGER
> +RTOS__PThreadAtFork(PThreadForkHandler prep,
> + PThreadForkHandler parent,
> + PThreadForkHandler child);
> +
> #ifdef __cplusplus
> } /* extern "C" */
> #endif
> Index: src/runtime/common/RTCollector.m3
> ===================================================================
> RCS file: /usr/cvs/cm3/m3-libs/m3core/src/runtime/common/RTCollector.m3,v
> retrieving revision 1.88
> diff -u -r1.88 RTCollector.m3
> --- src/runtime/common/RTCollector.m3 15 Dec 2009 19:52:08 -0000 1.88
> +++ src/runtime/common/RTCollector.m3 19 Mar 2010 17:08:42 -0000
> @@ -2033,19 +2033,31 @@
>
> VAR startedWeakCleaner := FALSE;
>
> -PROCEDURE WeakRefFromRef (r: REFANY; p: WeakRefCleanUpProc := NIL): WeakRef =
> +PROCEDURE StartWeakCleaner () =
> VAR
> start := FALSE;
> - result: WeakRef;
> BEGIN
> - <* ASSERT r # NIL *>
> TRY
> RTOS.LockHeap();
> - (* create a WeakCleaner thread the first time through *)
> - IF p # NIL AND NOT startedWeakCleaner THEN
> + IF NOT startedWeakCleaner THEN
> start := TRUE;
> startedWeakCleaner := TRUE;
> END;
> + FINALLY
> + RTOS.UnlockHeap();
> + END;
> + IF start THEN
> + EVAL Thread.Fork(NEW(Thread.Closure, apply := WeakCleaner));
> + END;
> + END StartWeakCleaner;
> +
> +PROCEDURE WeakRefFromRef (r: REFANY; p: WeakRefCleanUpProc := NIL): WeakRef =
> + VAR
> + result: WeakRef;
> + BEGIN
> + <* ASSERT r # NIL *>
> + TRY
> + RTOS.LockHeap();
> (* if necessary, expand weakTable *)
> IF weakFree0 = -1 THEN ExpandWeakTable(); END;
> IF p # NIL THEN
> @@ -2075,9 +2087,8 @@
> FINALLY
> RTOS.UnlockHeap();
> END;
> - IF start THEN
> - EVAL Thread.Fork(NEW(Thread.Closure, apply := WeakCleaner));
> - END;
> + (* create a WeakCleaner thread the first time through *)
> + StartWeakCleaner();
> RETURN result;
> END WeakRefFromRef;
I would leave WeakRefFromRef unchanged, and simply restart the thread in the child if startedWeakCleaner is TRUE:
PROCEDURE AtForkChild() =
BEGIN
RTOS.LockHeap();
IF startedForeground THEN
StartBackgroundCollection();
END;
IF startedBackground THEN
StartBackgroundCollection();
END;
IF startedWeakCleaner THEN
StarteWeakCleaner();
END:
END AtForkChild;
>
> @@ -2771,8 +2782,24 @@
>
> (*** INITIALIZATION ***)
>
> +PROCEDURE PThreadForkChild() =
> + BEGIN
> + IF startedForeground THEN
> + StartBackgroundCollection();
> + END;
> + IF startedBackground THEN
> + StartBackgroundCollection();
> + END;
> + IF startedWeakCleaner THEN
> + StartWeakCleaner();
> + END;
> + END PThreadForkChild;
> +
> PROCEDURE Init () =
> + VAR r: INTEGER;
> BEGIN
> + r := RTOS.PThreadAtFork((*prepare*)NIL, (*parent*)NIL, PThreadForkChild);
> + <* ASSERT r = 0 *>
> IF RTParams.IsPresent("paranoidgc") THEN InstallSanityCheck(); END;
> IF RTParams.IsPresent("nogc") THEN disableCount := 1; END;
> IF RTParams.IsPresent("noincremental") THEN incremental := FALSE; END;
> Index: src/runtime/common/RTOS.i3
> ===================================================================
> RCS file: /usr/cvs/cm3/m3-libs/m3core/src/runtime/common/RTOS.i3,v
> retrieving revision 1.10
> diff -u -r1.10 RTOS.i3
> --- src/runtime/common/RTOS.i3 8 Sep 2009 05:54:55 -0000 1.10
> +++ src/runtime/common/RTOS.i3 19 Mar 2010 17:08:42 -0000
> @@ -5,7 +5,7 @@
> (*| modified on Sun Feb 21 14:15:00 PST 1993 by jdd *)
> (*| modified on Wed Jan 27 22:27:27 PST 1993 by mjordan *)
>
> -(* "RTOS" is a private interface that provides the low-level,
> +(* "RTOS" is a semi-private interface that provides the low-level,
> OS-specific memory allocation and shutdown routines. *)
Leave this private!
>
> INTERFACE RTOS;
> @@ -40,4 +40,13 @@
> (* Write the "n" bytes beginning at address "a" to the standard
> error output file or console. *)
>
> +(* ThreadF__PThreadAtFork calls pthread_atfork
> + * on pthreads platform, otherwise does nothing.
> + * Return value is from pthread_atform, 0 for success,
> + * always 0 with user threads and Win32 threads.
> + *)
> +TYPE PThreadForkHandler = PROCEDURE();
> +<* EXTERNAL RTOS__PThreadAtFork *>
> +PROCEDURE PThreadAtFork(prep, parent, child: PThreadForkHandler): INTEGER;
> +
Put the pthread_atfork wrapper into Unix.i3.
> END RTOS.
> Index: src/runtime/common/m3makefile
> ===================================================================
> RCS file: /usr/cvs/cm3/m3-libs/m3core/src/runtime/common/m3makefile,v
> retrieving revision 1.27
> diff -u -r1.27 m3makefile
> --- src/runtime/common/m3makefile 14 Dec 2009 08:09:48 -0000 1.27
> +++ src/runtime/common/m3makefile 19 Mar 2010 17:08:42 -0000
> @@ -58,7 +58,7 @@
> Interface ("RTSignal")
> Interface ("RTStack")
> Interface ("RTTypeSRC")
> -interface ("RTOS")
> +Interface ("RTOS")
>
> proc FileExists(a) is
> return not stale (a, a)
> Index: src/thread/POSIX/ThreadPosixC.c
> ===================================================================
> RCS file: /usr/cvs/cm3/m3-libs/m3core/src/thread/POSIX/ThreadPosixC.c,v
> retrieving revision 1.32
> diff -u -r1.32 ThreadPosixC.c
> --- src/thread/POSIX/ThreadPosixC.c 16 Dec 2009 12:21:36 -0000 1.32
> +++ src/thread/POSIX/ThreadPosixC.c 19 Mar 2010 17:08:42 -0000
> @@ -279,6 +279,14 @@
> #endif
> }
>
> +INTEGER
> +RTOS__PThreadAtFork(PThreadForkHandler prep,
> + PThreadForkHandler parent,
> + PThreadForkHandler child)
> +{
> + return 0;
> +}
> +
> #ifdef __cplusplus
> } /* extern "C" */
> #endif
> Index: src/thread/PTHREAD/ThreadPThread.m3
> ===================================================================
> RCS file: /usr/cvs/cm3/m3-libs/m3core/src/thread/PTHREAD/ThreadPThread.m3,v
> retrieving revision 1.233
> diff -u -r1.233 ThreadPThread.m3
> --- src/thread/PTHREAD/ThreadPThread.m3 18 Mar 2010 11:12:10 -0000 1.233
> +++ src/thread/PTHREAD/ThreadPThread.m3 19 Mar 2010 17:08:42 -0000
> @@ -6,7 +6,7 @@
> Scheduler, SchedulerPosix, RTOS, RTHooks, ThreadPThread;
>
> IMPORT Cerrno, FloatMode, MutexRep,
> - RTCollectorSRC, RTError, RTHeapRep, RTIO, RTParams,
> + RTCollectorSRC, RTError, RTHeapRep, RTIO, RTOS, RTParams,
> RTPerfTool, RTProcess, ThreadEvent, Time,
> Unix, Utime, Word, Usched,
> Uerror, Uexec;
> @@ -1294,18 +1294,18 @@
>
> (*-------------------------------------------------------- Initialization ---*)
>
> -PROCEDURE Init ()=
> +PROCEDURE InitWithStackBase (stackbase: ADDRESS) =
> VAR
> self: T;
> me: Activation;
> BEGIN
> - InitC(ADR(self));
> + InitC(stackbase);
>
> me := NEW(Activation,
> mutex := pthread_mutex_new(),
> cond := pthread_cond_new());
> InitActivations(me);
> - me.stackbase := ADR(self); (* not quite accurate but hopefully ok *)
> + me.stackbase := stackbase;
> IF me.mutex = NIL OR me.cond = NIL THEN
> Die(ThisLine(), "Thread initialization failed.");
> END;
> @@ -1322,8 +1322,48 @@
> IF RTParams.IsPresent("foregroundgc") THEN
> RTCollectorSRC.StartForegroundCollection();
> END;
> + END InitWithStackBase;
> +
> +PROCEDURE Init ()=
> + VAR r: INTEGER;
> + BEGIN
> + r := RTOS.PThreadAtFork(PThreadAtForkPrepare, PThreadAtForkParent, PThreadAtForkChild);
> + IF r # 0 THEN DieI(ThisLine(), r) END;
> + InitWithStackBase(ADR(r)); (* not quite accurate but hopefully ok *)
> END Init;
>
> +VAR locks := ARRAY [0..3] OF pthread_mutex_t{
> + activeMu, slotsMu, initMu, perfMu};
> +
> +PROCEDURE PThreadAtForkPrepare() =
> + VAR r: int;
> + BEGIN
> + joinMu.acquire();
> + LockHeap();
> + FOR i := FIRST(locks) TO LAST(locks) DO
> + r := pthread_mutex_lock(locks[i]);
> + IF r # 0 THEN DieI(ThisLine(), r) END;
> + END;
> + (* Walk activations and lock all threads? *)
> + END PThreadAtForkPrepare;
I think there may be deadlock issues here!
Would it not be better simply to SuspendOthers() in prepare and ResumeOthers() in parent/child?
You would still need to acquire slotsMu/initMu/perfMu before SuspendOthers, and release them after ResumeOthers().
> +
> +PROCEDURE PThreadAtForkParent() =
> + VAR r: int;
> + BEGIN
> + FOR i := LAST(locks) TO FIRST(locks) BY -1 DO
> + r := pthread_mutex_unlock(locks[i]);
> + IF r # 0 THEN DieI(ThisLine(), r) END;
> + END;
> + UnlockHeap();
> + joinMu.release();
> + END PThreadAtForkParent;
> +
> +PROCEDURE PThreadAtForkChild() =
> + BEGIN
> + PThreadAtForkParent();
> + InitWithStackBase(GetActivation().stackbase);
> + END PThreadAtForkChild;
> +
> (*------------------------------------------------------------- collector ---*)
> (* These procedures provide synchronization primitives for the allocator
> and collector. *)
> Index: src/thread/PTHREAD/ThreadPThreadC.c
> ===================================================================
> RCS file: /usr/cvs/cm3/m3-libs/m3core/src/thread/PTHREAD/ThreadPThreadC.c,v
> retrieving revision 1.123
> diff -u -r1.123 ThreadPThreadC.c
> --- src/thread/PTHREAD/ThreadPThreadC.c 25 Feb 2010 08:31:36 -0000 1.123
> +++ src/thread/PTHREAD/ThreadPThreadC.c 19 Mar 2010 17:08:42 -0000
> @@ -414,6 +414,14 @@
> #endif
> }
>
> +INTEGER
> +RTOS__PThreadAtFork(PThreadForkHandler prep,
> + PThreadForkHandler parent,
> + PThreadForkHandler child)
> +{
> + return pthread_atfork(prep, parent, child);
> +}
> +
> #ifdef __cplusplus
> } /* extern "C" */
> #endif
> Index: src/thread/WIN32/ThreadWin32C.c
> ===================================================================
> RCS file: /usr/cvs/cm3/m3-libs/m3core/src/thread/WIN32/ThreadWin32C.c,v
> retrieving revision 1.64
> diff -u -r1.64 ThreadWin32C.c
> --- src/thread/WIN32/ThreadWin32C.c 16 Jan 2010 12:44:56 -0000 1.64
> +++ src/thread/WIN32/ThreadWin32C.c 19 Mar 2010 17:08:42 -0000
> @@ -146,6 +146,15 @@
> p(bottom, __getReg(?)); /* bsp? bspstore? try numbers until we find it? */
> #endif
> }
> +
> +/*-------------------------------------------------------------------------*/
> +
> +INTEGER
> +RTOS__PThreadAtFork(PThreadForkHandler prep,
> + PThreadForkHandler parent,
> + PThreadForkHandler child)
> +{
> + return 0;
> }
>
> /*-------------------------------------------------------------------------*/
On 19 Mar 2010, at 14:13, Tony Hosking wrote:
> Why not put it into RTProcess?
> Or into Unix?
> I want to avoid confusion between Thread.Fork (fork a thread) and process fork.
>
> On 19 Mar 2010, at 14:08, Jay K wrote:
>
>> > could go in any of ThreadF, RTOS, RTProcess?
>>
>>
>> Or RTThread. Or almost anywhere.
>>
>>
>> Some typos in the diffs:
>> in a comment, ThreadF__ should be RTOS__
>> function PThreadForkHandler named inconsistently,
>> should be PThreadAtForkHandler
>>
>>
>> Again, "PThread" appears in these names to indicate
>> their meaning is not portable. Their existance is, but not their meaning.
>> We have no way of saying "only call this function for pthreads",
>> instead as I understand, we provide a portable interface with
>> drastically varying semantics, such as "do something" vs. "do nothing".
>>
>>
>> Given that Init in ThreadPThread.m3 is private, we could probably
>> take the stackbase parameter from the caller instead.
>>
>>
>> The call to AtFork should probably follow InitWithStackbase,
>> in case it fails under low memory, the Die/assert more likely to "work".
>>
>>
>> I'm not completely happy making RTOS public.
>> So maybe ThreadF?
>> I had gone to RTOS because SetNextForkWillExec required LockHeap/UnlockHeap.
>> But for now they don't exist.
>>
>>
>> Maybe a new interface? ThreadPThread.i3, but is still
>> present on all platforms, so mostly portable can call it.
>>
>>
>> ThreadPThread.AtFork?
>> PThread.AtFork? That seems right.
>>
>>
>> - Jay
>>
>>
>> From: jay.krell at cornell.edu
>> To: m3devel at elegosoft.com
>> Date: Fri, 19 Mar 2010 17:21:13 +0000
>> Subject: Re: [M3devel] cvsup fix refinements
>>
>> diffs attached
>>
>>
>> For now I've removed the Get/SetNextForkWillExec.
>> I also think truly get/set is right, not inc/dec.
>> It was confusing enough to interpret and initialize
>> the value. And it is also not clear what the default
>> should be. The usual behavor is exec does follow fork.
>> The safer default if the handlers work ok is to assume
>> exec will not follow. It is a perf hint or a don't change
>> how things were hint? Hint to speed up fork+exec or hint
>> to avoid running the new code that might not work?
>>
>>
>> (For now I've removed the Get/SetNextForkWillExec.)
>> They'd have to be implemented three times, and
>> I had trouble defining them.
>> Obviously Win32/userthreads just return a
>> hardcoded true, but it is hard to explain
>> from the caller of Get's point of view.
>>
>> Maybe:
>> SetNextForkWillExec
>> GetNextForkWillExec
>>
>> and then:
>> PThreadAtForkNeeded() = GetNextForkWillExec = FALSE ?
>>
>> That is,
>> someone is who is calling fork and possibly exec,
>> can immediately tell what these functions mean.
>>
>>
>> But the implementer of an atfork handler has to
>> do quite a semantic translation I think.
>> I wrote it backwards the first time. That either
>> implies it is confusing, or I wasn't thinking.
>>
>>
>> If it really is a problem to run this stuff when exec
>> will follow, we should know quickly, as building cm3
>> is an aggressive user of this code.
>>
>>
>> This version has worked repeatedly on Darwin.
>> I didn't test user threads but it is structured such
>> as to make that obviously work.
>> The cvsup changes are in pthreaad_atfork handlers,
>> so no change for userthreads.
>>
>>
>> I didn't test on others but confidence is quite high now.
>>
>> description of changes at least where they are hard to read:
>>
>> m3core:
>> Init split into Init and InitWithStackBase,
>> We have to provide the old stack base in the child fork handler
>> instead of estimating from the address of a local. I don't
>> know what stack is used, maybe the caller of fork?
>> i.e. we might have eaten a fair amount of stack and
>> there could be arbitrary references above the current stack.
>>
>>
>> WeakRefFromRef split into WeakRefFromRef and StartWeakCleaner
>>
>> There is a resulting double lock in WeakRefFromRef, every time.
>> Could instead copy/paste more code around, i.e.:
>> EVAL Thread.Fork(NEW(Thread.Closure, apply := WeakCleaner));
>>
>>
>> - Jay
>>
>>
>>
>>
>>
>>
>> From: jay.krell at cornell.edu
>> To: m3devel at elegosoft.com
>> Date: Fri, 19 Mar 2010 16:01:03 +0000
>> Subject: [M3devel] cvsup fix refinements
>>
>> refinements:
>>
>>
>> new public functions in m3core:
>>
>>
>> TYPE PThreadForkHandler = PROCEDURE();
>>
>> <* EXTERNAL RTOS__PThreadAtFork *>
>> PROCEDURE PThreadAtFork(prep, parent, child: PThreadForkHandler): INTEGER;
>>
>>
>> (That's not a change yet.)
>>
>>
>> PROCEDURE SetNextForkWillExec(value: BOOLEAN);
>>
>> PROCEDURE GetNextForkWillExec(): BOOLEAN;
>>
>> These are only an optimization and should
>> perhaps be removed? They should be used
>> under LockHeap/UnlockHeap? which wasn't previously
>> public. This way, existing fork/exec paths
>> not affected, though maybe though might as well
>> ought to be?
>>
>>
>> could go in any of ThreadF, RTOS, RTProcess?
>>
>>
>> Should be called under RTOS.LockHeap?
>> Therefore I put in RTOS.
>> Also helps that RTOS is exported from *Thread.m3.
>> RTOS not previously public.
>>
>>
>> Should Get/Set be Inc/Dec? (therefore just Inc with +1,-1?)
>> I implemented them that way: true incs, false decs.
>>
>>
>> Or don't bother with them at all?
>>
>>
>> Furthermore, in RTCollector, instead of claiming the threads
>> aren't started, if they were started, restart them.
>> This makes more sense for the weak cleaner to me, and
>> seems reasonable for the other two.
>>
>>
>> Diffs not yet available.
>>
>>
>> - Jay
>
More information about the M3devel
mailing list