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.

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/efa: Remove fi_sendmsg/recvmsg but still use efa_ep's qp and av #8391

Merged
merged 2 commits into from
Jan 5, 2023

Conversation

sunkuamzn
Copy link
Contributor

For SHM EP, use fi_sendv and fi_recvv
For EFA EP

  • Allocate ibv_send_wr and ibv_recv_wr in rxr_pkt_entry
  • Still use efa_ep->qp, efa_ep->av and efa_ep->xmit_more_wr*
  • Directly call efa_post_flush or ibv_post_recv in rxr_pkt_entry.c

Signed-off-by: Sai Sunku sunkusa@amazon.com

Copy link
Contributor

@wzamazon wzamazon left a comment

Choose a reason for hiding this comment

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

With the change to efa_send_wr and efa_recv_wr you need to update how efa_ep->send_wr_bufpool and efa_ep->recv_wr_pool is created. Specifically the entry size should be just sizeof(efa_send_wr) and sizeof(efa_recv_wr)

@sunkuamzn
Copy link
Contributor Author

With the change to efa_send_wr and efa_recv_wr you need to update how efa_ep->send_wr_bufpool and efa_ep->recv_wr_pool is created. Specifically the entry size should be just sizeof(efa_send_wr) and sizeof(efa_recv_wr)

But the DGRAM provider needs memory for the SGEs if a higher applications calls the DGRAM provider directly

@wzamazon
Copy link
Contributor

wzamazon commented Jan 4, 2023

But the DGRAM provider needs memory for the SGEs if a higher applications calls the DGRAM provider directly

I do not understand the comment. Current code is:

ret = ofi_bufpool_create(&ep->send_wr_pool,

ret = ofi_bufpool_create(&ep->send_wr_pool,
		sizeof(struct efa_send_wr) +
		user_info->tx_attr->iov_limit * sizeof(struct ibv_sge),
		16, 0, 1024, 0);

The reason to add user_info->tx_attr->iov_limit * sizeof(struct ibv_sge) is because original efa_send_wr does not allocate space for ibv_sge, so when the bufpool added some space to the end of efa_send_wr. Now that you declared 2 sge. There is no need to add that. (Maybe add an assertion about user_info->tx_attr->iov_limit <=2)

@sunkuamzn
Copy link
Contributor Author

The reason to add user_info->tx_attr->iov_limit * sizeof(struct ibv_sge) is because original efa_send_wr does not allocate space for ibv_sge

OK I understand now. Fixed.

@sunkuamzn sunkuamzn force-pushed the ep-squash-3-1 branch 2 times, most recently from 65ef34a to 62df336 Compare January 4, 2023 15:57
For SHM EP, use fi_sendv and fi_recvv
For EFA EP
* Allocate ibv_send_wr and ibv_recv_wr in rxr_pkt_entry
* Still use efa_ep->qp, efa_ep->av and efa_ep->xmit_more_wr*
* Directly call efa_post_flush or ibv_post_recv in rxr_pkt_entry.c

Signed-off-by: Sai Sunku <sunkusa@amazon.com>
This variable always has the same value as the loop variable

Signed-off-by: Sai Sunku <sunkusa@amazon.com>
@wzamazon wzamazon merged commit 963951d into ofiwg:main Jan 5, 2023
sunkuamzn added a commit to sunkuamzn/libfabric that referenced this pull request Jan 10, 2023
This commit fixes a bug that was introduced in
ofiwg#8391

Signed-off-by: Sai Sunku <sunkusa@amazon.com>
sunkuamzn added a commit to sunkuamzn/libfabric that referenced this pull request Jan 11, 2023
This commit fixes a bug that was introduced in
ofiwg#8391

Signed-off-by: Sai Sunku <sunkusa@amazon.com>
sunkuamzn added a commit to sunkuamzn/libfabric that referenced this pull request Jan 12, 2023
This commit fixes a bug that was introduced in
ofiwg#8391

Signed-off-by: Sai Sunku <sunkusa@amazon.com>
sunkuamzn added a commit to sunkuamzn/libfabric that referenced this pull request Jan 13, 2023
This commit fixes a bug that was introduced in
ofiwg#8391

Signed-off-by: Sai Sunku <sunkusa@amazon.com>
@sunkuamzn sunkuamzn deleted the ep-squash-3-1 branch September 19, 2023 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants