Logo AND Algorithmique Numérique Distribuée

Public GIT Repository
privatize fields of ClusterZone + more explicit methods
authorSUTER Frederic <frederic.suter@cc.in2p3.fr>
Thu, 18 Mar 2021 18:26:03 +0000 (19:26 +0100)
committerSUTER Frederic <frederic.suter@cc.in2p3.fr>
Fri, 19 Mar 2021 06:58:46 +0000 (07:58 +0100)
MANIFEST.in
include/simgrid/kernel/routing/ClusterZone.hpp
src/kernel/routing/ClusterZone.cpp
src/kernel/routing/DragonflyZone.cpp
src/kernel/routing/FatTreeZone.cpp
src/kernel/routing/TorusZone.cpp
src/kernel/routing/VivaldiZone.cpp
src/surf/sg_platf.cpp

index c46e3c5..a28b7fc 100644 (file)
@@ -798,6 +798,7 @@ include teshsuite/smpi/coll-allgatherv/coll-allgatherv.c
 include teshsuite/smpi/coll-allgatherv/coll-allgatherv.tesh
 include teshsuite/smpi/coll-allreduce-with-leaks/coll-allreduce-with-leaks.c
 include teshsuite/smpi/coll-allreduce-with-leaks/coll-allreduce-with-leaks.tesh
+include teshsuite/smpi/coll-allreduce-with-leaks/mc-coll-allreduce-with-leaks.tesh
 include teshsuite/smpi/coll-allreduce/coll-allreduce-automatic.tesh
 include teshsuite/smpi/coll-allreduce/coll-allreduce-large.tesh
 include teshsuite/smpi/coll-allreduce/coll-allreduce-papi.tesh
index fdb8b65..e44037d 100644 (file)
@@ -66,9 +66,36 @@ namespace routing {
  */
 
 class ClusterZone : public NetZoneImpl {
+  /* We use a map instead of a std::vector here because that's a sparse vector. Some values may not exist */
+  /* The pair is {link_up, link_down} */
+  std::unordered_map<unsigned int, std::pair<resource::LinkImpl*, resource::LinkImpl*>> private_links_;
+  resource::LinkImpl* backbone_    = nullptr;
+  NetPoint* router_                = nullptr;
+  bool has_limiter_                = false;
+  bool has_loopback_               = false;
+  unsigned int num_links_per_node_ = 1; /* may be 1 (if only a private link), 2 or 3 (if limiter and loopback) */
+
+protected:
+  void set_num_links_per_node(unsigned int num) { num_links_per_node_ = num; }
+  resource::LinkImpl* get_uplink_from(unsigned int position) const { return private_links_.at(position).first; }
+  resource::LinkImpl* get_downlink_to(unsigned int position) const { return private_links_.at(position).second; }
+
 public:
   explicit ClusterZone(const std::string& name);
 
+  void set_loopback();
+  bool has_loopback() const { return has_loopback_; }
+  void set_limiter();
+  bool has_limiter() const { return has_limiter_; }
+  void set_backbone(resource::LinkImpl* bb) { backbone_ = bb; }
+  bool has_backbone() const { return backbone_ != nullptr; }
+  void set_router(NetPoint* router) { router_ = router; }
+  void add_private_link_at(unsigned int position, std::pair<resource::LinkImpl*, resource::LinkImpl*> link);
+  bool private_link_exists_at(unsigned int position) const
+  {
+    return private_links_.find(position) != private_links_.end();
+  }
+
   void get_local_route(NetPoint* src, NetPoint* dst, RouteCreationArgs* into, double* latency) override;
   void get_graph(const s_xbt_graph_t* graph, std::map<std::string, xbt_node_t, std::less<>>* nodes,
                  std::map<std::string, xbt_edge_t, std::less<>>* edges) override;
@@ -79,22 +106,12 @@ public:
     /* this routing method does not require any specific argument */
   }
 
-  /* We use a map instead of a std::vector here because that's a sparse vector. Some values may not exist */
-  /* The pair is {link_up, link_down} */
-  std::unordered_map<unsigned int, std::pair<kernel::resource::LinkImpl*, kernel::resource::LinkImpl*>> private_links_;
-
   unsigned int node_pos(int id) const { return id * num_links_per_node_; }
   unsigned int node_pos_with_loopback(int id) const { return node_pos(id) + (has_loopback_ ? 1 : 0); }
   unsigned int node_pos_with_loopback_limiter(int id) const
   {
     return node_pos_with_loopback(id) + (has_limiter_ ? 1 : 0);
   }
-
-  kernel::resource::LinkImpl* backbone_ = nullptr;
-  NetPoint* router_              = nullptr;
-  bool has_limiter_                = false;
-  bool has_loopback_               = false;
-  unsigned int num_links_per_node_ = 1; /* may be 1 (if only a private link), 2 or 3 (if limiter and loopback) */
 };
 } // namespace routing
 } // namespace kernel
