Add core and usnic support for fid_nic #4433

jsquyres · 2018-09-22T19:22:55Z

This PR adds support for https://ofiwg.github.io/libfabric/master/man/fi_nic.3.html.

However, there are (at least) 2 problems that need to be resolved:

EDIT: there's only 1 problem left

https://ofiwg.github.io/libfabric/master/man/fi_nic.3.html states that fid_nic.prov_attr is only understood by the provider.
1. How do we know how to tostr() it?
2. How do we know how to free it (during fi_freeinfo())?

Do we need additional provider ops for these functions?

shefty · 2018-09-22T19:29:21Z

info->handle is a fid, which includes a type field.

We either need to narrow the scope of what prov_attr can be (e.g. a pointer to a single allocated memory region), or find a way to hand it to the provider for processing.

jsquyres · 2018-09-22T21:15:31Z

Ok, fixed the code to check for FI_CLASS_NIC now.

prov_attr needs to be nailed down, though.

shefty · 2018-09-24T16:58:28Z

src/fabric.c

+				free(nic->prov_attr);
+			}
+			free(nic);
+		}


Can we move this to a separate function? I haven't looked and don't remember, but I'm guessing fi_dupinfo needs to be updated as well.

Moved to a subroutine. Also added handling for fi_dupinfo().

shefty

I'll try to see what we can do for the prov_attr. But I'm fine having whatever that is go in as separate changes.

shefty · 2018-09-24T20:17:53Z

prov/usnic/src/usdf_fabric.c

+
+	free(nic->prov_attr);
+	free(nic);
+}


Can you call ofi_free_nic() instead?

Yup! Fixed.

jsquyres · 2018-09-24T21:04:47Z

Ok, cleaned up the comments and the commit messages.

@bturrubiates usnic fi_info -v output on an old model VIC looks like this:

fi_info:
    caps: [ FI_MSG, FI_RECV, FI_SEND, FI_SOURCE ]
    mode: [ FI_MSG_PREFIX, FI_LOCAL_MR ]
    addr_format: FI_SOCKADDR_IN
    src_addrlen: 16
    dest_addrlen: 0
    src_addr: fi_sockaddr_in://10.10.0.6:0
    dest_addr: (null)
    handle (fid_nic):
        fi_device_attr:
            name: usnic_0
            device_id: UCSC-PCIE-CSC-02 (VIC 1225)
            device_version: 0xcf
            vendor_id: 0x1137
            driver: usnic_verbs
            firmware: 4.1(3f)
        fi_bus_attr:
            fi_bus_type: FI_BUS_UNKNOWN
        fi_link_attr:
            address: 10.10.0.6/16
            mtu: 9000
            speed: 10000
            state: FI_LINK_UP
            network_type: Ethernet
    fi_tx_attr:
...etc.

shefty

core changes look good

shefty · 2018-09-24T21:46:42Z

For the prov_attr, we need to extend struct fi_provider if we want to be able to print them or allow a complex structure. So, I guess the first decision is how flexible do we want provider attributes to be?

My current thinking is to add tostr(), allocinfo, and freeinfo() calls to fi_provider. The provider would only be responsible for handling provider specific portions of input data.

jsquyres · 2018-09-24T21:56:28Z

Did you mean allocnic and freenic? Or were you specifically being more general in terms of allocating/freeing an entire info? Can you flesh out a little more about what you're thinking for these 2 functions?

shefty · 2018-09-24T22:23:16Z

I was considering adding the other external API calls to the provider, but the provider would only deal with the provider specific pieces. For example, today if an app calls fi_dupinfo() on an fi_info where the handle is set to FI_CLASS_CONNREQ, the handle is ignored. So, I'd like to have a more generic solution.

These would be the additions:

dupinfo(const struct fi_info *info, struct fi_info *new_info);
freeinfo(struct fi_info *info);
tostr(const void *data, enum fi_type datatype);

There needs to be enough information in whatever input data there is in order to route the call to the correct provider. So, not all calls into the main API can find their way to the provider, even it provider specific data is present.

For dupinfo/freeinfo, the provider only deals with the handle field. But maybe this would allow a provider to store data beyond the end of the user visible structure(s)?

bturrubiates · 2018-09-24T23:31:29Z

prov/usnic/src/usdf_fabric.c

+
+	nic = calloc(1, sizeof(*nic));
+	if (!nic)
+		goto nomem;


The nomem label makes a call to ofi_free_nic if the calloc fails. With the current version of the code, that will cause a seg fault because ofi_free_nic is not safe if a NULL pointer is passed in.

Fixed ofi_free_nic() to handle NULL.

bturrubiates · 2018-09-24T23:32:23Z

src/fabric.c

@@ -481,6 +481,31 @@ FI_DESTRUCTOR(fi_fini(void))
 	ofi_osd_fini();
 }

+void ofi_free_nic(struct fid_nic *nic)
+{
+	if (nic->device_attr) {


Do you want to handle a NULL nic pointer in here somehow?

Maybe wrap this in one more if-block, or maybe make all of these something like:

if (nic && nic->device_attr) { }

bturrubiates · 2018-09-24T23:33:48Z

prov/usnic/src/usdf_fabric.c

+	if (ret < 0) {
+		USDF_DBG(
+			"asprintf failed while creating device_id\n");
+		ret = -ENOMEM;


I think you're missing a goto in this cleanup path. You set ret but then overwrite it immediately afterward.

bturrubiates · 2018-09-24T23:36:31Z

prov/usnic/src/usdf_fabric.c

+	if (ret < 0) {
+		USDF_DBG(
+			"asprintf failed while creating device_version\n");
+		ret = -ENOMEM;


Same here, think you're missing a goto.

bturrubiates · 2018-09-24T23:39:17Z

prov/usnic/src/usdf_fabric.c

+	if (ret < 0) {
+		USDF_DBG(
+			"asprintf failed while creating vendor_id\n");
+		ret = -ENOMEM;


goto missing.

bturrubiates · 2018-09-24T23:43:19Z

prov/usnic/src/usdf_fabric.c

+	if (!la)
+		goto nomem;
+	uint8_t *p = (uint8_t*) &dap->uda_ipaddr_be;
+	ret = asprintf(&la->address, "%d.%d.%d.%d/%d",


Not that we use anything other than v4, but if we ever do support anything else this conversion can probably be replaced with inet_ntop.

bturrubiates · 2018-09-24T23:43:28Z

prov/usnic/src/usdf_fabric.c

+	if (ret < 0) {
+		USDF_DBG(
+			"asprintf failed while creating link address\n");
+		ret = -ENOMEM;


Missing goto.

bturrubiates · 2018-09-24T23:46:27Z

src/fabric.c

@@ -507,6 +532,10 @@ void DEFAULT_SYMVER_PRE(fi_freeinfo)(struct fi_info *info)
 			free(info->fabric_attr->prov_name);
 			free(info->fabric_attr);
 		}
+		if (info->handle && info->handle->fclass == FI_CLASS_NIC) {
+			struct fid_nic *nic = (struct fid_nic*) info->handle;


I don't think you need this, you can just directly ofi_free_nic(info->handle);.

The types are different -- a cast of some flavor or another is necessary.

I chose to make the param of ofi_free_nic() be (struct fid_nic *) because, well, you're freeing a struct fid_nic, not a generic fid. So I figured the onus should be on the caller to make sure the right type is passed.

The types are different -- a cast of some flavor or another is necessary.

Ooops, I thought handle here was void *.

bturrubiates · 2018-09-24T23:49:01Z

src/fabric.c

+	la_dup->mtu = la->mtu;
+	la_dup->speed = la->speed;
+	la_dup->state = la->state;
+


Not that it's very important, but should you #undef MEMBER_STRDUP here?

Sure; I have no strong opinions about this. Done.

bturrubiates · 2018-09-24T23:50:41Z

src/fabric.c

@@ -853,6 +957,14 @@ struct fi_info *DEFAULT_SYMVER_PRE(fi_dupinfo)(const struct fi_info *info)
 			if (dup->fabric_attr->prov_name == NULL)
 				goto fail;
 		}
+		if (info->handle && info->handle->fclass == FI_CLASS_NIC) {
+			int ret;
+			ret = ofi_dup_nic((struct fid_nic*) info->handle,


You're not using the return value, and you don't need the cast. This can be expressed clearer as:

if (ofi_dup_nic(info->handle, &dup->handle) < 0) goto fail;

shefty · 2018-09-25T16:38:49Z

We discussed this some at today's ofiwg. @j-xiong made a suggestion to add function pointers to prov_attr, basically turning it into a struct fid. Along those lines, we have struct fid_nic. We can add additional ops to fid_nic for handling the prov_attr, or use the existing fid_nic::control() function to handle the prov_attr.

For example, fid_nic::close() could be called to free the nic data from fi_freeinfo(). Internally, the provider can call the generic helper ofi_free_nic() to free the common structures, then free the prov_attr themselves.

Similarly, we can define FI_CONTROL codes for fid_nic that would copy or print the NIC attributes. A generic helper would handle the common structures, but the provider would handle the prov_attr field.

bturrubiates · 2018-09-25T18:30:02Z

prov/usnic/src/usdf_fabric.c

+	ba->bus_type = FI_BUS_UNKNOWN;
+
+	la = calloc(1, sizeof(*la));
+	if (!la) {


For consistency, remove braces here.

bturrubiates · 2018-09-25T18:32:04Z

src/fabric.c

+	ofi_free_nic(nic_dup);
+	return -FI_ENOMEM;
+
+#undef MEMBER_STRUP


jsquyres · 2018-09-25T18:49:23Z

@bturrubiates Sigh. Where is my brain today? Fixed.

jsquyres · 2018-09-25T19:33:46Z

Any idea why AppVeyor is failing? That seems to have nothing to do with this PR:

msg_pingpong -I 5
SERVER OUTPUT: 
----
CLIENT OUTPUT: 
bytes   iters   total       time     MB/sec    usec/xfer   Mxfers/sec
64      5       640         0.10s      0.01   10099.70       0.00
256     5       2.5k        0.78s      0.00   78397.40       0.00
1k      5       10k         0.10s      0.10    9999.60       0.00
4k      5       40k         0.92s      0.04   92297.20       0.00
64k     5       640k        0.10s      6.55    9999.60       0.00
1m      5       10m         0.10s    101.81   10299.70       0.00
----
server error code = -1073741819 
test failed

See https://ci.appveyor.com/project/shefty/libfabric/build/4384/job/j40ldvxy4097d5by#L2146

shefty · 2018-09-25T19:50:31Z

I restarted appveyor to see if the error re-appears. I can't recall any recent changes that would have broken windows.

jsquyres · 2018-09-25T20:22:56Z

@shefty Failed again, but a different test this time -- https://ci.appveyor.com/project/shefty/libfabric/build/4387/job/i7nbp95e1w3v249x#L2037:

msg_sockets
SERVER OUTPUT: 
----
CLIENT OUTPUT: 
bound_addr: "[127.0.0.1]:9229"
Sending message...
Send completion received
----
server error code = -1073741819 
test failed

shefty · 2018-09-25T20:25:17Z

Other recent PRs passed appveyor, so maybe there's something in this change that the existing tests aren't handling, or there's some other issue. The changes you've made to the core look okay to me though.

shefty · 2018-09-25T21:53:32Z

I am in the process of pulling in the common pieces of this PR into a separate series, with an updated proposal to handle the prov_attr component. Please allow that PR to merge first, then we can update the usnic provider accordingly.

jsquyres · 2018-09-25T22:03:03Z

@shefty no problem

working on defining prov_attr

shefty · 2018-09-26T03:24:18Z

Please take a look at #4443. That contains a proposal for dealing with the prov_attr by making use of the fid_nic component of the structure.

shefty · 2018-10-04T16:55:57Z

FYI - I have been sidetracked from this, but I do plan on getting back to it. I have some patches in progress locally to add a fid_nic to fi_info. But that breaks the ABI, so it requires some additional compatibility code.

jsquyres · 2018-10-04T17:23:21Z

Me too -- I was planning on refreshing this PR according to the new fid_nic code on master "soon".

jsquyres · 2018-10-07T02:07:52Z

@shefty @bturrubiates Revamped, now there's two commits:

Added default fid_nic tostr functions.
- Open question here: the fid tostr function takes a len parameter that is effectively obviated in all the tostr.c functions by just using the BUFSIZ constant. I left in the len parameter and even pass it through to the sub functions in case we want to do something with it, but... we need to decide something here.
Re-did usnic fid_nic support for usnic to use the new core infrastructure.

shefty · 2018-10-08T16:40:44Z

I have a patch to fixup the fid tostr function.

I still need to update the ABI compat area to extend fi_info.

shefty · 2018-10-08T18:16:51Z

See PR #4495 for what I have. I also have an incomplete patch locally to address the ABI change

shefty · 2018-10-12T21:08:13Z

In theory, the core has all the updates that are needed now. This PR should just need the usnic specific patch now (c787f7e).

jsquyres · 2018-10-15T14:36:56Z

@bturrubiates All previous review comments addressed, and updated to utilize fid_nic core infrastructure. This PR now ready for review.

Output for usnic looks like this:

fi_info:
...
    fid_nic:
        fi_device_attr:
            name: usnic_0
            device_id: UCSC-PCIE-CSC-02 (VIC 1225)
            device_version: 0xcf
            vendor_id: 0x1137
            driver: usnic_verbs
            firmware: 4.1(3f)
        fi_bus_attr:
            fi_bus_type: FI_BUS_UNKNOWN
        fi_link_attr:
            address: 10.10.0.1
            mtu: 9000
            speed: 10000
            state: FI_LINK_UP
            network_type: Ethernet

bturrubiates · 2018-10-15T20:38:43Z

prov/usnic/src/usdf_fabric.c

+	la = nic->link_attr;
+
+	socklen_t size = 32;
+	la->address = calloc(1, size);


Use INET_ADDRSTRLEN.

bturrubiates · 2018-10-15T20:39:22Z

prov/usnic/src/usdf_fabric.c

+
+	nic = ofi_nic_dup(NULL);
+	if (!nic)
+		goto nomem;


The nomem label will dereference nic, so this will likely cause a crash in the error path.

bturrubiates · 2018-10-15T20:40:36Z

prov/usnic/src/usdf_fabric.c

+	if (!da->firmware)
+		goto nomem;
+
+	ba = nic->bus_attr;


Minor thing, but you probably don't need to use a variable for ba since you only use it once. Probably easier to just nic->bus_addr->bus_type = FI_BUS_UNKNOWN.

The usnic driver does not currently export PCI information, so that data is left empty in the fid_nic. Signed-off-by: Jeff Squyres <jsquyres@cisco.com>

jsquyres · 2018-10-16T00:40:42Z

@bturrubiates Fixed.

jsquyres added question ⚠️ Do not merge labels Sep 22, 2018

jsquyres force-pushed the pr/usnic-fid_nic branch from f54ed9a to 653641a Compare September 22, 2018 21:15

jsquyres force-pushed the pr/usnic-fid_nic branch from 653641a to d7efe4b Compare September 24, 2018 16:41

jsquyres requested review from bturrubiates and shefty September 24, 2018 16:52

shefty reviewed Sep 24, 2018

View reviewed changes

jsquyres force-pushed the pr/usnic-fid_nic branch from d7efe4b to 5b7d74b Compare September 24, 2018 18:13

shefty reviewed Sep 24, 2018

View reviewed changes

jsquyres force-pushed the pr/usnic-fid_nic branch 2 times, most recently from 40f75ca to 81db8ef Compare September 24, 2018 21:02

shefty previously approved these changes Sep 24, 2018

View reviewed changes

jsquyres force-pushed the pr/usnic-fid_nic branch from 81db8ef to bbff01c Compare September 24, 2018 21:41

bturrubiates suggested changes Sep 24, 2018

View reviewed changes

jsquyres force-pushed the pr/usnic-fid_nic branch 2 times, most recently from 8a9d812 to 8795e93 Compare September 25, 2018 17:37

bturrubiates reviewed Sep 25, 2018

View reviewed changes

jsquyres force-pushed the pr/usnic-fid_nic branch from 8795e93 to 00c050c Compare September 25, 2018 18:48

jsquyres mentioned this pull request Sep 25, 2018

core/fid_nic: better support prov_attr #4442

Closed

bturrubiates approved these changes Sep 25, 2018

View reviewed changes

jsquyres force-pushed the pr/usnic-fid_nic branch from 00c050c to c787f7e Compare October 7, 2018 02:04

jsquyres force-pushed the pr/usnic-fid_nic branch from 60bdc3a to 6c82f8a Compare October 7, 2018 03:38

jsquyres force-pushed the pr/usnic-fid_nic branch from 6c82f8a to be356c2 Compare October 15, 2018 14:33

jsquyres removed question ⚠️ Do not merge labels Oct 15, 2018

bturrubiates suggested changes Oct 15, 2018

View reviewed changes

usnic: add support for fid_nic

8d97e71

The usnic driver does not currently export PCI information, so that data is left empty in the fid_nic. Signed-off-by: Jeff Squyres <jsquyres@cisco.com>

jsquyres force-pushed the pr/usnic-fid_nic branch from be356c2 to 8d97e71 Compare October 16, 2018 00:39

bturrubiates approved these changes Oct 16, 2018

View reviewed changes

jsquyres merged commit 7e9c2bd into ofiwg:master Oct 16, 2018

jsquyres deleted the pr/usnic-fid_nic branch October 16, 2018 01:17

Add core and usnic support for fid_nic #4433

Add core and usnic support for fid_nic #4433

Conversation

jsquyres commented Sep 22, 2018 • edited Loading

shefty commented Sep 22, 2018

jsquyres commented Sep 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

jsquyres commented Sep 24, 2018

shefty left a comment

Choose a reason for hiding this comment

shefty commented Sep 24, 2018

jsquyres commented Sep 24, 2018

shefty commented Sep 24, 2018

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

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 Sep 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsquyres commented Sep 25, 2018

jsquyres commented Sep 25, 2018

shefty commented Sep 25, 2018

jsquyres commented Sep 25, 2018

shefty commented Sep 25, 2018

shefty commented Sep 25, 2018

jsquyres commented Sep 25, 2018

shefty commented Sep 26, 2018

shefty commented Oct 4, 2018

jsquyres commented Oct 4, 2018

jsquyres commented Oct 7, 2018 • edited Loading

shefty commented Oct 8, 2018

shefty commented Oct 8, 2018

shefty commented Oct 12, 2018

jsquyres commented Oct 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsquyres commented Oct 16, 2018

jsquyres commented Sep 22, 2018 •

edited

Loading

jsquyres commented Oct 7, 2018 •

edited

Loading