prov/efa: Add new efa-direct fi_info objects #10735

sunkuamzn · 2025-01-28T00:47:31Z

efa-direct is a new provider that provides direct access to the EFA
device with minimal overhead and features. It does not support SAS
ordering, tagged messaging, atomics. It does not have copy based
protocols and cannot support sending/receiving large message sizes
and HMEM devices that don't have p2p support.

The functionality provided by the efa-direct provider is a subset of the
efa rdm provider. When applications do not require capabilities only
supported by the efa rdm provider, it is more performant for them to use
the efa-direct provider. Accordingly, the efa-direct fi_info objects are
returned before the efa rdm fi_info objects in the fi_getinfo call.

shijin-aws

My high level comment is that I don't think efa direct should be a separate ep type. I understand you want to use this to simplify your branching. But the branching should be on prov_name instead of ep type. I thought our plan is to make efa direct cover both dgram and rdm ep type. But that will break existing dgram user which use efa as prov_name. I don't know if this user actually exists, but to avoid the risk I am ok to make dgram ep type stay in efa and make efa-direct only cover rdm.

prov/efa/src/efa_base_ep.h

prov/efa/src/efa_device.h

prov/efa/src/efa_domain.c

prov/efa/src/efa_domain.h

prov/efa/src/efa_prov_info.c

prov/efa/src/efa_user_info.c

prov/efa/src/efa_prov_info.c

prov/efa/src/efa_prov.c

prov/efa/src/efa_fabric.c

darrylabbate · 2025-01-28T20:32:03Z

prov/efa/src/efa_base_ep.h

+#define EFA_INFO_TYPE_IS_RDM(_info) \
+	(_info && _info->ep_attr && (_info->ep_attr->type == FI_EP_RDM) && !strcasecmp(_info->fabric_attr->name, EFA_PROV_NAME))


Are the linear string comparisons necessary? These are already pretty gnarly chained conditionals.

If the fi_info.fabric_attr.name is truly the only way to disambiguate efa-rdm from efa-direct (I.e. we can't add some other enum field elsewhere), I'd like to explore employing constant-time comparison schemes.

Is strncmp with the length of the longer string good enough? These macros are only used in the fi_getinfo and fi_domain calls, so they are not performance critical.

prov/efa/src/efa_prov_info.c

prov/efa/src/efa_base_ep.h

prov/efa/src/efa_fabric.c

prov/efa/src/efa_prov_info.c

prov/efa/src/efa_user_info.c

prov/efa/src/efa_base_ep.h

shijin-aws

Looks great to me, thank you! Only a few nits.

prov/efa/src/efa_base_ep.h

prov/efa/src/efa_prov_info.c

prov/efa/test/efa_unit_test_common.c

shijin-aws · 2025-01-30T21:10:59Z

AWS CI failure seems to still to be some tests are not using -f efa

prov/efa/src/efa_prov_info.c

shijin-aws · 2025-02-01T00:58:44Z

I still hit bad qpn error when I only test rdm_pingpong with 1K... which means there is something wrong. Need to address this before can merge the PR

libfabric/fabtests$ FI_LOG_LEVEL=warn fi_rma_bw -p efa -f efa-direct -E -S 1024 lo
calhost
libfabric:1266354:1738371468::efa:cq:efa_cq_handle_error():100<warn> err: 107, message: Bad queue pair (QP) number (QP does not exist or is in error state) My EFA addr: fi_addr_efa://[fe80::86f:e0ff:fe87:ae7b]:1:574304512 My host id: i-0be65d40744bbb17b Peer EFA addr: fi_addr_efa://[fe80::86f:e0ff:fe87:ae7b]:0:0 Peer host id: N/A (9)
libfabric:1266354:1738371468::efa:cq:efa_show_help():100<warn> This error is detected remotely; typically encountered when the peer process is no longer present
[error] fabtests:common/shared.c:3043: cq_readerr 107 (Transport endpoint is not connected), provider errno: 9 (Bad queue pair (QP) number (QP does not exist or is in error state) My EFA addr: fi_addr_efa://[fe80::86f:e0ff:fe87:ae7b]:1:574304512 My host id: i-0be65d40744bbb17b Peer EFA addr: fi_addr_efa://[fe80::86f:e0ff:fe87:ae7b]:0:0 Peer host id: N/A)

prov/efa/src/efa_prov_info.c

shijin-aws · 2025-02-01T01:10:32Z

I would carefully review int efa_prov_info_alloc_for_rdm(struct fi_info **prov_info_rdm_ptr, struct efa_device *device) to see whether we miss any configuration that should be supported for efa-direct as well.
Or just have another efa_prov_info_alloc_for_direct for similar usage.

prov/efa/src/efa_av.c

prov/efa/src/efa_prov.c

prov/efa/src/efa_prov_info.c

prov/efa/src/efa_user_info.c

prov/efa/src/efa_prov_info.c

prov/efa/src/efa_hmem.c

shijin-aws · 2025-02-05T23:21:19Z

prov/efa/src/efa_prov_info.c

+		}
+
+		/* efa-direct path requires FI_CONTEXT2 */
+		prov_info->tx_attr->mode |= FI_CONTEXT2;


Both dgram and direct requires FI_CONTEXT2.

shijin-aws · 2025-02-06T00:22:08Z

prov/efa/src/efa_prov_info.c

@@ -251,15 +217,29 @@ void efa_prov_info_set_tx_rx_attr(struct fi_info *prov_info,
 				  struct efa_device *device,
 				  enum fi_ep_type ep_type)
 {
+	/* efa-direct and DGRAM paths require FI_CONTEXT2 */
+	prov_info->tx_attr->mode |= FI_CONTEXT2;


This mode will be washed by the change in L225....

Need to move to the place after the if block

efa-direct is a new path that provides direct access to the EFA device with minimal overhead and features. It does not support SAS ordering, tagged messaging, atomics. It does not have copy based protocols and cannot support sending/receiving large message sizes and HMEM devices that don't have p2p support. The functionality provided by the efa-direct path is a subset of the efa rdm path. When applications do not require capabilities only supported by the efa rdm path, it is more performant for them to use the efa-direct path. Accordingly, the efa-direct fi_info objects are returned before the efa rdm fi_info objects in the fi_getinfo call. Signed-off-by: Sai Sunku <[email protected]>

Previously, device->rdm_info->ep_attr->max_msg_size would always be the max message size for FI_MSG operations (corresponding to the device MTU size). With efa direct and related changes, max_msg_size can be the maximum RMA size supported by the device which is 1GB. So use device->ibv_port_attr.max_msg_sz instead when MTU size is required. Signed-off-by: Sai Sunku <[email protected]>

Remove temporary hacks added previously to test the efa-direct code path and add new tests Signed-off-by: Sai Sunku <[email protected]>

shijin-aws · 2025-02-06T18:46:15Z

bot:aws:retest

jiaxiyan · 2025-02-06T21:06:22Z

prov/efa/src/efa_prov_info.c

+					return;
+			}
+		}
+		enable_hmem = true;


