Skip to content

Conversation

@wzamazon
Copy link
Contributor

@wzamazon wzamazon commented Dec 8, 2020

PMIx reigstration callback functions are used when regitering PMIx
event handler.

This patch adjusts two such callback functions:

    model_registration_callback()
         in ompi/interlib/interlib.c and

    ompi_errhandler_registration_callback()
         in ompi/errhandler/errhandler.c

Both of them employes the following code structure:

static void xxx_callback(int status,
			 size_t errhandler_ref,
			 void *cbdata)
{
    myreg_t *trk = (myreg_t*)cbdata;

    trk->status = status;
    interlibhandler_id = errhandler_ref;
    trk->active = false;
}

The workflow is:

  1. caller will call opal_pmix.register_evhandler() with
    callback function as an argument.
  2. caller will call OMPI_LAZY_WAIT_FOR_COMPLETION(trk.active)
    to wait for trk->active to became false,
  3. PMIx do the registration on anther thread, then call the
    registration callback function, which will set trk->active
    to false.
  4. caller check trk->status to determine whether registration
    succeeded.

The expected behavior of the registration callback functions therefore
is that trk->status be updated first, then trk->active be set to false.

However, on ARM based systems, the expected behavior is not guaranteed
because ARM uses a relaxed memory model.

To address this issue, this patch added a call to opal_atomic_wmb()
(write memory barrier) after trk->status being set, to achieve the
expected behavior.

Signed-off-by: Wei Zhang wzam@amazon.com

Refs #8274

PMIx reigstration callback functions are used when regitering PMIx
event handler.

This patch adjusts two such callback functions:

    model_registration_callback()
         in ompi/interlib/interlib.c and

    ompi_errhandler_registration_callback()
         in ompi/errhandler/errhandler.c

Both of them employes the following code structure:

static void xxx_callback(int status,
			 size_t errhandler_ref,
			 void *cbdata)
{
    myreg_t *trk = (myreg_t*)cbdata;

    trk->status = status;
    interlibhandler_id = errhandler_ref;
    trk->active = false;
}

The workflow is:

1. caller will call opal_pmix.register_evhandler() with
   callback function as an argument.
2. caller will call OMPI_LAZY_WAIT_FOR_COMPLETION(trk.active)
   to wait for trk->active to became false,
3. PMIx do the registration on anther thread, then call the
   registration callback function, which will set trk->active
   to false.
4. caller check trk->status to determine whether registration
   succeeded.

The expected behavior of the registration callback functions therefore
is that trk->status be updated first, then trk->active be set to false.

However, on ARM based systems, the expected behavior is not guaranteed
because ARM uses a relaxed memory model.

To address this issue, this patch added a call to opal_atomic_wmb()
(write memory barrier) after trk->status being set, to achieve the
expected behavior.

Signed-off-by: Wei Zhang <wzam@amazon.com>
@wzamazon
Copy link
Contributor Author

wzamazon commented Dec 8, 2020

Note this PR is applied to v4.1.x branch directly because master branch does not have this issue.

@jsquyres jsquyres merged commit 91b81d9 into open-mpi:v4.1.x Dec 8, 2020
@wzamazon
Copy link
Contributor Author

wzamazon commented Dec 8, 2020

Thank you for merging. I will open a PR for v4.0.x branch too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants