prov/verbs: Allow RDMACM to connect using GIDs #5605

sydidelot · 2020-02-05T14:10:04Z

Hi,

I recently wrote an email to the libfabric-users mailing list to ask if there was a way to run the verbs provider without RDMACM. I didn't get any response so far.

I initially supposed that RDMACM required to have an IP address set for every IB interfaces in order to work. I was wrong, and RDMACM can actually deal with GIDs directly for connection establishment.

The patch in this PR allows the Verbs provider to directly connect to the network adapters using the GID. In other words, the patch allows to use Libfabric even if there is no IP address set for the Infiniband interfaces.

There are significant issues of issues IP addresses for connection establishment:

It requires to set up/maintain IP addresses for every IB interfaces.
In the context of multirail (multiple local interfaces that belong to the same network subnet), it requires specific IP routes to prevent an interface to reply for another one. Connection establishment would fail otherwise.

The GID can be accessed by looking at the field src_addr returned by fi_info -p verbs -v.

Example of output:
src_addr: fi_sockaddr_ib://fe80:0000:0000:0000:248a:0703:003f:1f6a

The patch also modifies fabtest so anybody can start testing this new feature. A new option -F allows to specify the address format that is use for the source/destination addresses.

After figuring out the GID of interface that will be used for the server, one can run the following commands with fabtest:

Server:
fi_msg_bw -s fe80:0000:0000:0000:248a:0703:003f:1f6a -e msg -p verbs -F FI_SOCKADDR_IB

Client:
fi_msg_bw -e msg -p verbs -F FI_SOCKADDR_IB fe80:0000:0000:0000:248a:0703:003f:1f6a

Signed-off-by: Sylvain Didelot sdidelot@ddn.com

fabtests/common/shared.c

prov/verbs/src/verbs_info.c

src/common.c

fabtests/benchmarks/msg_bw.c

prov/verbs/src/verbs_info.c

src/common.c

fabtests/benchmarks/msg_bw.c

shefty · 2020-02-06T16:16:56Z

We need to continue to use struct sockaddr_ib. That's a commonly defined structure that contains the GID in sib_addr (struct ib_addr), plus other needed fields, like the pkey and scope_id. A GID by IBTA spec definition is an ipv6 address (except that in reality it's not), but it's safe to use the inet functions to process it.

I was suggesting we could construct the sockaddr_ib ourselves without calling into the librdmacm functions. As it is, we're letting the rdmacm pick the other fields in the sockaddr_ib without help. A GID could be associated with multiple pkeys, and despite G in GID meaning 'global', two devices could be assigned the same GID if they are on separate subnets.

sydidelot · 2020-02-07T16:16:45Z

@shefty, your comment raises several questions/concerns to me.

The sockaddr_ib strcuture is defined as below:

    unsigned short int  sib_family; /* AF_IB */                
    __be16          sib_pkey;                                  
    __be32          sib_flowinfo;                              
    struct ib_addr      sib_addr;                              
    __be64          sib_sid;                                   
    __be64          sib_sid_mask;                              
    __u64           sib_scope_id;                              
};

sib_addr can be set using ibv_query_gid() and sib_pkey with ibv_query_pkey(). I'm not exactly sure how to retrieve the other members of the structure without librdmacm.

Regarding your statement below:

A GID could be associated with multiple pkeys, and despite G in GID meaning 'global', two devices could be assigned the same GID if they are on separate subnets.

Does it mean that the GID is not sufficient for connection establishment and it needs to be associated with a pkey?
As for now, the patch assumes that the GID is the only required data for connection establishment. We will need to change this as well.

shefty · 2020-02-07T16:33:53Z

To establish a connection, we need all of the fields in sockaddr_ib, except flowinfo. A pkey is required. The scope_id is used to select a device when two ports have the same GID. The latter can occur if the ports are on separate subnets, and the SMs assign the same GID. Honestly, I think that will be a very rare occurrence, but it's just an index. The SID is the equivalent to the tcp port number. That is something the rdmacm knows how to fill in. I guess we could copy that formatting code. It just writes a 16-bit port number into a 64-bit value, with the other bits fixed based on the port space. There are requirements or changes on the CM private data when native IB addressing is used, but I don't remember what they are.

sydidelot · 2020-03-18T15:09:13Z

Guys, I have updated the patch but it doesn't include all the changes you requested so far.
Sorry, this PR was inactive for a while but I'm still very interested in using GIDs to establish connections.
@shefty would you please have a look at the function vrb_get_rdma_addrinfo() to verify if the construction of the input argument struct rdma_addrinfo looks correct to you? (I took some inspiration from rdmacm). I will address the remaining comments after I'm sure I'm going to the right direction :-). Thank you!

sydidelot · 2020-05-29T09:30:22Z

@shefty @rajachan, I have resurrected the patch with a new version that includes all your review comments. The new format for the fi_sockaddr_ib is now the following:

fi_sockaddr_ib://[guid]:pkey:port_space:scope_id

For example:

fi_sockaddr_ib://[fe80::248a:703:1c:dc0c]:ffff:13f:0

Please let me know what you think of this new version. And thanks for your time and your help to get this feature approved in Libfabric!

shefty

Please break the first patch up into multiple patches. E.g. changes to formatting the sockaddr_ib string, converting the string to sib, changes to selecting the port space, getting the gids during device init, etc.

prov/verbs/src/fi_verbs.c

prov/verbs/src/verbs_info.c

sydidelot · 2020-06-03T16:04:08Z

Please break the first patch up into multiple patches. E.g. changes to formatting the sockaddr_ib string, converting the string to sib, changes to selecting the port space, getting the gids during device init, etc.

@shefty Thanks for the review! I have addressed all of your comments and split up the changes into multiple patches as requested.

shefty

Thanks - splitting up the patches helps a lot with the review.

fabtests/common/shared.c

shefty · 2020-06-15T20:55:59Z

src/common.c

@@ -318,7 +320,20 @@ const char *ofi_straddr(char *buf, size_t *len,
 				str, *((uint16_t *)addr + 8), *((uint32_t *)addr + 5));
 		break;
 	case FI_SOCKADDR_IB:
-		size = snprintf(buf, *len, "fi_sockaddr_ib://%p", addr);
+		memset(str, 0, sizeof(str));
+		if (!inet_ntop(AF_INET6, ((uint64_t *)addr + 1), str, INET6_ADDRSTRLEN))


We need to cast address to some defined structure, rather than reading a bunch of byte offsets.

I have created a structure ofi_sockaddr_ib similar to sockaddr_ib, which avoids byte offsets.

Unrelated to this PR but FI_ADDR_IB_UD also uses a bunch of byte offsets, which should be converted into a known structure as well:

case FI_ADDR_IB_UD: memset(str, 0, sizeof(str)); if (!inet_ntop(AF_INET6, addr, str, INET6_ADDRSTRLEN)) return NULL; size = snprintf(buf, *len, "fi_addr_ib_ud://" "%s" /* GID */ ":%" PRIx32 /* QPN */ "/%" PRIx16 /* LID */ "/%" PRIx16 /* P_Key */ "/%" PRIx8 /* SL */, str, *((uint32_t *)addr + 4), *((uint16_t *)addr + 10), *((uint16_t *)addr + 11), *((uint8_t *)addr + 26));

Yeah, that shouldn't be defined like that.

Ok, I'll fix that code in a separate patch.

src/common.c

prov/verbs/src/verbs_info.c

prov/verbs/src/fi_verbs.h

prov/verbs/src/fi_verbs.c

sydidelot · 2020-06-29T15:36:22Z

@shefty Thanks for the review! I have updated the patch series with most of the changes you requested. There are still some open questions/points that need to be addressed (please see my comments above).

This patch defines 2 new formats for fi_sockaddr_if addresses: fi_sockaddr_ib://[<gid>]:<pkey>:<port_space>:<scope_id> and: fi_sockaddr_ib://[<gid>]:<pkey>:<port_space>:<scope_id>:<port> Change-Id: If7900b71e01adbed1510f35fbdd298800ca75758 Signed-off-by: Sylvain Didelot <sdidelot@ddn.com>

