core: Handle struct fid_nic through various API calls #4443

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

Check for and handle fid_nic when referenced by an fi_info structure.

We use the fid_nic ops to route calls to the correct provider, with default implementations of calls provided. For example, now to free a fid_nic, we call fi_close(fid_nic). The provider can set ops->close to an internal call, or simply use the common cleanup function ofi_nic_close(). The provider specific call can call ofi_nic_close() internally as well. This gives the provider a way to cleanup the prov_attr structure if one is used.

Similarly, we can duplicate a fid_nic by using a new fi_control() command, FI_DUP. E.g. fi_control(fid_nic, FI_DUP, &fid_nic_ptr). The fi_dupinfo() call will invoke fi_control(... FI_DUP...) for any fi_info->handle that exists (with proper checks). This will route the dup operation to the provider control function, which can be set to something internal or the common ofi_nic_dup() function.

If this approach makes sense, the control operation will be expanded to support a new FI_TOSTR command.

Although the new control commands target fid_nic, they can easily be expanded for any FID type, which would allow printing internal details on things like endpoints and completion queues.

Avoid anonymous unions in a struct (because we name union struct members in other places in include/rdma). Signed-off-by: Jeff Squyres <jsquyres@cisco.com>

Define a common function for freeing a fid_nic that providers can reference when a fid_nic is closed. Signed-off-by: Jeff Squyres <jsquyres@cisco.com> Signed-off-by: Sean Hefty <sean.hefty@intel.com>

jsquyres

There's a bit of a disparity that I don't think I understand: close is a 1st-class item on the fid ops struct. So is control. But dup is not -- it's a command to control. Why?

Isn't dup also a universal action that needs to be on all fids? (and tostr might also be universal...?)

jsquyres · 2018-09-26T14:54:31Z

include/ofi_mem.h

@@ -63,6 +63,17 @@ static inline void *mem_dup(const void *src, size_t size)
 	return dest;
 }

+static inline int str_dup(const char *src, char **dst)


Shouldn't this be ofi_str_dup(), just to make it 100% clear that this is an internal libfabric function?

(I know other inlines and symbols in this file aren't ofi_ prefixed, but perhaps they should be...)

Yes, I'll update

jsquyres · 2018-09-26T14:58:00Z

src/fabric.c

+
+	dup_nic->device_attr = calloc(1, sizeof(*dup_nic->device_attr));
+	dup_nic->bus_attr = calloc(1, sizeof(*dup_nic->bus_attr));
+	dup_nic->link_attr = calloc(1, sizeof(*dup_nic->link_attr));


This will make the device_attr, bus_attr, and link_attr always non-NULL on the new nic instance.

But below, you check to make sure device_attr, bus_attr, and link_attr are non-NULL on the old nic instance before copying them.

It might be better to either:

Allow them to be NULL (and therefore if the source is NULL, the target should be NULL)

Or mandate that they must always exist (and therefore don't need to be pointers?), but may effectively be empty.

The latter point raises an interesting question: is there a reason the device_attr, bus_attr, and link_attr are pointers? Is it just so that they can be NULL (so you can trivially tell that there's no useful information)? It does save memory if you have multiple fid_nic instances that point to the same device/bus/link attr structs (which implies that you need ref counting, which would have to be in the provider) -- but is that important?

I will update to make the function follow the same semantics that we have for dupinfo.
The fields are pointers, so that each attribute structure can be extended in the future separately without affecting the offsets of fields in unrelated structures. E.g. we can add a new bus type with new attributes without breaking the link_attr access.

But doesn't that just lead to more versioning? I.e., you have to version each of those individual structs (and the top-level struct) vs. just versioning the top-level struct.

We use a top level API version (e.g. from fi_getinfo) to know if we can access a field or not in any input structure (cq_attr, ep_attr, tx_attr, etc.). For output structures, we can add new fields to the end without version checks.

In this case, a fid_nic is an output structure from the provider. It's not something an app can craft and expect to pass down. So, we can add new fields to device_attr without impacting how an application will access the bus_attr or link_attr fields. If we fold everything into a single structure, the provider would actually need to deal with fid_nic_v1 and fid_nic_v2 etc. The proposal allows them to always just use the latest structure definition independent of what version the app understands.

@shefty and I just talked on the phone:

Key point that I didn't grok was that providers are not allowed to extend the (bus, link, device) structs. Providers can only put their own provider-specific information on the prov_attr blob.

We also agreed that it would be a good idea to make the provider-specific struct for prov_attr be available in the provider extension header file.

jsquyres · 2018-09-26T15:01:58Z

src/fabric.c

+fail:
+	ofi_nic_close(&dup_nic->fid);
+	return NULL;
+


Nit: extra blank line.

jsquyres · 2018-09-26T15:21:01Z

BTW the CI failures had a helpful message this time (e.g., https://travis-ci.org/ofiwg/libfabric/jobs/433261126#L6434):

*** Error in `fi_msg_sockets': double free or corruption (!prev): 0x00000000021acc20 ***

shefty · 2018-09-26T17:37:30Z

Note: I'm only posting this for review. I haven't done any testing with it, as I'm still working on the implementation.

shefty · 2018-09-26T17:49:02Z

Adding a new control command is just easier than extending the base ops for all data types. I honestly don't expect anyone to use dup on any other data type. It would actually be hard to support given that the fids are pointers to internal structures and not integers. I.e. in order to support dup, you would always need a second layer of indirection between the application's pointer and the provider's structure.

tostr is a different in my mind. I can see that being useful for all fids and may be worth extending the base ops for.

bturrubiates · 2018-09-26T21:10:16Z

src/fabric.c

+	}
+
+	free(nic);
+	return 0;


Right now this always returns 0, and can't fail. Should this function be void?

This maps to the default fi_close() call, which returns an int.

shefty · 2018-09-27T21:01:02Z

@jsquyres @bturrubiates -- This PR has been updated with what's intended as a close to final version. One of the commits changes the prefix from fi to ofi, which blows up the line count for the series.

The base fid is extended with a new tostr() function. This maps to a default implementation for fid_nic. When ofi_nic_tostr() is invoked from fi_tostr(), things should be okay. That path is based on Jeff's patch.

There is an issue that I'm working to resolve. The proposed changes assume that the buffer passed into ofi_nic_tostr() is the libfabric internal buffer, or at least as long as that buffer. This is assumed by the internal strcatf() call with checks against BUFSIZ. So an app that calls fid_nic->ops->tostr() directly will break that assumption.

shefty · 2018-09-28T16:27:20Z

src/fabric.c

+		if (info->handle && (info->handle->fclass == FI_CLASS_NIC) &&
+		    info->handle->ops && info->handle->ops->close)
+			fi_close(info->handle);
+


This change does not work with existing applications. We cannot safely deference info->handle without the risk of accessing freed memory.

For MSG EPs, info->handle is used to report a connection request (fid FI_CLASS_CONNREQ). That fid may be freed when an app calls fi_accept/fi_reject, but info->handle is left unmodified.

We need an alternate solution for obtaining/freeing the nic attributes. :/

Is info->handle overloaded with multiple values solely to avoid adding another member to the struct / for ABI reasons?

Yes. Adding fid_nic as a new field would solve the issue, but would require apps to move to the new version to get it. The proposed solution, if it worked, would have allowed existing apps that called fi_tostr() to display the new attributes.

Ok. Since handle can be one of several different types, then it seems like it is required to also set handle back to NULL once the thing pointed to it is freed.

Add a function that will allocated/duplicate a struct fid_nic. This may be used by providers to copy a fid_nic. Signed-off-by: Sean Hefty <sean.hefty@intel.com>

This will be used to duplicate an fid_nic structure. Signed-off-by: Sean Hefty <sean.hefty@intel.com>

jsquyres · 2018-09-28T17:18:13Z

src/fabric.c

+	switch(command) {
+	case FI_DUP:
+		*nic = ofi_nic_dup(*nic);
+		return *nic ? 0 : -FI_ENOMEM;


Nit: should 0 be FI_SUCCESS?

jsquyres · 2018-09-28T17:42:22Z

src/fi_tostr.c

+{
+	switch (type) {
+	CASEENUMSTR(FI_BUS_UNKNOWN);
+	CASEENUMSTR(FI_BUS_PCI);


Shouldn't FI_BUS_PCI output "PCI" or similar?

CASEENUMSTR will output the the name as a string, so this will be "FI_BUS_PCI".

That was me not reading carefully. I think I even wrote that code... duh!

jsquyres · 2018-09-28T17:43:52Z

src/fi_tostr.c

+	switch (state) {
+	CASEENUMSTR(FI_LINK_UNKNOWN);
+	CASEENUMSTR(FI_LINK_DOWN);
+	CASEENUMSTR(FI_LINK_UP);


Shouldn't LINK_UP/DOWN output their states?

This will output "FI_LINK_UP" or "FI_LINK_DOWN" as the state

shefty · 2018-09-29T00:00:46Z

So, I think that we will need to extend struct fi_info to embed a struct fid_nic * pointer directly. This will require an ABI update, with additional abi compat code, similar to the jump from 1.0 to release 1.5. Apps will need to recompile to get the new attributes, but won't need any code changes.

Signed-off-by: Sean Hefty <sean.hefty@intel.com>

This is needed in order to properly print the new fid_nic structure, specifically the prov_attr. But being able to print internal details about any fid could be a useful debugging tool, even though the output will be provider specific. Expand the base fi_ops with a tostr function and update the main fi_tostr() function to handle it. This replaces the FI_TYPE_DEVICE_ATTR tostr() functionality. Note that this value has never been part of a release, so it is safe to remove. Signed-off-by: Sean Hefty <sean.hefty@intel.com>

Replace fi prefix with ofi for internal calls Signed-off-by: Sean Hefty <sean.hefty@intel.com>

shefty · 2018-10-01T20:15:51Z

Updated PR. This now excludes the patch that partially implemented the fid_nic::tostr() function, but is otherwise the same as the previous PR. What is here now should be mergeable, but is not a complete solution. Follow on patches will fix nic::tostr() and add fid_nic to fi_info.

jsquyres added 2 commits September 25, 2018 08:12

fabric.h: name the union

c31a9be

Avoid anonymous unions in a struct (because we name union struct members in other places in include/rdma). Signed-off-by: Jeff Squyres <jsquyres@cisco.com>

fabric: Add support for freeing a fid_nic

d89624a

Define a common function for freeing a fid_nic that providers can reference when a fid_nic is closed. Signed-off-by: Jeff Squyres <jsquyres@cisco.com> Signed-off-by: Sean Hefty <sean.hefty@intel.com>

shefty added ⚠️ Do not merge work in progress labels Sep 25, 2018

shefty force-pushed the master branch from d5ddf47 to 84fdc85 Compare September 26, 2018 02:19

shefty mentioned this pull request Sep 26, 2018

Add core and usnic support for fid_nic #4433

Merged

shefty requested review from bturrubiates and jsquyres September 26, 2018 03:33

jsquyres reviewed Sep 26, 2018

View reviewed changes

shefty force-pushed the master branch from ee5ea4c to b9b5edc Compare September 26, 2018 18:55

bturrubiates reviewed Sep 26, 2018

View reviewed changes

shefty force-pushed the master branch from 3576365 to 286adfe Compare September 27, 2018 18:31

shefty removed work in progress ⚠️ Do not merge labels Sep 27, 2018

shefty commented Sep 28, 2018

View reviewed changes

shefty added 2 commits September 28, 2018 10:05

fabric: Add ability to duplicate a fid_nic

02e58ae

Add a function that will allocated/duplicate a struct fid_nic. This may be used by providers to copy a fid_nic. Signed-off-by: Sean Hefty <sean.hefty@intel.com>

fabric: Define new FI_DUP control code

4ed08d2

This will be used to duplicate an fid_nic structure. Signed-off-by: Sean Hefty <sean.hefty@intel.com>

shefty added ⚠️ Do not merge work in progress labels Sep 28, 2018

jsquyres requested changes Sep 28, 2018

View reviewed changes

shefty added 3 commits October 1, 2018 06:13

man/nic: Clarify casting of prov_attr

0397ef2

Signed-off-by: Sean Hefty <sean.hefty@intel.com>

tostr: Rename internal functions with ofi prefix

f5d2f1a

Replace fi prefix with ofi for internal calls Signed-off-by: Sean Hefty <sean.hefty@intel.com>

jsquyres approved these changes Oct 1, 2018

View reviewed changes

shefty force-pushed the master branch from d61db6a to 60fcf4b Compare October 1, 2018 20:09

shefty removed work in progress ⚠️ Do not merge labels Oct 1, 2018

shefty force-pushed the master branch from 60fcf4b to f5d2f1a Compare October 1, 2018 20:13

shefty merged commit 085efff into ofiwg:master Oct 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: Handle struct fid_nic through various API calls #4443

core: Handle struct fid_nic through various API calls #4443

shefty commented Sep 25, 2018 •

edited

Loading

jsquyres left a comment

jsquyres Sep 26, 2018

shefty Sep 26, 2018

jsquyres Sep 26, 2018

shefty Sep 26, 2018

jsquyres Sep 26, 2018

shefty Sep 26, 2018

jsquyres Sep 26, 2018

jsquyres Sep 26, 2018

jsquyres commented Sep 26, 2018

shefty commented Sep 26, 2018

shefty commented Sep 26, 2018

bturrubiates Sep 26, 2018

shefty Sep 26, 2018

shefty commented Sep 27, 2018

shefty Sep 28, 2018

jsquyres Sep 28, 2018

shefty Sep 28, 2018

jsquyres Sep 28, 2018

jsquyres Sep 28, 2018

jsquyres Sep 28, 2018

shefty Sep 28, 2018

jsquyres Sep 28, 2018

jsquyres Sep 28, 2018

shefty Sep 28, 2018

shefty commented Sep 29, 2018

shefty commented Oct 1, 2018

core: Handle struct fid_nic through various API calls #4443

core: Handle struct fid_nic through various API calls #4443

Conversation

shefty commented Sep 25, 2018 • edited Loading

jsquyres 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsquyres commented Sep 26, 2018

shefty commented Sep 26, 2018

shefty commented Sep 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shefty commented Sep 27, 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

shefty commented Sep 29, 2018

shefty commented Oct 1, 2018

shefty commented Sep 25, 2018 •

edited

Loading