prov/efa: Enable inject rdma write #9512

a-szegel · 2023-11-03T21:28:19Z

No description provided.

a-szegel · 2023-11-03T21:45:32Z

This file prov/efa/src/rdm/efa_rdm_pkt_type.c doesn't have a new line at the end... and it looks ugly in the diff so tried to add one to see if it made it look prettier, but it didn't, so then removed it. Also modified first commit message to be more clear.

sunkuamzn · 2023-11-03T22:13:23Z

prov/efa/src/rdm/efa_rdm_ope.c

@@ -1473,15 +1472,17 @@ int efa_rdm_ope_post_remote_write(struct efa_rdm_ope *ope)
 		if (OFI_UNLIKELY(!pkt_entry))
 			return -FI_EAGAIN;

+		/* Use bounce buffer (registered) for buffer/desc, user input doesn't matter */


Nit:

Use bounce buffer (registered) for buffer/desc, the buffer we pass to rdma-core doesn't matter

sounds better

didn't quite use your words exactly, but made this more clear.

sunkuamzn · 2023-11-03T22:25:06Z

prov/efa/src/rdm/efa_rdm_ope.c

@@ -1458,7 +1458,6 @@ int efa_rdm_ope_post_remote_write(struct efa_rdm_ope *ope)

 	assert(ope->iov_count > 0);
 	assert(ope->rma_iov_count > 0);
-	efa_rdm_ope_try_fill_desc(ope, 0, FI_WRITE);


What's the reasoning for avoiding an MR reg call for zero byte write?

efa_rdm_ope_try_fill_desc() calls fi_mr_regv() on the buffer. This line assumes that the user will pass a real registered buffer to do a 0 byte write. If you are doing a 0 byte write, then your buffer could be NULL, and your desc could be NULL b/c EFA shouldn't actually do anything with them. The fi_mr_regv() will segfault on NULL desc. Also segfaults for FI_INJECT which always gives a NULL desc.

The reason this is in its own commit is you could hit this bug and segfault via fi_write() with 0 bytes and an invalid buffer in main. I wanted to be explicit about fixing this bug is separate to adding fi_inject_write().

did you observe any performance impact for this commit?

Looks like a speedup usec/xfer 1.30 (before), .98 (after) for

"program": "/home/ec2-user/libfabric/fabtests/install/bin/fi_rma_bw", "args": ["-e", "rdm", "-o", "write", "-S", "10", "-p", "efa", "-E=9235"],

sunkuamzn · 2023-11-03T22:33:05Z

prov/efa/src/rdm/efa_rdm_ope.c

+			ope->desc[0] = fi_mr_desc(pkt_entry->mr);
+			ope->iov[0].iov_base = pkt_entry->wiredata + sizeof(struct efa_rdm_rma_context_pkt);
+		}
+
 		write_once_len = MIN(ope->iov[iov_idx].iov_len - iov_offset,
 				    ope->rma_iov[rma_iov_idx].len - rma_iov_offset);
 		write_once_len = MIN(write_once_len, max_write_once_len);


You copy the data from user buffer to the packet entry but the efa_rdm_pke_init_write_context call a few lines below is still using the user buffer

I hack this by:

ope->desc[0] = fi_mr_desc(pkt_entry->mr); ope->iov[0].iov_base = pkt_entry->wiredata + sizeof(struct efa_rdm_rma_context_pkt);

and that works b/c:

efa_rdm_pke_init_write_context( pkt_entry, ope, (char *) ope->iov[iov_idx].iov_base + iov_offset, write_once_len, ope->desc[iov_idx], ope->rma_iov[rma_iov_idx].addr + rma_iov_offset, ope->rma_iov[rma_iov_idx].key);

passes in ope->iov[iov_idx].iov_base + iov_offset and ope->desc[iov_idx], and it will only ever go around this loop once.

The txe has a separate cq_entry field that has the buffer and length data. That information is copied to txe when the txe is created. So changing the ope->iov doesn't affect the completion and this OK to do

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

Lines 85 to 86 in d43c492

txe->cq_entry.len = ofi_total_iov_len(txe->iov, txe->iov_count);

txe->cq_entry.buf = OFI_LIKELY(txe->cq_entry.len > 0) ? txe->iov[0].iov_base : NULL;

The non-inject 0 byte write path assumed that users would pass in valid buffers, and valid descriptors (memory registrations) for those buffers. The user should not have to pass in a valid buffer to do a 0 byte write. Use pre-registered bounce buffers for buffer/MR, to allow user to pass in whatever they choose. Signed-off-by: Seth Zegelstein <szegel@amazon.com>

Turns off emulated inject rdma write, and enables real inject rdma write. Inject rdma write is special for two reasons: 1. The user gets to re-use their immediately after we return 2. RDMA operations require MR's, and the inject write interfaces do not provide a memory descritpor In order to handle both cases, the users data is copied into a pre-registered bounce buffer. The bounce buffer is allocated to be the sizeof(struct efa_rdm_pke) + ep->mtu_size, where the wire data starts at ep->mtu_size. The RDMA operations use part of the wire data for a RDMA header struct efa_rdm_rma_context_pkt. This change needs to lower the maximum inject size to never be more than mtu_size - sizeof(struct efa_rdm_rma_context_pkt). I created a github issue for libfabric 2.0 API to seperate inject sizes for send/recv, atomic, and rdma operations (issue 9510). Signed-off-by: Seth Zegelstein <szegel@amazon.com>

shijin-aws · 2023-11-06T20:41:00Z

prov/efa/src/rdm/efa_rdm_ope.c

+		if (ope->fi_flags & FI_INJECT) {
+			assert(ope->iov_count == 1);
+			assert(ope->total_len <= ep->inject_size);
+			copied = ofi_copy_from_hmem_iov(pkt_entry->wiredata + sizeof(struct efa_rdm_rma_context_pkt),


line is too long

EFA doesn't enforce line length.

shijin-aws · 2023-11-06T20:50:13Z

prov/efa/src/rdm/efa_rdm_ope.c

+			assert(ope->iov_count == 1);
+			assert(ope->total_len <= ep->inject_size);
+			copied = ofi_copy_from_hmem_iov(pkt_entry->wiredata + sizeof(struct efa_rdm_rma_context_pkt),
+				ope->total_len, FI_HMEM_SYSTEM, 0, ope->iov, ope->iov_count, 0);


Sorry I am wrong here, it should be system as there is no MR passed in for inject

shijin-aws · 2023-11-06T20:53:20Z

prov/efa/src/rdm/efa_rdm_ope.c

+			assert(ope->total_len <= ep->inject_size);
+			copied = ofi_copy_from_hmem_iov(pkt_entry->wiredata + sizeof(struct efa_rdm_rma_context_pkt),
+				ope->total_len, FI_HMEM_SYSTEM, 0, ope->iov, ope->iov_count, 0);
+			assert(copied == ope->total_len);


This shouldn't be an assert. It should be warning and return error when copied != total_len

It is an assert here: https://github.com/ofiwg/libfabric/blob/main/prov/efa/src/rdm/efa_rdm_pke_utils.c#L123-L139

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

Lines 123 to 139 in 387a733

if (iov_mr && FI_HMEM_CUDA == iov_mr->peer.iface &&

(iov_mr->peer.flags & OFI_HMEM_DATA_DEV_REG_HANDLE)) {

assert(iov_mr->peer.hmem_data);

copied = ofi_gdrcopy_from_cuda_iov((uint64_t)iov_mr->peer.hmem_data,

pke->wiredata + payload_offset,

ope->iov, ope->iov_count,

segment_offset, data_size);

} else {

copied = ofi_copy_from_hmem_iov(pke->wiredata + payload_offset,

data_size,

iov_mr ? iov_mr->peer.iface : FI_HMEM_SYSTEM,

iov_mr ? iov_mr->peer.device.reserved : 0,

ope->iov, ope->iov_count, segment_offset);

}

assert(copied == data_size);

pke->payload = pke->wiredata + payload_offset;

a-szegel requested a review from a team November 3, 2023 21:28

a-szegel added the for-1.20.x label Nov 3, 2023

a-szegel force-pushed the rdma-write-with-inject branch 2 times, most recently from a09f91e to d004f5f Compare November 3, 2023 21:44

sunkuamzn suggested changes Nov 3, 2023

View reviewed changes

a-szegel force-pushed the rdma-write-with-inject branch from d004f5f to 872a1c5 Compare November 4, 2023 01:05

a-szegel added 2 commits November 4, 2023 01:07

a-szegel force-pushed the rdma-write-with-inject branch from 872a1c5 to 876ba80 Compare November 4, 2023 01:09

a-szegel requested review from sunkuamzn and a team and removed request for a team November 4, 2023 01:10

sunkuamzn approved these changes Nov 6, 2023

View reviewed changes

j-xiong merged commit 6840302 into ofiwg:main Nov 6, 2023
8 checks passed

shijin-aws reviewed Nov 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prov/efa: Enable inject rdma write #9512

prov/efa: Enable inject rdma write #9512

a-szegel commented Nov 3, 2023

a-szegel commented Nov 3, 2023 •

edited

Loading

sunkuamzn Nov 3, 2023

a-szegel Nov 4, 2023

sunkuamzn Nov 3, 2023

a-szegel Nov 4, 2023 •

edited

Loading

shijin-aws Nov 6, 2023

a-szegel Nov 7, 2023

sunkuamzn Nov 3, 2023

a-szegel Nov 4, 2023

sunkuamzn Nov 6, 2023

shijin-aws Nov 6, 2023 •

edited

Loading

a-szegel Nov 6, 2023

shijin-aws Nov 6, 2023

shijin-aws Nov 6, 2023 •

edited

Loading

a-szegel Nov 6, 2023

sunkuamzn Nov 7, 2023

	txe->cq_entry.len = ofi_total_iov_len(txe->iov, txe->iov_count);
	txe->cq_entry.buf = OFI_LIKELY(txe->cq_entry.len > 0) ? txe->iov[0].iov_base : NULL;

	if (iov_mr && FI_HMEM_CUDA == iov_mr->peer.iface &&
	(iov_mr->peer.flags & OFI_HMEM_DATA_DEV_REG_HANDLE)) {
	assert(iov_mr->peer.hmem_data);
	copied = ofi_gdrcopy_from_cuda_iov((uint64_t)iov_mr->peer.hmem_data,
	pke->wiredata + payload_offset,
	ope->iov, ope->iov_count,
	segment_offset, data_size);
	} else {
	copied = ofi_copy_from_hmem_iov(pke->wiredata + payload_offset,
	data_size,
	iov_mr ? iov_mr->peer.iface : FI_HMEM_SYSTEM,
	iov_mr ? iov_mr->peer.device.reserved : 0,
	ope->iov, ope->iov_count, segment_offset);
	}

	assert(copied == data_size);
	pke->payload = pke->wiredata + payload_offset;

prov/efa: Enable inject rdma write #9512

prov/efa: Enable inject rdma write #9512

Conversation

a-szegel commented Nov 3, 2023

a-szegel commented Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-szegel Nov 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shijin-aws Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shijin-aws Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-szegel commented Nov 3, 2023 •

edited

Loading

a-szegel Nov 4, 2023 •

edited

Loading

shijin-aws Nov 6, 2023 •

edited

Loading

shijin-aws Nov 6, 2023 •

edited

Loading