lstopo/draw: place I/O and Misc separately from CPU by default

The placement algorithm doesn't work well with objects
of very different sizes (e.g. CPU cores are small and all the same,
PCI is often big, Misc is very small).

I/O and Misc may now be placed on the right (by default, or below)
CPU children without considering all them at the same time.

By default --children-order is now memory:above,io:right,misc:right.
The old behavior is memory:above,plain.
io:below and misc:below are also accepted.
plain puts everything not specified with CPU children.

Signed-off-by: Brice Goglin <Brice.Goglin@inria.fr>

NEWS

@@ -30,13 +30,16 @@ Version 2.6.0
   + Expose cache os_index when available on Linux, it may be needed
     when using resctrl to configure cache partitioning, memory bandwidth
     monitoring, etc.
+* Tools
+  + lstopo has a better placement algorithm with respect to I/O
+    objects, see --children-order in the manpage for details.
 * Build
   + Allow to specify the ROCm installation for building the RSMI backend:
     - Use a custom installation path if specified with --with-rocm=<dir>.
     - Use /opt/rocm-<version> if specified with --with-rocm-version=<version>
       or the ROCM_VERSION environment variable.
     - Try /opt/rocm if it exists.
-  
+
 
 Version 2.5.0
 -------------

utils/lstopo/lstopo-draw.c