index c4782e4..23b711d 100644 (file)
@@ -19,6 +19,23 @@ namespace kernel {
 namespace routing {
 ClusterZone::ClusterZone(const std::string& name) : NetZoneImpl(name) {}
 
+void ClusterZone::set_loopback()
+{
+  num_links_per_node_++;
+  has_loopback_ = true;
+}
+
+void ClusterZone::set_limiter()
+{
+  num_links_per_node_++;
+  has_limiter_ = true;
+}
+
+void ClusterZone::add_private_link_at(unsigned int position, std::pair<resource::LinkImpl*, resource::LinkImpl*> link)
+{
+  private_links_.insert({position, link});
+}
+
 void ClusterZone::get_local_route(NetPoint* src, NetPoint* dst, RouteCreationArgs* route, double* lat)
 {
   XBT_VERB("cluster getLocalRoute from '%s'[%u] to '%s'[%u]", src->get_cname(), src->id(), dst->get_cname(), dst->id());
index 850ed0e..c8d532f 100644 (file)
@@ -255,12 +255,12 @@ void DragonflyZone::get_local_route(NetPoint* src, NetPoint* dst, RouteCreationA
   XBT_VERB("dragonfly getLocalRoute from '%s'[%u] to '%s'[%u]", src->get_cname(), src->id(), dst->get_cname(),
            dst->id());
 
-  if ((src->id() == dst->id()) && has_loopback_) {
-    std::pair<resource::LinkImpl*, resource::LinkImpl*> info = private_links_.at(node_pos(src->id()));
+  if ((src->id() == dst->id()) && has_loopback()) {
+    resource::LinkImpl* uplink = get_uplink_from(node_pos(src->id()));
 
-    route->link_list.push_back(info.first);
+    route->link_list.push_back(uplink);
     if (latency)
-      *latency += info.first->get_latency();
+      *latency += uplink->get_latency();
     return;
   }
 
@@ -282,9 +282,8 @@ void DragonflyZone::get_local_route(NetPoint* src, NetPoint* dst, RouteCreationA
   if (latency)
     *latency += myRouter->my_nodes_[myCoords.node * num_links_per_link_]->get_latency();
 
-  if (has_limiter_) { // limiter for sender
-    std::pair<resource::LinkImpl*, resource::LinkImpl*> info = private_links_.at(node_pos_with_loopback(src->id()));
-    route->link_list.push_back(info.first);
+  if (has_limiter()) { // limiter for sender
+    route->link_list.push_back(get_uplink_from(node_pos_with_loopback(src->id())));
   }
 
   if (targetRouter != myRouter) {
@@ -334,9 +333,8 @@ void DragonflyZone::get_local_route(NetPoint* src, NetPoint* dst, RouteCreationA
     }
   }
 
-  if (has_limiter_) { // limiter for receiver
-    std::pair<resource::LinkImpl*, resource::LinkImpl*> info = private_links_.at(node_pos_with_loopback(dst->id()));
-    route->link_list.push_back(info.first);
+  if (has_limiter()) { // limiter for receiver
+    route->link_list.push_back(get_downlink_to(node_pos_with_loopback(dst->id())));
   }
 
   // router->node local link
index 8ac6979..9889d4f 100644 (file)
@@ -77,7 +77,7 @@ void FatTreeZone::get_local_route(NetPoint* src, NetPoint* dst, RouteCreationArg
            dst->get_cname(), dst->id());
 
   /* In case destination is the source, and there is a loopback, let's use it instead of going up to a switch */
-  if (source->id == destination->id && this->has_loopback_) {
+  if (source->id == destination->id && has_loopback()) {
     into->link_list.push_back(source->loopback);
     if (latency)
       *latency += source->loopback->get_latency();
@@ -100,7 +100,7 @@ void FatTreeZone::get_local_route(NetPoint* src, NetPoint* dst, RouteCreationArg
     if (latency)
       *latency += currentNode->parents[d]->up_link_->get_latency();
 
-    if (this->has_limiter_)
+    if (has_limiter())
       into->link_list.push_back(currentNode->limiter_link_);
     currentNode = currentNode->parents[d]->up_node_;
   }
@@ -116,7 +116,7 @@ void FatTreeZone::get_local_route(NetPoint* src, NetPoint* dst, RouteCreationArg
         if (latency)
           *latency += currentNode->children[i]->down_link_->get_latency();
         currentNode = currentNode->children[i]->down_node_;
-        if (this->has_limiter_)
+        if (has_limiter())
           into->link_list.push_back(currentNode->limiter_link_);
         XBT_DEBUG("%d(%u,%u) is accessible through %d(%u,%u)", destination->id, destination->level,
                   destination->position, currentNode->id, currentNode->level, currentNode->position);
index 1a123c1..a31d60d 100644 (file)
@@ -55,7 +55,7 @@ void TorusZone::create_links_for_node(ClusterCreationArgs* cluster, int id, int
      * Note that position rankId*(xbt_dynar_length(dimensions)+has_loopback?+has_limiter?)
      * holds the link "rankId->rankId"
      */
-    private_links_.insert({position + j, {linkUp, linkDown}});
+    add_private_link_at(position + j, {linkUp, linkDown});
     dim_product *= current_dimension;
   }
 }
@@ -73,7 +73,7 @@ void TorusZone::parse_specific_arguments(ClusterCreationArgs* cluster)
     for (auto const& group : dimensions)
       dimensions_.push_back(surf_parse_get_int(group));
 
-    num_links_per_node_ = dimensions_.size();
+    set_num_links_per_node(dimensions_.size());
   }
 }
 
@@ -84,12 +84,12 @@ void TorusZone::get_local_route(NetPoint* src, NetPoint* dst, RouteCreationArgs*
   if (dst->is_router() || src->is_router())
     return;
 
-  if (src->id() == dst->id() && has_loopback_) {
-    std::pair<resource::LinkImpl*, resource::LinkImpl*> info = private_links_.at(src->id() * num_links_per_node_);
+  if (src->id() == dst->id() && has_loopback()) {
+    resource::LinkImpl* uplink = get_uplink_from(node_pos(src->id()));
 
-    route->link_list.push_back(info.first);
+    route->link_list.push_back(uplink);
     if (lat)
-      *lat += info.first->get_latency();
+      *lat += uplink->get_latency();
     return;
   }
 
@@ -140,8 +140,8 @@ void TorusZone::get_local_route(NetPoint* src, NetPoint* dst, RouteCreationArgs*
             next_node = (current_node + dim_product);
 
           // HERE: We use *CURRENT* node for calculation (as opposed to next_node)
-          nodeOffset = current_node * (num_links_per_node_);
-          linkOffset = nodeOffset + (has_loopback_ ? 1 : 0) + (has_limiter_ ? 1 : 0) + j;
+          nodeOffset = node_pos(current_node);
+          linkOffset = node_pos_with_loopback_limiter(current_node) + j;
           use_lnk_up = true;
           assert(linkOffset >= 0);
         } else { // Route to the left
@@ -151,8 +151,8 @@ void TorusZone::get_local_route(NetPoint* src, NetPoint* dst, RouteCreationArgs*
             next_node = (current_node - dim_product);
 
           // HERE: We use *next* node for calculation (as opposed to current_node!)
-          nodeOffset = next_node * (num_links_per_node_);
-          linkOffset = nodeOffset + j + (has_loopback_ ? 1 : 0) + (has_limiter_ ? 1 : 0);
+          nodeOffset = node_pos(next_node);
+          linkOffset = node_pos_with_loopback_limiter(next_node) + j;
           use_lnk_up = false;
 
           assert(linkOffset >= 0);
@@ -165,15 +165,15 @@ void TorusZone::get_local_route(NetPoint* src, NetPoint* dst, RouteCreationArgs*
       dim_product *= cur_dim;
     }
 
-    std::pair<resource::LinkImpl*, resource::LinkImpl*> info;
-
-    if (has_limiter_) { // limiter for sender
-      info = private_links_.at(nodeOffset + (has_loopback_ ? 1 : 0));
-      route->link_list.push_back(info.first);
+    if (has_limiter()) { // limiter for sender
+      route->link_list.push_back(get_uplink_from(node_pos_with_loopback(nodeOffset)));
     }
 
-    info                    = private_links_.at(linkOffset);
-    resource::LinkImpl* lnk = use_lnk_up ? info.first : info.second;
+    resource::LinkImpl* lnk;
+    if (use_lnk_up)
+      lnk = get_uplink_from(linkOffset);
+    else
+      lnk = get_downlink_to(linkOffset);
 
     route->link_list.push_back(lnk);
     if (lat)
index 70822eb..4fa3069 100644 (file)
@@ -78,7 +78,7 @@ void VivaldiZone::set_peer_link(NetPoint* netpoint, double bw_in, double bw_out,
   resource::LinkImpl* linkDown =
       get_network_model()->create_link(link_down, std::vector<double>(1, bw_in), s4u::Link::SharingPolicy::SHARED);
   linkDown->seal();
-  private_links_.insert({netpoint->id(), {linkUp, linkDown}});
+  add_private_link_at(netpoint->id(), {linkUp, linkDown});
 }
 
 void VivaldiZone::get_local_route(NetPoint* src, NetPoint* dst, RouteCreationArgs* route, double* lat)
@@ -93,30 +93,19 @@ void VivaldiZone::get_local_route(NetPoint* src, NetPoint* dst, RouteCreationArg
   }
 
   /* Retrieve the private links */
-  auto src_link = private_links_.find(src->id());
-  if (src_link != private_links_.end()) {
-    std::pair<resource::LinkImpl*, resource::LinkImpl*> info = src_link->second;
-    if (info.first) {
-      route->link_list.push_back(info.first);
-      if (lat)
-        *lat += info.first->get_latency();
-    }
-  } else {
-    XBT_DEBUG("Source of private link (%u) doesn't exist", src->id());
+  if (private_link_exists_at(src->id())) {
+    auto src_link = get_uplink_from(src->id());
+    route->link_list.push_back(src_link);
+    if (lat)
+      *lat += src_link->get_latency();
   }
 
-  auto dst_link = private_links_.find(dst->id());
-  if (dst_link != private_links_.end()) {
-    std::pair<resource::LinkImpl*, resource::LinkImpl*> info = dst_link->second;
-    if (info.second) {
-      route->link_list.push_back(info.second);
-      if (lat)
-        *lat += info.second->get_latency();
-    }
-  } else {
-    XBT_DEBUG("Destination of private link (%u) doesn't exist", dst->id());
+  if (private_link_exists_at(dst->id())) {
+    auto dst_link = get_downlink_to(dst->id());
+    route->link_list.push_back(dst_link);
+    if (lat)
+      *lat += dst_link->get_latency();
   }
-
   /* Compute the extra latency due to the euclidean distance if needed */
   if (lat) {
     std::vector<double>* srcCoords = netpoint_get_coords(src);
index c71b8b2..618e360 100644 (file)
@@ -175,13 +175,11 @@ void sg_platf_new_cluster(simgrid::kernel::routing::ClusterCreationArgs* cluster
       current_as->get_iface()->set_property(elm.first, elm.second);
 
   if (cluster->loopback_bw > 0 || cluster->loopback_lat > 0) {
-    current_as->num_links_per_node_++;
-    current_as->has_loopback_ = true;
+    current_as->set_loopback();
   }
 
   if (cluster->limiter_link > 0) {
-    current_as->num_links_per_node_++;
-    current_as->has_limiter_ = true;
+    current_as->set_limiter();
   }
 
   for (int const& i : *cluster->radicals) {
@@ -231,7 +229,7 @@ void sg_platf_new_cluster(simgrid::kernel::routing::ClusterCreationArgs* cluster
       linkDown = simgrid::s4u::Link::by_name_or_null(tmp_link);
 
       ClusterZone* as_cluster = current_as;
-      as_cluster->private_links_.insert({as_cluster->node_pos(rankId), {linkUp->get_impl(), linkDown->get_impl()}});
+      as_cluster->add_private_link_at(as_cluster->node_pos(rankId), {linkUp->get_impl(), linkDown->get_impl()});
     }
 
     // add a limiter link (shared link to account for maximal bandwidth of the node)
@@ -249,8 +247,8 @@ void sg_platf_new_cluster(simgrid::kernel::routing::ClusterCreationArgs* cluster
       sg_platf_new_link(&link);
       linkDown = simgrid::s4u::Link::by_name_or_null(tmp_link);
       linkUp   = linkDown;
-      current_as->private_links_.insert(
-          {current_as->node_pos_with_loopback(rankId), {linkUp->get_impl(), linkDown->get_impl()}});
+      current_as->add_private_link_at(current_as->node_pos_with_loopback(rankId),
+                                      {linkUp->get_impl(), linkDown->get_impl()});
     }
 
     // call the cluster function that adds the others links
@@ -268,7 +266,7 @@ void sg_platf_new_cluster(simgrid::kernel::routing::ClusterCreationArgs* cluster
   XBT_DEBUG("<router id=\"%s\"/>", cluster->router_id.c_str());
   if (cluster->router_id.empty())
     cluster->router_id = std::string(cluster->prefix) + cluster->id + "_router" + cluster->suffix;
-  current_as->router_ = sg_platf_new_router(cluster->router_id, nullptr);
+  current_as->set_router(sg_platf_new_router(cluster->router_id, nullptr));
 
   // Make the backbone
   if ((cluster->bb_bw > 0) || (cluster->bb_lat > 0)) {
@@ -296,9 +294,9 @@ void routing_cluster_add_backbone(simgrid::kernel::resource::LinkImpl* bb)
   auto* cluster = dynamic_cast<simgrid::kernel::routing::ClusterZone*>(current_routing);
 
   xbt_assert(cluster, "Only hosts from Cluster can get a backbone.");
-  xbt_assert(nullptr == cluster->backbone_, "Cluster %s already has a backbone link!", cluster->get_cname());
+  xbt_assert(not cluster->has_backbone(), "Cluster %s already has a backbone link!", cluster->get_cname());
 
-  cluster->backbone_ = bb;
+  cluster->set_backbone(bb);
   XBT_DEBUG("Add a backbone to AS '%s'", current_routing->get_cname());
 }
 
@@ -621,11 +619,11 @@ void sg_platf_new_hostlink(const simgrid::kernel::routing::HostLinkCreationArgs*
 
   auto* as_cluster = static_cast<simgrid::kernel::routing::ClusterZone*>(current_routing);
 
-  if (as_cluster->private_links_.find(netpoint->id()) != as_cluster->private_links_.end())
+  if (as_cluster->private_link_exists_at(netpoint->id()))
     surf_parse_error(std::string("Host_link for '") + hostlink->id.c_str() + "' is already defined!");
 
   XBT_DEBUG("Push Host_link for host '%s' to position %u", netpoint->get_cname(), netpoint->id());
-  as_cluster->private_links_.insert({netpoint->id(), {linkUp->get_impl(), linkDown->get_impl()}});
+  as_cluster->add_private_link_at(netpoint->id(), {linkUp->get_impl(), linkDown->get_impl()});
 }
 
 void sg_platf_new_trace(simgrid::kernel::routing::ProfileCreationArgs* profile)