prov/efa: Fail early in rma when > 1 iov is passed #11429

shijin-aws · 2025-09-19T22:30:38Z

efa-direct offloads the rma request to rdma WQEs in efa device directly. But today efa device doesn't support > 1 sge per WQE. This patch makes efa_post_read and efa_post_write fail early with clear error message for this scenario to avoid unexpected crashing.

amitrad-aws · 2025-09-21T10:51:53Z

prov/efa/src/efa_rma.c


-	assert(msg->iov_count > 0 &&
-	       msg->iov_count <= base_ep->domain->info->tx_attr->iov_limit);
+	if (OFI_UNLIKELY(msg->iov_count > 1)) {


There is "max_sge_rd" inside efa_device.ibv_attr that should hold this value.
We should use it to be future compatible

Thank you! I will use it

Think about it more, the current data path direct code is already hard coded to only support 1 sge https://github.com/ofiwg/libfabric/blob/main/prov/efa/src/efa_io_defs.h#L125-L129. Though it is the same code as rdma-core and the firmware code today, if rdma-core and firmware change in the future and make max_sge_rd returned by rdma-core increase in the future, we will need to accommodate in libfabric to follow up, otherwise we will get unexpected crash. In that regard maybe having a hard-coded 1 limit is better.

WDYT @amitrad-aws @mrgolin

Per offline discussion, we will introduce a defined macro for libfabric internally and use the min of the macro and the rdma-core return value

Please do this only when using Libfabric datapath implementation, otherwise it should be transparent.

It will make non-datapath-direct not fail early with the same warning message, though rdma-core itself is checking it and return EINVAL, but it's not in libfabric's logging system and users won't know what happened immediately

shijin-aws · 2025-09-24T17:45:04Z

The latest change broke unit test on platforms that don't support rdma, will update

Zhenye-Na · 2025-09-24T23:44:26Z

bot:aws:retest

shijin-aws · 2025-09-24T23:47:42Z

bot:aws:retest

sunkuamzn

fi_read can only provide a single buffer. fi_readv and fi_readmsg can provide multiple IOVs

So I think we should just implement our readv and readmsg to iterate over all IOV and post multiple read/write requests. Similarly for the write APIs.

sunkuamzn · 2025-09-26T21:23:28Z

prov/efa/src/efa_rma.c


-	assert(msg->iov_count > 0 &&
-	       msg->iov_count <= base_ep->domain->info->tx_attr->iov_limit);
+	if (OFI_UNLIKELY(msg->iov_count > base_ep->domain->device->ibv_attr.max_sge_rd)) {


This check happens even for fi_read calls which are guaranteed to only provide a single IOV. If we don't want to iterate and want the check, it should be in efa_rma_readv, efa_rma_readmsg and the equivalent functions for write and RDM protocol paths

RDM protocol path doesn't have such issue, it currently only construct 1 WQE per IOV

The RDM path looks messy

We have asserts like this

libfabric/prov/efa/src/rdm/efa_rdm_ope.c

Line 1478 in 223d0b6

assert(ope->iov_count > 0 && ope->iov_count <= efa_rdm_ep_domain(ep)->info->tx_attr->iov_limit);

But we also copy the iov_cnt

libfabric/prov/efa/src/rdm/efa_rdm_rma.c

Line 66 in 223d0b6

msg.iov_count = msg_rma->iov_count;

libfabric/prov/efa/src/rdm/efa_rdm_ope.c

Line 42 in 223d0b6

txe->iov_count = msg->iov_count;

But if you read the logic below in the while loop

libfabric/prov/efa/src/rdm/efa_rdm_ope.c

Line 1568 in 223d0b6

write_once_len = MIN(ope->iov[iov_idx].iov_len - iov_offset,

It actually only run 1 iov per efa_rdm_pke_write, the code is really messy.

The txe and msg can have multi iov, it will finally aggregate all the pkes from it

Yeah I saw that. Because we iterate and post a single IOV at a time, we should remove the asserts.

But the asserts you referred to are the ope/msg ones, which should still be restricted by the tx iov limit...

Should they? I'm not sure why we should have asserts that are not enforced by the code that follows. That just leads to different behavior with debug and production builds

If we want to restrict the number of IOVs to tx_iov_limit, then we need a check in production builds too.

I think they should be restricted as it is what we returned for the tx iov limit in fi_getinfo. We can add a check for sure

efa-direct offloads the rma request to rdma wqes in efa device directly. But today efa device doesn't support > 1 iov (queried as ibv_device_attr.max_sge_rd). This patch makes efa_post_read and efa_post_write fail early with clear error message for this scenario to avoid unexpected crashing. Signed-off-by: Shi Jin <[email protected]>

amitrad-aws · 2025-09-30T08:09:16Z

prov/efa/test/efa_unit_test_rma.c

 	/* Add rma caps explicitly to ep->info to allow local test */
 	base_ep->info->caps |= FI_RMA;
+	/* Mock the max rdma sge to allow local test */
+	base_ep->domain->device->ibv_attr.max_sge_rd = EFA_DEVICE_MAX_RDMA_SGE;


If you really want to test your minimum you shuold put a larger value

mrgolin · 2025-10-05T14:48:32Z

prov/efa/src/efa_rma.c

 	if (err)
 		return err;

+	err = efa_rma_check_iov_count(base_ep, msg->iov_count);


I think this should be called from efa_data_path_direct_wr_set_sge_list or adjacent to the calls to this function since 1) it's directly related to WQE format, 2) we should avoid enforcing Libfabric hardcoded limits when using rdma-core data path.

shijin-aws requested a review from a team September 19, 2025 22:31

amitrad-aws reviewed Sep 21, 2025

View reviewed changes

shijin-aws force-pushed the rdma_iov_limit branch from 8e25874 to 59e87bf Compare September 23, 2025 21:18

shijin-aws requested a review from amitrad-aws September 23, 2025 21:19

shijin-aws force-pushed the rdma_iov_limit branch from 59e87bf to 2e1bab2 Compare September 24, 2025 20:29

shijin-aws added evaluating ⚠️ Do not merge and removed evaluating labels Sep 25, 2025

sunkuamzn reviewed Sep 26, 2025

View reviewed changes

shijin-aws force-pushed the rdma_iov_limit branch from 2e1bab2 to 42f45ac Compare September 29, 2025 21:50

shijin-aws requested a review from sunkuamzn September 29, 2025 21:50

shijin-aws force-pushed the rdma_iov_limit branch 2 times, most recently from da794db to 8c123b7 Compare September 29, 2025 21:52

shijin-aws force-pushed the rdma_iov_limit branch from 8c123b7 to ddf8745 Compare September 29, 2025 21:56

amitrad-aws approved these changes Sep 30, 2025

View reviewed changes

shijin-aws removed the ⚠️ Do not merge label Oct 1, 2025

mrgolin reviewed Oct 5, 2025

View reviewed changes

prov/efa: Fail early in rma when > 1 iov is passed #11429

Are you sure you want to change the base?

prov/efa: Fail early in rma when > 1 iov is passed #11429

Conversation

shijin-aws commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shijin-aws Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shijin-aws Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shijin-aws commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zhenye-Na commented Sep 24, 2025

Uh oh!

shijin-aws commented Sep 24, 2025

Uh oh!

sunkuamzn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunkuamzn Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shijin-aws commented Sep 19, 2025 •

edited

Loading

shijin-aws Sep 25, 2025 •

edited

Loading

shijin-aws Sep 30, 2025 •

edited

Loading

shijin-aws commented Sep 24, 2025 •

edited

Loading

sunkuamzn left a comment •

edited

Loading

sunkuamzn Sep 26, 2025 •

edited

Loading