Fix for GPU-aware MPI bug by bmhan12 · Pull Request #1595 · llnl/axom

bmhan12 · 2025-06-26T21:13:18Z

This PR:

Fixes bug related to enabling GPU-aware MPI.
More on the issue and temporary workaround can be found here (thanks to @gberg617 for opening the issue and creating a reproducer!): https://rzlc.llnl.gov/jira/browse/ELCAP-851
~~- (Tentatively?) Added a CI job on tioga that replicates the bug conditions. Job run illustrating the bug: https://lc.llnl.gov/gitlab/axom/axom/-/jobs/2706859~~

bmhan12 · 2025-06-26T21:18:40Z

+tioga-rocmcc_6_3_1_hip-gpu-aware-src:
+  variables:
+    COMPILER: "rocmcc@6.3.1_hip"
+    HOST_CONFIG: "tioga-toss_4_x86_64_ib_cray-${COMPILER}.cmake"
+    ALLOC_TIME: "60"
+    NUM_NODES: "2"
+  extends: .src_build_on_tioga
+  before_script:
+    - module load cmake/3.21.1
+    - export MPICH_GPU_SUPPORT_ENABLED=1


This is the CI job I added to replicate the failure. Notably this does require more than one node (2) allocation to reproduce the error.

My concern is there are only 4 nodes in the special partition CI queue: https://lc.llnl.gov/confluence/spaces/GITLAB/pages/785285769/Special+partitions+for+CI+batch+jobs, so adding this job will make an entire CI run take up most of the queue.
I am leaning towards removing the additional CI job from this PR for that reason.

@bmhan12 can you reproduce the error on two nodes? If so, then maybe we can re-enable the test when we move our CI off tioga to tuolumne where there may be more nodes available for CI. Hopefully, the issue will be fixed in Cray mpich -- I believe Guy submitted an issue with a reproducer that doesn't rely on Axom. Alternatively, we could try to modify the Lumberjack implementation that is causing the issue.

Yes, I was able reproduce the error with two nodes, and the workaround in this PR resolves the issue in Lumberjack. That's a good point, I could comment out this job, and move it to tuolumne once we're setup there.

However, from what I can see here: https://lc.llnl.gov/confluence/spaces/GITLAB/pages/785285769/Special+partitions+for+CI+batch+jobs
tuolumne will also only have 4 nodes in the CI queue (and I verified with sinfo that is in fact the case on the machine), which seems small relative to the total overall number of nodes.

We may be able to get LC to bump the number of CI nodes.

Commented out the GPI-aware MPI job for now.

white238 · 2025-06-26T22:17:23Z

-const char* const zeroMessage = "0";
+// GPU-aware MPI issue:
+// https://rzlc.llnl.gov/jira/browse/ELCAP-851
+// const char * zeroMessage = "0";


Is the idea that this will be reverted when it's fixed? If so, we should have a comment here to go delete the added lines.

If it were reverted, then it would cause variable shadowing with line 365 below.

Right, hence the desire for a comment =) I was assuming the reverting would also remove the added lines and a comment would stop that hopefully.

Is the idea that this will be reverted when it's fixed? If so, we should have a comment here to go delete the added lines.

I'm not sure what the cleanest way to organize this, so it is definitely messy.
Right now from the JIRA ticket discussion, I'm not sure if the problem that is causing this will get fixed soon or if the workaround is just the new reality.
Additionally, more importantly, I would like to preserve the comment above about zeroMessage :

/*! ***************************************************************************** * \brief Message to indicate no messages need to be sent from child node. ***************************************************************************** */

but without a snippet/comment block/??? it doesn't really make any sense by itself.
Maybe it would be better to have this comment for every zeroMessage variable?

Changed it to comment per zeroMessage declaration.

…claration

kennyweiss · 2025-06-27T17:40:02Z

- * \brief Message to indicate no messages need to be sent from child node.
- *****************************************************************************
- */
-const char* const zeroMessage = "0";


If I'm understanding the issue properly, would another possible fix be to keep the global variable zeroMessage as is, but copy it to a local variable before each usage?

(Not necessarily requesting any changes, just trying to understand the issue).

Yes, that should work, and the local variable should be a char array instead of a pointer (Following this bulletpoint from the JIRA ticket: use local variables (although for the CHAR type, local constant needs to be a char array))

Would it make sense to still have this pointer but point it to the storage that was added in Message.cpp with the same name? That way there is only one place that this is stored.

Would it make sense to still have this pointer but point it to the storage that was added in Message.cpp with the same name? That way there is only one place that this is stored.

I'll give this a try.

Based on previous discussion from yesterday, this current iteration has zeroMessage be a function that returns a std::string.

I tried the following:

// Message.hpp extern const char * zeroMessage; --------------------------------------------------------- // Message.cpp const char * zeroMessage = "0";

