Skip to content
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.

Sign up for GitHub

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

Jump to bottom

prov/efa, fabtests: Implement FI_MORE for fi_recv #10532

Closed
wants to merge 3 commits into from
Closed

prov/efa, fabtests: Implement FI_MORE for fi_recv #10532

wants to merge 3 commits into from

Conversation

jiaxiyan
Copy link
Contributor

Implement FI_MORE for fi_recv in zero-copy recv mode.
Allow tests with FI_MORE flag by using fi_recvmsg.
Add FI_MORE pytest for fi_recv in test_rdm_bw_zcpy_recv_use_fi_more.

@jiaxiyan jiaxiyan requested a review from a team November 13, 2024 00:12
shijin-aws
shijin-aws reviewed Nov 13, 2024
View reviewed changes
prov/efa/src/rdm/efa_rdm_pke.c

assert(pke_cnt);

ep = pke_vec[0]->ep;
assert(ep);

wr_index = ep->base_ep.efa_recv_wr_index;

for (i = 0; i < pke_cnt; ++i) {
Copy link
Contributor

@shijin-aws shijin-aws Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Another approach may be having efa_rdm_ep_post_internal_rx_pkts call this function in a loop of pkt_cnt times, where the first pkt_cnt-1 times are with FI_MORE. So we can get rid of this loop inside

But this version should be good as well... We can revisit it later if necessary.

@shijin-aws
Copy link
Contributor

@j-xiong can you review the fabtests change ad408c1, thanks!

@jiaxiyan jiaxiyan mentioned this pull request Nov 13, 2024
Merged
@shijin-aws
Copy link
Contributor

Some offline discussion shows that this PR will cause issue when both internal rx buffer and user fi_recv calling efa_rdm_pke_recvv as the efa_recv_wr_vec are shared among both mode and we don't want internal rx buffer post calls ibv_post_recv for the earlier staged wr to the default qp. This PR needs more work to address this issue.

@jiaxiyan jiaxiyan force-pushed the write_fi_more branch 2 times, most recently from 91c10ea to 0eec1b0 Compare November 14, 2024 00:30
shijin-aws
shijin-aws reviewed Nov 18, 2024
View reviewed changes
prov/efa/src/rdm/efa_rdm_pke.c
ep = pke_vec[0]->ep;
assert(ep);

wr_index = ep->base_ep.recv_wr_index;
Copy link
Contributor

Choose a reason for hiding this comment

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

need a check for the wr_index as I commented earlier. But for this code path, we already have a protection in #10538 (we have another pkt pool of fixed rx size). So having an assert here should be enough.

@jiaxiyan
prov/efa: Implement FI_MORE for fi_recv in zero copy recv mode
7296e76 
Track the last index of wr with recv_wr_index. Append wr when
FI_MORE is set. Only post recv when FI_MORE is not set.

Signed-off-by: Jessie Yang <[email protected]>
@jiaxiyan
fabtests: Allow tests with FI_MORE flag by using fi_recvmsg
e177b8b 
Also fix a bug in ft_recvmsg that uses tx_seq tag.

Signed-off-by: Jessie Yang <[email protected]>
@jiaxiyan
fabtests: Add FI_MORE pytest for fi_recv in zcpy recv mode
d8812f0 
Signed-off-by: Jessie Yang <[email protected]>
@shijin-aws
Copy link
Contributor

Need a rebase

@jiaxiyan
Copy link
Contributor Author

Included in #10527

@jiaxiyan jiaxiyan closed this Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shijin-aws shijin-aws shijin-aws approved these changes

@j-xiong j-xiong j-xiong approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jiaxiyan @shijin-aws @j-xiong