Skip to content

rmaps/base: fix logic (crash, in some cases) when num_procs > num_obj… #7643

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 24, 2020
Merged

rmaps/base: fix logic (crash, in some cases) when num_procs > num_obj… #7643

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1cd89c9
rmaps/base: fix logic (crash, in some cases) when num_procs > num_obj…
alex--m Apr 17, 2020
79c4268
Correct the error codes and clarify/correct logic
rhc54 Apr 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
192 changes: 115 additions & 77 deletions orte/mca/rmaps/base/rmaps_base_ranking.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@
* Copyright (c) 2004-2005 The Regents of the University of California.
* All rights reserved.
* Copyright (c) 2011-2017 Cisco Systems, Inc. All rights reserved
* Copyright (c) 2014-2018 Intel, Inc. All rights reserved.
* Copyright (c) 2014-2020 Intel, Inc. All rights reserved.
* Copyright (c) 2017 Research Organization for Information Science
* and Technology (RIST). All rights reserved.
* Copyright (c) 2020 Huawei Technologies Co., Ltd. All rights reserved.
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -137,7 +138,13 @@ static int rank_span(orte_job_t *jdata,
}
/* protect against bozo case */
locale = NULL;
if (!orte_get_attribute(&proc->attributes, ORTE_PROC_HWLOC_LOCALE, (void**)&locale, OPAL_PTR)) {
if (!orte_get_attribute(&proc->attributes, ORTE_PROC_HWLOC_LOCALE, (void**)&locale, OPAL_PTR) ||
NULL == locale) {
/* all mappers are _required_ to set the locale where the proc
* has been mapped - it is therefore an error for this attribute
* not to be set. Likewise, only a programming error could allow
* the attribute to be set to a NULL value - however, we add that
* conditional here to silence any compiler warnings */
ORTE_ERROR_LOG(ORTE_ERROR);
return ORTE_ERROR;
}
Expand Down Expand Up @@ -175,6 +182,11 @@ static int rank_span(orte_job_t *jdata,
}
}
}

/* Are all the procs ranked? we don't want to crash on INVALID ranks */
if (cnt < app->num_procs) {
return ORTE_ERR_FAILED_TO_MAP;
}
}

return ORTE_SUCCESS;
Expand Down Expand Up @@ -258,7 +270,13 @@ static int rank_fill(orte_job_t *jdata,
}
/* protect against bozo case */
locale = NULL;
if (!orte_get_attribute(&proc->attributes, ORTE_PROC_HWLOC_LOCALE, (void**)&locale, OPAL_PTR)) {
if (!orte_get_attribute(&proc->attributes, ORTE_PROC_HWLOC_LOCALE, (void**)&locale, OPAL_PTR) ||
NULL == locale) {
/* all mappers are _required_ to set the locale where the proc
* has been mapped - it is therefore an error for this attribute
* not to be set. Likewise, only a programming error could allow
* the attribute to be set to a NULL value - however, we add that
* conditional here to silence any compiler warnings */
ORTE_ERROR_LOG(ORTE_ERROR);
return ORTE_ERROR;
}
Expand Down Expand Up @@ -293,6 +311,11 @@ static int rank_fill(orte_job_t *jdata,
}
}
}

/* Are all the procs ranked? we don't want to crash on INVALID ranks */
if (cnt < app->num_procs) {
return ORTE_ERR_FAILED_TO_MAP;
}
}

