-
Notifications
You must be signed in to change notification settings - Fork 6
Provide put & get message passing primitives for the bsp1d backend #407
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.
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
base: develop
Are you sure you want to change the base?
Provide put & get message passing primitives for the bsp1d backend #407
Conversation
anyzelman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intermediary review request to double-check code structure -- and all looks good from that perspective to me=)) Also some minor comments I saw while going through it quickly just now
include/graphblas/bsp/rdma.hpp
Outdated
| const void* buf_void = reinterpret_cast< const void* >( buf ); | ||
|
|
||
| data.ensureMemslotAvailable( 1 ); | ||
| data.signalMemslotTaken(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one should come after line 62 (and only if that call returned SUCCESS probably?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree for after line 62, but I am not sure about the SUCCESS check. There are many kinds of possible errors that could happen, so I think it's safer to take a memslot in any case
include/graphblas/bsp/rdma.hpp
Outdated
|
|
||
| // data.ensureMemslotAvailable( 1 ); // this function calls lpf_sync | ||
| { | ||
| const auto it = data.registered_slots.find( reinterpret_cast< const void* >( dst ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it may be more robust to introduce a grb::Scalar type ...but maybe let's worry about that later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave this one open for the day I will actually implement this
|
Currently I register locally objects needed for put/get and then forget about them.
In both cases I will have to implement something to keep track of them. It might also happen that we a locally registered address is registered globally, and in that case I guess we should deregister locally before registering globally |
Resolves #405