…rs ... ... in the system, independent of whether ipoib is enabled on that pair or not. That change allows fi_getinfo() to retrieve IB interfaces in the case ipoib is not available on the system. Signed-off-by: Sylvain Didelot <sdidelot@ddn.com>

sydidelot · 2020-07-23T13:13:52Z

@shefty I have updated the patch series with all your review comments. I also fixed a (bad) issue where OFI_RDMA_PS_IB was mistakenly set to the wrong value.

This is the final patch of the series that adds the support of GID-base connection establishment. The Verbs provider now can directly connect to the network adapters using the GID. In other words, the patch allows to use Libfabric even if there is no IP address set for the Infiniband interfaces. There are significant issues of issues IP addresses for connection establishment: - It requires to set up/maintain IP addresses for every IB interfaces. - In the context of multirail (multiple local interfaces that belong to the same network subnet), it requires specific IP routes to prevent an interface to reply for another one. Connection establishment would fail otherwise. The GID can be accessed by looking at the field src_addr returned by "fi_info -p verbs -v". Example of output: src_addr: fi_sockaddr_ib://[fe80::248a:703:1c:dc0c]:ffff:13f:0 The patch also modifies fabtest so anybody can start testing this new feature. A new option -F allows to specify the address format that is use for the source/destination addresses. After figuring out the GID of interface that will be used for the server, one can run the following commands with fabtest: Server: fi_msg_bw -s [fe80::248a:703:1c:dc0c]:ffff:13f:0 -e msg \ -p verbs -F fi_sockaddr_ib Client: fi_msg_bw -e msg -p verbs \ -F fi_sockaddr_ib [fe80::248a:703:1c:dc0c]:ffff:13f:0 Signed-off-by: Sylvain Didelot <sdidelot@ddn.com>

With AF_INET, user space should fill out the RDMA CM header and pass that to the kernel. References: - RDMA CM header format: https://github.com/linux-rdma/rdma-core/blob/master/\ librdmacm/cma.h#L105 - https://www.spinics.net/lists/linux-rdma/msg22381.html - IBTA Architecture Specification Vol 1. Annex A11: RDMA IP CM Service. Signed-off-by: Sylvain Didelot <sdidelot@ddn.com>

Signed-off-by: Sylvain Didelot <sdidelot@ddn.com>

Remove verbs specific functions to manipulate sockaddr addresses and use the ofi functions provided by common code. Signed-off-by: Sylvain Didelot <sdidelot@ddn.com>

sydidelot · 2020-07-23T15:34:28Z

@shefty Would you please let me know the failure reported by the Intel CI pipeline? I ran fabtest locally with the verbs and it didn't report any error.

shefty · 2020-07-23T17:18:48Z

Intel CI failure is unrelated

name:   fi_multinode -C rma -n 3 -p "psm2"
07:15:42    result: Fail

shefty

Only a couple minor changes needed, which could even be added as separate patches.

prov/verbs/src/fi_verbs.c

shefty · 2020-07-23T17:37:45Z

prov/verbs/src/verbs_cm.c

+{
+	struct vrb_rdma_cm_hdr *rdma_cm_hdr = priv_data;
+
+	rdma_cm_hdr->ip_version = 0;


This should be 6 << 4

I created a new patch to fix the port and the ip_version.

shefty · 2020-07-23T17:38:16Z

prov/verbs/src/verbs_cm.c

+	struct vrb_rdma_cm_hdr *rdma_cm_hdr = priv_data;
+
+	rdma_cm_hdr->ip_version = 0;
+	rdma_cm_hdr->port = 0;


This is the source port. Lower 16-bits of src_addr->sib_sid.

Thanks. I created a new patch to fix the port and the ip_version.

Can you fix the commit message? I'm not sure 'based on Sean's review' will be particularly useful when reading the git log. :)

I fixed the commit message. Sorry for the rush.

