Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MPI_T Events #8057

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

MPI_T Events #8057

wants to merge 23 commits into from

Conversation

cchambreau
Copy link

This PR includes Nathan Hjelm's reworking of PERUSE functionality to implement the MPI_T Events API as specified in the MPI 4.0 proposal. Subsequent minor changes to the MPI_T Events API were made and included here. The API and an initial set of events were tested against Marc-André Hermann's MEL tracing tool and other test cases. History, a description of the API and experience with tools can be found in "Enabling callback-driven runtime introspection via MPI_T."

@ompiteam-bot
Copy link

Can one of the admins verify this patch?

@jsquyres
Copy link
Member

ok to test

@gpaulsen
Copy link
Member

@cchambreau, Hi. Is this desirable for Open MPI v5.0 (not MPI-4.0 compliant)? If so please, rebase it and get it into master and then cherry-pick to v5.0.x branch after it's in master.

jsquyres
jsquyres reviewed Jul 29, 2021
View reviewed changes
Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

Can you rebase and fix all current conflicts?

Do we have any tests for this MPI_T event functionality?

ompi/mpi/tool/event_get_info.c Outdated Show resolved Hide resolved
@hppritcha
Copy link
Member

@cchambreau could you please rebase? thanks.

@hppritcha hppritcha self-assigned this Nov 16, 2021
@hppritcha
Copy link
Member

Howard will rebase and merge unless objections are raised.

jsquyres
jsquyres reviewed Dec 5, 2021
View reviewed changes
ompi/mpi/tool/event_get_info.c Outdated Show resolved Hide resolved
ompi/mpi/tool/event_get_info.c Outdated Show resolved Hide resolved
ompi/mpi/tool/event_get_info.c Outdated Show resolved Hide resolved
ompi/mpi/tool/event_handle_alloc.c Outdated Show resolved Hide resolved
ompi/mpi/tool/event_handle_free.c Outdated Show resolved Hide resolved
opal/mca/base/mca_base_event.c Outdated Show resolved Hide resolved
opal/mca/base/mca_base_event.c Outdated Show resolved Hide resolved
opal/mca/base/mca_base_event.c Outdated Show resolved Hide resolved
opal/mca/base/mca_base_event.c Outdated Show resolved Hide resolved
opal/mca/base/mca_base_event.c Outdated Show resolved Hide resolved
@ibm-ompi
Copy link

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

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

@ibm-ompi
Copy link

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

Gist: https://gist.github.com/20f1009df186bd52f322544e39a3595c

@cchambreau cchambreau mentioned this pull request Dec 24, 2021
Merged
@jsquyres jsquyres mentioned this pull request Jan 1, 2022
Merged
@jsquyres
Copy link
Member

jsquyres commented Jan 1, 2022

@cchambreau can you fix the conflicts on this PR? Thanks!

jsquyres
jsquyres requested changes Jan 4, 2022
View reviewed changes
Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

In addition to the other comments on this PR, please update this PR to use opal_clock_gettime() (and, if necessary, opal_clock_getres()) from #9798.

@hppritcha hppritcha force-pushed the topic/mpi_t_events branch 2 times, most recently from 3ed1cd9 to 14ccc8d Compare January 7, 2022 22:11
@hppritcha
Copy link
Member

bot:ompi:retest

@hppritcha hppritcha requested a review from jsquyres January 19, 2022 02:23
jsquyres
jsquyres reviewed Jan 23, 2022
View reviewed changes
ompi/mpi/tool/event_callback_get_info.c Outdated Show resolved Hide resolved
ompi/mpi/tool/event_callback_get_info.c Show resolved Hide resolved
ompi/mpi/tool/event_get_info.c Outdated Show resolved Hide resolved
ompi/mpi/tool/event_get_info.c Outdated Show resolved Hide resolved
ompi/mpi/tool/event_handle_alloc.c Outdated Show resolved Hide resolved
ompi/mpi/tool/source_get_num.c Show resolved Hide resolved
opal/mca/base/mca_base_event.h Show resolved Hide resolved
opal/mca/base/mca_base_source.c Show resolved Hide resolved
opal/mca/base/mca_base_source.c Outdated Show resolved Hide resolved
opal/mca/base/mca_base_source.c Show resolved Hide resolved
@cchambreau
Copy link
Author

@jquyres I'm hoping to get back to this in April.

hjelmn and others added 23 commits December 21, 2023 15:32
@hjelmn @hppritcha
Modify PERUSE code to implement MPI_T Events based on MPI 4.0 proposal.
9fd3e4e 
Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
@cchambreau @hppritcha
Modifications for changes to proposed MPI_T Events API as well as min…
ed1aead 
…or fixes.

Signed-off-by: Chris Chambreau <chambreau1@llnl.gov>
@hppritcha
remove include of dss.h
1564ea4 
dss.h has been removed since this PR was originally opened

