-
Notifications
You must be signed in to change notification settings - Fork 371
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
prov/shm: add iface parameter to smr_select_proto #8849
Conversation
{ | ||
if (op == ofi_op_read_req) { | ||
if (use_ipc) | ||
return smr_src_ipc; | ||
if (cma_avail && FI_HMEM_SYSTEM) |
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 guess cma_avail && FI_HMEM_SYSTEM
does not make much sense. Are we trying to test cma_avail && FI_HMEM_SYSTEM == iface
instead?
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.
Yeah definitely a bug that this fixes. Thank you!
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.
Thanks for confirming!
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.
Code change looks good. Would add something to the commit message about fixing/reenabling CMA for reads. Can you backport this as well please? Thank you for catching!
{ | ||
if (op == ofi_op_read_req) { | ||
if (use_ipc) | ||
return smr_src_ipc; | ||
if (cma_avail && FI_HMEM_SYSTEM) |
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.
Woah, that's a big miss. Can you add something in the commit message about how this fixes a bug where CMA would never be selected for a read?
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.
Thanks. Updated commit message. I will backport the change to 1.18.x branch.
Do we need it for 1.17.x as well?
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.
Well the only real use case of shm is efa so I'll let you decide how far back you want to go. The issue technically goes all the way back to 1.15.x
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.
Cool then we only need to backport to 1.18.x
Pass through hmem iface to proto selection logic. This change re-enables CMA for read ops. Signed-off-by: Wenduo Wang <wenduwan@amazon.com>
3f4c274
to
c1de6b8
Compare
bot:aws:retest |
Please back port |
Pass through hmem iface to proto selection logic.