From 98764fb7f88025109fbc0afe683577da397edbaf Mon Sep 17 00:00:00 2001 From: SUTER Frederic Date: Thu, 18 Mar 2021 19:26:03 +0100 Subject: [PATCH] privatize fields of ClusterZone + more explicit methods --- MANIFEST.in | 1 + .../simgrid/kernel/routing/ClusterZone.hpp | 37 ++++++++++++++----- src/kernel/routing/ClusterZone.cpp | 17 +++++++++ src/kernel/routing/DragonflyZone.cpp | 18 ++++----- src/kernel/routing/FatTreeZone.cpp | 6 +-- src/kernel/routing/TorusZone.cpp | 34 ++++++++--------- src/kernel/routing/VivaldiZone.cpp | 33 ++++++----------- src/surf/sg_platf.cpp | 22 +++++------ 8 files changed, 94 insertions(+), 74 deletions(-) diff --git a/MANIFEST.in b/MANIFEST.in index c46e3c5718..a28b7fcc09 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -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 diff --git a/include/simgrid/kernel/routing/ClusterZone.hpp b/include/simgrid/kernel/routing/ClusterZone.hpp index fdb8b6512a..e44037de1d 100644 --- a/include/simgrid/kernel/routing/ClusterZone.hpp +++ b/include/simgrid/kernel/routing/ClusterZone.hpp @@ -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> 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 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>* nodes, std::map>* 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> 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 diff --git a/src/kernel/routing/ClusterZone.cpp b/src/kernel/routing/ClusterZone.cpp index c4782e4f26..23b711d8bd 100644 --- a/src/kernel/routing/ClusterZone.cpp +++ b/src/kernel/routing/ClusterZone.cpp @@ -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 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()); diff --git a/src/kernel/routing/DragonflyZone.cpp b/src/kernel/routing/DragonflyZone.cpp index 850ed0e220..c8d532f325 100644 --- a/src/kernel/routing/DragonflyZone.cpp +++ b/src/kernel/routing/DragonflyZone.cpp @@ -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 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 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 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 diff --git a/src/kernel/routing/FatTreeZone.cpp b/src/kernel/routing/FatTreeZone.cpp index 8ac6979ab2..9889d4fda5 100644 --- a/src/kernel/routing/FatTreeZone.cpp +++ b/src/kernel/routing/FatTreeZone.cpp @@ -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); diff --git a/src/kernel/routing/TorusZone.cpp b/src/kernel/routing/TorusZone.cpp index 1a123c1e50..a31d60d1c3 100644 --- a/src/kernel/routing/TorusZone.cpp +++ b/src/kernel/routing/TorusZone.cpp @@ -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 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 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) diff --git a/src/kernel/routing/VivaldiZone.cpp b/src/kernel/routing/VivaldiZone.cpp index 70822eb758..4fa30698a8 100644 --- a/src/kernel/routing/VivaldiZone.cpp +++ b/src/kernel/routing/VivaldiZone.cpp @@ -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(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 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 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* srcCoords = netpoint_get_coords(src); diff --git a/src/surf/sg_platf.cpp b/src/surf/sg_platf.cpp index c71b8b2dda..618e36063f 100644 --- a/src/surf/sg_platf.cpp +++ b/src/surf/sg_platf.cpp @@ -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("", 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(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(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) -- 2.20.1