Signed-off-by: Howard Pritchard <howardp@lanl.gov>
@cchambreau @hppritcha
Remove non-conformant event_get_info extent argument and internal tra…
45fe4a0 
…cking.

Signed-off-by: Chris Chambreau <chambreau1@llnl.gov>
@cchambreau @hppritcha
Add user_data argument to MPI_T_event_handle_free function.
9350bee 
Signed-off-by: Chris Chambreau <chambreau1@llnl.gov>
@cchambreau @hppritcha
Fix generation of event_get_info displacements and related copy_data …
a0a398e 
…functionality

Signed-off-by: Chris Chambreau <chambreau1@llnl.gov>
@cchambreau @hppritcha
Minor cleanup
4b953c4 
- Correct MPI_T_event_dropped_cb_function arguments
- Check for negative event index

Signed-off-by: Chris Chambreau <chambreau1@llnl.gov>
@hppritcha
mpi_t events: convert to new profiling approach
2152799 
Signed-off-by: Howard Pritchard <howardp@lanl.gov>
@hppritcha
mpi_t events fix some missing files for
7d81266 
new profile interface generation method

Signed-off-by: Howard Pritchard <howardp@lanl.gov>
@hppritcha
pr review feedback
50b84fc 
Signed-off-by: Howard Pritchard <howardp@lanl.gov>
@hppritcha
pr feedback - switch to opal_clock_gettime
fb8a5f5 
Signed-off-by: Howard Pritchard <howardp@lanl.gov>
@hppritcha
suppress a compiler warning
f4ffec2 
by fixing some code

Signed-off-by: Howard Pritchard <howardp@lanl.gov>
@cchambreau @hppritcha
pr feedback - remove assert
6cf3312 
Signed-off-by: Chris Chambreau <chambreau1@llnl.gov>
@cchambreau @hppritcha
pr feedback - corrected comment references to events
12dca5d 
Signed-off-by: Chris Chambreau <chambreau1@llnl.gov>
@cchambreau @hppritcha
pr feedback - added locking for setting dropped event handler
e6a0389 
Signed-off-by: Chris Chambreau <chambreau1@llnl.gov>
@cchambreau @hppritcha
pr feedback - copyright consistency for all event files
7e617d4 
Signed-off-by: Chris Chambreau <chambreau1@llnl.gov>
@cchambreau @hppritcha
Protect source data access with thread locking.
d91a7ee 
Signed-off-by: Chris Chambreau <chambreau1@llnl.gov>
@cchambreau @hppritcha
Add OPAL_LIKELY
35e202a 
Signed-off-by: Chris Chambreau <chambreau1@llnl.gov>
@cchambreau @hppritcha
Correct OSC event descriptions.
88c7717 
Signed-off-by: Chris Chambreau <chambreau1@llnl.gov>
@cchambreau @hppritcha
Ensure consistency between ompi and opal callback safety requirement
9ac3f5f 
values.

Signed-off-by: Chris Chambreau <chambreau1@llnl.gov>
@cchambreau @hppritcha
Ensure thread safety for incremented variables.
8853c60 
Signed-off-by: Chris Chambreau <chambreau1@llnl.gov>
@cchambreau @hppritcha
Add function to lookup ompi datatype from opal datatype id.
750af0c 
Signed-off-by: Chris Chambreau <chambreau1@llnl.gov>
@hppritcha
fix compile problem after rebase
66b29d5 
Signed-off-by: Howard Pritchard <howardp@lanl.gov>
@hppritcha
Copy link
Member

@cchambreau i'm trying to get this PR fixed up for merging so did a rebase and pushed to your repo.

@hppritcha
Copy link
Member

Discussed some at the 2024 F2F. Not sure who really needs this. Joseph will ping scorep people about whether they may want/need this feature.

@jsquyres
Copy link
Member

Is it really a question of who needs this?

I.e., isn't this part of conformance to the MPI standard?

@hppritcha
Copy link
Member

Some tools folks were pinged about use of MPI_T events as part of Open MPI devel f2f meeting 4/2024. ScoreP developer said its on their todo list to use this feature in some future release. Tau may already use events if the MPI being used supports it. Possibly some unreleased version of MPI Advisor can use them.

@kingshuk00
Copy link

It would be nice to have this standard-compliant interface from tools perspective.
I have posted another PR PR-6 on the source branch of this PR. This fixes a potential problem that is captured in this test events_callbacks.c. This should be there for correctness.

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

@hppritcha hppritcha hppritcha left review comments

@bosilca bosilca bosilca left review comments

@jsquyres jsquyres jsquyres requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@hppritcha hppritcha

Labels
enhancement MPI-4.0 Target: main
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove PERUSE in favour of upcoming MPI_T pvar implementation