IIUC the first commit fixes a bug, the remaining ones are (welcome!) re-workings in systhreads. If that's right, please could the first commit be separated as I think that's then definitely a 5.0 change, where the rest might have to be 5.1?

On whether this is necessary for 5.0. The "bug" part fixed by the first commit is also fixed by the parts of the PR series on reworking async callbacks for multicore that are already necessary for 5.0 (by not calling into OCaml inside caml_thread_yield). So I would not say that the first one is necessary for 5.0 by itself (unless calling caml_thread_yield in the backup thread causes more issues than I know). So this PR is not in itself necessary for 5.0. On the other hand, if people like the approach at #11272 which depends on this PR, then it should be in for 5.0 (API change), in which case this PR is also necessary for 5.0.

Personally I think of such "documentation / cleanups / refactoring" PRs (I wrote a couple myself) as a continuation of the review effort for the multicore merge, which has happened in an unusual way with several "big picture" discussions but much less "looking at the details" review work than typical PRs. So: in scope for "whenever before or after the merge", including now -- and probably a better long-term use of our collective time than giving up on documentations and rushing to finish new feature X or Y in time for 5.0.

But of course it's a matter of finding reviewers able to do a review. Here I wonder if @Engil would be available? (Or maybe @ctk21?) We need someone (else than @gadmm) who knows how threads work in Multicore. Personally I don't; is there some clear documentation in the implementation already?

Personally I don't; is there some clear documentation in the implementation already?

I do not claim to be an expert; but it is fairly similar to the pre-multicore systhreads implementation, with some subtlety in the interaction with the backup thread.

I'd be more inclined to review if the motivations were given for some of the changes. For example "Use a thread-local variable this_thread instead of explicit TLS; prefer using this_thread over Current_thread." doesn't tell me why a thread-local variable is preferable: is that for performance? safety (w.r.t. signals for example)? portability?.

