Repository navigation
UCP/RNDV/ROCM: add support for staging rndv protocol for ROCm - #5742
Conversation
|
Can one of the admins verify this patch? |
|
ok to test
…On Fri, Sep 25, 2020 at 2:44 PM swx-jenkins3 ***@***.***> wrote:
Can one of the admins verify this patch?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#5742 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AARB5WCXO344OI2ZCRI3KMLSHTXKNANCNFSM4R2J2MBQ>
.
|
|
Please add WIP-DNM label |
99ceacb to
f1c80f7
Compare
|
@yosefe @bureddy Can you please explain what this error means? I am not able to view the report. |
|
Is is coverity error, you can find the details by using the link you provided (copying it here just in case). |
f1c80f7 to
6419267
Compare
|
The failure seems unrelated @bureddy can you please remove WIP-DNM label and review the PR? |
|
bot:pipe:retest |
|
@souravzzz please resolve conflicts |
6419267 to
690c1f9
Compare
|
Force pushed 690c1f9 to resolve conflicts |
|
Will retest after #5797 is merged |
690c1f9 to
a877c3f
Compare
|
bot:pipe:retest |
|
relevant error |
| } | ||
|
|
||
| key->vaddr = packed->vaddr; | ||
| key->dev_ptr = packed->dev_ptr; |
|
@yosefe Should I do a force push to resolve the conflicts or wait for your review? |
| if (rkey_p != NULL && *rkey_p == NULL && | ||
| md_attr->cap.flags & UCT_MD_FLAG_NEED_RKEY) | ||
| { | ||
| status = ucp_rkey_pack(mem_type_ep->worker->context, mdesc->memh, | ||
| &rkey_buffer, &rkey_size); | ||
| ucs_assert_always(status == UCS_OK); | ||
|
|
||
| status = ucp_ep_rkey_unpack(mem_type_ep, rkey_buffer, rkey_p); | ||
| ucs_assert_always(status == UCS_OK); | ||
| ucp_rkey_buffer_release(rkey_buffer); | ||
|
|
||
| ucs_assert_always(rkey_index != NULL); | ||
| memset(rkey_index, 0, UCP_MAX_LANES * sizeof(uint8_t)); | ||
| } |
There was a problem hiding this comment.
It's setting the freq->send.rndv_get.rkey to the rkey for the frag buffer so that transports that need rkey (e.g. rocm_copy) can do the D2H transfer. cuda_copy didn't need rkey so previously it was set to NULL.
(The frag buffers are registered with the memtype transports when they are allocated. This was the best way I found to retrieve the rkey.)
There was a problem hiding this comment.
can we set the rkey when fragment buffer is created and save it on mdesc?
There was a problem hiding this comment.
@yosefe I looked into this approach but not sure how feasible that is. My understanding is that the mdesc stores the list of uct mem handles for different mds and the ucp_rkey is created at runtime by calling ucp_rkey_pack and passing in the storage. We can probably have a pointer to the rkey inside ucp_mem_t struct but I'm not sure if that's a good idea because managing that memory becomes a problem. Thoughts?
There was a problem hiding this comment.
we can manage the lifetime of rkey handle same way as we do for memh itself
maybe need a set of ucp_mpool_malloc/free functions for rndv frag pool, which will have rkey handle in addition to memh
There was a problem hiding this comment.
we'd like to avoid rkey pack/unpack for every fragment, if possible, for performance reason
There was a problem hiding this comment.
Is it okay to push this improvement in a follow up PR? a) This codepath doesn't get triggered for non-ROCm memory and b) the performance is bottlenecked by rocm copy performance which is much larger than cost of packing rkey.
There was a problem hiding this comment.
@yosefe I have rebased onto latest master and squashed the changes.
0c38ce0 to
ea5aaa2
Compare
ea5aaa2 to
973003e
Compare
|
@souravzzz PR is conflicting with master |
973003e to
0ed167e
Compare
|
@hoopoepg Resolved conflicts and rebased on latest master |
724732c to
ad110e4
Compare
|
Updated to rebase on latest master and fix conflict with 583b37f |
@souravzzz pls avoid force push during review, use only merge and not rebase, force push doesn't allow to review only the changes since last review cycle |
|
RNDV common part is good now, only minor style comment on ROCM code |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
00de81e to
697b0a0
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
What
Add support for host-staged transfers for ROCm D2D instead of GPUDirect RDMA
Why ?
Improves performance on platforms where GDR read performance is poor
How ?
hsa_amd_memory_async_copytorocm_copytransportNEED_RKEYflag and registration cache support torocm_copymd@bureddy @yosefe