openshmem-org / specification

OpenSHMEM Application Programming Interface
http://www.openshmem.org
49 stars 32 forks source link

Add shmem_{initialized, finalized} #470

Closed nspark closed 1 month ago

nspark commented 3 years ago

This PR adds shmem_initialized and shmem_finalized to query the state of the library.

Closes #457

nspark commented 2 years ago

Suggestion from 8/16 WG: Add note clarifying that shmem_initialized() == 0 is not necessarily sufficient to prevent multiple threads from racing to call shmem_init.

manjugv commented 2 years ago

August spec meeting: Please add this note in the shmem_initialized section as API notes.

BryantLam commented 2 years ago

These were the states I could think of in a K-map style:

  1. Before the library is initialized, shmem_initialized() == 0 and shmem_finalized() == 0.

    • If initialization routine is called, go to (2).
    • If finalization routine is called, spec doesn't say what happens, but implies undefined behavior :turtle: backed up by non-authoritative error note in backmatter.
    • (mentioned above and will be added as a note) If library is not initialized (and is it valid to do so), it is user error to have multiple threads race on calling shmem_init.
  2. After library is initialized, shmem_initialized() == 1 and shmem_finalized() == 0.

  3. After library is finalized, shmem_initialized() == <don't care> and shmem_finalized() == 1.

    • If initialization routine is called, undefined behavior like (2) :snail:.
    • If finalization routine is called, undefined behavior like (1) :turtle: and because it's not the last OpenSHMEM call :octopus:.
    • As currently defined, we can get away with shmem_initialized() == <don't care> because we are now finalized and any subsequent OpenSHMEM call (except shmem_initialized() and shmem_finalized()) would result in undefined behavior :snail::octopus:. As written, we have to set shmem_initialized() to some value but the value doesn't matter; it's effectively useless (or in a different respect, error prone) due to all the undefined behavior.

From these states, this would be one of the only valid conditional guards:

void app_startup() {
    if (shmem_finalized()) {
        // OpenSHMEM is done. Undefined behavior to call any OpenSHMEM routine.
        return or abort();
    }
    if (!shmem_initialized()) {
        // OpenSHMEM is not initialized. Okay to call OpenSHMEM initialization logic.
        shmem_init() or shmem_init_thread();
        this_app_initialized_openshmem = 1;
    }
    // OpenSHMEM is initialized.
}

void app_teardown() {
    if (this_app_initialized_openshmem == 1) {
        shmem_finalize();
    }
}

Or if your apps are not tearing down in reverse-startup order, you can choose to not (RE: must not?) put shmem_finalize() in app_teardown() and finalize OpenSHMEM from the parent scope:

in parent {
    app_startup();
    // do work
    app_teardown();
    if (!shmem_finalized()) { // Note: Should not use `shmem_initialized()` due to aforementioned `<don't care>`.
        shmem_finalize();
    }
}

I think this is a bit error-prone in my mind, but it works and it's what we've specified.

:snail: If subsequent calls to initialization routines were allowed in an active OpenSHMEM program, one behavior could be to "do nothing and increment value of shmem_initialized()" for nesting or counting references (shmem_finalized() is counting now too :octopus:). This would then change the conditional guard to something like:

void app_startup() {
    if (shmem_finalized() == shmem_initialized()) {
        return or abort();
    }
    shmem_init() or shmem_init_thread();
    // OpenSHMEM is initialized.
}

void app_teardown() {
    shmem_finalize();
}

in parent {
    app_startup();
    // do work
    app_teardown();
    assert(shmem_finalized() == shmem_initialized());
    // All apps tore down correctly, or either one of them or you have a bug.
}

The "do nothing" behavior could be a problem if subsequent calls of shmem_init_thread had incompatible arguments.

jdinan commented 2 years ago

If we follow the MPI semantics, the guard for calling shmem_init would be !shmem_initialized() && !shmem_finalized().

nspark commented 2 years ago

Added the following note:

Although shmem_initialized is thread-safe, its return value is not a sufficient guard to prevent multiple threads from racing to initialize the OpenSHMEM library concurrently, as shmem_initialized may return 0 to one thread while library initialization is in progress due to a call from another thread. Applications must ensure that only one call to shmem_init[_thread] is made to initialize the OpenSHMEM library.

jdinan commented 2 years ago

@nspark The note is reasonable, but why not add reference counted init/finalize and require that init functions are thread safe?

nspark commented 2 years ago

@jdinan IDK, that just seemed like a change of a larger scope. I'm happy to draft it, but I'd probably do it as a separate PR.

jdinan commented 1 month ago

Superseded by #512