Do you need this variable enable_hmem?

Yes. To catch the case when HAVE_CUDA is defined but ofi_hmem_is_initialized(FI_HMEM_CUDA) is false. That happens when you compile with CUDA but run without it.

It seems better to do an early return forif !ofi_hmem_is_initialized, then you do not need this enable_hmem.

prov/efa/test/efa_unit_test_info.c

jiaxiyan · 2025-02-06T22:43:38Z

prov/efa/src/efa_prov_info.c

+			return;
+		}
+
+		EFA_HMEM_IFACE_FOREACH(i) {


Do we need to check for system here?

No but FI_HMEM_SYSTEM will never fail the check below, so I don't think it's worth skipping FI_HMEM_SYSTEM explicitly

libfabric/prov/efa/src/efa_hmem.c

Lines 265 to 268 in c3420a7

info->initialized = true;

if (iface == FI_HMEM_SYNAPSEAI || iface == FI_HMEM_SYSTEM) {

info->p2p_supported_by_device = true;

There is a EFA_HMEM_IFACE_FOREACH_NON_SYSTEM

sunkuamzn · 2025-02-07T05:25:56Z

bot:aws:retest

sunkuamzn requested a review from a team January 28, 2025 00:48

shijin-aws requested changes Jan 28, 2025

View reviewed changes

sunkuamzn force-pushed the efa-direct-prov branch 3 times, most recently from 2437ba1 to 470cd6d Compare January 28, 2025 17:35

shijin-aws reviewed Jan 28, 2025

View reviewed changes

prov/efa/src/efa_prov_info.c Outdated Show resolved Hide resolved

shijin-aws reviewed Jan 28, 2025

View reviewed changes

prov/efa/src/efa_prov_info.c Outdated Show resolved Hide resolved

shijin-aws reviewed Jan 28, 2025

View reviewed changes

prov/efa/src/efa_prov_info.c Outdated Show resolved Hide resolved

shijin-aws reviewed Jan 28, 2025

View reviewed changes

prov/efa/src/efa_prov_info.c Outdated Show resolved Hide resolved

prov/efa/src/efa_prov_info.c Outdated Show resolved Hide resolved

darrylabbate reviewed Jan 28, 2025

View reviewed changes

shijin-aws reviewed Jan 28, 2025

View reviewed changes

prov/efa/src/efa_prov_info.c Outdated Show resolved Hide resolved

sunkuamzn force-pushed the efa-direct-prov branch from 470cd6d to 7e90a94 Compare January 29, 2025 18:11

shijin-aws reviewed Jan 29, 2025

View reviewed changes

sunkuamzn force-pushed the efa-direct-prov branch 5 times, most recently from 41129e0 to 0d73582 Compare January 30, 2025 17:34

sunkuamzn changed the title ~~core, prov/efa: Add new efa-direct provider~~ prov/efa: Add new efa-direct provider Jan 30, 2025

sunkuamzn changed the title ~~prov/efa: Add new efa-direct provider~~ prov/efa: Add new efa-direct fi_info objects Jan 30, 2025

shijin-aws previously approved these changes Jan 30, 2025

View reviewed changes

prov/efa/src/efa_base_ep.h Outdated Show resolved Hide resolved

prov/efa/src/efa_prov_info.c Outdated Show resolved Hide resolved

prov/efa/test/efa_unit_test_common.c Outdated Show resolved Hide resolved

sunkuamzn dismissed shijin-aws’s stale review via c76cca8 January 31, 2025 15:12

sunkuamzn force-pushed the efa-direct-prov branch from 0d73582 to c76cca8 Compare January 31, 2025 15:12

shijin-aws reviewed Jan 31, 2025

View reviewed changes

prov/efa/src/efa_prov_info.c Show resolved Hide resolved

shijin-aws reviewed Feb 1, 2025

View reviewed changes

prov/efa/src/efa_prov_info.c Outdated Show resolved Hide resolved

shijin-aws reviewed Feb 1, 2025

View reviewed changes

prov/efa/src/efa_prov_info.c Outdated Show resolved Hide resolved

shijin-aws reviewed Feb 1, 2025

View reviewed changes

prov/efa/src/efa_prov_info.c Outdated Show resolved Hide resolved

sunkuamzn mentioned this pull request Feb 3, 2025

prov/efa, fabtests: Changes to support adding efa-direct fi_info objects #10759

Merged

shijin-aws reviewed Feb 4, 2025

View reviewed changes

prov/efa/src/efa_av.c Outdated Show resolved Hide resolved

sunkuamzn force-pushed the efa-direct-prov branch from c76cca8 to 76c9ecf Compare February 5, 2025 06:48

shijin-aws reviewed Feb 5, 2025

View reviewed changes

sunkuamzn force-pushed the efa-direct-prov branch 4 times, most recently from 46210fe to 300556d Compare February 5, 2025 23:15

shijin-aws reviewed Feb 5, 2025

View reviewed changes

prov/efa/src/efa_hmem.c Show resolved Hide resolved

shijin-aws reviewed Feb 5, 2025

View reviewed changes

sunkuamzn force-pushed the efa-direct-prov branch from 300556d to 1226f67 Compare February 5, 2025 23:29

shijin-aws previously approved these changes Feb 5, 2025

View reviewed changes

shijin-aws reviewed Feb 6, 2025

View reviewed changes

shijin-aws self-requested a review February 6, 2025 02:48

sunkuamzn added 3 commits February 6, 2025 04:53

prov/efa: Unit test additions and fixes for efa-direct

ea02120

Remove temporary hacks added previously to test the efa-direct code path and add new tests Signed-off-by: Sai Sunku <[email protected]>

sunkuamzn dismissed shijin-aws’s stale review via ea02120 February 6, 2025 04:53

sunkuamzn force-pushed the efa-direct-prov branch from 1226f67 to ea02120 Compare February 6, 2025 04:53

shijin-aws approved these changes Feb 6, 2025

View reviewed changes

jiaxiyan reviewed Feb 6, 2025

View reviewed changes

prov/efa/test/efa_unit_test_info.c Show resolved Hide resolved

jiaxiyan reviewed Feb 6, 2025

View reviewed changes

shijin-aws merged commit af4a3eb into ofiwg:main Feb 7, 2025
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prov/efa: Add new efa-direct fi_info objects #10735

prov/efa: Add new efa-direct fi_info objects #10735

sunkuamzn commented Jan 28, 2025

shijin-aws left a comment

darrylabbate Jan 28, 2025

sunkuamzn Jan 29, 2025

shijin-aws left a comment

shijin-aws commented Jan 30, 2025

shijin-aws commented Feb 1, 2025

shijin-aws commented Feb 1, 2025 •

edited

Loading

shijin-aws Feb 5, 2025

sunkuamzn Feb 5, 2025

shijin-aws Feb 6, 2025

shijin-aws Feb 6, 2025

shijin-aws commented Feb 6, 2025

jiaxiyan Feb 6, 2025

sunkuamzn Feb 6, 2025

jiaxiyan Feb 6, 2025

jiaxiyan Feb 6, 2025

sunkuamzn Feb 6, 2025

jiaxiyan Feb 6, 2025

sunkuamzn commented Feb 7, 2025

		#define EFA_INFO_TYPE_IS_RDM(_info) \
		(_info && _info->ep_attr && (_info->ep_attr->type == FI_EP_RDM) && !strcasecmp(_info->fabric_attr->name, EFA_PROV_NAME))

	info->initialized = true;

	if (iface == FI_HMEM_SYNAPSEAI \|\| iface == FI_HMEM_SYSTEM) {
	info->p2p_supported_by_device = true;

prov/efa: Add new efa-direct fi_info objects #10735

prov/efa: Add new efa-direct fi_info objects #10735

Conversation

sunkuamzn commented Jan 28, 2025

shijin-aws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shijin-aws left a comment

Choose a reason for hiding this comment

shijin-aws commented Jan 30, 2025

shijin-aws commented Feb 1, 2025

shijin-aws commented Feb 1, 2025 • 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

shijin-aws commented Feb 6, 2025

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

sunkuamzn commented Feb 7, 2025

shijin-aws commented Feb 1, 2025 •

edited

Loading