return ORTE_SUCCESS;
Expand All @@ -308,11 +331,12 @@ static int rank_by(orte_job_t *jdata,
orte_vpid_t num_ranked=0;
orte_node_t *node;
orte_proc_t *proc, *pptr;
orte_vpid_t vpid, np;
orte_vpid_t vpid;
int cnt;
opal_pointer_array_t objs;
hwloc_obj_t locale;
orte_app_idx_t napp;
bool noassign;

if (ORTE_RANKING_SPAN & ORTE_GET_RANKING_DIRECTIVE(jdata->map->ranking)) {
return rank_span(jdata, target, cache_level);
Expand Down Expand Up @@ -372,92 +396,106 @@ static int rank_by(orte_job_t *jdata,
* of procs on the node can't be used to tell us when we
* are done. Instead, we have to just keep going until all
* procs are ranked - which means we have to make one extra
* pass thru the loop
* pass thru the loop. In addition, if we pass thru the entire
* loop without assigning anything then we are done
*
* Perhaps someday someone will come up with a more efficient
* algorithm, but this works for now.
*/
i = 0;
while (cnt < app->num_procs && i < (int)node->num_procs) {
/* get the next object */
obj = (hwloc_obj_t)opal_pointer_array_get_item(&objs, i % num_objs);
if (NULL == obj) {
break;
}
/* scan across the procs and find the one that is on this object */
np = 0;
for (j=0; np < node->num_procs && j < node->procs->size && cnt < app->num_procs; j++) {
if (NULL == (proc = (orte_proc_t*)opal_pointer_array_get_item(node->procs, j))) {
continue;
}
np++;
/* ignore procs from other jobs */
if (proc->name.jobid != jdata->jobid) {
opal_output_verbose(5, orte_rmaps_base_framework.framework_output,
"mca:rmaps:rank_by skipping proc %s - from another job, num_ranked %d",
ORTE_NAME_PRINT(&proc->name), num_ranked);
continue;
}
/* ignore procs that are already ranked */
if (ORTE_VPID_INVALID != proc->name.vpid) {
opal_output_verbose(5, orte_rmaps_base_framework.framework_output,
"mca:rmaps:rank_by skipping proc %s - already ranked, num_ranked %d",
ORTE_NAME_PRINT(&proc->name), num_ranked);
continue;
}
/* ignore procs from other apps */
if (proc->app_idx != app->idx) {
opal_output_verbose(5, orte_rmaps_base_framework.framework_output,
"mca:rmaps:rank_by skipping proc %s - from another app, num_ranked %d",
ORTE_NAME_PRINT(&proc->name), num_ranked);
continue;
}
/* protect against bozo case */
locale = NULL;
if (!orte_get_attribute(&proc->attributes, ORTE_PROC_HWLOC_LOCALE, (void**)&locale, OPAL_PTR)) {
ORTE_ERROR_LOG(ORTE_ERROR);
return ORTE_ERROR;
while (cnt < app->num_procs) {
noassign = true;
for (i=0; i < num_objs && cnt < app->num_procs; i++) {
/* get the next object */
obj = (hwloc_obj_t)opal_pointer_array_get_item(&objs, i);
if (NULL == obj) {
break;
}
/* ignore procs not on this object */
if (NULL == locale ||
!hwloc_bitmap_intersects(obj->cpuset, locale->cpuset)) {
/* scan across the procs and find the first unassigned one that includes this object */
for (j=0; j < node->procs->size && cnt < app->num_procs; j++) {
if (NULL == (proc = (orte_proc_t*)opal_pointer_array_get_item(node->procs, j))) {
continue;
}
/* ignore procs from other jobs */
if (proc->name.jobid != jdata->jobid) {
opal_output_verbose(5, orte_rmaps_base_framework.framework_output,
"mca:rmaps:rank_by skipping proc %s - from another job, num_ranked %d",
ORTE_NAME_PRINT(&proc->name), num_ranked);
continue;
}
/* ignore procs that are already ranked */
if (ORTE_VPID_INVALID != proc->name.vpid) {
opal_output_verbose(5, orte_rmaps_base_framework.framework_output,
"mca:rmaps:rank_by skipping proc %s - already ranked, num_ranked %d",
ORTE_NAME_PRINT(&proc->name), num_ranked);
continue;
}
/* ignore procs from other apps - we will get to them */
if (proc->app_idx != app->idx) {
opal_output_verbose(5, orte_rmaps_base_framework.framework_output,
"mca:rmaps:rank_by skipping proc %s - from another app, num_ranked %d",
ORTE_NAME_PRINT(&proc->name), num_ranked);
continue;
}
/* protect against bozo case */
locale = NULL;
if (!orte_get_attribute(&proc->attributes, ORTE_PROC_HWLOC_LOCALE, (void**)&locale, OPAL_PTR) ||
NULL == locale) {
/* all mappers are _required_ to set the locale where the proc
* has been mapped - it is therefore an error for this attribute
* not to be set. Likewise, only a programming error could allow
* the attribute to be set to a NULL value - however, we add that
* conditional here to silence any compiler warnings */
ORTE_ERROR_LOG(ORTE_ERROR);
return ORTE_ERROR;
}
/* ignore procs not on this object */
if (!hwloc_bitmap_intersects(obj->cpuset, locale->cpuset)) {
opal_output_verbose(5, orte_rmaps_base_framework.framework_output,
"mca:rmaps:rank_by: proc at position %d is not on object %d",
j, i);
continue;
}
/* assign the vpid */
proc->name.vpid = vpid;
if (0 == cnt) {
app->first_rank = proc->name.vpid;
}
cnt++;
noassign = false;
opal_output_verbose(5, orte_rmaps_base_framework.framework_output,
"mca:rmaps:rank_by: proc at position %d is not on object %d",
j, i);
continue;
}
/* assign the vpid */
proc->name.vpid = vpid++;
if (0 == cnt) {
app->first_rank = proc->name.vpid;
}
cnt++;
opal_output_verbose(5, orte_rmaps_base_framework.framework_output,
"mca:rmaps:rank_by: proc in position %d is on object %d assigned rank %s",
j, i, ORTE_VPID_PRINT(proc->name.vpid));
/* insert the proc into the jdata array */
if (NULL != (pptr = (orte_proc_t*)opal_pointer_array_get_item(jdata->procs, proc->name.vpid))) {
OBJ_RELEASE(pptr);
}
OBJ_RETAIN(proc);
if (ORTE_SUCCESS != (rc = opal_pointer_array_set_item(jdata->procs, proc->name.vpid, proc))) {
ORTE_ERROR_LOG(rc);
OBJ_DESTRUCT(&objs);
return rc;
"mca:rmaps:rank_by: proc in position %d is on object %d assigned rank %s",
j, i, ORTE_VPID_PRINT(proc->name.vpid));
/* insert the proc into the jdata array */
if (NULL != (pptr = (orte_proc_t*)opal_pointer_array_get_item(jdata->procs, proc->name.vpid))) {
OBJ_RELEASE(pptr);
}
OBJ_RETAIN(proc);
if (ORTE_SUCCESS != (rc = opal_pointer_array_set_item(jdata->procs, proc->name.vpid, proc))) {
ORTE_ERROR_LOG(rc);
OBJ_DESTRUCT(&objs);
return rc;
}
num_ranked++;
/* track where the highest vpid landed - this is our
* new bookmark
*/
jdata->bookmark = node;
/* move to next object */
break;
}
num_ranked++;
/* track where the highest vpid landed - this is our
* new bookmark
*/
jdata->bookmark = node;
/* move to next object */
}
if (noassign) {
break;
}
i++;
}
}
/* cleanup */
OBJ_DESTRUCT(&objs);

/* Are all the procs ranked? we don't want to crash on INVALID ranks */
if (cnt < app->num_procs) {
return ORTE_ERR_FAILED_TO_MAP;
}
}
return ORTE_SUCCESS;
}
Expand Down