The same bug appears in this case. (Code-wise, I'm not sure if this is what you had in mind, if so, let me know!)

I was thinking something more like this.

// Message.hpp extern const char * const zeroMessage; --------------------------------------------------------- // Message.cpp char zeroMessageStorage[] = "0"; const char * const zeroMessage = zeroMessageStorage;

You could also use a std::array<char, 2> zeroMessage{"0"}; instead of a std::string to make it easy to make a local copy. Then use zeroMessage.data() when you need the pointer.

I was thinking something more like this.

Thanks @MrBurmark, that works and is pretty clean!

rhornung67 · 2025-06-30T23:23:03Z

@MrBurmark please take a look at this and offer suggestions/corrections, etc.

…ocation in packMessages for the case of a message of size 0 (zeroMessage is not in static storage)

kennyweiss · 2025-07-01T18:50:39Z

+    // Allocate space for zero and null character
+    char* zeroString = new char[zeroMessage().size() + 1];
+    std::memcpy(zeroString, zeroMessage().c_str(), zeroMessage().size());
+    zeroString[zeroMessage().size()] = '\0';
+    return zeroString;


@bmhan12 -- looks like this will leak memory each time it's called.

Can zeroMessage == "0" be a class member of Message? (or even better, a static class member?)

Also, we probably don't want an allocation each time this is called with no messages.

kennyweiss

Thanks @bmhan12 and @MrBurmark for working through this -- this is a nice solution

gberg617 · 2025-07-02T01:22:53Z

+
+// Global workaround: https://rzlc.llnl.gov/jira/browse/ELCAP-851
+char zeroMessageStorage[] = "0";
+const char* const zeroMessage = zeroMessageStorage;


Aren't you still treating zeroMessage as global data in this case? If I remember correctly, that was the main issue stated later in the ticket.

Aren't you still treating zeroMessage as global data in this case? If I remember correctly, that was the main issue stated later in the ticket.

I think we're still hitting one of the working cases in this scenario: "use global non-constant (for CHAR type this has to be either a char array or a pointer to malloc'ed memory)".

My understanding is that global constants and global non-constants are put in different sections of memory with different permissions. So it may only be the case that global constants are affected but global non-constants are not.

bmhan12 added 3 commits June 23, 2025 16:19

Attempt to reproduce gpu aware failures on tioga

33f480e

Extend alloc time

ec33a9e

Apply workaround for GPU-aware MPI

48d517d

bmhan12 added Slic Issues related to Axom's 'slic' component CI Issues related to continuous integration mpi Related to MPI communication labels Jun 26, 2025

bmhan12 commented Jun 26, 2025

View reviewed changes

bmhan12 requested review from gberg617, kennyweiss, rhornung67 and white238 June 26, 2025 21:20

white238 reviewed Jun 26, 2025

View reviewed changes

kennyweiss added Lumberjack Issues related to Axom's 'lumberjack' component bug Something isn't working labels Jun 26, 2025

Comment out GPU-aware MPI job for now, add zeroMessage comment per de…

411fd5f

…claration

kennyweiss approved these changes Jun 27, 2025

View reviewed changes

Merge branch 'develop' into ci/han12/gpu_aware

72e3978

gberg617 approved these changes Jun 30, 2025

View reviewed changes

Merge branch 'develop' into ci/han12/gpu_aware

3870067

Turn zeroMessage into a function that returns a string; create an all…

9545cd3

…ocation in packMessages for the case of a message of size 0 (zeroMessage is not in static storage)

adayton1 approved these changes Jul 1, 2025

View reviewed changes

kennyweiss reviewed Jul 1, 2025

View reviewed changes

kennyweiss self-requested a review July 1, 2025 18:51

bmhan12 added 2 commits July 1, 2025 16:05

Define zeroMessage in source, point there from header

4c4288d

Merge branch 'develop' into ci/han12/gpu_aware

d9368f7

white238 approved these changes Jul 1, 2025

View reviewed changes

kennyweiss approved these changes Jul 2, 2025

View reviewed changes

gberg617 reviewed Jul 2, 2025

View reviewed changes

Merge branch 'develop' into ci/han12/gpu_aware

3a46723

bmhan12 merged commit 2a75435 into develop Jul 2, 2025
15 checks passed

bmhan12 deleted the ci/han12/gpu_aware branch July 2, 2025 22:54

bmhan12 mentioned this pull request Aug 26, 2025

Gitlab CI - Add tuolumne #1648

Merged

Uh oh!

Conversation

bmhan12 commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmhan12 Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmhan12 Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

white238 Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmhan12 Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrBurmark Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhornung67 commented Jun 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennyweiss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bmhan12 commented Jun 26, 2025 •

edited

Loading

bmhan12 Jun 26, 2025 •

edited

Loading

bmhan12 Jun 26, 2025 •

edited

Loading

white238 Jun 26, 2025 •

edited

Loading

bmhan12 Jun 26, 2025 •

edited

Loading

MrBurmark Jul 1, 2025 •

edited

Loading