prov/verbs/src/verbs_cm.c

The patch also simplifies a 'if' statement. Signed-off-by: Sylvain Didelot <sdidelot@ddn.com>

shefty · 2020-07-27T17:27:01Z

bot:aws:retest

shefty

CI failure was unrelated, but restarted it anyway. Waiting for CI to finish, but this looks good to merge.

sydidelot · 2020-07-27T20:12:36Z

That's good news! Thank you!

swelch · 2020-07-27T21:01:46Z

@shefty - I'm assuming with these changes the intent is not supporting GIDs using the IB Verbs XRC transport at this time?

shefty · 2020-07-27T21:17:49Z

This only updates the rdma_cm path. I think Sylvain, who submitted the patches, is only concerned with RC QPs and MSG EPs.

swelch · 2020-07-27T21:32:43Z

This only updates the rdma_cm path. I think Sylvain, who submitted the patches, is only concerned with RC QPs and MSG EPs.

Thanks, sounds good. I had taken a look at the changes and just wanted to make sure he knew there would be additional work required to extend this to XRC if that transport was desired. Since XRC is intended to be used with RxM, it makes sense to me to not worry about XRC support at this time.

sydidelot · 2020-09-14T06:29:50Z

@swelch. I'm sorry for my late reply - I didn't notice there was a discussion on-going in the PR. I am not interested in XRC for the moment, but that might change in the future :)

sydidelot requested review from shefty and arn314 February 5, 2020 14:10

shefty reviewed Feb 5, 2020

View reviewed changes

fabtests/common/shared.c Show resolved Hide resolved

prov/verbs/src/verbs_info.c Outdated Show resolved Hide resolved

prov/verbs/src/verbs_info.c Show resolved Hide resolved

prov/verbs/src/verbs_info.c Show resolved Hide resolved

src/common.c Outdated Show resolved Hide resolved

rajachan reviewed Feb 5, 2020

View reviewed changes

src/common.c Outdated Show resolved Hide resolved

fabtests/benchmarks/msg_bw.c Show resolved Hide resolved

sydidelot commented Feb 6, 2020

View reviewed changes

sydidelot force-pushed the rdmacm_gid branch from 0326c82 to c68cdbf Compare March 18, 2020 14:53

sydidelot force-pushed the rdmacm_gid branch from c68cdbf to 6cbbe2c Compare May 28, 2020 15:33

sydidelot requested review from shefty and rajachan May 30, 2020 14:31

shefty requested changes Jun 1, 2020

View reviewed changes

sydidelot force-pushed the rdmacm_gid branch from 6cbbe2c to b14cb9e Compare June 3, 2020 16:02

sydidelot requested a review from shefty June 3, 2020 16:59

sydidelot force-pushed the rdmacm_gid branch 2 times, most recently from 0128aee to 81f5bee Compare June 8, 2020 17:14

sydidelot mentioned this pull request Jun 9, 2020

Update all providers to support FI_ADDR_STR #6016

Open

sydidelot force-pushed the rdmacm_gid branch 2 times, most recently from bf2b439 to 5e5402d Compare June 15, 2020 13:06

shefty reviewed Jun 18, 2020

View reviewed changes

sydidelot force-pushed the rdmacm_gid branch 2 times, most recently from f06c9e1 to 0e08392 Compare June 29, 2020 15:21

sydidelot force-pushed the rdmacm_gid branch 2 times, most recently from 4dab702 to 62d6a66 Compare June 30, 2020 10:09

sydidelot requested a review from shefty June 30, 2020 11:40

sydidelot added 2 commits July 23, 2020 08:54

sydidelot force-pushed the rdmacm_gid branch from d9b9ad3 to 244a90e Compare July 23, 2020 12:59

sydidelot added 4 commits July 23, 2020 09:51

common: Add support of AF_IB to addr manipulation functions

b3c6bb2

Signed-off-by: Sylvain Didelot <sdidelot@ddn.com>

prov/verbs: Use ofi common functions for addr manipulation

330d80e

Remove verbs specific functions to manipulate sockaddr addresses and use the ofi functions provided by common code. Signed-off-by: Sylvain Didelot <sdidelot@ddn.com>

sydidelot force-pushed the rdmacm_gid branch from 244a90e to 330d80e Compare July 23, 2020 13:52

sydidelot requested a review from shefty July 23, 2020 13:52

shefty reviewed Jul 23, 2020

View reviewed changes

sydidelot requested a review from shefty July 23, 2020 18:39

shefty reviewed Jul 23, 2020

View reviewed changes

prov/verbs/src/verbs_cm.c Outdated Show resolved Hide resolved

sydidelot force-pushed the rdmacm_gid branch from cc79361 to c4cec6c Compare July 23, 2020 20:17

prov/verbs: set ip_version and port correctly in RDMA CM header

523a71f

The patch also simplifies a 'if' statement. Signed-off-by: Sylvain Didelot <sdidelot@ddn.com>

sydidelot force-pushed the rdmacm_gid branch from c4cec6c to 523a71f Compare July 23, 2020 20:20

sydidelot requested a review from shefty July 23, 2020 20:44

shefty approved these changes Jul 27, 2020

View reviewed changes

shefty added the cray-jenkins-build label Jul 27, 2020

shefty merged commit e4b3367 into ofiwg:master Jul 27, 2020

sydidelot mentioned this pull request Sep 14, 2020

prov/verbs: connection establishement with AF_IB requires IPoIB and IP links to be up #6247

Closed

SnaKyEyeS mentioned this pull request Oct 18, 2024

prov/verbs: establishing verbs connection using the GID doesn't work #10472

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prov/verbs: Allow RDMACM to connect using GIDs #5605

prov/verbs: Allow RDMACM to connect using GIDs #5605

sydidelot commented Feb 5, 2020

shefty commented Feb 6, 2020

sydidelot commented Feb 7, 2020

shefty commented Feb 7, 2020

sydidelot commented Mar 18, 2020

sydidelot commented May 29, 2020

shefty left a comment

sydidelot commented Jun 3, 2020 •

edited

Loading

shefty left a comment

shefty Jun 15, 2020

sydidelot Jun 29, 2020

sydidelot Jun 29, 2020

shefty Jun 29, 2020

sydidelot Jun 29, 2020

sydidelot commented Jun 29, 2020

sydidelot commented Jul 23, 2020

sydidelot commented Jul 23, 2020

shefty commented Jul 23, 2020

shefty left a comment

shefty Jul 23, 2020

sydidelot Jul 23, 2020

shefty Jul 23, 2020

sydidelot Jul 23, 2020

shefty Jul 23, 2020

sydidelot Jul 23, 2020

shefty commented Jul 27, 2020

shefty left a comment

sydidelot commented Jul 27, 2020

swelch commented Jul 27, 2020

shefty commented Jul 27, 2020

swelch commented Jul 27, 2020

sydidelot commented Sep 14, 2020

prov/verbs: Allow RDMACM to connect using GIDs #5605

prov/verbs: Allow RDMACM to connect using GIDs #5605

Conversation

sydidelot commented Feb 5, 2020

shefty commented Feb 6, 2020

sydidelot commented Feb 7, 2020

shefty commented Feb 7, 2020

sydidelot commented Mar 18, 2020

sydidelot commented May 29, 2020

shefty left a comment

Choose a reason for hiding this comment

sydidelot commented Jun 3, 2020 • edited Loading

shefty left a 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

Choose a reason for hiding this comment

sydidelot commented Jun 29, 2020

sydidelot commented Jul 23, 2020

sydidelot commented Jul 23, 2020

shefty commented Jul 23, 2020

shefty left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shefty commented Jul 27, 2020

shefty left a comment

Choose a reason for hiding this comment

sydidelot commented Jul 27, 2020

swelch commented Jul 27, 2020

shefty commented Jul 27, 2020

swelch commented Jul 27, 2020

sydidelot commented Sep 14, 2020

sydidelot commented Jun 3, 2020 •

edited

Loading