(cc @ctk21 @kayceesrk)

@ctk21 I think you are right, the current code runs domain-state-cleanup code outside the all_domains_lock. This is not symmetric with create_domain, which does keep all_domains_lock during all initialisation code.

I wondered about how this can work without races between domains terminating and domains being created. The answer is that there is another lock domain_self->domain_lock (that seems used most of the time to orchestrate control transfer with the backup thread) that is taken on domain creation, held by the domain during its lieftime, and released after the cleanup code has run.

Could we instead keep all_domains_lock until termination is done? Basically instead of

  while (!finished) {
   [...]
    caml_empty_minor_heaps_once();
    caml_plat_lock(&all_domains_lock);

    if (!caml_incoming_interrupts_queued() &&
        domain_state->marking_done &&
        domain_state->sweeping_done) {

      finished = 1;
      s->terminating = 0;
      s->running = 0;

      remove_from_stw_domains(domain_self);
      [...]
    }
    caml_plat_unlock(&all_domains_lock);
  }

  /* rest of the domain cleanup code, without holding all_domains_lock */
  [...]

we could use

  do {
   [...]
    caml_empty_minor_heaps_once();
    caml_plat_lock(&all_domains_lock);

    if (!caml_incoming_interrupts_queued() &&
        domain_state->marking_done &&
        domain_state->sweeping_done) {

      finished = 1;
      s->terminating = 0;
      s->running = 0;

      remove_from_stw_domains(domain_self);
      [...]
    } else {
      caml_plat_unlock(&all_domains_lock);
    }
  } while (!finished);

  /* rest of the domain cleanup code, still holding all_domains_lock */
  [...]
  caml_plat_unlock(&all_domains_lock);

Would this work? Would this provide the STW exclusion guarantees I have optimistically written down in this PR?

(This could not work if any of the cleanup code needed to run an STW section, but currently I don't think it does.)

The reason I care about exclusion from domain initialisation and cleanup code is #10971: if we want to be able to change minor heap reservations in-flight, we want to be able to touch the dom_internal data of all domains within one domain of a STW section, and feel confident that this will not race with (initialisation or) cleanup.

Another option would be to ensure that the cleanup code works on the domain_state alone, and leaves the dom_internal variable alone. (I think this is the case for now.) Then we could not provide general exclusion guarantees, but at least the fact that dom_internal structures (but not their inner domain_state field) can be safely mutated from STW without races with initialisation and cleaup. (But keeping all_domains_lock sounds much simpler, unless I am missing something obvious again.)

It might work but there are a lot of moving pieces.

From a design perspective, there is another option. Maybe create_domain should only use all_domain_lock at the point it inserts the domain into the stop-the-world set? Not immediately clear to me if we want finer grained or coarser grained locking in the long run. It depends on what we are trying to achieve and why.

Does it make sense to present the other code you have in mind and then look at this topic within that concrete proposal?

I think the best thing is to focus in this PR on documenting the current state as it is, and then send other PRs if we want to change the behavior. So I'll go back to doing that.

@ctk21 I merged your suggestions, and did a couple small changes. Would you by chance be happy with the current state?

For people following from a distance, here is what the main new explanation block now looks like:

STW sections use [all_domains_lock] and the variable [stw_leader]
(0 when no STW section is running, the dom_internal* pointer of the
STW leader when a STW section is running) to guarantee that no
domain is running something else:

• If two STW sections are attempted in parallel, only one will
  manage to take the lock, and the domain starting the other will
  join that winning STW section, without running its own STW code
  at all. (This is the [try] in the function name: if it returns
  0, the STW section did not run at all, so you should call this
  function in a loop.)

• Domain initialization code from [create_domain] will not run in
  parallel with a STW section, as [create_domain] starts by
  looping until (1) it has the [all_domains_lock] and (2) there is
  no current STW section (using the [stw_leader] variable).

• Domain cleanup code runs after the terminating domain may run in
  parallel to a STW section, but only after that domain has safely
  removed itself from the STW participant set: the
  [domain_terminate] function is careful to only leave the STW set
  when (1) it has the [all_domains_lock] and (2) it hasn't received
  any request to participate in a STW section.

Each domain leaves the section as soon as it is finished running
the section code. In particular, a mutator may resume while some
other domain are still in the section. Section code that needs to
happen before any mutator must be followed by a barrier, forcing
all STW participants to synchronize.

Taken together, these properties guarantee that STW sections act as
a proper exclusion mechanism: for example, some mutable state
global to all domains can be "protected by STW" if it is only
mutated within STW section, with a barrier before the next
read. Such state can be safely updated by domain initialization,
but additional synchronization would be required to update it
during domain cleanup.

@ctk21: for the record, I tried to change terminate_domain in a branch so that the cleanup code runs under all_domains_lock: gasche/ocaml@document-stw-synchronization...domain-termination-synchronization. The code is not simpler (it is different), but I find the provided guarantees simpler (and more symmetric between domain creation and termination), and I'm assuming that the performance difference does not matter as we decided that domain creation and termination were unfrequent oprations. I'm thinking of submitting this change as a PR -- but it would be easier if we could converge on this one first.

I did a light review, and the documentation looks good to me.

I'm assuming that the performance difference does not matter as we decided that domain creation and termination were unfrequent oprations.

We have Sandmark nightly and performance-tuned machines for benchmarking parallel programs. Let's use them. Mostly, we may just observe that there is no noticeable performance degradation, and we can move on. :-) CC @shakthimaan.

FYI currently the nightly runs are broken since the version change from 5.00 to 5.0 broke some dependencies. These should be resolved in a week. After the service is back up, we can point your branch at sandmark nightly and get the results out.

Thanks @kayceesrk for the review, I'll try to go over the comments soon.

We have Sandmark nightly and performance-tuned machines for benchmarking parallel programs. Let's use them. Mostly, we may just observe that there is no noticeable performance degradation, and we can move on. :-) CC @shakthimaan.

Just to clarify, the change whose performance impact we are discussing are not included in the present PR. They may turn into a PR later (for now it's only at the stage "think hard about whether I dare sending this to @ctk21's review"), and even then I think that a review for correctness would naturally come before performance tests, because this code is easier to get wrong than to get "too slow". So: any benchmarking help is warmly welcome, and I'll take the liberty to ping @shakthimaan again, but right now it's too soon.

I've looked at the changes, I think they are good.

@kayceesrk's comments to tighten up what we mean by being in the STW participant set (i.e. guaranteed to service the STW sections) vs being just a pthread entering and exiting, look reasonable.

off topic for this PR (:

Let me argue the case to leave the door slightly open on the assumption: "we don't care about the performance of domain spawn & join". Maybe there are reasonable programs and libraries where good spawn/join performance is useful? Once we bake in firmly that domain spawn/join is expensive, it may be difficult to come back.

That said, moving this lock around might not make a meaningful performance difference (it could be desirable in the context of another change to come). I'm a bit skeptical we have programs or use cases that help measure the performance of spawn/join. If no sensible use cases arrive, then I guess it is a mute point.

I have pushed another commit to address @kayceesrk's comments. Thanks again for the review.

Thanks @kayceesrk (and @ctk21, @jhjourdan) for your reviews. I took the last batch of comments from KC into account and rebased the PR, this should be good to go.