[infiniband-diags] [PATCH] [2/2] split out scan specific data from ibnd_node_t

[infiniband-diags] [PATCH] [2/2] split out scan specific data from ibnd_node_t

[Al Chu]

[-- Attachment #1: Type: text/plain, Size: 262 bytes --]

Hey Sasha,

This splits out some scan specific data from ibnd_node_t that doesn't
need to be in the public struct.

Al

-- 
Albert Chu
chu11-i2BcT+NCU+M@public.gmane.org
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory

[-- Attachment #2: 0002-split-out-scan-specific-data-from-ibnd_node_t.patch --]
[-- Type: text/plain, Size: 6962 bytes --]

From: Albert Chu <chu11-i2BcT+NCU+M@public.gmane.org>
Date: Thu, 29 Oct 2009 18:59:26 -0700
Subject: [PATCH] split out scan specific data from ibnd_node_t


Signed-off-by: Albert Chu <chu11-i2BcT+NCU+M@public.gmane.org>
---
 .../libibnetdisc/include/infiniband/ibnetdisc.h    |    2 -
 infiniband-diags/libibnetdisc/src/chassis.c        |   18 ++++++++--
 infiniband-diags/libibnetdisc/src/ibnetdisc.c      |   32 +++++++++++++++----
 infiniband-diags/libibnetdisc/src/internal.h       |    8 ++++-
 4 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/infiniband-diags/libibnetdisc/include/infiniband/ibnetdisc.h b/infiniband-diags/libibnetdisc/include/infiniband/ibnetdisc.h
index 6120453..f1cb00c 100644
--- a/infiniband-diags/libibnetdisc/include/infiniband/ibnetdisc.h
+++ b/infiniband-diags/libibnetdisc/include/infiniband/ibnetdisc.h
@@ -48,7 +48,6 @@ struct ibnd_port;		/* forward declare */
 typedef struct ibnd_node {
 	struct ibnd_node *next;	/* all node list in fabric */
 
-	ib_portid_t path_portid;	/* path from "from_node" */
 	int smalid;
 	int smalmc;
 
@@ -81,7 +80,6 @@ typedef struct ibnd_node {
 	/* internal use only */
 	unsigned char ch_found;
 	struct ibnd_node *htnext;	/* hash table list */
-	struct ibnd_node *dnext;	/* nodesdist next */
 	struct ibnd_node *type_next;	/* next based on type */
 } ibnd_node_t;
 
diff --git a/infiniband-diags/libibnetdisc/src/chassis.c b/infiniband-diags/libibnetdisc/src/chassis.c
index 15c17d2..3bd0108 100644
--- a/infiniband-diags/libibnetdisc/src/chassis.c
+++ b/infiniband-diags/libibnetdisc/src/chassis.c
@@ -822,6 +822,7 @@ int group_nodes(ibnd_fabric_t * fabric, ibnd_scan_t *scan)
 	int chassisnum = 0;
 	ibnd_chassis_t *chassis;
 	ibnd_chassis_t *ch, *ch_next;
+	ibnd_node_scan_t *node_scan;
 
 	scan->first_chassis = NULL;
 	scan->current_chassis = NULL;
@@ -832,16 +833,21 @@ int group_nodes(ibnd_fabric_t * fabric, ibnd_scan_t *scan)
 	/* according to internal connectivity */
 	/* not very efficient but clear code so... */
 	for (dist = 0; dist <= fabric->maxhops_discovered; dist++)
-		for (node = scan->nodesdist[dist]; node; node = node->dnext)
+		for (node_scan = scan->nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
+			node = node_scan->node;
+
 			if (mad_get_field(node->info, 0,
 					  IB_NODE_VENDORID_F) == VTR_VENDOR_ID
 			    && fill_voltaire_chassis_record(node))
 				goto cleanup;
+		}
 
 	/* separate every Voltaire chassis from each other and build linked list of them */
 	/* algorithm: catch spine and find all surrounding nodes */
 	for (dist = 0; dist <= fabric->maxhops_discovered; dist++)
-		for (node = scan->nodesdist[dist]; node; node = node->dnext) {
+		for (node_scan = scan->nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
+			node = node_scan->node;
+
 			if (mad_get_field(node->info, 0,
 					  IB_NODE_VENDORID_F) != VTR_VENDOR_ID)
 				continue;
@@ -859,7 +865,9 @@ int group_nodes(ibnd_fabric_t * fabric, ibnd_scan_t *scan)
 	/* now make pass on nodes for chassis which are not Voltaire */
 	/* grouped by common SystemImageGUID */
 	for (dist = 0; dist <= fabric->maxhops_discovered; dist++)
-		for (node = scan->nodesdist[dist]; node; node = node->dnext) {
+		for (node_scan = scan->nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
+			node = node_scan->node;
+
 			if (mad_get_field(node->info, 0,
 					  IB_NODE_VENDORID_F) == VTR_VENDOR_ID)
 				continue;
@@ -885,7 +893,9 @@ int group_nodes(ibnd_fabric_t * fabric, ibnd_scan_t *scan)
 	/* now, make another pass to see which nodes are part of chassis */
 	/* (defined as chassis->nodecount > 1) */
 	for (dist = 0; dist <= MAXHOPS;) {
-		for (node = scan->nodesdist[dist]; node; node = node->dnext) {
+		for (node_scan = scan->nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
+			node = node_scan->node;
+
 			if (mad_get_field(node->info, 0,
 					  IB_NODE_VENDORID_F) == VTR_VENDOR_ID)
 				continue;
diff --git a/infiniband-diags/libibnetdisc/src/ibnetdisc.c b/infiniband-diags/libibnetdisc/src/ibnetdisc.c
index ffa35e4..283584b 100644
--- a/infiniband-diags/libibnetdisc/src/ibnetdisc.c
+++ b/infiniband-diags/libibnetdisc/src/ibnetdisc.c
@@ -332,13 +332,26 @@ static void add_to_type_list(ibnd_node_t * node, ibnd_fabric_t * fabric)
 	}
 }
 
-static void add_to_nodedist(ibnd_node_t * node, ibnd_scan_t * scan, int dist)
+static int add_to_nodedist(ibnd_node_t * node, ibnd_scan_t * scan,
+			   ib_portid_t * path, int dist)
 {
+	ibnd_node_scan_t *node_scan;
+
+	node_scan = malloc(sizeof(*node_scan));
+	if (!node_scan) {
+		IBND_ERROR("OOM: node scan creation failed\n");
+		return -1;
+	}
+	node_scan->node = node;
+	node_scan->path_portid = *path;
+
 	if (node->type != IB_NODE_SWITCH)
 		dist = MAXHOPS;	/* special Ca list */
 
-	node->dnext = scan->nodesdist[dist];
-	scan->nodesdist[dist] = node;
+	node_scan->dnext = scan->nodesdist[dist];
+	scan->nodesdist[dist] = node_scan;
+	
+	return 0;
 }
 
 static ibnd_node_t *create_node(ibnd_fabric_t * fabric, ibnd_scan_t * scan,
@@ -354,7 +367,6 @@ static ibnd_node_t *create_node(ibnd_fabric_t * fabric, ibnd_scan_t * scan,
 	}
 
 	memcpy(node, temp, sizeof(*node));
-	node->path_portid = *path;
 
 	add_to_nodeguid_hash(node, fabric->nodestbl);
 
@@ -363,7 +375,11 @@ static ibnd_node_t *create_node(ibnd_fabric_t * fabric, ibnd_scan_t * scan,
 	fabric->nodes = node;
 
 	add_to_type_list(node, fabric);
-	add_to_nodedist(node, scan, dist);
+
+	if (add_to_nodedist(node, scan, path, dist) < 0) {
+		free(node);
+		return NULL;
+	}
 
 	return node;
 }
@@ -492,6 +508,7 @@ ibnd_fabric_t *ibnd_discover_fabric(struct ibmad_port * ibmad_port,
 	ibnd_node_t node_buf;
 	ibnd_port_t port_buf;
 	ibnd_node_t *node;
+	ibnd_node_scan_t *node_scan;
 	ibnd_port_t *port;
 	int i;
 	int dist = 0;
@@ -552,9 +569,10 @@ ibnd_fabric_t *ibnd_discover_fabric(struct ibmad_port * ibmad_port,
 
 	for (dist = 0; dist <= max_hops; dist++) {
 
-		for (node = scan.nodesdist[dist]; node; node = node->dnext) {
+		for (node_scan = scan.nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
+			node = node_scan->node;
 
-			path = &node->path_portid;
+			path = &node_scan->path_portid;
 
 			IBND_DEBUG("dist %d node %p\n", dist, node);
 			dump_endnode(path, "processing", node, port);
diff --git a/infiniband-diags/libibnetdisc/src/internal.h b/infiniband-diags/libibnetdisc/src/internal.h
index eac1a29..d8bcf87 100644
--- a/infiniband-diags/libibnetdisc/src/internal.h
+++ b/infiniband-diags/libibnetdisc/src/internal.h
@@ -52,8 +52,14 @@
 
 #define MAXHOPS         63
 
+typedef struct ibnd_node_scan {
+	ibnd_node_t *node;
+	ib_portid_t path_portid;	/* path from "from_node" */
+	struct ibnd_node_scan *dnext;	/* nodesdist next */
+} ibnd_node_scan_t;
+
 typedef struct ibnd_scan {
-	ibnd_node_t *nodesdist[MAXHOPS + 1];
+	ibnd_node_scan_t *nodesdist[MAXHOPS + 1];
 	ibnd_chassis_t *first_chassis;
 	ibnd_chassis_t *current_chassis;
 	ibnd_chassis_t *last_chassis;
-- 
1.5.4.5

Re: [infiniband-diags] [PATCH] [2/2] split out scan specific data from ibnd_node_t

[Al Chu]

[-- Attachment #1: Type: text/plain, Size: 412 bytes --]

Hi Sasha,

Oops.  I forgot to free the newly created memory.  Here's a new patch.

Al

On Mon, 2009-11-02 at 11:33 -0800, Al Chu wrote:
> Hey Sasha,
> 
> This splits out some scan specific data from ibnd_node_t that doesn't
> need to be in the public struct.
> 
> Al
> 
-- 
Albert Chu
chu11-i2BcT+NCU+M@public.gmane.org
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory

[-- Attachment #2: 0002-split-out-scan-specific-data-from-ibnd_node_t.patch --]
[-- Type: text/plain, Size: 7736 bytes --]

From: Albert Chu <chu11-i2BcT+NCU+M@public.gmane.org>
Date: Thu, 29 Oct 2009 18:59:26 -0700
Subject: [PATCH] split out scan specific data from ibnd_node_t


Signed-off-by: Albert Chu <chu11-i2BcT+NCU+M@public.gmane.org>
---
 .../libibnetdisc/include/infiniband/ibnetdisc.h    |    2 -
 infiniband-diags/libibnetdisc/src/chassis.c        |   18 +++++--
 infiniband-diags/libibnetdisc/src/ibnetdisc.c      |   51 +++++++++++++++++---
 infiniband-diags/libibnetdisc/src/internal.h       |    8 +++-
 4 files changed, 65 insertions(+), 14 deletions(-)

diff --git a/infiniband-diags/libibnetdisc/include/infiniband/ibnetdisc.h b/infiniband-diags/libibnetdisc/include/infiniband/ibnetdisc.h
index 6120453..f1cb00c 100644
--- a/infiniband-diags/libibnetdisc/include/infiniband/ibnetdisc.h
+++ b/infiniband-diags/libibnetdisc/include/infiniband/ibnetdisc.h
@@ -48,7 +48,6 @@ struct ibnd_port;		/* forward declare */
 typedef struct ibnd_node {
 	struct ibnd_node *next;	/* all node list in fabric */
 
-	ib_portid_t path_portid;	/* path from "from_node" */
 	int smalid;
 	int smalmc;
 
@@ -81,7 +80,6 @@ typedef struct ibnd_node {
 	/* internal use only */
 	unsigned char ch_found;
 	struct ibnd_node *htnext;	/* hash table list */
-	struct ibnd_node *dnext;	/* nodesdist next */
 	struct ibnd_node *type_next;	/* next based on type */
 } ibnd_node_t;
 
diff --git a/infiniband-diags/libibnetdisc/src/chassis.c b/infiniband-diags/libibnetdisc/src/chassis.c
index 15c17d2..3bd0108 100644
--- a/infiniband-diags/libibnetdisc/src/chassis.c
+++ b/infiniband-diags/libibnetdisc/src/chassis.c
@@ -822,6 +822,7 @@ int group_nodes(ibnd_fabric_t * fabric, ibnd_scan_t *scan)
 	int chassisnum = 0;
 	ibnd_chassis_t *chassis;
 	ibnd_chassis_t *ch, *ch_next;
+	ibnd_node_scan_t *node_scan;
 
 	scan->first_chassis = NULL;
 	scan->current_chassis = NULL;
@@ -832,16 +833,21 @@ int group_nodes(ibnd_fabric_t * fabric, ibnd_scan_t *scan)
 	/* according to internal connectivity */
 	/* not very efficient but clear code so... */
 	for (dist = 0; dist <= fabric->maxhops_discovered; dist++)
-		for (node = scan->nodesdist[dist]; node; node = node->dnext)
+		for (node_scan = scan->nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
+			node = node_scan->node;
+
 			if (mad_get_field(node->info, 0,
 					  IB_NODE_VENDORID_F) == VTR_VENDOR_ID
 			    && fill_voltaire_chassis_record(node))
 				goto cleanup;
+		}
 
 	/* separate every Voltaire chassis from each other and build linked list of them */
 	/* algorithm: catch spine and find all surrounding nodes */
 	for (dist = 0; dist <= fabric->maxhops_discovered; dist++)
-		for (node = scan->nodesdist[dist]; node; node = node->dnext) {
+		for (node_scan = scan->nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
+			node = node_scan->node;
+
 			if (mad_get_field(node->info, 0,
 					  IB_NODE_VENDORID_F) != VTR_VENDOR_ID)
 				continue;
@@ -859,7 +865,9 @@ int group_nodes(ibnd_fabric_t * fabric, ibnd_scan_t *scan)
 	/* now make pass on nodes for chassis which are not Voltaire */
 	/* grouped by common SystemImageGUID */
 	for (dist = 0; dist <= fabric->maxhops_discovered; dist++)
-		for (node = scan->nodesdist[dist]; node; node = node->dnext) {
+		for (node_scan = scan->nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
+			node = node_scan->node;
+
 			if (mad_get_field(node->info, 0,
 					  IB_NODE_VENDORID_F) == VTR_VENDOR_ID)
 				continue;
@@ -885,7 +893,9 @@ int group_nodes(ibnd_fabric_t * fabric, ibnd_scan_t *scan)
 	/* now, make another pass to see which nodes are part of chassis */
 	/* (defined as chassis->nodecount > 1) */
 	for (dist = 0; dist <= MAXHOPS;) {
-		for (node = scan->nodesdist[dist]; node; node = node->dnext) {
+		for (node_scan = scan->nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
+			node = node_scan->node;
+
 			if (mad_get_field(node->info, 0,
 					  IB_NODE_VENDORID_F) == VTR_VENDOR_ID)
 				continue;
diff --git a/infiniband-diags/libibnetdisc/src/ibnetdisc.c b/infiniband-diags/libibnetdisc/src/ibnetdisc.c
index ffa35e4..233654c 100644
--- a/infiniband-diags/libibnetdisc/src/ibnetdisc.c
+++ b/infiniband-diags/libibnetdisc/src/ibnetdisc.c
@@ -332,13 +332,26 @@ static void add_to_type_list(ibnd_node_t * node, ibnd_fabric_t * fabric)
 	}
 }
 
-static void add_to_nodedist(ibnd_node_t * node, ibnd_scan_t * scan, int dist)
+static int add_to_nodedist(ibnd_node_t * node, ibnd_scan_t * scan,
+			   ib_portid_t * path, int dist)
 {
+	ibnd_node_scan_t *node_scan;
+
+	node_scan = malloc(sizeof(*node_scan));
+	if (!node_scan) {
+		IBND_ERROR("OOM: node scan creation failed\n");
+		return -1;
+	}
+	node_scan->node = node;
+	node_scan->path_portid = *path;
+
 	if (node->type != IB_NODE_SWITCH)
 		dist = MAXHOPS;	/* special Ca list */
 
-	node->dnext = scan->nodesdist[dist];
-	scan->nodesdist[dist] = node;
+	node_scan->dnext = scan->nodesdist[dist];
+	scan->nodesdist[dist] = node_scan;
+	
+	return 0;
 }
 
 static ibnd_node_t *create_node(ibnd_fabric_t * fabric, ibnd_scan_t * scan,
@@ -354,7 +367,6 @@ static ibnd_node_t *create_node(ibnd_fabric_t * fabric, ibnd_scan_t * scan,
 	}
 
 	memcpy(node, temp, sizeof(*node));
-	node->path_portid = *path;
 
 	add_to_nodeguid_hash(node, fabric->nodestbl);
 
@@ -363,7 +375,11 @@ static ibnd_node_t *create_node(ibnd_fabric_t * fabric, ibnd_scan_t * scan,
 	fabric->nodes = node;
 
 	add_to_type_list(node, fabric);
-	add_to_nodedist(node, scan, dist);
+
+	if (add_to_nodedist(node, scan, path, dist) < 0) {
+		free(node);
+		return NULL;
+	}
 
 	return node;
 }
@@ -483,6 +499,23 @@ error:
 	return rc;
 }
 
+static void ibnd_scan_destroy(ibnd_scan_t *scan)
+{
+	int dist = 0;
+	int max_hops = MAXHOPS - 1;
+	ibnd_node_scan_t *node_scan;
+	ibnd_node_scan_t *next;
+
+	for (dist = 0; dist <= max_hops; dist++) {
+		node_scan = scan->nodesdist[dist];
+		while (node_scan) {
+			next = node_scan->dnext;
+			free(node_scan);
+			node_scan = next;
+		}
+	}
+}
+
 ibnd_fabric_t *ibnd_discover_fabric(struct ibmad_port * ibmad_port,
 				    ib_portid_t * from, int hops)
 {
@@ -492,6 +525,7 @@ ibnd_fabric_t *ibnd_discover_fabric(struct ibmad_port * ibmad_port,
 	ibnd_node_t node_buf;
 	ibnd_port_t port_buf;
 	ibnd_node_t *node;
+	ibnd_node_scan_t *node_scan;
 	ibnd_port_t *port;
 	int i;
 	int dist = 0;
@@ -552,9 +586,10 @@ ibnd_fabric_t *ibnd_discover_fabric(struct ibmad_port * ibmad_port,
 
 	for (dist = 0; dist <= max_hops; dist++) {
 
-		for (node = scan.nodesdist[dist]; node; node = node->dnext) {
+		for (node_scan = scan.nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
+			node = node_scan->node;
 
-			path = &node->path_portid;
+			path = &node_scan->path_portid;
 
 			IBND_DEBUG("dist %d node %p\n", dist, node);
 			dump_endnode(path, "processing", node, port);
@@ -598,8 +633,10 @@ ibnd_fabric_t *ibnd_discover_fabric(struct ibmad_port * ibmad_port,
 	if (group_nodes(fabric, &scan))
 		goto error;
 
+	ibnd_scan_destroy(&scan);
 	return fabric;
 error:
+	ibnd_scan_destroy(&scan);
 	ibnd_destroy_fabric(fabric);
 	return NULL;
 }
diff --git a/infiniband-diags/libibnetdisc/src/internal.h b/infiniband-diags/libibnetdisc/src/internal.h
index eac1a29..d8bcf87 100644
--- a/infiniband-diags/libibnetdisc/src/internal.h
+++ b/infiniband-diags/libibnetdisc/src/internal.h
@@ -52,8 +52,14 @@
 
 #define MAXHOPS         63
 
+typedef struct ibnd_node_scan {
+	ibnd_node_t *node;
+	ib_portid_t path_portid;	/* path from "from_node" */
+	struct ibnd_node_scan *dnext;	/* nodesdist next */
+} ibnd_node_scan_t;
+
 typedef struct ibnd_scan {
-	ibnd_node_t *nodesdist[MAXHOPS + 1];
+	ibnd_node_scan_t *nodesdist[MAXHOPS + 1];
 	ibnd_chassis_t *first_chassis;
 	ibnd_chassis_t *current_chassis;
 	ibnd_chassis_t *last_chassis;
-- 
1.5.4.5

Re: [infiniband-diags] [PATCH] [2/2] split out scan specific data from ibnd_node_t

[Sasha Khapyorsky]

On 13:11 Mon 02 Nov     , Al Chu wrote:
> Hi Sasha,
> 
> Oops.  I forgot to free the newly created memory.  Here's a new patch.
> 
> Al
> 
> On Mon, 2009-11-02 at 11:33 -0800, Al Chu wrote:
> > Hey Sasha,
> > 
> > This splits out some scan specific data from ibnd_node_t that doesn't
> > need to be in the public struct.
> > 
> > Al
> > 
> -- 
> Albert Chu
> chu11-i2BcT+NCU+M@public.gmane.org
> Computer Scientist
> High Performance Systems Division
> Lawrence Livermore National Laboratory

> From: Albert Chu <chu11-i2BcT+NCU+M@public.gmane.org>
> Date: Thu, 29 Oct 2009 18:59:26 -0700
> Subject: [PATCH] split out scan specific data from ibnd_node_t
> 
> 
> Signed-off-by: Albert Chu <chu11-i2BcT+NCU+M@public.gmane.org>
> ---
>  .../libibnetdisc/include/infiniband/ibnetdisc.h    |    2 -
>  infiniband-diags/libibnetdisc/src/chassis.c        |   18 +++++--
>  infiniband-diags/libibnetdisc/src/ibnetdisc.c      |   51 +++++++++++++++++---
>  infiniband-diags/libibnetdisc/src/internal.h       |    8 +++-
>  4 files changed, 65 insertions(+), 14 deletions(-)
> 
> diff --git a/infiniband-diags/libibnetdisc/include/infiniband/ibnetdisc.h b/infiniband-diags/libibnetdisc/include/infiniband/ibnetdisc.h
> index 6120453..f1cb00c 100644
> --- a/infiniband-diags/libibnetdisc/include/infiniband/ibnetdisc.h
> +++ b/infiniband-diags/libibnetdisc/include/infiniband/ibnetdisc.h
> @@ -48,7 +48,6 @@ struct ibnd_port;		/* forward declare */
>  typedef struct ibnd_node {
>  	struct ibnd_node *next;	/* all node list in fabric */
>  
> -	ib_portid_t path_portid;	/* path from "from_node" */
>  	int smalid;
>  	int smalmc;
>  
> @@ -81,7 +80,6 @@ typedef struct ibnd_node {
>  	/* internal use only */
>  	unsigned char ch_found;
>  	struct ibnd_node *htnext;	/* hash table list */
> -	struct ibnd_node *dnext;	/* nodesdist next */
>  	struct ibnd_node *type_next;	/* next based on type */
>  } ibnd_node_t;

Why do you want to remove this? port->path_portid can be useful for
logging, specific querying, etc.. Even node->dnext can be helpful for
some "advanced" use too.

Sasha

>  
> diff --git a/infiniband-diags/libibnetdisc/src/chassis.c b/infiniband-diags/libibnetdisc/src/chassis.c
> index 15c17d2..3bd0108 100644
> --- a/infiniband-diags/libibnetdisc/src/chassis.c
> +++ b/infiniband-diags/libibnetdisc/src/chassis.c
> @@ -822,6 +822,7 @@ int group_nodes(ibnd_fabric_t * fabric, ibnd_scan_t *scan)
>  	int chassisnum = 0;
>  	ibnd_chassis_t *chassis;
>  	ibnd_chassis_t *ch, *ch_next;
> +	ibnd_node_scan_t *node_scan;
>  
>  	scan->first_chassis = NULL;
>  	scan->current_chassis = NULL;
> @@ -832,16 +833,21 @@ int group_nodes(ibnd_fabric_t * fabric, ibnd_scan_t *scan)
>  	/* according to internal connectivity */
>  	/* not very efficient but clear code so... */
>  	for (dist = 0; dist <= fabric->maxhops_discovered; dist++)
> -		for (node = scan->nodesdist[dist]; node; node = node->dnext)
> +		for (node_scan = scan->nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
> +			node = node_scan->node;
> +
>  			if (mad_get_field(node->info, 0,
>  					  IB_NODE_VENDORID_F) == VTR_VENDOR_ID
>  			    && fill_voltaire_chassis_record(node))
>  				goto cleanup;
> +		}
>  
>  	/* separate every Voltaire chassis from each other and build linked list of them */
>  	/* algorithm: catch spine and find all surrounding nodes */
>  	for (dist = 0; dist <= fabric->maxhops_discovered; dist++)
> -		for (node = scan->nodesdist[dist]; node; node = node->dnext) {
> +		for (node_scan = scan->nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
> +			node = node_scan->node;
> +
>  			if (mad_get_field(node->info, 0,
>  					  IB_NODE_VENDORID_F) != VTR_VENDOR_ID)
>  				continue;
> @@ -859,7 +865,9 @@ int group_nodes(ibnd_fabric_t * fabric, ibnd_scan_t *scan)
>  	/* now make pass on nodes for chassis which are not Voltaire */
>  	/* grouped by common SystemImageGUID */
>  	for (dist = 0; dist <= fabric->maxhops_discovered; dist++)
> -		for (node = scan->nodesdist[dist]; node; node = node->dnext) {
> +		for (node_scan = scan->nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
> +			node = node_scan->node;
> +
>  			if (mad_get_field(node->info, 0,
>  					  IB_NODE_VENDORID_F) == VTR_VENDOR_ID)
>  				continue;
> @@ -885,7 +893,9 @@ int group_nodes(ibnd_fabric_t * fabric, ibnd_scan_t *scan)
>  	/* now, make another pass to see which nodes are part of chassis */
>  	/* (defined as chassis->nodecount > 1) */
>  	for (dist = 0; dist <= MAXHOPS;) {
> -		for (node = scan->nodesdist[dist]; node; node = node->dnext) {
> +		for (node_scan = scan->nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
> +			node = node_scan->node;
> +
>  			if (mad_get_field(node->info, 0,
>  					  IB_NODE_VENDORID_F) == VTR_VENDOR_ID)
>  				continue;
> diff --git a/infiniband-diags/libibnetdisc/src/ibnetdisc.c b/infiniband-diags/libibnetdisc/src/ibnetdisc.c
> index ffa35e4..233654c 100644
> --- a/infiniband-diags/libibnetdisc/src/ibnetdisc.c
> +++ b/infiniband-diags/libibnetdisc/src/ibnetdisc.c
> @@ -332,13 +332,26 @@ static void add_to_type_list(ibnd_node_t * node, ibnd_fabric_t * fabric)
>  	}
>  }
>  
> -static void add_to_nodedist(ibnd_node_t * node, ibnd_scan_t * scan, int dist)
> +static int add_to_nodedist(ibnd_node_t * node, ibnd_scan_t * scan,
> +			   ib_portid_t * path, int dist)
>  {
> +	ibnd_node_scan_t *node_scan;
> +
> +	node_scan = malloc(sizeof(*node_scan));
> +	if (!node_scan) {
> +		IBND_ERROR("OOM: node scan creation failed\n");
> +		return -1;
> +	}
> +	node_scan->node = node;
> +	node_scan->path_portid = *path;
> +
>  	if (node->type != IB_NODE_SWITCH)
>  		dist = MAXHOPS;	/* special Ca list */
>  
> -	node->dnext = scan->nodesdist[dist];
> -	scan->nodesdist[dist] = node;
> +	node_scan->dnext = scan->nodesdist[dist];
> +	scan->nodesdist[dist] = node_scan;
> +	
> +	return 0;
>  }
>  
>  static ibnd_node_t *create_node(ibnd_fabric_t * fabric, ibnd_scan_t * scan,
> @@ -354,7 +367,6 @@ static ibnd_node_t *create_node(ibnd_fabric_t * fabric, ibnd_scan_t * scan,
>  	}
>  
>  	memcpy(node, temp, sizeof(*node));
> -	node->path_portid = *path;
>  
>  	add_to_nodeguid_hash(node, fabric->nodestbl);
>  
> @@ -363,7 +375,11 @@ static ibnd_node_t *create_node(ibnd_fabric_t * fabric, ibnd_scan_t * scan,
>  	fabric->nodes = node;
>  
>  	add_to_type_list(node, fabric);
> -	add_to_nodedist(node, scan, dist);
> +
> +	if (add_to_nodedist(node, scan, path, dist) < 0) {
> +		free(node);
> +		return NULL;
> +	}
>  
>  	return node;
>  }
> @@ -483,6 +499,23 @@ error:
>  	return rc;
>  }
>  
> +static void ibnd_scan_destroy(ibnd_scan_t *scan)
> +{
> +	int dist = 0;
> +	int max_hops = MAXHOPS - 1;
> +	ibnd_node_scan_t *node_scan;
> +	ibnd_node_scan_t *next;
> +
> +	for (dist = 0; dist <= max_hops; dist++) {
> +		node_scan = scan->nodesdist[dist];
> +		while (node_scan) {
> +			next = node_scan->dnext;
> +			free(node_scan);
> +			node_scan = next;
> +		}
> +	}
> +}
> +
>  ibnd_fabric_t *ibnd_discover_fabric(struct ibmad_port * ibmad_port,
>  				    ib_portid_t * from, int hops)
>  {
> @@ -492,6 +525,7 @@ ibnd_fabric_t *ibnd_discover_fabric(struct ibmad_port * ibmad_port,
>  	ibnd_node_t node_buf;
>  	ibnd_port_t port_buf;
>  	ibnd_node_t *node;
> +	ibnd_node_scan_t *node_scan;
>  	ibnd_port_t *port;
>  	int i;
>  	int dist = 0;
> @@ -552,9 +586,10 @@ ibnd_fabric_t *ibnd_discover_fabric(struct ibmad_port * ibmad_port,
>  
>  	for (dist = 0; dist <= max_hops; dist++) {
>  
> -		for (node = scan.nodesdist[dist]; node; node = node->dnext) {
> +		for (node_scan = scan.nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
> +			node = node_scan->node;
>  
> -			path = &node->path_portid;
> +			path = &node_scan->path_portid;
>  
>  			IBND_DEBUG("dist %d node %p\n", dist, node);
>  			dump_endnode(path, "processing", node, port);
> @@ -598,8 +633,10 @@ ibnd_fabric_t *ibnd_discover_fabric(struct ibmad_port * ibmad_port,
>  	if (group_nodes(fabric, &scan))
>  		goto error;
>  
> +	ibnd_scan_destroy(&scan);
>  	return fabric;
>  error:
> +	ibnd_scan_destroy(&scan);
>  	ibnd_destroy_fabric(fabric);
>  	return NULL;
>  }
> diff --git a/infiniband-diags/libibnetdisc/src/internal.h b/infiniband-diags/libibnetdisc/src/internal.h
> index eac1a29..d8bcf87 100644
> --- a/infiniband-diags/libibnetdisc/src/internal.h
> +++ b/infiniband-diags/libibnetdisc/src/internal.h
> @@ -52,8 +52,14 @@
>  
>  #define MAXHOPS         63
>  
> +typedef struct ibnd_node_scan {
> +	ibnd_node_t *node;
> +	ib_portid_t path_portid;	/* path from "from_node" */
> +	struct ibnd_node_scan *dnext;	/* nodesdist next */
> +} ibnd_node_scan_t;
> +
>  typedef struct ibnd_scan {
> -	ibnd_node_t *nodesdist[MAXHOPS + 1];
> +	ibnd_node_scan_t *nodesdist[MAXHOPS + 1];
>  	ibnd_chassis_t *first_chassis;
>  	ibnd_chassis_t *current_chassis;
>  	ibnd_chassis_t *last_chassis;
> -- 
> 1.5.4.5
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [infiniband-diags] [PATCH] [2/2] split out scan specific data from ibnd_node_t

[Al Chu]

Hey Sasha,

> >  typedef struct ibnd_node {
> >  	struct ibnd_node *next;	/* all node list in fabric */
> >  
> > -	ib_portid_t path_portid;	/* path from "from_node" */
> >  	int smalid;
> >  	int smalmc;
> >  
> > @@ -81,7 +80,6 @@ typedef struct ibnd_node {
> >  	/* internal use only */
> >  	unsigned char ch_found;
> >  	struct ibnd_node *htnext;	/* hash table list */
> > -	struct ibnd_node *dnext;	/* nodesdist next */
> >  	struct ibnd_node *type_next;	/* next based on type */
> >  } ibnd_node_t;
> 
> Why do you want to remove this? port->path_portid can be useful for
> logging, specific querying, etc.. Even node->dnext can be helpful for
> some "advanced" use too.

The 'nodesdist' array (which is what the 'dnext' pointer is used for) is
only used during the scan and is no longer available publicly.  So the
'dnext' pointer doesn't serve a purpose being in the public ibnd_node_t
struct.

Post scan, the 'path_portid' was ony used in ibnd_update_node(), which
is now removed.  In addition, Ira and I felt it is one of the fields
that shouldn't have been exported out of libibnetdisc, it is far too
"scan specific" and shouldn't be public.

Al

On Fri, 2009-11-06 at 20:16 +0200, Sasha Khapyorsky wrote:
> On 13:11 Mon 02 Nov     , Al Chu wrote:
> > Hi Sasha,
> > 
> > Oops.  I forgot to free the newly created memory.  Here's a new patch.
> > 
> > Al
> > 
> > On Mon, 2009-11-02 at 11:33 -0800, Al Chu wrote:
> > > Hey Sasha,
> > > 
> > > This splits out some scan specific data from ibnd_node_t that doesn't
> > > need to be in the public struct.
> > > 
> > > Al
> > > 
> > -- 
> > Albert Chu
> > chu11-i2BcT+NCU+M@public.gmane.org
> > Computer Scientist
> > High Performance Systems Division
> > Lawrence Livermore National Laboratory
> 
> > From: Albert Chu <chu11-i2BcT+NCU+M@public.gmane.org>
> > Date: Thu, 29 Oct 2009 18:59:26 -0700
> > Subject: [PATCH] split out scan specific data from ibnd_node_t
> > 
> > 
> > Signed-off-by: Albert Chu <chu11-i2BcT+NCU+M@public.gmane.org>
> > ---
> >  .../libibnetdisc/include/infiniband/ibnetdisc.h    |    2 -
> >  infiniband-diags/libibnetdisc/src/chassis.c        |   18 +++++--
> >  infiniband-diags/libibnetdisc/src/ibnetdisc.c      |   51 +++++++++++++++++---
> >  infiniband-diags/libibnetdisc/src/internal.h       |    8 +++-
> >  4 files changed, 65 insertions(+), 14 deletions(-)
> > 
> > diff --git a/infiniband-diags/libibnetdisc/include/infiniband/ibnetdisc.h b/infiniband-diags/libibnetdisc/include/infiniband/ibnetdisc.h
> > index 6120453..f1cb00c 100644
> > --- a/infiniband-diags/libibnetdisc/include/infiniband/ibnetdisc.h
> > +++ b/infiniband-diags/libibnetdisc/include/infiniband/ibnetdisc.h
> > @@ -48,7 +48,6 @@ struct ibnd_port;		/* forward declare */
> >  typedef struct ibnd_node {
> >  	struct ibnd_node *next;	/* all node list in fabric */
> >  
> > -	ib_portid_t path_portid;	/* path from "from_node" */
> >  	int smalid;
> >  	int smalmc;
> >  
> > @@ -81,7 +80,6 @@ typedef struct ibnd_node {
> >  	/* internal use only */
> >  	unsigned char ch_found;
> >  	struct ibnd_node *htnext;	/* hash table list */
> > -	struct ibnd_node *dnext;	/* nodesdist next */
> >  	struct ibnd_node *type_next;	/* next based on type */
> >  } ibnd_node_t;
> 
> Why do you want to remove this? port->path_portid can be useful for
> logging, specific querying, etc.. Even node->dnext can be helpful for
> some "advanced" use too.
> 
> Sasha
> 
> >  
> > diff --git a/infiniband-diags/libibnetdisc/src/chassis.c b/infiniband-diags/libibnetdisc/src/chassis.c
> > index 15c17d2..3bd0108 100644
> > --- a/infiniband-diags/libibnetdisc/src/chassis.c
> > +++ b/infiniband-diags/libibnetdisc/src/chassis.c
> > @@ -822,6 +822,7 @@ int group_nodes(ibnd_fabric_t * fabric, ibnd_scan_t *scan)
> >  	int chassisnum = 0;
> >  	ibnd_chassis_t *chassis;
> >  	ibnd_chassis_t *ch, *ch_next;
> > +	ibnd_node_scan_t *node_scan;
> >  
> >  	scan->first_chassis = NULL;
> >  	scan->current_chassis = NULL;
> > @@ -832,16 +833,21 @@ int group_nodes(ibnd_fabric_t * fabric, ibnd_scan_t *scan)
> >  	/* according to internal connectivity */
> >  	/* not very efficient but clear code so... */
> >  	for (dist = 0; dist <= fabric->maxhops_discovered; dist++)
> > -		for (node = scan->nodesdist[dist]; node; node = node->dnext)
> > +		for (node_scan = scan->nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
> > +			node = node_scan->node;
> > +
> >  			if (mad_get_field(node->info, 0,
> >  					  IB_NODE_VENDORID_F) == VTR_VENDOR_ID
> >  			    && fill_voltaire_chassis_record(node))
> >  				goto cleanup;
> > +		}
> >  
> >  	/* separate every Voltaire chassis from each other and build linked list of them */
> >  	/* algorithm: catch spine and find all surrounding nodes */
> >  	for (dist = 0; dist <= fabric->maxhops_discovered; dist++)
> > -		for (node = scan->nodesdist[dist]; node; node = node->dnext) {
> > +		for (node_scan = scan->nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
> > +			node = node_scan->node;
> > +
> >  			if (mad_get_field(node->info, 0,
> >  					  IB_NODE_VENDORID_F) != VTR_VENDOR_ID)
> >  				continue;
> > @@ -859,7 +865,9 @@ int group_nodes(ibnd_fabric_t * fabric, ibnd_scan_t *scan)
> >  	/* now make pass on nodes for chassis which are not Voltaire */
> >  	/* grouped by common SystemImageGUID */
> >  	for (dist = 0; dist <= fabric->maxhops_discovered; dist++)
> > -		for (node = scan->nodesdist[dist]; node; node = node->dnext) {
> > +		for (node_scan = scan->nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
> > +			node = node_scan->node;
> > +
> >  			if (mad_get_field(node->info, 0,
> >  					  IB_NODE_VENDORID_F) == VTR_VENDOR_ID)
> >  				continue;
> > @@ -885,7 +893,9 @@ int group_nodes(ibnd_fabric_t * fabric, ibnd_scan_t *scan)
> >  	/* now, make another pass to see which nodes are part of chassis */
> >  	/* (defined as chassis->nodecount > 1) */
> >  	for (dist = 0; dist <= MAXHOPS;) {
> > -		for (node = scan->nodesdist[dist]; node; node = node->dnext) {
> > +		for (node_scan = scan->nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
> > +			node = node_scan->node;
> > +
> >  			if (mad_get_field(node->info, 0,
> >  					  IB_NODE_VENDORID_F) == VTR_VENDOR_ID)
> >  				continue;
> > diff --git a/infiniband-diags/libibnetdisc/src/ibnetdisc.c b/infiniband-diags/libibnetdisc/src/ibnetdisc.c
> > index ffa35e4..233654c 100644
> > --- a/infiniband-diags/libibnetdisc/src/ibnetdisc.c
> > +++ b/infiniband-diags/libibnetdisc/src/ibnetdisc.c
> > @@ -332,13 +332,26 @@ static void add_to_type_list(ibnd_node_t * node, ibnd_fabric_t * fabric)
> >  	}
> >  }
> >  
> > -static void add_to_nodedist(ibnd_node_t * node, ibnd_scan_t * scan, int dist)
> > +static int add_to_nodedist(ibnd_node_t * node, ibnd_scan_t * scan,
> > +			   ib_portid_t * path, int dist)
> >  {
> > +	ibnd_node_scan_t *node_scan;
> > +
> > +	node_scan = malloc(sizeof(*node_scan));
> > +	if (!node_scan) {
> > +		IBND_ERROR("OOM: node scan creation failed\n");
> > +		return -1;
> > +	}
> > +	node_scan->node = node;
> > +	node_scan->path_portid = *path;
> > +
> >  	if (node->type != IB_NODE_SWITCH)
> >  		dist = MAXHOPS;	/* special Ca list */
> >  
> > -	node->dnext = scan->nodesdist[dist];
> > -	scan->nodesdist[dist] = node;
> > +	node_scan->dnext = scan->nodesdist[dist];
> > +	scan->nodesdist[dist] = node_scan;
> > +	
> > +	return 0;
> >  }
> >  
> >  static ibnd_node_t *create_node(ibnd_fabric_t * fabric, ibnd_scan_t * scan,
> > @@ -354,7 +367,6 @@ static ibnd_node_t *create_node(ibnd_fabric_t * fabric, ibnd_scan_t * scan,
> >  	}
> >  
> >  	memcpy(node, temp, sizeof(*node));
> > -	node->path_portid = *path;
> >  
> >  	add_to_nodeguid_hash(node, fabric->nodestbl);
> >  
> > @@ -363,7 +375,11 @@ static ibnd_node_t *create_node(ibnd_fabric_t * fabric, ibnd_scan_t * scan,
> >  	fabric->nodes = node;
> >  
> >  	add_to_type_list(node, fabric);
> > -	add_to_nodedist(node, scan, dist);
> > +
> > +	if (add_to_nodedist(node, scan, path, dist) < 0) {
> > +		free(node);
> > +		return NULL;
> > +	}
> >  
> >  	return node;
> >  }
> > @@ -483,6 +499,23 @@ error:
> >  	return rc;
> >  }
> >  
> > +static void ibnd_scan_destroy(ibnd_scan_t *scan)
> > +{
> > +	int dist = 0;
> > +	int max_hops = MAXHOPS - 1;
> > +	ibnd_node_scan_t *node_scan;
> > +	ibnd_node_scan_t *next;
> > +
> > +	for (dist = 0; dist <= max_hops; dist++) {
> > +		node_scan = scan->nodesdist[dist];
> > +		while (node_scan) {
> > +			next = node_scan->dnext;
> > +			free(node_scan);
> > +			node_scan = next;
> > +		}
> > +	}
> > +}
> > +
> >  ibnd_fabric_t *ibnd_discover_fabric(struct ibmad_port * ibmad_port,
> >  				    ib_portid_t * from, int hops)
> >  {
> > @@ -492,6 +525,7 @@ ibnd_fabric_t *ibnd_discover_fabric(struct ibmad_port * ibmad_port,
> >  	ibnd_node_t node_buf;
> >  	ibnd_port_t port_buf;
> >  	ibnd_node_t *node;
> > +	ibnd_node_scan_t *node_scan;
> >  	ibnd_port_t *port;
> >  	int i;
> >  	int dist = 0;
> > @@ -552,9 +586,10 @@ ibnd_fabric_t *ibnd_discover_fabric(struct ibmad_port * ibmad_port,
> >  
> >  	for (dist = 0; dist <= max_hops; dist++) {
> >  
> > -		for (node = scan.nodesdist[dist]; node; node = node->dnext) {
> > +		for (node_scan = scan.nodesdist[dist]; node_scan; node_scan = node_scan->dnext) {
> > +			node = node_scan->node;
> >  
> > -			path = &node->path_portid;
> > +			path = &node_scan->path_portid;
> >  
> >  			IBND_DEBUG("dist %d node %p\n", dist, node);
> >  			dump_endnode(path, "processing", node, port);
> > @@ -598,8 +633,10 @@ ibnd_fabric_t *ibnd_discover_fabric(struct ibmad_port * ibmad_port,
> >  	if (group_nodes(fabric, &scan))
> >  		goto error;
> >  
> > +	ibnd_scan_destroy(&scan);
> >  	return fabric;
> >  error:
> > +	ibnd_scan_destroy(&scan);
> >  	ibnd_destroy_fabric(fabric);
> >  	return NULL;
> >  }
> > diff --git a/infiniband-diags/libibnetdisc/src/internal.h b/infiniband-diags/libibnetdisc/src/internal.h
> > index eac1a29..d8bcf87 100644
> > --- a/infiniband-diags/libibnetdisc/src/internal.h
> > +++ b/infiniband-diags/libibnetdisc/src/internal.h
> > @@ -52,8 +52,14 @@
> >  
> >  #define MAXHOPS         63
> >  
> > +typedef struct ibnd_node_scan {
> > +	ibnd_node_t *node;
> > +	ib_portid_t path_portid;	/* path from "from_node" */
> > +	struct ibnd_node_scan *dnext;	/* nodesdist next */
> > +} ibnd_node_scan_t;
> > +
> >  typedef struct ibnd_scan {
> > -	ibnd_node_t *nodesdist[MAXHOPS + 1];
> > +	ibnd_node_scan_t *nodesdist[MAXHOPS + 1];
> >  	ibnd_chassis_t *first_chassis;
> >  	ibnd_chassis_t *current_chassis;
> >  	ibnd_chassis_t *last_chassis;
> > -- 
> > 1.5.4.5
> > 
> 
-- 
Albert Chu
chu11-i2BcT+NCU+M@public.gmane.org
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [infiniband-diags] [PATCH] [2/2] split out scan specific data from ibnd_node_t

[Sasha Khapyorsky]

On 10:34 Fri 06 Nov     , Al Chu wrote:
> > 
> > Why do you want to remove this? port->path_portid can be useful for
> > logging, specific querying, etc.. Even node->dnext can be helpful for
> > some "advanced" use too.
> 
> The 'nodesdist' array (which is what the 'dnext' pointer is used for) is
> only used during the scan and is no longer available publicly.

Really? I thought that it could be a useful data for "advanced" uses.

> So the
> 'dnext' pointer doesn't serve a purpose being in the public ibnd_node_t
> struct.
> 
> Post scan, the 'path_portid' was ony used in ibnd_update_node(), which
> is now removed.

You are saying about libibnetdisc itself. My example was about an
application which uses this. For instance after discovery application
may want to query some ports for its own purpose. What is wrong with
that?

> In addition, Ira and I felt it is one of the fields
> that shouldn't have been exported out of libibnetdisc, it is far too
> "scan specific" and shouldn't be public.

I cannot understand why are you trying to make things there as "private"
as technically possible (even on price of extra code size and
complexity). Finally it is an open source stuff, so let to users to use
it how they want and for their own responsibility. :)

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [infiniband-diags] [PATCH] [2/2] split out scan specific data from ibnd_node_t

[Al Chu]

Hey Sasha,

> Really? I thought that it could be a useful data for "advanced" uses.

It was removed in commit 094f922a34d6378d6a3bd1d137f90d6530685f94.  It
was a simpler version of a patch that Ira had proposed on the mailing
list.

> I cannot understand why are you trying to make things there as 
> "private" as technically possible (even on price of extra code size 
> and complexity). Finally it is an open source stuff, so let to users 
> to use it how they want and for their own responsibility. :)

At the core of this patch (as well as some other patches I've submitted
on libibnetdiscover before), is cleaning up the interface of
libibnetdisc to be just the "core" of libibnetdisc.  We could stick
anything into the public structs that could have potential usefulness,
but at some point I think we need to limit ourselves to only the core
stuff.  Why not add the ibmad_port to the structs?  Or instead of
putting just the guids or lids in the structs, why not also the pkeys,
capability masks, or VL tables?

Al

On Thu, 2009-11-12 at 18:31 +0200, Sasha Khapyorsky wrote:
> On 10:34 Fri 06 Nov     , Al Chu wrote:
> > > 
> > > Why do you want to remove this? port->path_portid can be useful for
> > > logging, specific querying, etc.. Even node->dnext can be helpful for
> > > some "advanced" use too.
> > 
> > The 'nodesdist' array (which is what the 'dnext' pointer is used for) is
> > only used during the scan and is no longer available publicly.
> 
> Really? I thought that it could be a useful data for "advanced" uses.
> 
> > So the
> > 'dnext' pointer doesn't serve a purpose being in the public ibnd_node_t
> > struct.
> > 
> > Post scan, the 'path_portid' was ony used in ibnd_update_node(), which
> > is now removed.
> 
> You are saying about libibnetdisc itself. My example was about an
> application which uses this. For instance after discovery application
> may want to query some ports for its own purpose. What is wrong with
> that?
> 
> > In addition, Ira and I felt it is one of the fields
> > that shouldn't have been exported out of libibnetdisc, it is far too
> > "scan specific" and shouldn't be public.
> 
> I cannot understand why are you trying to make things there as "private"
> as technically possible (even on price of extra code size and
> complexity). Finally it is an open source stuff, so let to users to use
> it how they want and for their own responsibility. :)
> 
> Sasha
-- 
Albert Chu
chu11-i2BcT+NCU+M@public.gmane.org
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [infiniband-diags] [PATCH] [2/2] split out scan specific data from ibnd_node_t

[Ira Weiny]

On Thu, 12 Nov 2009 18:31:05 +0200
Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org> wrote:

> On 10:34 Fri 06 Nov     , Al Chu wrote:
> > > 
> > > Why do you want to remove this? port->path_portid can be useful for
> > > logging, specific querying, etc.. Even node->dnext can be helpful for
> > > some "advanced" use too.
> > 
> > The 'nodesdist' array (which is what the 'dnext' pointer is used for) is
> > only used during the scan and is no longer available publicly.
> 
> Really? I thought that it could be a useful data for "advanced" uses.

nodesdist was remove from the public interface.  Although an "advanced" user
might have been able to use it, the data stored there was very scan specific.
Removing it was a good idea for 2 reasons, 1) simplify the interface, 2) if
the scan algorithm changed users might have to change the way they use the
data; not good for compatibility.

> 
> > So the
> > 'dnext' pointer doesn't serve a purpose being in the public ibnd_node_t
> > struct.
> > 
> > Post scan, the 'path_portid' was ony used in ibnd_update_node(), which
> > is now removed.
> 
> You are saying about libibnetdisc itself. My example was about an
> application which uses this. For instance after discovery application
> may want to query some ports for its own purpose. What is wrong with
> that?

I agree there was some usefulness, sometimes.  However the path_portid can not
be guaranteed to be valid.  Again there are multiple issues.  First I don't
think we want to support to this to the users.  Second Al is working toward is
the ability to cache the fabric information to be read back later.  Storing
all this "scan" specific information is going to be extra work which is
superfluous to the layout of the fabric.

> 
> > In addition, Ira and I felt it is one of the fields
> > that shouldn't have been exported out of libibnetdisc, it is far too
> > "scan specific" and shouldn't be public.
> 
> I cannot understand why are you trying to make things there as "private"
> as technically possible (even on price of extra code size and
> complexity). Finally it is an open source stuff, so let to users to use
> it how they want and for their own responsibility. :)

Making things "private" allows us to change the library without breaking the
interface.  Furthermore, it simplifies the interface for users who want to
write code at a higher level.  My original design goals were 2 fold.  1. make
a library which speeds up the functionality of the perl script tools.  2.
Provide a C interface to a fast scan library which can be used to implement
further tools which would have formerly been implemented via scripts around
ibnetdiscover.

Here is one use case we have been working off of.

One of our MPI developers here is not familiar with Infiniband.  He has
written many scripts around the current suite of tools for various research
that he does.  The concepts of nodes, ports, and links are familiar to him but
sending a "MAD" or differentiating between a GSI MAD vs SMP is not.  I want to
give him information about nodes, links, ports, errors, data counters, routing
tables, etc. without making him use the MAD layer, or worse yet, umad layer.
In this use case he does not care that libibnetdisc does a BFS and creates a
nodesdist structure of some sort resulting from that algorithm.  Presenting a
"fabric" with a list of "nodes" which further have some "ports" makes sense to
a user like this.

This use case in addition to trying to cache the data makes a simpler, cleaner
interface much more attractive.  :-D

Ira

[snip]

-- 
Ira Weiny
Math Programmer/Computer Scientist
Lawrence Livermore National Lab
925-423-8008
weiny2-i2BcT+NCU+M@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [infiniband-diags] [PATCH] [2/2] split out scan specific data from ibnd_node_t

[Sasha Khapyorsky]

Hi Al,

On 09:51 Thu 12 Nov     , Al Chu wrote:
> 
> > Really? I thought that it could be a useful data for "advanced" uses.
> 
> It was removed in commit 094f922a34d6378d6a3bd1d137f90d6530685f94.  It
> was a simpler version of a patch that Ira had proposed on the mailing
> list.

This means that I'm applying patches too quickly :)

Then 'dest' is likely useless without such array, but 'portid' is not.

> > I cannot understand why are you trying to make things there as 
> > "private" as technically possible (even on price of extra code size 
> > and complexity). Finally it is an open source stuff, so let to users 
> > to use it how they want and for their own responsibility. :)
> 
> At the core of this patch (as well as some other patches I've submitted
> on libibnetdiscover before), is cleaning up the interface of
> libibnetdisc to be just the "core" of libibnetdisc.  We could stick
> anything into the public structs that could have potential usefulness,
> but at some point I think we need to limit ourselves to only the core
> stuff.  Why not add the ibmad_port to the structs?  Or instead of
> putting just the guids or lids in the structs, why not also the pkeys,
> capability masks, or VL tables?

This can be done (if needed), but will require some efforts, and this is
not what I'm asking for. I'm just proposing to not remove potentially
useful things, that is all and this is for no price.

And cleaning interface is a good thing.

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [infiniband-diags] [PATCH] [2/2] split out scan specific data from ibnd_node_t

[Sasha Khapyorsky]

Hi Ira,

On 10:59 Thu 12 Nov     , Ira Weiny wrote:
> 
> nodesdist was remove from the public interface.  Although an "advanced" user
> might have been able to use it, the data stored there was very scan specific.
> Removing it was a good idea for 2 reasons, 1) simplify the interface,

I don't see how this conceptually simplifies interface. I think that
it started from a wrong approach about having two data structure sets -
public and private. Now it requires cleaning again and again in order to
have simpler interface.

> 2) if
> the scan algorithm changed users might have to change the way they use the
> data; not good for compatibility.

Maybe, but let him to decide. Such usage is not mandatory.

> I agree there was some usefulness, sometimes.  However the path_portid can not
> be guaranteed to be valid.

Why not? Isn't it a valid on the last scan?

> Again there are multiple issues.  First I don't
> think we want to support to this to the users.

But you don't need to support it. Advanced use is developer's
responsibility.

> Second Al is working toward is
> the ability to cache the fabric information to be read back later.  Storing
> all this "scan" specific information is going to be extra work which is
> superfluous to the layout of the fabric.

Hard to say really without seeing any code, but you can simply keep all
this scan specific information over session and have a pointer field on
ibnd_fabric structure which refer this.

> > I cannot understand why are you trying to make things there as "private"
> > as technically possible (even on price of extra code size and
> > complexity). Finally it is an open source stuff, so let to users to use
> > it how they want and for their own responsibility. :)
> 
> Making things "private" allows us to change the library without breaking the
> interface.

I don't think

> Furthermore, it simplifies the interface for users who want to
> write code at a higher level.

I'm not asking to make high level life harder :). My point is to not
prevent from advanced developers to use available low level too,
especially when such preventing requires some extra efforts.

> My original design goals were 2 fold.  1. make
> a library which speeds up the functionality of the perl script tools.  2.
> Provide a C interface to a fast scan library which can be used to implement
> further tools which would have formerly been implemented via scripts around
> ibnetdiscover.

My purposes serve (2) very well. Isn't it?

> 
> Here is one use case we have been working off of.
> 
> One of our MPI developers here is not familiar with Infiniband. He has
> written many scripts around the current suite of tools for various research
> that he does.  The concepts of nodes, ports, and links are familiar to him but
> sending a "MAD" or differentiating between a GSI MAD vs SMP is not.  I want to
> give him information about nodes, links, ports, errors, data counters, routing
> tables, etc. without making him use the MAD layer, or worse yet, umad layer.

How having 'path_portid' in node structure enforces him to use the MAD
or umad (which is simpler than mad in general, IMO) layers? It doesn't.

> In this use case he does not care that libibnetdisc does a BFS and creates a
> nodesdist structure of some sort resulting from that algorithm.  Presenting a
> "fabric" with a list of "nodes" which further have some "ports" makes sense to
> a user like this.

Again, how having access to internal discovery stuff makes a life of
such users harder?

> This use case in addition to trying to cache the data makes a simpler, cleaner
> interface much more attractive.  :-D

And there is another use case (hypothetical yet) - one of our IB
developers is familiar with infiniband... Would such "high-level"-only
interface be so attractive for him?

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [infiniband-diags] [PATCH] [2/2] split out scan specific data from ibnd_node_t

[Sean Hefty]

>> Making things "private" allows us to change the library without breaking the
>> interface.
>
>I don't think
>
>> Furthermore, it simplifies the interface for users who want to
>> write code at a higher level.
>
>I'm not asking to make high level life harder :). My point is to not
>prevent from advanced developers to use available low level too,
>especially when such preventing requires some extra efforts.

I haven't been following the details of this thread, but it's very common to
expose only a portion of a data structure to the user, while keeping some of it
private.  As long as the lower library allocates the structure and exchanges
pointers, the interface can be maintained.

Trying to expose what should be internal data structures through a public
interface tosses encapsulation out the window, along with any benefits that it
provides.  In the end, all you get is a poorly designed interface.

Advanced developers can use lower level interfaces, like mad or umad.

- Sean

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [infiniband-diags] [PATCH] [2/2] split out scan specific data from ibnd_node_t

[Sasha Khapyorsky]

On 15:10 Thu 12 Nov     , Sean Hefty wrote:
> >
> >I'm not asking to make high level life harder :). My point is to not
> >prevent from advanced developers to use available low level too,
> >especially when such preventing requires some extra efforts.
> 
> I haven't been following the details of this thread, but it's very common to
> expose only a portion of a data structure to the user, while keeping some of it
> private.

Of course we can easily find such examples, and also will find others
not less useful where all internal data is exposed (libibumad for
example).

I think that if library interface is designed so that some typical
task/tools (high or low level) cannot be implemented easily than likely
this would be example of a poor interface regardless to how public/private
data structures were separated in this library.

> As long as the lower library allocates the structure and exchanges
> pointers, the interface can be maintained.

Just to be clear - the example of "low level" data triggered this round
of the discussion is node's direct path generated during subnet
discovery (another was discovery BFS graph, but this one "was lost"
already).

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [infiniband-diags] [PATCH] [2/2] split out scan specific data from ibnd_node_t

[Al Chu]

Hey Sasha,

On Fri, 2009-11-13 at 00:31 +0200, Sasha Khapyorsky wrote:
> Hi Al,
> 
> On 09:51 Thu 12 Nov     , Al Chu wrote:
> > 
> > > Really? I thought that it could be a useful data for "advanced" uses.
> > 
> > It was removed in commit 094f922a34d6378d6a3bd1d137f90d6530685f94.  It
> > was a simpler version of a patch that Ira had proposed on the mailing
> > list.
> 
> This means that I'm applying patches too quickly :)
> 
> Then 'dest' is likely useless without such array, but 'portid' is not.

I'll go ahead and post a modified patch that only splits out 'dnext'.

Al

> > > I cannot understand why are you trying to make things there as 
> > > "private" as technically possible (even on price of extra code size 
> > > and complexity). Finally it is an open source stuff, so let to users 
> > > to use it how they want and for their own responsibility. :)
> > 
> > At the core of this patch (as well as some other patches I've submitted
> > on libibnetdiscover before), is cleaning up the interface of
> > libibnetdisc to be just the "core" of libibnetdisc.  We could stick
> > anything into the public structs that could have potential usefulness,
> > but at some point I think we need to limit ourselves to only the core
> > stuff.  Why not add the ibmad_port to the structs?  Or instead of
> > putting just the guids or lids in the structs, why not also the pkeys,
> > capability masks, or VL tables?
> 
> This can be done (if needed), but will require some efforts, and this is
> not what I'm asking for. I'm just proposing to not remove potentially
> useful things, that is all and this is for no price.
> 
> And cleaning interface is a good thing.
> 
> Sasha
-- 
Albert Chu
chu11-i2BcT+NCU+M@public.gmane.org
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html