Skip to content

Conversation

@hjelmn
Copy link
Member

@hjelmn hjelmn commented Nov 28, 2018

This commit contains the following changes:

  • Remove the unused opal_test_init/opal_test_finalize
    functions. These functions are not used by anything in the code
    base or MTT. Tests use opal_init_util/opal_finalize_util instead.

  • Get rid of gotos in opal_init_util and opal_init. Replaced them
    with a cleaner solution.

  • Automatically register cleanup functions in init functions. The
    cleanup functions are executed in the reverse order of the
    initialization functions. The cleanup functions are run in
    opal_finalize_util() before tearing down the class system.

Signed-off-by: Nathan Hjelm hjelmn@lanl.gov

@hjelmn
Copy link
Member Author

hjelmn commented Nov 28, 2018

This came out of the sessions work. Since we will be bringing up parts of ompi piecemeal we need to keep track of what need to be finalized and in what order. To handle this each init function is expected to register its finalize function.

@bosilca Any objections? The only downside is that the datatype tests now have to call opal_init_util() and opal_finalize_util() instead of just initializing the datatype engine. Outside of the tests nothing is calling the internal init/finalize functions directly (and shouldn't be).

@hjelmn
Copy link
Member Author

hjelmn commented Nov 28, 2018

@jsquyres A much larger PR will be headed for ompi. I need to rewrite that since that commit is intertwined with the sessions work.

@ibm-ompi
Copy link

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/1a9c5bbe409d07204aca7abaa7ec7c8b

@ibm-ompi
Copy link

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/79ce1fd1142ef82c29e544c1af4061f5

@hjelmn
Copy link
Member Author

hjelmn commented Nov 28, 2018

Opps. Looks like I didn't update ompi_datatype for the changes. Will fix that tomorrow. For now this is for comments anyway.

@ibm-ompi
Copy link

The IBM CI (PGI Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/53a512f69a4dbc4035211ab369396690

Copy link
Member

@bosilca bosilca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good move, providing a simpler and much cleaner tear down solution. I would propose to add a second parameter to opal_finalize_register_cleanup, a void* that is then passed to the cleanup function when called. This will allow us to add not only single global frameworks to the finalization, but MCA modules as well.

@hjelmn hjelmn force-pushed the opal_cleanup branch 2 times, most recently from 0ccbc02 to 6d55739 Compare December 5, 2018 21:41
@hjelmn
Copy link
Member Author

hjelmn commented Dec 5, 2018

@bosilca Take a look now. I added additional functions to opal_framework.h and automated the teardown of frameworks. Now opal_finalize() and opal_finalize_util() simply cleanup their finalize domain and nothing (util does have the class teardown but that has to happen last).

@hjelmn
Copy link
Member Author

hjelmn commented Dec 5, 2018

@jsquyres This will become the underpinning of the ompi_mpi_init/ompi_mpi_finalize cleanup. I generalized it so that we can have multiple finalize domains.

/* As the synonyms are just copies of the internal data we should not free them.
* Anyway they are over the limit of OPAL_DATATYPE_MAX_PREDEFINED so they will never get freed.
*/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s invert these 2 so that the destruction of the master convertors could use the dedicated opal_datatype_dfd.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, makes sense. Will make that change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ibm-ompi
Copy link

ibm-ompi commented Dec 5, 2018

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/ea8b18f8e7fc6c34782bd59ad6386737

@ibm-ompi
Copy link

ibm-ompi commented Dec 5, 2018

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/ba1fc09a3acc92a21daea07ca92aa0f8

@ibm-ompi
Copy link

ibm-ompi commented Dec 5, 2018

The IBM CI (PGI Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/0c6db4d4102aa47c478df533dd508377

@ibm-ompi
Copy link

ibm-ompi commented Dec 5, 2018

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/949a76a8a82edeb39d22e0cf1aa02c88

@hjelmn
Copy link
Member Author

hjelmn commented Dec 6, 2018

@jsquyres Any comments?

@ibm-ompi
Copy link

ibm-ompi commented Dec 6, 2018

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/f73b2265afb7b494902c89e84f6de2f3

@ibm-ompi
Copy link

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/56a7a2219bf9069a5ca08b51c510e442

This commit contains the following changes:

 - Remove the unused opal_test_init/opal_test_finalize
   functions. These functions are not used by anything in the code
   base or MTT. Tests use opal_init_util/opal_finalize_util instead.

 - Get rid of gotos in opal_init_util and opal_init. Replaced them
   with a cleaner solution.

 - Automatically register cleanup functions in init functions. The
   cleanup functions are executed in the reverse order of the
   initialization functions. The cleanup functions are run in
   opal_finalize_util() before tearing down the class system.

Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
@hjelmn hjelmn merged commit 4944508 into open-mpi:master Dec 18, 2018
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.

3 participants