Cleanup thread split interfaces. (#270)

Signed-off-by: Samuel K. Gutierrez <[email protected]>

Author: samuelkgutierrez

src/quo-vadis-pthread.cc

@@ -67,7 +67,7 @@ qv_pthread_scope_split(
         return QV_ERR_INVLD_ARG;
     }
     try {
-        return scope->thsplit(
+        return scope->thread_split(
             npieces, color_array, nthreads, QV_HW_OBJ_LAST, subscope
         );
     }

src/qvi-bbuff-rmi.h

@@ -16,7 +16,7 @@
  * a = size_t
  * b = qvi_bbuff_rmi_bytes_in_t, qvi_bbuff_rmi_bytes_out_t
  * c = hwloc_cpuset_t
- * c = qvi_hwloc_bitmap_s
+ * c = qvi_hwloc_bitmap
  * d = qv_scope_create_hints_t
  * h = qvi_hwpool *
  * i = int
@@ -479,7 +479,7 @@ qvi_bbuff_rmi_pack_item(
 }
 
 /**
- * Packs qvi_hwloc_bitmap_s
+ * Packs qvi_hwloc_bitmap
  */
 inline int
 qvi_bbuff_rmi_pack_item(
@@ -777,7 +777,7 @@ qvi_bbuff_rmi_unpack_item(
 }
 
 /**
- * Unpacks qvi_hwloc_bitmap_s.
+ * Unpacks qvi_hwloc_bitmap.
  */
 inline int
 qvi_bbuff_rmi_unpack_item(

src/qvi-hwsplit.cc

@@ -170,30 +170,6 @@ qvi_hwsplit::cpuset(void) const
     return m_hwpools[0]->cpuset();
 }
 
-std::vector<pid_t> &
-qvi_hwsplit::tids(void)
-{
-    return m_group_tids;
-}
-
-std::vector<int> &
-qvi_hwsplit::colors(void)
-{
-    return m_colors;
-}
-
-std::vector<qvi_hwpool *> &
-qvi_hwsplit::hwpools(void)
-{
-    return m_hwpools;
-}
-
-qvi_hwloc_cpusets_t &
-qvi_hwsplit::affinities(void)
-{
-    return m_affinities;
-}
-
 int
 qvi_hwsplit::split_cpuset(
     qvi_hwloc_cpusets_t &result
@@ -218,6 +194,63 @@ qvi_hwsplit::split_cpuset(
     return rc;
 }
 
+int
+qvi_hwsplit::thread_split(
+    qv_scope_t *parent,
+    uint_t npieces,
+    int *kcolors,
+    uint_t k,
+    qv_hw_obj_type_t maybe_obj_type,
+    qvi_hwpool ***khwpools
+) {
+    const uint_t group_size = k;
+    // Construct the hardware split.
+    qvi_hwsplit hwsplit(parent, group_size, npieces, maybe_obj_type);
+    // Eagerly make room for the group member information.
+    hwsplit.reserve();
+    // Since this is called by a single task, get its ID and associated
+    // hardware affinity here, and replicate them in the following loop
+    // that populates splitagg.
+    //No point in doing this in a loop.
+    const pid_t taskid = qvi_task::mytid();
+    // Get the task's current affinity.
+    hwloc_cpuset_t task_affinity = nullptr;
+    int rc = parent->group()->task()->bind_top(&task_affinity);
+    if (qvi_unlikely(rc != QV_SUCCESS)) return rc;
+    // Prepare the hwsplit with our parent's information.
+    for (uint_t i = 0; i < group_size; ++i) {
+        // Store requested colors in aggregate.
+        hwsplit.m_colors[i] = kcolors[i];
+        // Since the parent hardware pool is the resource we are splitting and
+        // agg_split_* calls expect |group_size| elements, replicate by dups.
+        rc = qvi_dup(*parent->hwpool(), &hwsplit.m_hwpools[i]);
+        if (qvi_unlikely(rc != QV_SUCCESS)) break;
+        // Since this is called by a single task, replicate its task ID, too.
+        hwsplit.m_group_tids[i] = taskid;
+        // Same goes for the task's affinity.
+        hwsplit.m_affinities[i].set(task_affinity);
+    }
+    // Cleanup: we don't need task_affinity anymore.
+    qvi_hwloc_bitmap_delete(&task_affinity);
+    if (qvi_unlikely(rc != QV_SUCCESS)) return rc;
+    // Split the hardware resources based on the provided split parameters.
+    rc = hwsplit.split();
+    if (qvi_unlikely(rc != QV_SUCCESS)) return rc;
+    // Now populate the hardware pools as the result.
+    qvi_hwpool **ikhwpools = new qvi_hwpool *[group_size];
+    for (uint_t i = 0; i < group_size; ++i) {
+        // Copy out, since the hardware pools in splitagg will get freed.
+        rc = qvi_dup(*hwsplit.m_hwpools[i], &ikhwpools[i]);
+        if (qvi_unlikely(rc != QV_SUCCESS)) break;
+    }
+    if (qvi_unlikely(rc != QV_SUCCESS)) {
+        delete[] ikhwpools;
+        ikhwpools = nullptr;
+    }
+    *khwpools = ikhwpools;
+    return rc;
+}
+
 int
 qvi_hwsplit::osdev_cpusets(
     qvi_hwloc_cpusets_t &result
@@ -272,7 +305,6 @@ qvi_hwsplit::affinity_preserving_policy(void) const
     }
 }
 
-/** Releases all devices contained in the provided split aggregate. */
 int
 qvi_hwsplit::release_devices(void)
 {
@@ -452,12 +484,12 @@ int
 qvi_hwsplit::split_affinity_preserving_pass1(void)
 {
     // cpusets used for first mapping pass.
-    qvi_hwloc_cpusets_t cpusets{};
+    qvi_hwloc_cpusets_t cpusets;
     // Get the primary cpusets used for the first pass of mapping.
     int rc = primary_cpusets(cpusets);
     if (rc != QV_SUCCESS) return rc;
     // Maintains the mapping between task (consumer) IDs and resource IDs.
-    qvi_map_t map{};
+    qvi_map_t map;
     // Map tasks based on their affinity to resources encoded by the cpusets.
     const auto policy = affinity_preserving_policy();
     rc = qvi_map_affinity_preserving(
@@ -735,24 +767,24 @@ qvi_hwsplit_coll::gather_hwpools(
 int
 qvi_hwsplit_coll::gather(void)
 {
-    int rc = gather_values(qvi_task::mytid(), m_hwsplit.tids());
+    int rc = gather_values(qvi_task::mytid(), m_hwsplit.m_group_tids);
     if (qvi_unlikely(rc != QV_SUCCESS)) return rc;
-    rc = gather_values(m_color, m_hwsplit.colors());
+    rc = gather_values(m_color, m_hwsplit.m_colors);
     if (qvi_unlikely(rc != QV_SUCCESS)) return rc;
     // Note that the result hwpools are copies, so we can modify them freely.
-    rc = gather_hwpools(m_parent->hwpool(), m_hwsplit.hwpools());
+    rc = gather_hwpools(m_parent->hwpool(), m_hwsplit.m_hwpools);
     if (qvi_unlikely(rc != QV_SUCCESS)) return rc;
 
     const int myrank = m_parent->group()->rank();
     const uint_t group_size = m_parent->group()->size();
     if (myrank == qvi_hwsplit_coll::rootid) {
-        m_hwsplit.affinities().resize(group_size);
+        m_hwsplit.m_affinities.resize(group_size);
         for (uint_t tid = 0; tid < group_size; ++tid) {
             hwloc_cpuset_t cpuset = nullptr;
             rc = m_parent->group()->task()->bind_top(&cpuset);
             if (qvi_unlikely(rc != QV_SUCCESS)) break;
             //
-            rc = m_hwsplit.affinities()[tid].set(cpuset);
+            rc = m_hwsplit.m_affinities[tid].set(cpuset);
             // Clean up.
             qvi_hwloc_bitmap_delete(&cpuset);
             if (qvi_unlikely(rc != QV_SUCCESS)) break;
@@ -806,9 +838,9 @@ qvi_hwsplit_coll::scatter(
     int *colorp,
     qvi_hwpool **result
 ) {
-    const int rc = scatter_values(m_hwsplit.colors(), colorp);
+    const int rc = scatter_values(m_hwsplit.m_colors, colorp);
     if (qvi_unlikely(rc != QV_SUCCESS)) return rc;
-    return scatter_hwpools(m_hwsplit.hwpools(), result);
+    return scatter_hwpools(m_hwsplit.m_hwpools, result);
 }
 
 int

src/qvi-hwsplit.h

@@ -19,6 +19,8 @@
 #include "qvi-hwpool.h"
 #include "qvi-map.h"
 
+struct qvi_hwsplit_coll;
+
 /**
  * Hardware split aggregation: a collection of information relevant to split
  * operations requiring aggregated (e.g., global) knowledge to perform a split.
@@ -30,6 +32,7 @@
  * this structure, but that isn't a requirement.
  */
 struct qvi_hwsplit {
+    friend qvi_hwsplit_coll;
 private:
     /** A pointer to my RMI. */
     qvi_rmi_client_t *m_rmi = nullptr;
@@ -92,18 +95,6 @@ struct qvi_hwsplit {
      */
     qvi_hwloc_bitmap
     cpuset(void) const;
-    /** Returns a reference to the group TIDs. */
-    std::vector<pid_t> &
-    tids(void);
-    /** Returns a reference to the group colors. */
-    std::vector<int> &
-    colors(void);
-    /** Returns a reference to the group hardware pools. */
-    std::vector<qvi_hwpool *> &
-    hwpools(void);
-    /** Returns a reference to the group task affinities. */
-    qvi_hwloc_cpusets_t &
-    affinities(void);
     /**
      * Performs a straightforward splitting of the provided cpuset:
      * split the provided base cpuset into split_size distinct pieces.
@@ -112,9 +103,17 @@ struct qvi_hwsplit {
     split_cpuset(
         qvi_hwloc_cpusets_t &result
     ) const;
-    /**
-     * Returns device affinities that are part of the split.
-     */
+    /** Performs a thread-split operation, returns relevant hardare pools. */
+    static int
+    thread_split(
+        qv_scope_t *parent,
+        uint_t npieces,
+        int *kcolors,
+        uint_t k,
+        qv_hw_obj_type_t maybe_obj_type,
+        qvi_hwpool ***khwpools
+    );
+    /** Returns device affinities that are part of the split. */
     int
     osdev_cpusets(
         qvi_hwloc_cpusets_t &result
@@ -127,35 +126,25 @@ struct qvi_hwsplit {
 
     qvi_map_fn_t
     affinity_preserving_policy(void) const;
-    /** Releases all devices contained in the provided split aggregate. */
+    /** Releases all devices contained in the hardware split. */
     int
     release_devices(void);
-    /**
-     * Straightforward user-defined device splitting.
-     */
+    /** Straightforward user-defined device splitting. */
     int
     split_devices_user_defined(void);
-    /**
-     * Affinity preserving device splitting.
-     */
+    /** Affinity preserving device splitting. */
     int
     split_devices_affinity_preserving(void);
-    /**
-     * User-defined split.
-     */
+    /** User-defined split. */
     int
     split_user_defined(void);
 
     int
     split_affinity_preserving_pass1(void);
-    /**
-     * Affinity preserving split.
-     */
+    /** Affinity preserving split. */
     int
     split_affinity_preserving(void);
-    /**
-     * Splits aggregate scope data.
-     */
+    /** Splits aggregate scope data. */
     int
     split(void);
 };

src/qvi-scope.cc

@@ -264,46 +264,20 @@ qv_scope::split_at(
 }
 
 int
-qv_scope::thsplit(
+qv_scope::thread_split(
     uint_t npieces,
     int *kcolors,
     uint_t k,
     qv_hw_obj_type_t maybe_obj_type,
     qv_scope_t ***thchildren
 ) {
     *thchildren = nullptr;
-
     const uint_t group_size = k;
-    // Construct the hardware split.
-    qvi_hwsplit hwsplit(this, group_size, npieces, maybe_obj_type);
-    // Eagerly make room for the group member information.
-    hwsplit.reserve();
-    // Since this is called by a single task, get its ID and associated
-    // hardware affinity here, and replicate them in the following loop
-    // that populates splitagg.
-    //No point in doing this in a loop.
-    const pid_t taskid = qvi_task::mytid();
-    hwloc_cpuset_t task_affinity = nullptr;
-    // Get the task's current affinity.
-    int rc = m_group->task()->bind_top(&task_affinity);
-    if (rc != QV_SUCCESS) return rc;
-    for (uint_t i = 0; i < group_size; ++i) {
-        // Store requested colors in aggregate.
-        hwsplit.colors()[i] = kcolors[i];
-        // Since the parent hardware pool is the resource we are splitting and
-        // agg_split_* calls expect |group_size| elements, replicate by dups.
-        rc = qvi_dup(*m_hwpool, &hwsplit.hwpools()[i]);
-        if (rc != QV_SUCCESS) break;
-        // Since this is called by a single task, replicate its task ID, too.
-        hwsplit.tids()[i] = taskid;
-        // Same goes for the task's affinity.
-        hwsplit.affinities()[i].set(task_affinity);
-    }
-    // Cleanup: we don't need task_affinity anymore.
-    qvi_hwloc_bitmap_delete(&task_affinity);
-    if (rc != QV_SUCCESS) return rc;
-    // Split the hardware resources based on the provided split parameters.
-    rc = hwsplit.split();
+    // Split the hardware, get the hardare pools.
+    qvi_hwpool **hwpools = nullptr;
+    int rc = qvi_hwsplit::thread_split(
+        this, npieces, kcolors, k, maybe_obj_type, &hwpools
+    );
     if (rc != QV_SUCCESS) return rc;
     // Split off from our parent group. This call is called from a context in
     // which a process is splitting its resources across threads, so create a
@@ -314,13 +288,9 @@ qv_scope::thsplit(
     // Now create and populate the children.
     qv_scope_t **ithchildren = new qv_scope_t *[group_size];
     for (uint_t i = 0; i < group_size; ++i) {
-        // Copy out, since the hardware pools in splitagg will get freed.
-        qvi_hwpool *hwpool = nullptr;
-        rc = qvi_dup(*hwsplit.hwpools()[i], &hwpool);
-        if (rc != QV_SUCCESS) break;
         // Create and initialize the new scope.
         qv_scope_t *child = nullptr;
-        rc = qvi_new(&child, thgroup, hwpool);
+        rc = qvi_new(&child, thgroup, hwpools[i]);
         if (rc != QV_SUCCESS) break;
         thgroup->retain();
         ithchildren[i] = child;
@@ -344,7 +314,7 @@ qv_scope::thsplit_at(
     uint_t k,
     qv_scope_t ***kchildren
 ) {
-    return thsplit(hwpool_nobjects(type), kgroup_ids, k, type, kchildren);
+    return thread_split(hwpool_nobjects(type), kgroup_ids, k, type, kchildren);
 }
 
 /*

src/qvi-scope.h

@@ -111,7 +111,7 @@ struct qv_scope {
     );
 
     int
-    thsplit(
+    thread_split(
         uint_t npieces,
         int *kcolors,
         uint_t k,