this_thread is more efficient than the previous Current_thread which goes through a Caml_state->id indexed table. (But I won't claim the performance difference is measurable.) I guess the circonvolution was due to the fact that accessing the explicit TLS was syntactically heavy. I imagine that using explicit TLS was inspired by the previous implementation and perhaps previous problems with thread_local on MacOS, but here using a static thread_local is just fine for multicore. This also removes some lines of code, and removes a couple of corner cases in caml_register/unregister_c_thread. It adds one inside caml_thread_scan_root but I'd argue that this clarifies code (it makes explicit that we can be in the backup thread). This change is also motivated by not accessing Caml_state without the domain lock. The change is pretty superficial otherwise.

The change from Caml_state->id-based indexing to this_thread->domain_id-based indexing forces to move some code around in the same function (when this_thread is not yet initialized or is reset). The motivation here is again to not access Caml_state without holding the domain lock.

Merging All_threads with Current_thread also makes it more natural in two places (where lines of code are removed).

In this file, the accesses to Caml_state->id without holding the domain lock were safe, so part of the motivation will stem from how people feel about #11272.

(Thanks for asking)

Thanks for the review @kayceesrk! I think you have inadvertently written your comments in the wrong thread (#11272). I am a bit busy for the coming days but I will address your comments in the present PR.

Sorry for the long delay answering this, @gadmm, I was off for a while.
I am back now and will review this in the coming days.

I have found a few more fixes to do so I have split the PR in two (i.e. I removed some commits and will reintroduce them in another PR).

I have renamed Current_thread as proposed by @kayceesrk. I have also checked that no other function is unexpectedly callable from the backup thread.

I have found a nice solution to caml_thread_yield, please have a look.

Please accept a small tooling documentation commit, it is unrelated except for the fact that debugging the issue with caml_thread_yield had me to try and find one time too much the information which I wrote in the commit.

For instance, the this_thread and Current_thread situation is a bit hard to understand, I can't see a consistency in when to use one over the other (?).

[Current_thread -> Active_thread]
I tried to use this_thread whenever possible (because it does not go through an indirection), and Active_thread only when we really mean it, never as a synonymous for this_thread when the two match by chance. This should make the distinction between the two clearer. I have a further commit for a later PR where I clarified the invariant concerning Active_thread notably by using the same set_active function everywhere (this forces to fix another bug, so I leave it for later).

Also some of these simplifications I am not super confident about and will need a deeper look into the corner cases removed by this PR.

One commit was split in two so that should make it easier. I have removed other commits to deal with later.

(The other PR: gadmm/ocaml@systhread_simpl_and_fixes...gadmm:systhread_simpl_and_fixes2)

This PR had a first round of review by @Engil and @kayceesrk, the comments have been addressed (I think). The PR has been simplified in the hope that we could converge quickly and move to the second PR I mentioned (which I think fixes the turning-off of the tick threads) as well as #11272 which depends on this PR. Is that ok with you?

Thanks, let's discuss your question in the next PR.

CI is green, no change entry is needed.

Thanks, could you please cherry-pick on 5.0?

I would normally think twice about merging a subtle PR to the release branch without going through our glorious release manager @Octachron, but this is 5.0-the-alpha-release and reviewers agreed that this fixes a bug, so let's go for it.

That may have been unwise. Jenkins CI is rather unhappy with these commits (https://ci.inria.fr/ocaml/job/main/3090/):

Alpine Linux x86_64
    tests/parallel/'fib_threads.ml' with 1.1 (bytecode) Process 17083 got signal 11(Segmentation fault), no core dumped
    tests/lib-systhreads/'multicore_lifecycle.ml' with 1.1 (bytecode) Process 13556 got signal 11(Segmentation fault), no core dumped

PPC64

    tests/lib-systhreads/'multicore_lifecycle.ml' with 1.1 (bytecode) Timeout expired, killing all child processes
    tests/parallel/'multicore_systhreads.ml' with 1.1 (bytecode) Timeout expired, killing all child processes
    tests/parallel/'fib_threads.ml' with 1.1 (bytecode) Timeout expired, killing all child processes

PPC32
    tests/lib-systhreads/'multicore_lifecycle.ml' with 1.1 (bytecode) Timeout expired, killing all child processes
    tests/parallel/'multicore_systhreads.ml' with 1.1 (bytecode) Timeout expired, killing all child processes
    tests/parallel/'fib_threads.ml' with 1.1 (bytecode) Timeout expired, killing all child processes

There's also a boatload of Windows failures, but possibly unrelated.

Time to debug...

Yep, both 5.0 and trunk appear to fail CI. (The testsuite passes with 5.0 on my machine, i checked before pushing).

I will revert the cherry-pick commit from 5.0 for now to un-break that CI. I don't know if we should revert from trunk as well.

@gadmm this PR appears to break bytecode tests (with a segfault) under some architectures including Alpine-x86_64 (that one is easy to test from Docker I suppose).

I just pushed 373c941 to trunk which is a missing part of #11304, but that doesn't eliminate the Windows problems caused by this PR on Jenkins either.

I'd suggest we revert this on trunk as well (I'm also taking another look at restoring more of the Windows CI testing, as that would have been a useful canary here)

I agree with reverting the change on 5.0 and trunk. I’ll study the failure early next week if @gadmm does not get to it earlier.

For what it's worth, here is what GDB says about the Alpine segfault in fib_thread:

Thread 14 "ocamlrun" received signal SIGSEGV, Segmentation fault.
[Switching to LWP 30081]
0x00007fffe7e5e1db in caml_thread_restore_runtime_state () at st_stubs.c:183
183	  Caml_state->current_stack = Active_thread->current_stack;
(gdb) bt
#0  0x00007fffe7e5e1db in caml_thread_restore_runtime_state ()
    at st_stubs.c:183
#1  0x00007fffe7e5e6db in caml_thread_stop () at st_stubs.c:450
#2  caml_thread_start (v=<optimized out>) at st_stubs.c:483
#3  0x00007ffff7fba221 in ?? () from /lib/ld-musl-x86_64.so.1
#4  0x0000000000000000 in ?? ()

Same context for multicore_lifecycle.

Thanks for the feedback, the PR is now reverted from both 5.0 and trunk.

Once the bug are found, we need to open another PR (github won't less us "reopen" a reverted PR), and to run precheck on it to test the INRIA CI before merging.

(I wonder why the Github CI did not observe the issue. Once we understand what was at fault, maybe we will be able to strengthen the CI.)

I am looking into it. I could bisect the failure on Alpine to the third commit. I opened #11390 to run the INRIA CI on it the other commits and check if the other failures come from the same commit. Could someone please run precheck on it?

Thanks everyone for your assistance. I have opened #11393 to fix the bug (see more details there). So now we have 3 PRs for 5.0 (in order: #11390 -> #11385 -> #11393) and 1 PR for 5.1 only (#11386).