@@ -511,8 +511,33 @@ place_children(struct lstopo_output *loutput, hwloc_obj_t parent,
   unsigned separator_below_cache = loutput->gridsize;
   unsigned normal_children_separator = loutput->gridsize;
   unsigned totwidth = plud->width, totheight = plud->height;
+
+  /* Children placement is divided in 4 zones:
+   *
+   *   +-------------------------------------------------------------------+
+   *   | Above = Memory Children (by default)                              |
+   *   +-----------------------------------+-------------------------------+
+   *   | Main = CPU (always),              | Right = I/O+Misc (by default) |
+   *   |        Memory+I/O+Misc (optional) |                               |
+   *   +-----------------------------------+-------------------------------+
+   *   | Below = I/O+Misc (optional)       |
+   *   +-----------------------------------+
+   *
+   * All these children are placed inside the parent box, except:
+   * - Cache parent are between Above and Main.
+   * - Bridges have a special drawing for PCI buses and links
+   *
+   * I/O and Misc parents only have the Main section because
+   * their children are either I/O or Misc, no CPU or Memory.
+   *
+   * Memory parents may have Main (for Memory children)
+   * and below/right for Misc.
+   */
   unsigned children_width = 0, children_height = 0; /* Main children size */
   unsigned above_children_width = 0, above_children_height = 0; /* Above children size */
+  unsigned right_children_width = 0, right_children_height = 0; /* Right children size */
+  unsigned below_children_width = 0, below_children_height = 0; /* Below children size */
+  unsigned mrb_children_width = 0, mrb_children_height = 0; /* sum of Main+Right+Below sizes */
   unsigned existing_kinds;
   int normal_children_are_PUs;
   hwloc_obj_t child;
@@ -522,6 +547,8 @@ place_children(struct lstopo_output *loutput, hwloc_obj_t parent,
   /* defaults */
   plud->children.box = 0;
   plud->above_children.box = 0;
+  plud->right_children.box = 0;
+  plud->below_children.box = 0;
 
   /* list the kinds of children that exist in that parent */
   existing_kinds = (parent->arity ? LSTOPO_CHILD_KIND_NORMAL : 0)
@@ -531,11 +558,37 @@ place_children(struct lstopo_output *loutput, hwloc_obj_t parent,
   /* all children together by default */
   plud->children.kinds = existing_kinds;
   plud->above_children.kinds = 0;
+  plud->right_children.kinds = 0;
+  plud->below_children.kinds = 0;
   /* if we're not inside a memory object, put memory children above if requested */
   if (!hwloc_obj_type_is_memory(parent->type)
       && (loutput->children_order & LSTOPO_ORDER_MEMORY_ABOVE)) {
     plud->children.kinds &= ~LSTOPO_CHILD_KIND_MEMORY;
-    plud->above_children.kinds = existing_kinds & LSTOPO_CHILD_KIND_MEMORY;
+    plud->above_children.kinds |= existing_kinds & LSTOPO_CHILD_KIND_MEMORY;
+  }
+  /* if we're not inside a I/O, put I/O on the right if requested */
+  if (!hwloc_obj_type_is_io(parent->type)
+      && (loutput->children_order & LSTOPO_ORDER_IO_RIGHT)) {
+    plud->children.kinds &= ~LSTOPO_CHILD_KIND_IO;
+    plud->right_children.kinds |= existing_kinds & LSTOPO_CHILD_KIND_IO;
+  }
+  /* if we're not inside a I/O, put I/O below if requested */
+  if (!hwloc_obj_type_is_io(parent->type)
+      && (loutput->children_order & LSTOPO_ORDER_IO_BELOW)) {
+    plud->children.kinds &= ~LSTOPO_CHILD_KIND_IO;
+    plud->below_children.kinds |= existing_kinds & LSTOPO_CHILD_KIND_IO;
+  }
+  /* if we're not inside a Misc, put Misc on the right if requested */
+  if (parent->type != HWLOC_OBJ_MISC
+      && (loutput->children_order & LSTOPO_ORDER_MISC_RIGHT)) {
+    plud->children.kinds &= ~LSTOPO_CHILD_KIND_MISC;
+    plud->right_children.kinds |= existing_kinds & LSTOPO_CHILD_KIND_MISC;
+  }
+  /* if we're not inside a Misc, put Misc below if requested */
+  if (parent->type != HWLOC_OBJ_MISC
+      && (loutput->children_order & LSTOPO_ORDER_MISC_BELOW)) {
+    plud->children.kinds &= ~LSTOPO_CHILD_KIND_MISC;
+    plud->below_children.kinds |= existing_kinds & LSTOPO_CHILD_KIND_MISC;
   }
 
   /* bridge children always vertical */
@@ -588,6 +641,24 @@ place_children(struct lstopo_output *loutput, hwloc_obj_t parent,
   if (plud->children.kinds)
     place__children(loutput, parent, plud->children.kinds, &orient, 0, normal_children_separator, &children_width, &children_height);
 
+  /* compute the size of the right children section (I/O and Misc), if any */
+  if (plud->right_children.kinds) {
+    enum lstopo_orient_e rorient = loutput->force_orient[parent->type];
+    place__children(loutput, parent, plud->right_children.kinds, &rorient, 0, separator, &right_children_width, &right_children_height);
+  }
+
+  /* compute the size of the below children section (I/O and Misc), if any */
+  if (plud->below_children.kinds) {
+    enum lstopo_orient_e borient = loutput->force_orient[parent->type];
+    place__children(loutput, parent, plud->below_children.kinds, &borient, 0, separator, &below_children_width, &below_children_height);
+  }
+
+  /* compute the width of the MRB children sections, it may be need for the above children section below */
+  mrb_children_width = children_width + right_children_width + (children_width && right_children_width ? separator : 0);
+  if (mrb_children_width < below_children_width)
+    mrb_children_width = below_children_width;
+  /* MRB height will be computed later, it's more difficult because of possible overlaps */
+
   /* compute the size of the above children section (Memory), if any */
   if (plud->above_children.kinds) {
     enum lstopo_orient_e morient = LSTOPO_ORIENT_HORIZ;
@@ -608,7 +679,7 @@ place_children(struct lstopo_output *loutput, hwloc_obj_t parent,
     if (need_box) {
       /* if there are multiple memory children, add a box, as large as the parent */
       if (above_children_width < children_width) {
-	above_children_width = children_width;
+	above_children_width = mrb_children_width;
       }
       plud->above_children.boxcolor = &MEMORIES_COLOR;
       plud->above_children.box = 1;
@@ -617,8 +688,8 @@ place_children(struct lstopo_output *loutput, hwloc_obj_t parent,
       /* if there's a single memory child without wide memory box, enlarge that child */
       struct lstopo_obj_userdata *clud = parent->memory_first_child->userdata;
       if (clud->width < children_width) {
-	clud->width = children_width;
-	above_children_width = children_width;
+	clud->width = mrb_children_width;
+	above_children_width = mrb_children_width;
       }
     }
   }
@@ -636,31 +707,63 @@ place_children(struct lstopo_output *loutput, hwloc_obj_t parent,
     plud->above_children.yrel = yrel;
     plud->children.yrel += above_children_height + separator;
   }
+  /* place the right section */
+  if (plud->right_children.kinds) {
+    plud->right_children.width = right_children_width;
+    plud->right_children.height = right_children_height;
+    plud->right_children.xrel = plud->children.xrel + children_width + (children_width ? separator : 0);
+    plud->right_children.yrel = plud->children.yrel;
+  }
+  /* place the below section */
+  if (plud->below_children.kinds) {
+    plud->below_children.width = below_children_width;
+    plud->below_children.height = below_children_height;
+    plud->below_children.xrel = plud->children.xrel;
+
+    if (plud->right_children.kinds
+        && below_children_width > children_width
+        && right_children_height > children_height) {
+      /* below section is larger than CPU section, and right section is higher than CPU section.
+       * right and below would overlap.
+       * move the below section below right instead of below main
+       */
+      plud->below_children.yrel = plud->children.yrel + right_children_height + separator;
+      mrb_children_height = right_children_height + below_children_height + separator;
+    } else {
+      plud->below_children.yrel = plud->children.yrel + children_height + (children_height ? separator : 0);
+      mrb_children_height = children_height + below_children_height + (children_height ? separator : 0);
+    }
+  } else {
+    mrb_children_height = children_height > right_children_height ? children_height : right_children_height;
+  }
 
   /* adjust parent size */
   if (hwloc_obj_type_is_cache(parent->type) || parent->type == HWLOC_OBJ_MEMCACHE) {
     /* cache children are below */
-    if (children_width > totwidth)
-      totwidth = children_width;
-    if (children_height)
-      totheight += children_height + separator_below_cache;
+    if (mrb_children_width > totwidth)
+      totwidth = mrb_children_width;
+    if (mrb_children_height)
+      totheight += mrb_children_height + separator_below_cache;
     if (plud->above_children.kinds) {
       totheight += above_children_height + separator;
       if (above_children_width > totwidth)
 	totwidth = above_children_width;
     }
+
   } else if (parent->type == HWLOC_OBJ_BRIDGE) {
     /* bridge children are on the right, within any space between bridge and children */
     if (children_width)
       totwidth += children_width;
     if (children_height > totheight)
       totheight = children_height;
+    /* no right or below sections here */
+
   } else {
     /* normal objects have children inside their box, with space around them */
-    if (children_width + 2*border > totwidth)
-      totwidth = children_width + 2*border;
-    if (children_height)
-      totheight += children_height + border;
+    if (mrb_children_width + 2*border > totwidth)
+      totwidth = mrb_children_width + 2*border;
+    if (mrb_children_height)
+      totheight += mrb_children_height + border;
     if (plud->above_children.kinds) {
       totheight += above_children_height + separator;
       if (above_children_width + 2*border > totwidth)
@@ -708,6 +811,12 @@ draw_children(struct lstopo_output *loutput, hwloc_obj_t parent, unsigned depth,
 
   if (plud->above_children.kinds)
     draw__children(loutput, parent, &plud->above_children, depth, x + plud->above_children.xrel, y + plud->above_children.yrel);
+
+  if (plud->right_children.kinds)
+    draw__children(loutput, parent, &plud->right_children, depth, x + plud->right_children.xrel, y + plud->right_children.yrel);
+
+  if (plud->below_children.kinds)
+    draw__children(loutput, parent, &plud->below_children, depth, x + plud->below_children.xrel, y + plud->below_children.yrel);
 }
 
 /*******

utils/lstopo/lstopo-no-graphics.1in

@@ -299,17 +299,35 @@ making the hwloc-ps program more practical.
 \fB\-\-children\-order <order>\fR
 Change the order of the different kinds of children with respect to
 their parent in the graphical output.
+\fI<order>\fR may be a comma-separated list of keywords among:
 
-The default order is \fImemory:above\fR:
-it displays memory children above other children
+\fImemory:above\fR displays memory children above other children
 (and above the parent if it is a cache).
 PUs are therefore below their local NUMA nodes, like hwloc 1.x did.
 
-If the order is changed to \fIplain\fR, lstopo displays the topology
+\fIio:right\fR and \fImisc:right\fR place I/O or Misc children
+on the right of CPU children.
+
+\fIio:below\fR  and \fImisc:below\fR place I/O or Misc children
+below CPU children.
+
+\fIplain\fR places everything not specified together with
+normal CPU children.
+
+If only \fIplain\fR is specified, lstopo displays the topology
 in a basic manner that strictly matches the actual tree:
-memory children are listed below their parent just like any other child.
+Memory, I/O and Misc children are listed below their parent just like any other child.
 PUs are therefore on the side of their local NUMA nodes,
 below a common ancestor.
+This output may result in strange layouts since the size of Memory,
+CPU and I/O children may be very different, causing the placement
+algorithm to poorly arrange them in rows.
+
+The default order is \fImemory:above,io:right,misc:right\fR which means
+Memory children are above CPU children while I/O and Misc are together
+on the right.
+
+Up to hwloc 2.5, the default was rather to \fImemory:above,plain\fR.
 
 See also the GRAPHICAL OUTPUT section below.
 .TP
@@ -707,8 +725,8 @@ complete graphs, etc.) is currently ignored.
 The layout of a level may be changed with \-\-vert, \-\-horiz,
 and \-\-rect.
 
-The position of memory children with respect to other children objects
-may be changed using \-\-children\-order.
+The position of Memory, I/O and Misc children with respect to other children
+objects may be changed using \-\-children\-order.
 .
 .\" **************************
 .\"    Examples Section

utils/lstopo/lstopo.c

@@ -333,6 +333,14 @@ lstopo_parse_children_order(char *s, unsigned *children_order_p)
 
     if (!strcmp(tmp, "memory:above") || !strcmp(tmp, "memoryabove") /* backward compat with 2.5 */)
       children_order |= LSTOPO_ORDER_MEMORY_ABOVE;
+    else if (!strcmp(tmp, "io:right"))
+      children_order |= LSTOPO_ORDER_IO_RIGHT;
+    else if (!strcmp(tmp, "io:below"))
+      children_order |= LSTOPO_ORDER_IO_BELOW;
+    else if (!strcmp(tmp, "misc:right"))
+      children_order |= LSTOPO_ORDER_MISC_RIGHT;
+    else if (!strcmp(tmp, "misc:below"))
+      children_order |= LSTOPO_ORDER_MISC_BELOW;
     else if (strcmp(tmp, "plain"))
       fprintf(stderr, "Unsupported children order `%s', ignoring.\n", tmp);
 
@@ -475,8 +483,8 @@ void usage(const char *name, FILE *where)
   fprintf (where, "  --allow <all|local|...>   Change the set of objects marked as allowed\n");
   fprintf (where, "  --flags <n>           Set the topology flags\n");
   fprintf (where, "Graphical output options:\n");
-  fprintf (where, "  --children-order plain\n"
-		  "                        Display memory children below the parent like any other child\n");
+  fprintf (where, "  --children-order <memory:above|io:right|...|plain>\n"
+		  "                        Change the layout of Memory, I/O or Misc children\n");
   fprintf (where, "  --no-factorize        Do not factorize identical objects\n");
   fprintf (where, "  --no-factorize=<type> Do not factorize identical objects of type <type>\n");
   fprintf (where, "  --factorize           Factorize identical objects (default)\n");
@@ -790,7 +798,7 @@ main (int argc, char *argv[])
   loutput.backend_flags = 0;
   loutput.methods = NULL;
 
-  loutput.children_order = LSTOPO_ORDER_MEMORY_ABOVE;
+  loutput.children_order = LSTOPO_ORDER_MEMORY_ABOVE | LSTOPO_ORDER_IO_RIGHT | LSTOPO_ORDER_MISC_RIGHT;
   loutput.fontsize = 10;
   loutput.gridsize = 7;
   loutput.linespacing = 4;

utils/lstopo/lstopo.h

@@ -41,7 +41,11 @@ enum lstopo_show_legend_e {
 
 enum lstopo_order_e {
   LSTOPO_ORDER_PLAIN = 0,
-  LSTOPO_ORDER_MEMORY_ABOVE = (1<<0)
+  LSTOPO_ORDER_MEMORY_ABOVE = (1<<0),
+  LSTOPO_ORDER_IO_RIGHT = (1<<1),
+  LSTOPO_ORDER_IO_BELOW = (1<<2),
+  LSTOPO_ORDER_MISC_RIGHT = (1<<3),
+  LSTOPO_ORDER_MISC_BELOW = (1<<4)
 };
 
 FILE *open_output(const char *filename, int overwrite) __hwloc_attribute_malloc;
@@ -205,6 +209,10 @@ struct lstopo_obj_userdata {
   } children;
   /* if memory is displayed separately above normal children */
   struct lstopo_children_position above_children;
+  /* if I/O and/or Misc are displayed separately on the right */
+  struct lstopo_children_position right_children;
+  /* if I/O and/or Misc are displayed separately below */
+  struct lstopo_children_position below_children;
 
   /* relative position of this object within its parent children zone */
   unsigned xrel;