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

Al Chu
2009-11-06T18:34:54Z

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