libibnetdisc: Convert to a multi-smp algorithm

From: Ira Weiny <[email protected]>
Date: Fri, 22 Jan 2010 17:33:30 -0800
Subject: [PATCH] libibnetdisc: Convert to a multi-smp algorithm

v3: change DEFAULT_MAX_SMP_ON_WIRE to 2

Allow for multiple SMP's to be on the wire at a single time. This algorithm splits the processing of SMP's to a small smp engine which may be useful to split out in the future.

Signed-off-by Ira Weiny <[email protected]>

infiniband-diags/libibnetdisc/src/ibnetdisc.c +40 -1

+static int recv_port_info(smp_engine_t *engine, ibnd_smp_t * smp,
+			  uint8_t *mad, void *cb_data)
+{
+	ibnd_fabric_t *fabric = ((ibnd_scan_t *)engine->user_data)->fabric;
+	ibnd_node_t *node = (ibnd_node_t *)cb_data;
+	ibnd_port_t *port;
+	uint8_t *port_info = mad + IB_SMP_DATA_OFFS;
+	uint8_t port_num, local_port;
+
+	port_num = mad_get_field(mad, 0, IB_MAD_ATTRMOD_F);
+	local_port = mad_get_field(port_info, 0, IB_PORT_LOCAL_PORT_F);
+
+	/* this may have been created before */
+	port = node->ports[port_num];
+	if (!port) {
+		port = node->ports[port_num] = calloc(1, sizeof(*port));
+		if (!port) {
+			IBND_ERROR("Failed to allocate port\n");
+			return -1;
+		}
+	}
+
+	memcpy(port->info, port_info, sizeof(port->info));
+	port->node = node;
+	port->portnum = port_num;
+	port->ext_portnum = 0;
+	port->base_lid = (uint16_t) mad_get_field(port->info, 0, IB_PORT_LID_F);
+	port->lmc = (uint8_t) mad_get_field(port->info, 0, IB_PORT_LMC_F);
+
+	if (port_num == 0) {
+		node->smalid = port->base_lid;
+		node->smalmc = port->lmc;
+	} else if (node->type == IB_NODE_SWITCH) {
+		port->base_lid = node->smalid;
+		port->lmc = node->smalmc;
+	}
+
+	add_to_portguid_hash(port, fabric->portstbl);
+
+	debug_port(&smp->path, port);
 
-	mad_dump_node_type(type, 64, &node->type, sizeof(int));

infiniband-diags/libibnetdisc/src/ibnetdisc.c +8 -0

+
+		ib_portid_t path = smp->path;
+		if (extend_dpath(engine, &path, port_num) != -1)
+			query_node_info(engine, &path, node);
+	}
+
+	return 0;
+}

infiniband-diags/libibnetdisc/src/ibnetdisc.c +50 -4

+static int recv_node_info(smp_engine_t *engine, ibnd_smp_t * smp,
+			  uint8_t *mad, void *cb_data)
+{
+	ibnd_fabric_t *fabric = ((ibnd_scan_t *)engine->user_data)->fabric;
+	int i = 0;
+	uint8_t *node_info = mad + IB_SMP_DATA_OFFS;
+	ibnd_node_t * rem_node = (ibnd_node_t *)cb_data;
 	ibnd_node_t *node;
+	int node_is_new = 0;
+	uint64_t node_guid = mad_get_field64(node_info, 0, IB_NODE_GUID_F);
+	uint64_t port_guid = mad_get_field64(node_info, 0, IB_NODE_PORT_GUID_F);
+	int port_num = mad_get_field(node_info, 0, IB_NODE_LOCAL_PORT_F);
+	ibnd_port_t *port = NULL;
 
-	for (node = fabric->nodestbl[hash]; node; node = node->htnext)
-		if (node->guid == new->guid)
-			return node;
+	node = ibnd_find_node_guid(fabric, node_guid);
+	if (!node) {
+		node = create_node(engine, &smp->path, node_info);
+		if (!node)
+			return -1;
+		node_is_new = 1;
+	}
+	IBND_DEBUG("Found %s node GUID %lx (%s)\n",
+		   (node_is_new) ? "new": "old", node->guid,
+		   portid2str(&smp->path));
 
-	return NULL;
+	port = node->ports[port_num];
+	if (!port) {
+		/* If we have not see this port before create a shell for it */
+		port = node->ports[port_num] = calloc(1, sizeof(*port));
+		port->node = node;
+		port->portnum = port_num;
+	}
+	port->guid = port_guid;
+
+	if (rem_node == NULL) /* this is the start node */
+		fabric->from_node = node;
+	else {
+		/* link ports... */
+		int rem_port_num = get_last_port(&smp->path);
+
+		if (!rem_node->ports[rem_port_num]) {
+			IBND_ERROR("Internal Error; "
+				   "Node(%p) %lx Port %d no port created!?!?!?\n\n",
+				   rem_node, rem_node->guid, rem_port_num);
+			return (-1);
+		}
+
+		link_ports(node, port, rem_node, rem_node->ports[rem_port_num]);
+	}
+
+	if (!node_is_new)
+		return 0;
+

infiniband-diags/libibnetdisc/src/ibnetdisc.c +3 -0

+
+	return 0;
+}

infiniband-diags/libibnetdisc/src/internal.h +33 -5

 #define	IBND_DEBUG(fmt, ...) \
 	if (ibdebug) { \

@@ -52,16 +53,44 @@
 
 #define MAXHOPS         63
 
-typedef struct ibnd_node_scan {
-	ibnd_node_t *node;
-	struct ibnd_node_scan *dnext;	/* nodesdist next */
-} ibnd_node_scan_t;
+#define DEFAULT_MAX_SMP_ON_WIRE 2
 
 typedef struct ibnd_scan {
-	ibnd_node_scan_t *nodesdist[MAXHOPS + 1];
 	ib_portid_t selfportid;
+	ibnd_fabric_t *fabric;
 } ibnd_scan_t;
 
+
+typedef struct ibnd_smp ibnd_smp_t;
+typedef struct smp_engine smp_engine_t;
+typedef int (*smp_comp_cb_t)(smp_engine_t *engine, ibnd_smp_t * smp,
+			     uint8_t *mad_resp, void *cb_data);
+struct ibnd_smp {
+	cl_map_item_t on_wire;
+	struct ibnd_smp * qnext;
+	smp_comp_cb_t cb;
+	void * cb_data;
+	ib_portid_t path;
+	ib_rpc_t rpc;
+};
+struct smp_engine {
+	struct ibmad_port *ibmad_port;
+	ibnd_smp_t *smp_queue_head;
+	ibnd_smp_t *smp_queue_tail;
+	void * user_data;
+	cl_qmap_t smps_on_wire;
+	int num_smps_outstanding;
+	int max_smps_on_wire;
+};
+
+void smp_engine_init(smp_engine_t * engine, struct ibmad_port *ibmad_port,
+		     void * user_data, int max_smps_on_wire);
+int issue_smp(smp_engine_t *engine, ib_portid_t * portid,
+	      unsigned attrid, unsigned mod,
+	      smp_comp_cb_t cb, void * cb_data);
+int process_mads(smp_engine_t *engine);
+void smp_engine_destroy(smp_engine_t *engine);
+
 void add_to_nodeguid_hash(ibnd_node_t * node, ibnd_node_t * hash[]);
 
 void add_to_portguid_hash(ibnd_port_t * port, ibnd_port_t * hash[]);

infiniband-diags/libibnetdisc/src/query_smp.c new file +118 -0

new file mode 100644
index 0000000..5571314
@@ -0,0 +1,249 @@
+/*
+ * Copyright (c) 2010 Lawrence Livermore National Laboratory
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      - Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *
+ *      - Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ */
+
+#include <errno.h>
+#include <infiniband/ibnetdisc.h>
+#include <infiniband/umad.h>
+#include "internal.h"
+
+void
+queue_smp(smp_engine_t *engine, ibnd_smp_t *smp)
+{
+	smp->qnext = NULL;
+	if (!engine->smp_queue_head) {
+		engine->smp_queue_head = smp;
+		engine->smp_queue_tail = smp;
+	} else {
+		engine->smp_queue_tail->qnext = smp;
+		engine->smp_queue_tail = smp;
+	}
+}
+
+ibnd_smp_t *
+get_smp(smp_engine_t *engine)
+{
+	ibnd_smp_t *head = engine->smp_queue_head;
+	ibnd_smp_t *tail = engine->smp_queue_tail;
+	ibnd_smp_t *rc = head;
+	if (head) {
+		if (tail == head)
+			engine->smp_queue_tail = NULL;
+		engine->smp_queue_head = head->qnext;
+	}
+	return rc;
+}
+
+int send_smp(ibnd_smp_t * smp, struct ibmad_port *srcport)
+{
+	int rc = 0;
+	uint8_t umad[1024];
+	ib_rpc_t *rpc = &smp->rpc;
+
+	memset(umad, 0, umad_size() + IB_MAD_SIZE);
+
+	if ((rc = mad_build_pkt(umad, &smp->rpc, &smp->path, NULL, NULL))
+	    < 0) {
+		IBND_ERROR("mad_build_pkt failed; %d", rc);
+		return rc;
+	}
+
+	if ((rc = umad_send(mad_rpc_portid(srcport),
+			    mad_rpc_class_agent(srcport, rpc->mgtclass),
+			    umad, IB_MAD_SIZE,
+			    mad_get_timeout(srcport, rpc->timeout),
+			    mad_get_retries(srcport))) < 0) {
+		IBND_ERROR("send failed; %d", rc);
+		return rc;
+	}
+
+	return 0;
+}
+
+static int process_smp_queue(smp_engine_t *engine)
+{
+	int rc = 0;
+	ibnd_smp_t *smp;
+	while (cl_qmap_count(&engine->smps_on_wire) < engine->max_smps_on_wire) {
+		smp = get_smp(engine);
+		if (!smp)
+			return 0;
+
+		cl_qmap_insert(&engine->smps_on_wire, (uint32_t)smp->rpc.trid,
+			       (cl_map_item_t *)smp);
+		if ((rc = send_smp(smp, engine->ibmad_port)) != 0)
+			return rc;
+	}
+	return 0;
+}
+
+int issue_smp(smp_engine_t *engine, ib_portid_t * portid,
+	      unsigned attrid, unsigned mod,
+	      smp_comp_cb_t cb, void * cb_data)
+{
+	ibnd_smp_t *smp = calloc(1, sizeof *smp);
+	if (!smp) {
+		IBND_ERROR("OOM");
+		return -ENOMEM;
+	}
+

infiniband-diags/libibnetdisc/src/query_smp.c new file +104 -0

+
+	if ((portid->lid <= 0) ||
+	    (portid->drpath.drslid == 0xffff) ||
+	    (portid->drpath.drdlid == 0xffff))
+		smp->rpc.mgtclass = IB_SMI_DIRECT_CLASS; /* direct SMI */
+	else
+		smp->rpc.mgtclass = IB_SMI_CLASS; /* Lid routed SMI */
+
+	portid->sl = 0;
+	portid->qp = 0;
+
+	engine->num_smps_outstanding++;
+	queue_smp(engine, smp);
+	return (process_smp_queue(engine));
+}
+
+int process_one_recv(smp_engine_t *engine)
+{
+	int rc = 0;
+	int status = 0;
+	ibnd_smp_t *smp;
+	uint8_t *mad;
+	uint32_t trid;
+	uint8_t umad[umad_size() + IB_MAD_SIZE];
+	int length = umad_size() + IB_MAD_SIZE;
+
+	memset(umad, 0, sizeof(umad));
+
+	/* wait for the next message */
+	if ((rc = umad_recv(mad_rpc_portid(engine->ibmad_port), umad, &length,
+			    0)) < 0) {
+		if (rc == -EWOULDBLOCK)
+			return 0;
+		IBND_ERROR("umad_recv failed: %d\n", rc);
+		return -1;
+	}
+
+	rc = process_smp_queue(engine);
+
+	mad = umad_get_mad(umad);
+	trid = (uint32_t)mad_get_field64(mad, 0, IB_MAD_TRID_F);
+
+	smp = (ibnd_smp_t *)cl_qmap_remove(&engine->smps_on_wire, trid);
+	if ((cl_map_item_t *)smp == cl_qmap_end(&engine->smps_on_wire)) {
+		IBND_ERROR("Failed to find matching smp for trid (%x)\n",
+			   trid);
+		return -1;
+	}
+
+	if (rc)
+		goto error;
+
+	if ((status = umad_status(umad))) {
+		IBND_ERROR("umad (%s Attr 0x%x:%u) bad status %d; %s\n",
+			portid2str(&smp->path),
+			smp->rpc.attr.id, smp->rpc.attr.mod,
+			status, strerror(status));
+	} else if ((status = mad_get_field(mad, 0, IB_DRSMP_STATUS_F))) {
+			IBND_ERROR("mad (%s Attr 0x%x:%u) bad status 0x%x\n",
+				   portid2str(&smp->path),
+				   smp->rpc.attr.id, smp->rpc.attr.mod,
+				   status);
+	} else
+		rc = smp->cb(engine, smp, mad, smp->cb_data);
+
+error:
+	free(smp);
+	engine->num_smps_outstanding--;
+	return (rc);
+}
+
+void smp_engine_init(smp_engine_t * engine, struct ibmad_port *ibmad_port,
+		     void * user_data, int max_smps_on_wire)
+{
+	memset(engine, '\0', sizeof(*engine));
+	engine->ibmad_port = ibmad_port;
+	engine->user_data = user_data;
+	cl_qmap_init(&engine->smps_on_wire);
+	engine->num_smps_outstanding = 0;
+	engine->max_smps_on_wire = max_smps_on_wire;
+}
+
+void smp_engine_destroy(smp_engine_t *engine)
+{
+	cl_map_item_t *item;
+	ibnd_smp_t *smp;
+
+	/* remove queued smps */
+	smp = get_smp(engine);
+	if (smp)
+		IBND_ERROR("outstanding SMP's\n");
+	for (/* */; smp; smp = get_smp(engine)) {
+		free(smp);
+	}
+
+	/* remove smps from the wire queue */
+	item = cl_qmap_head(&engine->smps_on_wire);
+	if (item != cl_qmap_end(&engine->smps_on_wire))
+		IBND_ERROR("outstanding SMP's on wire\n");
+	for (/* */; item != cl_qmap_end(&engine->smps_on_wire);
+	     item = cl_qmap_head(&engine->smps_on_wire)) {
+		cl_qmap_remove_item(&engine->smps_on_wire, item);
+		free(item);
+	}

infiniband-diags/libibnetdisc/src/query_smp.c new file +7 -0

+		while (!cl_is_qmap_empty(&engine->smps_on_wire))
+			if ((rc = process_one_recv(engine)) != 0)
+				return rc;
+	}
+	return 0;
+}
+

Signature · 1 line

1.5.4.5

However see some comments and questions below.

Quoted 2 lines · Sasha Khapyorsky

However see some comments and questions below.

Another thought. What about API like:

	ibnd_discover_fabric(cosnt char *ca_name, unsigned port_num,
			     struct ibnd_config *cfg);

So libibnetdisc will be responsible for opening and closing its own port and in this way will be fully reenterable?

Sasha

If we are going to do something like this why not more like a context?

Something like this?

ibqueryerrors.c

	query_errors_ibmad_port = mad_rpc_open_port(ibd_ca, ibd_ca_port, mgmt_classes, 4);

	{
		ibnd_context_t ctx;
		ctx.ca_name = ibd_ca;
		ctx.ca_port = ibd_ca_port;
		ctx.portid = portid;
		ctx.hops = hops;
		fabric = ibnd_discover_fabric(&ctx);  /* opens it's own ibmad_port */
	}

...

Quoted 2 lines · Sasha Khapyorsky

However see some comments and questions below.

Another thought. What about API like:

	ibnd_discover_fabric(cosnt char *ca_name, unsigned port_num,
			     struct ibnd_config *cfg);

So libibnetdisc will be responsible for opening and closing its own port and in this way will be fully reenterable?

Quoted 11 lines · On Tue, 13 Apr 2010 16:25:31 +0300 Sasha Khapyorsky…

Quoted 2 lines · Sasha Khapyorsky

However see some comments and questions below.

Another thought. What about API like:

	ibnd_discover_fabric(cosnt char *ca_name, unsigned port_num,
			     struct ibnd_config *cfg);

So libibnetdisc will be responsible for opening and closing its own port and in this way will be fully reenterable?

I think we need this... But this alone will not fix ibnetdisc...

I found out why "iblinkinfo -S" is hanging for me. The new algorithm has the library using both libibmad and libibumad calls simultaneously.

These libraries were not designed to be used this way. Therefore, I don't think there is any direct bug in those layers. However, this is why I thought it was better for ibnetdisc to sit on top of ibmad and not use the umad layer.

Anyway, I think it is a mistake to pass an ibmad_port construct directly to this library now. Here is why.

umad_recv in query_smp.c and ib_resolve_self_via in ibnetdisc.c conflicted. The underlying call to _do_madrpc was discarding the response MAD which query_smp.c:umad_recv was expecting.

If we change the API to specify the ca name and port then the library can open 2 ports (or as many as it wants) and use them appropriately. I think this is the only solution which does not involve fixing libibmad.

So what about something like this:

int ibnd_discover_fabric(ibnd_fabric_t **fabric,

			    cosnt char *ca_name,  <== could we even default this?

			    struct ibnd_config *cfg);

I don't mind the ibnd_config_t struct but I don't think it should be visible to the user. Make it opaque and use "set" functions. Something like.

ibnd_fabric_t *fabric;
ibnd_config_t cfg;
ib_portid_t * from;

ibnd_set_hops(&cfg, hops); <== default -1
ibnd_set_port_num(&cfg, port_num); <== default 1
ibnd_set_max_smps(&cfg, max_smps); <== default 2
ibnd_set_from_node(&cfg, from); <== default NULL

if (ibnd_discover_fabric(&fabric, "foo", &cfg)) { <== anything not in cfg is
defaulted here
fprintf(stderr, "Wow it failed\n");
}

This allows us to change ibnd_config structure any time we want without affecting the API. I don't think the "pad" you used is a good idea.

Also since we are breaking the API we might as well return the fabric as a parameter and have an error code. But I could go either way on this one.

Ira

[*] query_smp.c probably should have it's own timeout here but we can discuss later.

[#] What sucks about this is that libibmad already has the functionality to open the umad port and configure it (50 line function). Now we will be duplicating this functionality.

Hi Ira,

Quoted 24 lines · Ira Weiny

Quoted 11 lines · On Tue, 13 Apr 2010 16:25:31 +0300 Sasha Khapyorsky…

Quoted 2 lines · Sasha Khapyorsky

However see some comments and questions below.

Another thought. What about API like:

	ibnd_discover_fabric(cosnt char *ca_name, unsigned port_num,
			     struct ibnd_config *cfg);

So libibnetdisc will be responsible for opening and closing its own port and in this way will be fully reenterable?

I think we need this... But this alone will not fix ibnetdisc...

I found out why "iblinkinfo -S" is hanging for me. The new algorithm has the library using both libibmad and libibumad calls simultaneously.

These libraries were not designed to be used this way. Therefore, I don't think there is any direct bug in those layers. However, this is why I thought it was better for ibnetdisc to sit on top of ibmad and not use the umad layer.

Quoted 6 lines

Anyway, I think it is a mistake to pass an ibmad_port construct directly to this library now. Here is why.

umad_recv in query_smp.c and ib_resolve_self_via in ibnetdisc.c conflicted. The underlying call to _do_madrpc was discarding the response MAD which query_smp.c:umad_recv was expecting.

This makes sense for me.

Quoted 4 lines

If we change the API to specify the ca name and port then the library can open 2 ports (or as many as it wants) and use them appropriately. I think this is the only solution which does not involve fixing libibmad.

Quoted 4 lines

So what about something like this:

int ibnd_discover_fabric(ibnd_fabric_t **fabric,

			    cosnt char *ca_name,  <== could we even default this?

I would think about ca_name and port_number. And this is of course may have default (NULL, 0).

Quoted 2 lines

			    struct ibnd_config *cfg);

What is wrong with current ibdn_fabric_t *ibnd_discover_fabric(...)? Why do we need an extra parameter?

Quoted 9 lines

I don't mind the ibnd_config_t struct but I don't think it should be visible to the user. Make it opaque and use "set" functions. Something like.

ibnd_fabric_t *fabric;
ibnd_config_t cfg;
ib_portid_t * from;

Quoted 4 lines

ibnd_set_hops(&cfg, hops); <== default -1
ibnd_set_port_num(&cfg, port_num); <== default 1
ibnd_set_max_smps(&cfg, max_smps); <== default 2
ibnd_set_from_node(&cfg, from); <== default NULL

I would prefer to not complicate API with ibnd_set_this() helpers. It would be necessary to add new ones in the future which will lead to API changes.

Quoted 8 lines

if (ibnd_discover_fabric(&fabric, "foo", &cfg)) { <== anything not in cfg is
defaulted here
fprintf(stderr, "Wow it failed\n");
}

This allows us to change ibnd_config structure any time we want without affecting the API. I don't think the "pad" you used is a good idea.

Without padding we will break ABI each time when new field is added to the config structure.

Quoted 10 lines

Also since we are breaking the API we might as well return the fabric as a parameter and have an error code. But I could go either way on this one.

Ira

[*] query_smp.c probably should have it's own timeout here but we can discuss later.

Quoted 3 lines

[#] What sucks about this is that libibmad already has the functionality to open the umad port and configure it (50 line function). Now we will be duplicating this functionality.

I think you can use mad_rpc_open_port(ca_name, port_number, ...) just fine (and so the rest of libibmad stuff) - it will open separate fd.

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://*vger.kernel.org/majordomo-info.html

Quoted 6 lines

Quoted 3 lines

int ibnd_discover_fabric(ibnd_fabric_t **fabric,

			    cosnt char *ca_name,  <== could we even default this?

I would think about ca_name and port_number. And this is of course may have default (NULL, 0).

Ok, ca_name and ca_port will be explicit.

Quoted 1 line

Quoted 4 lines

Quoted 1 line

			    struct ibnd_config *cfg);

What is wrong with current ibdn_fabric_t *ibnd_discover_fabric(...)? Why do we need an extra parameter?

Well we are breaking the interface again so I figure we might as well clean some things up. Returning an int allows us to have a reason for the failure returned to the caller rather than just "NULL". We have cleaned up most of the internals of the library to allow for this.

Quoted 10 lines

Quoted 10 lines

I don't mind the ibnd_config_t struct but I don't think it should be visible to the user. Make it opaque and use "set" functions. Something like.

ibnd_fabric_t *fabric;
ibnd_config_t cfg;
ib_portid_t * from;

Quoted 8 lines

Quoted 4 lines

ibnd_set_hops(&cfg, hops); <== default -1
ibnd_set_port_num(&cfg, port_num); <== default 1
ibnd_set_max_smps(&cfg, max_smps); <== default 2
ibnd_set_from_node(&cfg, from); <== default NULL

I would prefer to not complicate API with ibnd_set_this() helpers. It would be necessary to add new ones in the future which will lead to API changes.

See below.

Quoted 6 lines

Quoted 6 lines

if (ibnd_discover_fabric(&fabric, "foo", &cfg)) { <== anything not in cfg is
defaulted here
fprintf(stderr, "Wow it failed\n");
}

Quoted 5 lines

Quoted 2 lines

This allows us to change ibnd_config structure any time we want without affecting the API. I don't think the "pad" you used is a good idea.

Without padding we will break ABI each time when new field is added to the config structure.

No it does not iff you use the ibnd_set_this() helpers and make the config private.

Quoted 11 lines

Quoted 11 lines

Also since we are breaking the API we might as well return the fabric as a parameter and have an error code. But I could go either way on this one.

Ira

[*] query_smp.c probably should have it's own timeout here but we can discuss later.

Quoted 6 lines

Quoted 3 lines

[#] What sucks about this is that libibmad already has the functionality to open the umad port and configure it (50 line function). Now we will be duplicating this functionality.

I think you can use mad_rpc_open_port(ca_name, port_number, ...) just fine (and so the rest of libibmad stuff) - it will open separate fd.

Yes for the libibmad functionality we can do that. I was speaking of the use of the umad layer. To use that layer we have to duplicate the functionality of mad_rpc_open_port in every tool which requires umad layer access, correct? Right now we are mixing the 2 layers (mad and umad) in saquery (get_bind_handle) as well as libibnetdisc.

I think we need to be careful we don't do this again!

Ira

Quoted 1 line

Quoted 6 lines

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://*vger.kernel.org/majordomo-info.html

Signature · 5 lines

Ira Weiny
Math Programmer/Computer Scientist
Lawrence Livermore National Lab
925-423-8008
[email protected]

Quoted 10 lines · Ira Weiny

Quoted 6 lines

Quoted 3 lines

int ibnd_discover_fabric(ibnd_fabric_t **fabric,

			    cosnt char *ca_name,  <== could we even default this?

I would think about ca_name and port_number. And this is of course may have default (NULL, 0).

Ok, ca_name and ca_port will be explicit.

Quoted 1 line

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

			    struct ibnd_config *cfg);

What is wrong with current ibdn_fabric_t *ibnd_discover_fabric(...)? Why do we need an extra parameter?

Well we are breaking the interface again so I figure we might as well clean some things up. Returning an int allows us to have a reason for the failure returned to the caller rather than just "NULL". We have cleaned up most of the internals of the library to allow for this.

But we want to keep API simple, no?

Quoted 31 lines

Quoted 11 lines

Quoted 11 lines

I don't mind the ibnd_config_t struct but I don't think it should be visible to the user. Make it opaque and use "set" functions. Something like.

ibnd_fabric_t *fabric;
ibnd_config_t cfg;
ib_portid_t * from;

Quoted 8 lines

Quoted 4 lines

ibnd_set_hops(&cfg, hops); <== default -1
ibnd_set_port_num(&cfg, port_num); <== default 1
ibnd_set_max_smps(&cfg, max_smps); <== default 2
ibnd_set_from_node(&cfg, from); <== default NULL

I would prefer to not complicate API with ibnd_set_this() helpers. It would be necessary to add new ones in the future which will lead to API changes.

See below.

Quoted 9 lines

Quoted 8 lines

if (ibnd_discover_fabric(&fabric, "foo", &cfg)) { <== anything not in cfg is
defaulted here
fprintf(stderr, "Wow it failed\n");
}

This allows us to change ibnd_config structure any time we want without affecting the API. I don't think the "pad" you used is a good idea.

Quoted 4 lines

Quoted 2 lines

Without padding we will break ABI each time when new field is added to the config structure.

No it does not iff you use the ibnd_set_this() helpers and make the config private.

In you example 'ibnd_config_t cfg' is on the stack... :)

I would really suggest to keep API simple and useful - to fill up a structure described in header files is much simpler than pass over various helpers calls.

Sasha

Quoted 25 lines

Quoted 11 lines

Quoted 11 lines

Also since we are breaking the API we might as well return the fabric as a parameter and have an error code. But I could go either way on this one.

Ira

[*] query_smp.c probably should have it's own timeout here but we can discuss later.

Quoted 6 lines

Quoted 3 lines

[#] What sucks about this is that libibmad already has the functionality to open the umad port and configure it (50 line function). Now we will be duplicating this functionality.

I think you can use mad_rpc_open_port(ca_name, port_number, ...) just fine (and so the rest of libibmad stuff) - it will open separate fd.

Yes for the libibmad functionality we can do that. I was speaking of the use of the umad layer. To use that layer we have to duplicate the functionality of mad_rpc_open_port in every tool which requires umad layer access, correct? Right now we are mixing the 2 layers (mad and umad) in saquery (get_bind_handle) as well as libibnetdisc.

I think we need to be careful we don't do this again!

Ira

Quoted 1 line

Quoted 15 lines

Quoted 6 lines

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://**vger.kernel.org/majordomo-info.html

Signature · 5 lines

Ira Weiny
Math Programmer/Computer Scientist
Lawrence Livermore National Lab
925-423-8008
[email protected]

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://*vger.kernel.org/majordomo-info.html

Quoted 42 lines

infiniband-diags/libibnetdisc/src/ibnetdisc.c +40 -0

+static int recv_port_info(smp_engine_t *engine, ibnd_smp_t * smp,
+			  uint8_t *mad, void *cb_data)
+{
+	ibnd_fabric_t *fabric = ((ibnd_scan_t *)engine->user_data)->fabric;
+	ibnd_node_t *node = (ibnd_node_t *)cb_data;
+	ibnd_port_t *port;
+	uint8_t *port_info = mad + IB_SMP_DATA_OFFS;
+	uint8_t port_num, local_port;
+
+	port_num = mad_get_field(mad, 0, IB_MAD_ATTRMOD_F);
+	local_port = mad_get_field(port_info, 0, IB_PORT_LOCAL_PORT_F);
+
+	/* this may have been created before */
+	port = node->ports[port_num];
+	if (!port) {
+		port = node->ports[port_num] = calloc(1, sizeof(*port));
+		if (!port) {
+			IBND_ERROR("Failed to allocate port\n");
+			return -1;
+		}
+	}
+
+	memcpy(port->info, port_info, sizeof(port->info));
+	port->node = node;
+	port->portnum = port_num;
+	port->ext_portnum = 0;
+	port->base_lid = (uint16_t) mad_get_field(port->info, 0, IB_PORT_LID_F);
+	port->lmc = (uint8_t) mad_get_field(port->info, 0, IB_PORT_LMC_F);
+
+	if (port_num == 0) {
+		node->smalid = port->base_lid;
+		node->smalmc = port->lmc;
+	} else if (node->type == IB_NODE_SWITCH) {
+		port->base_lid = node->smalid;
+		port->lmc = node->smalmc;
+	}
+
+	add_to_portguid_hash(port, fabric->portstbl);
+
+	debug_port(&smp->path, port);

infiniband-diags/libibnetdisc/src/ibnetdisc.c static int extend_dpath(struct ibmad_port *ibmad_port, ibnd_fabric_t * fabric, · line 211

Quoted 11 lines

infiniband-diags/libibnetdisc/src/ibnetdisc.c +5 -6

-	mad_dump_node_type(type, 64, &node->type, sizeof(int));
-	printf("%s -> %s %s {%016" PRIx64 "} portnum %d base lid %d-%d\"%s\"\n",
-	       portid2str(path), prompt, type, node->guid,
-	       node->type == IB_NODE_SWITCH ? 0 : port->portnum,
-	       port->base_lid,
-	       port->base_lid + (1 << port->lmc) - 1, node->nodedesc);
+	if (port_num &&
+	    (mad_get_field(port->info, 0, IB_PORT_PHYS_STATE_F)
+	    == IB_PORT_PHYS_STATE_LINKUP)
+		&&
+	    (node->type == IB_NODE_SWITCH || node == fabric->from_node)) {

What will happen when ibnetdiscover runs from host connected by two ports to different subnets? Wouldn't it run over both fabrics (following PortInfo querying loop in recv_node_info() below)?

infiniband-diags/libibnetdisc/src/ibnetdisc.c static int extend_dpath(struct ibmad_port *ibmad_port, ibnd_fabric_t * fabric, · line 219

Quoted 9 lines

infiniband-diags/libibnetdisc/src/ibnetdisc.c +8 -0

+
+		ib_portid_t path = smp->path;
+		if (extend_dpath(engine, &path, port_num) != -1)
+			query_node_info(engine, &path, node);
+	}
+
+	return 0;
+}

Quoted 5 lines · Ira Weiny

From: Ira Weiny <[email protected]>
Date: Fri, 22 Jan 2010 17:33:30 -0800
Subject: [PATCH] libibnetdisc: Convert to a multi-smp algorithm

Quoted 7 lines

v3: change DEFAULT_MAX_SMP_ON_WIRE to 2

Allow for multiple SMP's to be on the wire at a single time. This algorithm splits the processing of SMP's to a small smp engine which may be useful to split out in the future.

Signed-off-by Ira Weiny <[email protected]>

Applied. Thanks.

However see some comments and questions below.

Quoted 41 lines

infiniband-diags/libibnetdisc/src/ibnetdisc.c +40 -0

+static int recv_port_info(smp_engine_t *engine, ibnd_smp_t * smp,
+			  uint8_t *mad, void *cb_data)
+{
+	ibnd_fabric_t *fabric = ((ibnd_scan_t *)engine->user_data)->fabric;
+	ibnd_node_t *node = (ibnd_node_t *)cb_data;
+	ibnd_port_t *port;
+	uint8_t *port_info = mad + IB_SMP_DATA_OFFS;
+	uint8_t port_num, local_port;
+
+	port_num = mad_get_field(mad, 0, IB_MAD_ATTRMOD_F);
+	local_port = mad_get_field(port_info, 0, IB_PORT_LOCAL_PORT_F);
+
+	/* this may have been created before */
+	port = node->ports[port_num];
+	if (!port) {
+		port = node->ports[port_num] = calloc(1, sizeof(*port));
+		if (!port) {
+			IBND_ERROR("Failed to allocate port\n");
+			return -1;
+		}
+	}
+
+	memcpy(port->info, port_info, sizeof(port->info));
+	port->node = node;
+	port->portnum = port_num;
+	port->ext_portnum = 0;
+	port->base_lid = (uint16_t) mad_get_field(port->info, 0, IB_PORT_LID_F);
+	port->lmc = (uint8_t) mad_get_field(port->info, 0, IB_PORT_LMC_F);
+
+	if (port_num == 0) {
+		node->smalid = port->base_lid;
+		node->smalmc = port->lmc;
+	} else if (node->type == IB_NODE_SWITCH) {
+		port->base_lid = node->smalid;
+		port->lmc = node->smalmc;
+	}
+
+	add_to_portguid_hash(port, fabric->portstbl);
+
+	debug_port(&smp->path, port);

infiniband-diags/libibnetdisc/src/ibnetdisc.c static int extend_dpath(struct ibmad_port *ibmad_port, ibnd_fabric_t * fabric, · line 211

Quoted 11 lines

infiniband-diags/libibnetdisc/src/ibnetdisc.c +5 -6

-	mad_dump_node_type(type, 64, &node->type, sizeof(int));
-	printf("%s -> %s %s {%016" PRIx64 "} portnum %d base lid %d-%d\"%s\"\n",
-	       portid2str(path), prompt, type, node->guid,
-	       node->type == IB_NODE_SWITCH ? 0 : port->portnum,
-	       port->base_lid,
-	       port->base_lid + (1 << port->lmc) - 1, node->nodedesc);
+	if (port_num &&
+	    (mad_get_field(port->info, 0, IB_PORT_PHYS_STATE_F)
+	    == IB_PORT_PHYS_STATE_LINKUP)
+		&&
+	    (node->type == IB_NODE_SWITCH || node == fabric->from_node)) {

What will happen when ibnetdiscover runs from host connected by two ports to different subnets? Wouldn't it run over both fabrics (following PortInfo querying loop in recv_node_info() below)?

Quoted 9 lines

infiniband-diags/libibnetdisc/src/ibnetdisc.c +8 -0

+
+		ib_portid_t path = smp->path;
+		if (extend_dpath(engine, &path, port_num) != -1)
+			query_node_info(engine, &path, node);
+	}
+
+	return 0;
+}

[snip]

Quoted 57 lines

infiniband-diags/libibnetdisc/src/ibnetdisc.c +50 -4

+static int recv_node_info(smp_engine_t *engine, ibnd_smp_t * smp,
+			  uint8_t *mad, void *cb_data)
+{
+	ibnd_fabric_t *fabric = ((ibnd_scan_t *)engine->user_data)->fabric;
+	int i = 0;
+	uint8_t *node_info = mad + IB_SMP_DATA_OFFS;
+	ibnd_node_t * rem_node = (ibnd_node_t *)cb_data;
 	ibnd_node_t *node;
+	int node_is_new = 0;
+	uint64_t node_guid = mad_get_field64(node_info, 0, IB_NODE_GUID_F);
+	uint64_t port_guid = mad_get_field64(node_info, 0, IB_NODE_PORT_GUID_F);
+	int port_num = mad_get_field(node_info, 0, IB_NODE_LOCAL_PORT_F);
+	ibnd_port_t *port = NULL;
 
-	for (node = fabric->nodestbl[hash]; node; node = node->htnext)
-		if (node->guid == new->guid)
-			return node;
+	node = ibnd_find_node_guid(fabric, node_guid);
+	if (!node) {
+		node = create_node(engine, &smp->path, node_info);
+		if (!node)
+			return -1;
+		node_is_new = 1;
+	}
+	IBND_DEBUG("Found %s node GUID %lx (%s)\n",
+		   (node_is_new) ? "new": "old", node->guid,
+		   portid2str(&smp->path));
 
-	return NULL;
+	port = node->ports[port_num];
+	if (!port) {
+		/* If we have not see this port before create a shell for it */
+		port = node->ports[port_num] = calloc(1, sizeof(*port));
+		port->node = node;
+		port->portnum = port_num;
+	}
+	port->guid = port_guid;
+
+	if (rem_node == NULL) /* this is the start node */
+		fabric->from_node = node;
+	else {
+		/* link ports... */
+		int rem_port_num = get_last_port(&smp->path);
+
+		if (!rem_node->ports[rem_port_num]) {
+			IBND_ERROR("Internal Error; "
+				   "Node(%p) %lx Port %d no port created!?!?!?\n\n",
+				   rem_node, rem_node->guid, rem_port_num);
+			return (-1);
+		}
+
+		link_ports(node, port, rem_node, rem_node->ports[rem_port_num]);
+	}
+
+	if (!node_is_new)
+		return 0;
+

infiniband-diags/libibnetdisc/src/ibnetdisc.c static int extend_dpath(struct ibmad_port *ibmad_port, ibnd_fabric_t * fabric, · line 346

Quoted 10 lines

infiniband-diags/libibnetdisc/src/ibnetdisc.c +10 -0

+	query_node_desc(engine, &smp->path, node);
+
+	if (node->type == IB_NODE_SWITCH)
+		query_switch_info(engine, &smp->path, node);
+
+	/* process all the ports on this node */
+	for (i = (node->type == IB_NODE_SWITCH) ? 0 : 1;
+		i <= node->numports; i++) {
+			query_port_info(engine, &smp->path, node, i);
+	}

This one.

Quoted 3 lines

infiniband-diags/libibnetdisc/src/ibnetdisc.c +3 -0

+
+	return 0;
+}

[snip]

Quoted 7 lines

infiniband-diags/libibnetdisc/src/internal.h +0 -0

index 348bd0f..61b644d 100644
@@ -39,6 +39,7 @@
 #define _INTERNAL_H_
 

infiniband-diags/libibnetdisc/src/internal.h line 42

Quoted 2 lines

infiniband-diags/libibnetdisc/src/internal.h +1 -0

 #include <infiniband/ibnetdisc.h>
+#include <complib/cl_qmap.h>

I'm not really happy with complib dependency introduction - we had an issues with it in the past.

Also I think that we can implement simpler transaction tracker (likely even more efficient) by storing sent MADs in cyclic array and encoding its index as part of TRID.

Quoted 178 lines

infiniband-diags/libibnetdisc/src/internal.h +33 -5

 #define	IBND_DEBUG(fmt, ...) \
 	if (ibdebug) { \

@@ -52,16 +53,44 @@
 
 #define MAXHOPS         63
 
-typedef struct ibnd_node_scan {
-	ibnd_node_t *node;
-	struct ibnd_node_scan *dnext;	/* nodesdist next */
-} ibnd_node_scan_t;
+#define DEFAULT_MAX_SMP_ON_WIRE 2
 
 typedef struct ibnd_scan {
-	ibnd_node_scan_t *nodesdist[MAXHOPS + 1];
 	ib_portid_t selfportid;
+	ibnd_fabric_t *fabric;
 } ibnd_scan_t;
 
+
+typedef struct ibnd_smp ibnd_smp_t;
+typedef struct smp_engine smp_engine_t;
+typedef int (*smp_comp_cb_t)(smp_engine_t *engine, ibnd_smp_t * smp,
+			     uint8_t *mad_resp, void *cb_data);
+struct ibnd_smp {
+	cl_map_item_t on_wire;
+	struct ibnd_smp * qnext;
+	smp_comp_cb_t cb;
+	void * cb_data;
+	ib_portid_t path;
+	ib_rpc_t rpc;
+};
+struct smp_engine {
+	struct ibmad_port *ibmad_port;
+	ibnd_smp_t *smp_queue_head;
+	ibnd_smp_t *smp_queue_tail;
+	void * user_data;
+	cl_qmap_t smps_on_wire;
+	int num_smps_outstanding;
+	int max_smps_on_wire;
+};
+
+void smp_engine_init(smp_engine_t * engine, struct ibmad_port *ibmad_port,
+		     void * user_data, int max_smps_on_wire);
+int issue_smp(smp_engine_t *engine, ib_portid_t * portid,
+	      unsigned attrid, unsigned mod,
+	      smp_comp_cb_t cb, void * cb_data);
+int process_mads(smp_engine_t *engine);
+void smp_engine_destroy(smp_engine_t *engine);
+
 void add_to_nodeguid_hash(ibnd_node_t * node, ibnd_node_t * hash[]);
 
 void add_to_portguid_hash(ibnd_port_t * port, ibnd_port_t * hash[]);

infiniband-diags/libibnetdisc/src/query_smp.c new file +118 -0

new file mode 100644
index 0000000..5571314
@@ -0,0 +1,249 @@
+/*
+ * Copyright (c) 2010 Lawrence Livermore National Laboratory
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      - Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *
+ *      - Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ */
+
+#include <errno.h>
+#include <infiniband/ibnetdisc.h>
+#include <infiniband/umad.h>
+#include "internal.h"
+
+void
+queue_smp(smp_engine_t *engine, ibnd_smp_t *smp)
+{
+	smp->qnext = NULL;
+	if (!engine->smp_queue_head) {
+		engine->smp_queue_head = smp;
+		engine->smp_queue_tail = smp;
+	} else {
+		engine->smp_queue_tail->qnext = smp;
+		engine->smp_queue_tail = smp;
+	}
+}
+
+ibnd_smp_t *
+get_smp(smp_engine_t *engine)
+{
+	ibnd_smp_t *head = engine->smp_queue_head;
+	ibnd_smp_t *tail = engine->smp_queue_tail;
+	ibnd_smp_t *rc = head;
+	if (head) {
+		if (tail == head)
+			engine->smp_queue_tail = NULL;
+		engine->smp_queue_head = head->qnext;
+	}
+	return rc;
+}
+
+int send_smp(ibnd_smp_t * smp, struct ibmad_port *srcport)
+{
+	int rc = 0;
+	uint8_t umad[1024];
+	ib_rpc_t *rpc = &smp->rpc;
+
+	memset(umad, 0, umad_size() + IB_MAD_SIZE);
+
+	if ((rc = mad_build_pkt(umad, &smp->rpc, &smp->path, NULL, NULL))
+	    < 0) {
+		IBND_ERROR("mad_build_pkt failed; %d", rc);
+		return rc;
+	}
+
+	if ((rc = umad_send(mad_rpc_portid(srcport),
+			    mad_rpc_class_agent(srcport, rpc->mgtclass),
+			    umad, IB_MAD_SIZE,
+			    mad_get_timeout(srcport, rpc->timeout),
+			    mad_get_retries(srcport))) < 0) {
+		IBND_ERROR("send failed; %d", rc);
+		return rc;
+	}
+
+	return 0;
+}
+
+static int process_smp_queue(smp_engine_t *engine)
+{
+	int rc = 0;
+	ibnd_smp_t *smp;
+	while (cl_qmap_count(&engine->smps_on_wire) < engine->max_smps_on_wire) {
+		smp = get_smp(engine);
+		if (!smp)
+			return 0;
+
+		cl_qmap_insert(&engine->smps_on_wire, (uint32_t)smp->rpc.trid,
+			       (cl_map_item_t *)smp);
+		if ((rc = send_smp(smp, engine->ibmad_port)) != 0)
+			return rc;
+	}
+	return 0;
+}
+
+int issue_smp(smp_engine_t *engine, ib_portid_t * portid,
+	      unsigned attrid, unsigned mod,
+	      smp_comp_cb_t cb, void * cb_data)
+{
+	ibnd_smp_t *smp = calloc(1, sizeof *smp);
+	if (!smp) {
+		IBND_ERROR("OOM");
+		return -ENOMEM;
+	}
+

infiniband-diags/libibnetdisc/src/query_smp.c line 128

Quoted 10 lines

infiniband-diags/libibnetdisc/src/query_smp.c new file +10 -0

+	smp->cb = cb;
+	smp->cb_data = cb_data;
+	smp->path = *portid;
+	smp->rpc.method = IB_MAD_METHOD_GET;
+	smp->rpc.attr.id = attrid;
+	smp->rpc.attr.mod = mod;
+	smp->rpc.timeout = mad_get_timeout(engine->ibmad_port, 0);
+	smp->rpc.datasz = IB_SMP_DATA_SIZE;
+	smp->rpc.dataoffs = IB_SMP_DATA_OFFS;
+	smp->rpc.trid = mad_trid();

BTW, by reviewing mad_trid() code in libibmad:

uint64_t mad_trid(void)

{
	static uint64_t base;
	static uint64_t trid;
	uint64_t next;

	if (!base) {
		srandom((int)time(0) * getpid());
		base = random();
		trid = random();
	}
	next = ++trid | (base << 32);
	return next;
}

For me it doesn't look as thread safe function.

Quoted 112 lines

infiniband-diags/libibnetdisc/src/query_smp.c new file +111 -0

+
+	if ((portid->lid <= 0) ||
+	    (portid->drpath.drslid == 0xffff) ||
+	    (portid->drpath.drdlid == 0xffff))
+		smp->rpc.mgtclass = IB_SMI_DIRECT_CLASS; /* direct SMI */
+	else
+		smp->rpc.mgtclass = IB_SMI_CLASS; /* Lid routed SMI */
+
+	portid->sl = 0;
+	portid->qp = 0;
+
+	engine->num_smps_outstanding++;
+	queue_smp(engine, smp);
+	return (process_smp_queue(engine));
+}
+
+int process_one_recv(smp_engine_t *engine)
+{
+	int rc = 0;
+	int status = 0;
+	ibnd_smp_t *smp;
+	uint8_t *mad;
+	uint32_t trid;
+	uint8_t umad[umad_size() + IB_MAD_SIZE];
+	int length = umad_size() + IB_MAD_SIZE;
+
+	memset(umad, 0, sizeof(umad));
+
+	/* wait for the next message */
+	if ((rc = umad_recv(mad_rpc_portid(engine->ibmad_port), umad, &length,
+			    0)) < 0) {
+		if (rc == -EWOULDBLOCK)
+			return 0;
+		IBND_ERROR("umad_recv failed: %d\n", rc);
+		return -1;
+	}
+
+	rc = process_smp_queue(engine);
+
+	mad = umad_get_mad(umad);
+	trid = (uint32_t)mad_get_field64(mad, 0, IB_MAD_TRID_F);
+
+	smp = (ibnd_smp_t *)cl_qmap_remove(&engine->smps_on_wire, trid);
+	if ((cl_map_item_t *)smp == cl_qmap_end(&engine->smps_on_wire)) {
+		IBND_ERROR("Failed to find matching smp for trid (%x)\n",
+			   trid);
+		return -1;
+	}
+
+	if (rc)
+		goto error;
+
+	if ((status = umad_status(umad))) {
+		IBND_ERROR("umad (%s Attr 0x%x:%u) bad status %d; %s\n",
+			portid2str(&smp->path),
+			smp->rpc.attr.id, smp->rpc.attr.mod,
+			status, strerror(status));
+	} else if ((status = mad_get_field(mad, 0, IB_DRSMP_STATUS_F))) {
+			IBND_ERROR("mad (%s Attr 0x%x:%u) bad status 0x%x\n",
+				   portid2str(&smp->path),
+				   smp->rpc.attr.id, smp->rpc.attr.mod,
+				   status);
+	} else
+		rc = smp->cb(engine, smp, mad, smp->cb_data);
+
+error:
+	free(smp);
+	engine->num_smps_outstanding--;
+	return (rc);
+}
+
+void smp_engine_init(smp_engine_t * engine, struct ibmad_port *ibmad_port,
+		     void * user_data, int max_smps_on_wire)
+{
+	memset(engine, '\0', sizeof(*engine));
+	engine->ibmad_port = ibmad_port;
+	engine->user_data = user_data;
+	cl_qmap_init(&engine->smps_on_wire);
+	engine->num_smps_outstanding = 0;
+	engine->max_smps_on_wire = max_smps_on_wire;
+}
+
+void smp_engine_destroy(smp_engine_t *engine)
+{
+	cl_map_item_t *item;
+	ibnd_smp_t *smp;
+
+	/* remove queued smps */
+	smp = get_smp(engine);
+	if (smp)
+		IBND_ERROR("outstanding SMP's\n");
+	for (/* */; smp; smp = get_smp(engine)) {
+		free(smp);
+	}
+
+	/* remove smps from the wire queue */
+	item = cl_qmap_head(&engine->smps_on_wire);
+	if (item != cl_qmap_end(&engine->smps_on_wire))
+		IBND_ERROR("outstanding SMP's on wire\n");
+	for (/* */; item != cl_qmap_end(&engine->smps_on_wire);
+	     item = cl_qmap_head(&engine->smps_on_wire)) {
+		cl_qmap_remove_item(&engine->smps_on_wire, item);
+		free(item);
+	}
+
+	engine->num_smps_outstanding = 0;
+}
+
+int process_mads(smp_engine_t *engine)
+{
+	int rc = 0;

infiniband-diags/libibnetdisc/src/query_smp.c line 242

Quoted 3 lines

infiniband-diags/libibnetdisc/src/query_smp.c new file +3 -0

+	while (engine->num_smps_outstanding > 0) {
+		if ((rc = process_smp_queue(engine)) != 0)
+			return rc;

Is it really needed to run process_smp_queue() here (assuming that it is already executed in issue_smp() and process_one_recv())? Or this is just for handling process_one_recv() error case? If so wouldn't it better to run it there?

Sasha

Quoted 10 lines

infiniband-diags/libibnetdisc/src/query_smp.c new file +7 -0

+		while (!cl_is_qmap_empty(&engine->smps_on_wire))
+			if ((rc = process_one_recv(engine)) != 0)
+				return rc;
+	}
+	return 0;
+}
+

Signature · 1 line

1.5.4.5

Quoted 60 lines

(quoted diff) +52 -4

+static int recv_node_info(smp_engine_t *engine, ibnd_smp_t * smp,
+			  uint8_t *mad, void *cb_data)
+{
+	ibnd_fabric_t *fabric = ((ibnd_scan_t *)engine->user_data)->fabric;
+	int i = 0;
+	uint8_t *node_info = mad + IB_SMP_DATA_OFFS;
+	ibnd_node_t * rem_node = (ibnd_node_t *)cb_data;
 	ibnd_node_t *node;
+	int node_is_new = 0;
+	uint64_t node_guid = mad_get_field64(node_info, 0, IB_NODE_GUID_F);
+	uint64_t port_guid = mad_get_field64(node_info, 0, IB_NODE_PORT_GUID_F);
+	int port_num = mad_get_field(node_info, 0, IB_NODE_LOCAL_PORT_F);
+	ibnd_port_t *port = NULL;
 
-	for (node = fabric->nodestbl[hash]; node; node = node->htnext)
-		if (node->guid == new->guid)
-			return node;
+	node = ibnd_find_node_guid(fabric, node_guid);
+	if (!node) {
+		node = create_node(engine, &smp->path, node_info);
+		if (!node)
+			return -1;
+		node_is_new = 1;
+	}
+	IBND_DEBUG("Found %s node GUID %lx (%s)\n",
+		   (node_is_new) ? "new": "old", node->guid,
+		   portid2str(&smp->path));
 
-	return NULL;
+	port = node->ports[port_num];
+	if (!port) {
+		/* If we have not see this port before create a shell for it */
+		port = node->ports[port_num] = calloc(1, sizeof(*port));
+		port->node = node;
+		port->portnum = port_num;
+	}
+	port->guid = port_guid;
+
+	if (rem_node == NULL) /* this is the start node */
+		fabric->from_node = node;
+	else {
+		/* link ports... */
+		int rem_port_num = get_last_port(&smp->path);
+
+		if (!rem_node->ports[rem_port_num]) {
+			IBND_ERROR("Internal Error; "
+				   "Node(%p) %lx Port %d no port created!?!?!?\n\n",
+				   rem_node, rem_node->guid, rem_port_num);
+			return (-1);
+		}
+
+		link_ports(node, port, rem_node, rem_node->ports[rem_port_num]);
+	}
+
+	if (!node_is_new)
+		return 0;
+
+	query_node_desc(engine, &smp->path, node);
+

Review

Quoted 8 lines

(quoted diff) +8 -0

+	if (node->type == IB_NODE_SWITCH)
+		query_switch_info(engine, &smp->path, node);
+
+	/* process all the ports on this node */
+	for (i = (node->type == IB_NODE_SWITCH) ? 0 : 1;
+		i <= node->numports; i++) {
+			query_port_info(engine, &smp->path, node, i);
+	}

Quoted 60 lines · Sasha Khapyorsky

Quoted 60 lines

(quoted diff) +52 -4

+static int recv_node_info(smp_engine_t *engine, ibnd_smp_t * smp,
+			  uint8_t *mad, void *cb_data)
+{
+	ibnd_fabric_t *fabric = ((ibnd_scan_t *)engine->user_data)->fabric;
+	int i = 0;
+	uint8_t *node_info = mad + IB_SMP_DATA_OFFS;
+	ibnd_node_t * rem_node = (ibnd_node_t *)cb_data;
 	ibnd_node_t *node;
+	int node_is_new = 0;
+	uint64_t node_guid = mad_get_field64(node_info, 0, IB_NODE_GUID_F);
+	uint64_t port_guid = mad_get_field64(node_info, 0, IB_NODE_PORT_GUID_F);
+	int port_num = mad_get_field(node_info, 0, IB_NODE_LOCAL_PORT_F);
+	ibnd_port_t *port = NULL;
 
-	for (node = fabric->nodestbl[hash]; node; node = node->htnext)
-		if (node->guid == new->guid)
-			return node;
+	node = ibnd_find_node_guid(fabric, node_guid);
+	if (!node) {
+		node = create_node(engine, &smp->path, node_info);
+		if (!node)
+			return -1;
+		node_is_new = 1;
+	}
+	IBND_DEBUG("Found %s node GUID %lx (%s)\n",
+		   (node_is_new) ? "new": "old", node->guid,
+		   portid2str(&smp->path));
 
-	return NULL;
+	port = node->ports[port_num];
+	if (!port) {
+		/* If we have not see this port before create a shell for it */
+		port = node->ports[port_num] = calloc(1, sizeof(*port));
+		port->node = node;
+		port->portnum = port_num;
+	}
+	port->guid = port_guid;
+
+	if (rem_node == NULL) /* this is the start node */
+		fabric->from_node = node;
+	else {
+		/* link ports... */
+		int rem_port_num = get_last_port(&smp->path);
+
+		if (!rem_node->ports[rem_port_num]) {
+			IBND_ERROR("Internal Error; "
+				   "Node(%p) %lx Port %d no port created!?!?!?\n\n",
+				   rem_node, rem_node->guid, rem_port_num);
+			return (-1);
+		}
+
+		link_ports(node, port, rem_node, rem_node->ports[rem_port_num]);
+	}
+
+	if (!node_is_new)
+		return 0;
+
+	query_node_desc(engine, &smp->path, node);
+

Quoted 8 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+	if (node->type == IB_NODE_SWITCH)
+		query_switch_info(engine, &smp->path, node);
+
+	/* process all the ports on this node */
+	for (i = (node->type == IB_NODE_SWITCH) ? 0 : 1;
+		i <= node->numports; i++) {
+			query_port_info(engine, &smp->path, node, i);
+	}

Actually it seems possible to save some MADs by not querying CA/Router ports which is not connected to our fabric. Something like this:

infiniband-diags/libibnetdisc/src/ibnetdisc.c +9 -12

index 2ce15b7..1c7d6f2 100644
@@ -339,19 +339,16 @@ static int recv_node_info(smp_engine_t * engine, ibnd_smp_t * smp,
 		link_ports(node, port, rem_node, rem_node->ports[rem_port_num]);
 	}
 
-	if (!node_is_new)
-		return 0;
-
-	query_node_desc(engine, &smp->path, node);
-
-	if (node->type == IB_NODE_SWITCH)
-		query_switch_info(engine, &smp->path, node);
+	if (node_is_new) {
+		query_node_desc(engine, &smp->path, node);
 
-	/* process all the ports on this node */
-	for (i = (node->type == IB_NODE_SWITCH) ? 0 : 1;
-	     i <= node->numports; i++) {
-		query_port_info(engine, &smp->path, node, i);
-	}
+		if (node->type == IB_NODE_SWITCH) {
+			query_switch_info(engine, &smp->path, node);
+			for (i = 0; i <= node->numports; i++)
+				query_port_info(engine, &smp->path, node, i);
+		}
+	} else if (node->type != IB_NODE_SWITCH)
+		query_port_info(engine, &smp->path, node, port_num);
 
 	return 0;
 }

What do you think?

Sasha

send_smp() failure blocks whole discovery execution (it never returns). Fix this by updating all outstanding SMPs related counters only after successful MAD sending.

Signed-off-by Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/[email protected]>

---
 infiniband-diags/libibnetdisc/src/query_smp.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

infiniband-diags/libibnetdisc/src/query_smp.c +4 -4

index b4322bc..08e3ef7 100644
@@ -96,10 +96,12 @@ static int process_smp_queue(smp_engine_t * engine)
 		if (!smp)
 			return 0;
 
-		cl_qmap_insert(&engine->smps_on_wire, (uint32_t) smp->rpc.trid,
-			       (cl_map_item_t *) smp);
 		if ((rc = send_smp(smp, engine->ibmad_port)) != 0)
 			return rc;
+		engine->num_smps_outstanding++;
+		cl_qmap_insert(&engine->smps_on_wire, (uint32_t) smp->rpc.trid,
+			       (cl_map_item_t *) smp);
+		engine->total_smps++;
 	}
 	return 0;
 }

@@ -133,8 +135,6 @@ int issue_smp(smp_engine_t * engine, ib_portid_t * portid,
 	portid->sl = 0;
 	portid->qp = 0;
 
-	engine->total_smps++;
-	engine->num_smps_outstanding++;
 	queue_smp(engine, smp);
 	return process_smp_queue(engine);
 }

Signature · 6 lines

1.7.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://*vger.kernel.org/majordomo-info.html

This changes the logic. "num_smps_outstanding" is NOT the number on the wire, but it appears you have made it so. This is the number which will cause process_smp_queue to continue being called.

If you are going to do this I think you need to change process_mads as well as process_one_recv. We discussed process_one_recv in the error case.

What were you trying to fix?

Ira

Quoted 46 lines · On Tue, 13 Apr 2010 19:38:36 +0300 Sasha Khapyorsky…

send_smp() failure blocks whole discovery execution (it never returns). Fix this by updating all outstanding SMPs related counters only after successful MAD sending.

Signed-off-by Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/[email protected]>

---
 infiniband-diags/libibnetdisc/src/query_smp.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

infiniband-diags/libibnetdisc/src/query_smp.c +4 -4

index b4322bc..08e3ef7 100644
@@ -96,10 +96,12 @@ static int process_smp_queue(smp_engine_t * engine)
 		if (!smp)
 			return 0;
 
-		cl_qmap_insert(&engine->smps_on_wire, (uint32_t) smp->rpc.trid,
-			       (cl_map_item_t *) smp);
 		if ((rc = send_smp(smp, engine->ibmad_port)) != 0)
 			return rc;
+		engine->num_smps_outstanding++;
+		cl_qmap_insert(&engine->smps_on_wire, (uint32_t) smp->rpc.trid,
+			       (cl_map_item_t *) smp);
+		engine->total_smps++;
 	}
 	return 0;
 }

@@ -133,8 +135,6 @@ int issue_smp(smp_engine_t * engine, ib_portid_t * portid,
 	portid->sl = 0;
 	portid->qp = 0;
 
-	engine->total_smps++;
-	engine->num_smps_outstanding++;
 	queue_smp(engine, smp);
 	return process_smp_queue(engine);
 }

Signature · 6 lines

1.7.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://*vger.kernel.org/majordomo-info.html

Signature · 5 lines

Ira Weiny
Math Programmer/Computer Scientist
Lawrence Livermore National Lab
925-423-8008
[email protected]

Quoted 2 lines

This changes the logic. "num_smps_outstanding" is NOT the number on the wire, but it appears you have made it so.

Quoted 3 lines

This is the number which will cause process_smp_queue to continue being called.

If you are going to do this I think you need to change process_mads as well as process_one_recv. We discussed process_one_recv in the error case.

Quoted 1 line

What were you trying to fix?

Ok, I think I see. We should move cl_qmap_insert to after a successful umad_send and putting total_smps here is ok. But num_smps_outstanding should be put back I think.

Quoted 2 lines · Ira Weiny

Quoted 2 lines

This changes the logic. "num_smps_outstanding" is NOT the number on the wire, but it appears you have made it so.

Actually yes, it made it so.

Quoted 3 lines

Quoted 3 lines

This is the number which will cause process_smp_queue to continue being called.

If you are going to do this I think you need to change process_mads as well as process_one_recv. We discussed process_one_recv in the error case.

process_one_recv() failure breaks the loop anyway.

Quoted 3 lines

Quoted 1 line

What were you trying to fix?

Ok, I think I see. We should move cl_qmap_insert to after a successful umad_send and putting total_smps here is ok. But num_smps_outstanding should be put back I think.

But then it blocks process_mads() to loop forever after single send_smp() failure (with all empty queues and umad_recv() running without timeout).

Sasha

Quoted 4 lines

But then it blocks process_mads() to loop forever after single send_smp() failure (with all empty queues and umad_recv() running without timeout).

But moving the cl_qmap_insert below the send call fixes that.

However, it does cause a memory leak because the smp is no longer in the smp_queue_head list.

It needs to be put back on that list to be
retried with a limit on the retries (to prevent what you are saying here.)

Are you seeing a hang?

I have seen a hang when running "iblinkinfo -S <guid>".

However, the
problem is not with send_smp. I am seeing the mad going on the wire and returning (according to madeye) but I am not receiving it from umad_recv. I don't know why. If I run with 1 outstanding mad it works???

I'm seeing endless loop.