Skip to content

Remove abort path from a failed rdma registration in gasnet#346

Open
muraj wants to merge 1 commit intomainfrom
cperry/gasnet-rdma-abort
Open

Remove abort path from a failed rdma registration in gasnet#346
muraj wants to merge 1 commit intomainfrom
cperry/gasnet-rdma-abort

Conversation

@muraj
Copy link
Copy Markdown
Contributor

@muraj muraj commented Oct 31, 2025

No description provided.

@muraj muraj self-assigned this Oct 31, 2025
@muraj muraj added bug Something isn't working 12.25 release labels Oct 31, 2025
@muraj muraj requested review from eddy16112 and manopapad October 31, 2025 20:37
@muraj
Copy link
Copy Markdown
Contributor Author

muraj commented Oct 31, 2025

@marcinz this should remove the crash for issue #339

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 31, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 27.15%. Comparing base (8ce6b72) to head (8d480c7).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff            @@
##             main     #346     +/-   ##
=========================================
  Coverage   27.15%   27.15%             
=========================================
  Files         190      190             
  Lines       39174    39174             
  Branches    14170    14359    +189     
=========================================
  Hits        10637    10637             
- Misses      27128    28144   +1016     
+ Partials     1409      393   -1016     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@muraj muraj force-pushed the cperry/gasnet-rdma-abort branch 2 times, most recently from 53000a0 to 0a54cbe Compare November 4, 2025 00:08
@muraj muraj force-pushed the cperry/gasnet-rdma-abort branch from 0a54cbe to 8d480c7 Compare November 4, 2025 00:09

if(gex_wrapper_handle.gex_ep_bind_segment(ep, segment, 0 /*flags*/) != 0) {
// TODO: destroy the segment and ep we created?
return false;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's add a warning message here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why, so application can just disable it? Enabling gasnet warnings when debugging this is enough for now I think.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Enabling gasnet warnings when debugging this is enough for now I think.

We do not have warnings if bind segment is failed.

@muraj muraj marked this pull request as ready for review November 4, 2025 19:33
@muraj muraj requested a review from eddy16112 November 4, 2025 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

12.25 release bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants