Re: [infiniband-diags] [PATCH] [2/2] split out scan specific data from ibnd_node_t
On Thu, 12 Nov 2009 18:31:05 +0200 Sasha Khapyorsky sas...@voltaire.com 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 wei...@llnl.gov -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.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
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 majord...@vger.kernel.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
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[infiniband-diags] [PATCH] [2/2] split out scan specific data from ibnd_node_t
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
ch...@llnl.gov
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory
From: Albert Chu ch...@llnl.gov
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 ch...@llnl.gov
---
.../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
---
Re: [infiniband-diags] [PATCH] [2/2] split out scan specific data from ibnd_node_t
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
ch...@llnl.gov
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory
From: Albert Chu ch...@llnl.gov
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 ch...@llnl.gov
---
.../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
