Skip to content

Commit

Permalink
fixes for Dave's get/set info code
Browse files Browse the repository at this point in the history 
The expected sequence of events for processing info during object creation
is that if there's an incoming info arg, it is opal_info_dup()ed into the obj
at obj->s_info first. Then interested components register callbacks for
keys they want to know about using opal_infosubscribe_infosubscribe().

Inside info_subscribe_subscribe() the specified callback() is called with
whatever matching k/v is in the object's info, or with the default. The
return string from the callback goes into the new k/v stored in info, and
the input k/v is saved as __IN_<key>/<val>. It's saved the same way
whether the input came from info or whether it was a default. A null return
from the callback indicates an ignored key/val, and no k/v is stored for
it, but an __IN_<key>/<val> is still kept so we still have access to the
original.

At MPI_*_set_info() time, opal_infosubscribe_change_info() is used. That
function calls the registered callbacks for each item in the provided info.
If the callback returns non-null, the info is updated with that k/v, or if
the callback returns null, that key is deleted from info. An __IN_<key>/<val>
is saved either way, and overwrites any previously saved value.

When MPI_*_get_info() is called, opal_info_dup_mpistandard() is used, which
allows relatively easy changes in interpretation of the standard, by looking
at both the <key>/<val> and __IN_<key>/<val> in info. Right now it does
  1. includes system extras, eg k/v defaults not expliclty set by the user
  2. omits ignored keys
  3. shows input values, not callback modifications, eg not the internal values

Currently the callbacks are doing things like
    return some_condition ? "true" : "false"
that is, returning static strings that are not to be freed. If the return
strings start becoming more dynamic in the future I don't see how unallocated
strings could support that, so I'd propose a change for the future that
the callback()s registered with info_subscribe_subscribe() do a strdup on
their return, and we change the callers of callback() to free the strings
it returns (there are only two callers).

Rough outline of the smaller changes spread over the less central files:
  comm.c
    initialize comm->super.s_info to NULL
    copy into comm->super.s_info in comm creation calls that provide info
    OBJ_RELEASE comm->super.s_info at free time
  comm_init.c
    initialize comm->super.s_info to NULL
  file.c
    copy into file->super.s_info if file creation provides info
    OBJ_RELEASE file->super.s_info at free time
  win.c
    copy into win->super.s_info if win creation provides info
    OBJ_RELEASE win->super.s_info at free time

  comm_get_info.c
  file_get_info.c
  win_get_info.c
    change_info() if there's no info attached (shouldn't happen if callbacks
      are registered)
    copy the info for the user

The other category of change is generally addressing compiler warnings where
ompi_info_t and opal_info_t were being used a little too interchangably. An
ompi_info_t* contains an opal_info_t*, at &(ompi_info->super)

Also this commit updates the copyrights.

Signed-off-by: Mark Allen <markalle@us.ibm.com>
  • Loading branch information
@markalle
markalle committed Feb 14, 2017
1 parent eaa84fc commit ffaf595
Show file tree
Hide file tree
Showing 62 changed files with 765 additions and 203 deletions.
2 changes: 0 additions & 2 deletions AUTHORS
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,6 @@ Dave Goodell, Cisco
dgoodell@cisco.com
David Daniel, Los Alamos National Laboratory
ddd@lanl.gov
David Solt, IBM
dsolt@us.ibm.com
Denis Dimick, Los Alamos National Laboratory
dgdimick@lnal.gov
Devendar Bureddy, Mellanox
Expand Down
28 changes: 27 additions & 1 deletion ompi/communicator/comm.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
* and Technology (RIST). All rights reserved.
* Copyright (c) 2014-2015 Intel, Inc. All rights reserved.
* Copyright (c) 2015 Mellanox Technologies. All rights reserved.
* Copyright (c) 2016 IBM Corp. All rights reserved.
* Copyright (c) 2016-2017 IBM Corporation. All rights reserved.
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -158,6 +158,7 @@ int ompi_comm_set_nb ( ompi_communicator_t **ncomm,

/* ompi_comm_allocate */
newcomm = OBJ_NEW(ompi_communicator_t);
newcomm->super.s_info = NULL;
/* fill in the inscribing hyper-cube dimensions */
newcomm->c_cube_dim = opal_cube_dim(local_size);
newcomm->c_id_available = MPI_UNDEFINED;
Expand Down Expand Up @@ -918,6 +919,12 @@ int ompi_comm_split_type (ompi_communicator_t *comm, int split_type, int key,
break;
}

// Copy info if there is one.
newcomp->super.s_info = OBJ_NEW(opal_info_t);
if (info) {
opal_info_dup(info, &(newcomp->super.s_info));
}

/* Activate the communicator and init coll-component */
rc = ompi_comm_activate (&newcomp, comm, NULL, NULL, NULL, false, mode);
if (OPAL_UNLIKELY(OMPI_SUCCESS != rc)) {
Expand Down Expand Up @@ -1015,6 +1022,12 @@ int ompi_comm_dup_with_info ( ompi_communicator_t * comm, opal_info_t *info, omp
snprintf(newcomp->c_name, MPI_MAX_OBJECT_NAME, "MPI COMMUNICATOR %d DUP FROM %d",
newcomp->c_contextid, comm->c_contextid );

// Copy info if there is one.
newcomp->super.s_info = OBJ_NEW(opal_info_t);
if (info) {
opal_info_dup(info, &(newcomp->super.s_info));
}

/* activate communicator and init coll-module */
rc = ompi_comm_activate (&newcomp, comm, NULL, NULL, NULL, false, mode);
if ( OMPI_SUCCESS != rc ) {
Expand Down Expand Up @@ -1095,6 +1108,15 @@ static int ompi_comm_idup_internal (ompi_communicator_t *comm, ompi_group_t *gro
return rc;
}

// Copy info if there is one.
{
ompi_communicator_t *newcomp = context->newcomp;
newcomp->super.s_info = OBJ_NEW(opal_info_t);
if (info) {
opal_info_dup(info, &(newcomp->super.s_info));
}
}

ompi_comm_request_schedule_append (request, ompi_comm_idup_getcid, subreq, subreq[0] ? 1 : 0);

/* assign the newcomm now */
Expand Down Expand Up @@ -1472,6 +1494,10 @@ int ompi_comm_free( ompi_communicator_t **comm )
ompi_mpi_comm_parent = &ompi_mpi_comm_null.comm;
}

if (NULL != ((*comm)->super.s_info)) {
OBJ_RELEASE((*comm)->super.s_info);
}

/* Release the communicator */
if ( OMPI_COMM_IS_DYNAMIC (*comm) ) {
ompi_comm_num_dyncomm --;
Expand Down
3 changes: 2 additions & 1 deletion ompi/communicator/comm_init.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
* Copyright (c) 2015 Research Organization for Information Science
* and Technology (RIST). All rights reserved.
* Copyright (c) 2015 Intel, Inc. All rights reserved.
* Copyright (c) 2016 IBM Corp. All rights reserved.
* Copyright (c) 2016-2017 IBM Corporation. All rights reserved.
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -221,6 +221,7 @@ ompi_communicator_t *ompi_comm_allocate ( int local_size, int remote_size )

/* create new communicator element */
new_comm = OBJ_NEW(ompi_communicator_t);
new_comm->super.s_info = NULL;
new_comm->c_local_group = ompi_group_allocate ( local_size );
if ( 0 < remote_size ) {
new_comm->c_remote_group = ompi_group_allocate (remote_size);
Expand Down
2 changes: 1 addition & 1 deletion ompi/communicator/communicator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
* Copyright (c) 2014-2015 Intel, Inc. All rights reserved.
* Copyright (c) 2015 Research Organization for Information Science
* and Technology (RIST). All rights reserved.
* Copyright (c) 2016 IBM Corp. All rights reserved.
* Copyright (c) 2016-2017 IBM Corporation. All rights reserved.
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down
2 changes: 1 addition & 1 deletion ompi/debuggers/predefined_gap_test.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* of Tennessee Research Foundation. All rights
* reserved.
* Copyright (c) 2012-2013 Inria. All rights reserved.
* Copyright (c) 2016 IBM Corp. All rights reserved.
* Copyright (c) 2016-2017 IBM Corporation. All rights reserved.
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down
44 changes: 22 additions & 22 deletions ompi/dpm/dpm.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
* Copyright (c) 2013-2016 Intel, Inc. All rights reserved.
* Copyright (c) 2014-2017 Research Organization for Information Science
* and Technology (RIST). All rights reserved.
* Copyright (c) 2016 IBM Corp. All rights reserved.
* Copyright (c) 2016-2017 IBM Corporation. All rights reserved.
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -704,7 +704,7 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
if ( array_of_info != NULL && array_of_info[i] != MPI_INFO_NULL ) {

/* check for personality - this is a job-level key */
opal_info_get (array_of_info[i], "personality", sizeof(host) - 1, host, &flag);
ompi_info_get (array_of_info[i], "personality", sizeof(host) - 1, host, &flag);
if ( flag ) {
personality = true;
info = OBJ_NEW(opal_value_t);
Expand All @@ -714,7 +714,7 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
}

/* check for 'host' */
opal_info_get (array_of_info[i], "host", sizeof(host) - 1, host, &flag);
ompi_info_get (array_of_info[i], "host", sizeof(host) - 1, host, &flag);
if ( flag ) {
info = OBJ_NEW(opal_value_t);
info->key = strdup(OPAL_PMIX_HOST);
Expand All @@ -723,7 +723,7 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
}

/* check for 'hostfile' */
opal_info_get (array_of_info[i], "hostfile", sizeof(host) - 1, host, &flag);
ompi_info_get (array_of_info[i], "hostfile", sizeof(host) - 1, host, &flag);
if ( flag ) {
info = OBJ_NEW(opal_value_t);
info->key = strdup(OPAL_PMIX_HOSTFILE);
Expand All @@ -732,7 +732,7 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
}

/* check for 'add-hostfile' */
opal_info_get (array_of_info[i], "add-hostfile", sizeof(host) - 1, host, &flag);
ompi_info_get (array_of_info[i], "add-hostfile", sizeof(host) - 1, host, &flag);
if ( flag ) {
info = OBJ_NEW(opal_value_t);
info->key = strdup(OPAL_PMIX_ADD_HOSTFILE);
Expand All @@ -741,7 +741,7 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
}

/* check for 'add-host' */
opal_info_get (array_of_info[i], "add-host", sizeof(host) - 1, host, &flag);
ompi_info_get (array_of_info[i], "add-host", sizeof(host) - 1, host, &flag);
if ( flag ) {
info = OBJ_NEW(opal_value_t);
info->key = strdup(OPAL_PMIX_ADD_HOST);
Expand All @@ -750,7 +750,7 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
}

/* check for env */
opal_info_get (array_of_info[i], "env", sizeof(host)-1, host, &flag);
ompi_info_get (array_of_info[i], "env", sizeof(host)-1, host, &flag);
if ( flag ) {
envars = opal_argv_split(host, '\n');
for (j=0; NULL != envars[j]; j++) {
Expand All @@ -766,7 +766,7 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
*
* This is a job-level key
*/
opal_info_get (array_of_info[i], "ompi_prefix", sizeof(prefix) - 1, prefix, &flag);
ompi_info_get (array_of_info[i], "ompi_prefix", sizeof(prefix) - 1, prefix, &flag);
if ( flag ) {
info = OBJ_NEW(opal_value_t);
info->key = strdup(OPAL_PMIX_PREFIX);
Expand All @@ -775,7 +775,7 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
}

/* check for 'wdir' */
opal_info_get (array_of_info[i], "wdir", sizeof(cwd) - 1, cwd, &flag);
ompi_info_get (array_of_info[i], "wdir", sizeof(cwd) - 1, cwd, &flag);
if ( flag ) {
info = OBJ_NEW(opal_value_t);
info->key = strdup(OPAL_PMIX_WDIR);
Expand All @@ -785,7 +785,7 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
}

/* check for 'mapper' - a job-level key */
opal_info_get(array_of_info[i], "mapper", sizeof(mapper) - 1, mapper, &flag);
ompi_info_get(array_of_info[i], "mapper", sizeof(mapper) - 1, mapper, &flag);
if ( flag ) {
info = OBJ_NEW(opal_value_t);
info->key = strdup(OPAL_PMIX_MAPPER);
Expand All @@ -794,7 +794,7 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
}

/* check for 'display_map' - a job-level key */
opal_info_get_bool(array_of_info[i], "display_map", &local_spawn, &flag);
ompi_info_get_bool(array_of_info[i], "display_map", &local_spawn, &flag);
if ( flag ) {
info = OBJ_NEW(opal_value_t);
info->key = strdup(OPAL_PMIX_DISPLAY_MAP);
Expand All @@ -803,22 +803,22 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
}

/* check for 'npernode' and 'ppr' - job-level key */
opal_info_get (array_of_info[i], "npernode", sizeof(slot_list) - 1, slot_list, &flag);
ompi_info_get (array_of_info[i], "npernode", sizeof(slot_list) - 1, slot_list, &flag);
if ( flag ) {
info = OBJ_NEW(opal_value_t);
info->key = strdup(OPAL_PMIX_PPR);
info->type = OPAL_STRING;
(void)asprintf(&(info->data.string), "%s:n", slot_list);
opal_list_append(&job_info, &info->super);
}
opal_info_get (array_of_info[i], "pernode", sizeof(slot_list) - 1, slot_list, &flag);
ompi_info_get (array_of_info[i], "pernode", sizeof(slot_list) - 1, slot_list, &flag);
if ( flag ) {
info = OBJ_NEW(opal_value_t);
info->key = strdup(OPAL_PMIX_PPR);
opal_value_load(info, "1:n", OPAL_STRING);
opal_list_append(&job_info, &info->super);
}
opal_info_get (array_of_info[i], "ppr", sizeof(slot_list) - 1, slot_list, &flag);
ompi_info_get (array_of_info[i], "ppr", sizeof(slot_list) - 1, slot_list, &flag);
if ( flag ) {
info = OBJ_NEW(opal_value_t);
info->key = strdup(OPAL_PMIX_PPR);
Expand All @@ -827,7 +827,7 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
}

/* check for 'map_by' - job-level key */
opal_info_get(array_of_info[i], "map_by", sizeof(slot_list) - 1, slot_list, &flag);
ompi_info_get(array_of_info[i], "map_by", sizeof(slot_list) - 1, slot_list, &flag);
if ( flag ) {
info = OBJ_NEW(opal_value_t);
info->key = strdup(OPAL_PMIX_MAPBY);
Expand All @@ -836,7 +836,7 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
}

/* check for 'rank_by' - job-level key */
opal_info_get(array_of_info[i], "rank_by", sizeof(slot_list) - 1, slot_list, &flag);
ompi_info_get(array_of_info[i], "rank_by", sizeof(slot_list) - 1, slot_list, &flag);
if ( flag ) {
info = OBJ_NEW(opal_value_t);
info->key = strdup(OPAL_PMIX_RANKBY);
Expand All @@ -845,7 +845,7 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
}

/* check for 'bind_to' - job-level key */
opal_info_get(array_of_info[i], "bind_to", sizeof(slot_list) - 1, slot_list, &flag);
ompi_info_get(array_of_info[i], "bind_to", sizeof(slot_list) - 1, slot_list, &flag);
if ( flag ) {
info = OBJ_NEW(opal_value_t);
info->key = strdup(OPAL_PMIX_BINDTO);
Expand All @@ -854,7 +854,7 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
}

/* check for 'preload_binary' - job-level key */
opal_info_get_bool(array_of_info[i], "ompi_preload_binary", &local_spawn, &flag);
ompi_info_get_bool(array_of_info[i], "ompi_preload_binary", &local_spawn, &flag);
if ( flag ) {
info = OBJ_NEW(opal_value_t);
info->key = strdup(OPAL_PMIX_PRELOAD_BIN);
Expand All @@ -863,7 +863,7 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
}

/* check for 'preload_files' - job-level key */
opal_info_get (array_of_info[i], "ompi_preload_files", sizeof(cwd) - 1, cwd, &flag);
ompi_info_get (array_of_info[i], "ompi_preload_files", sizeof(cwd) - 1, cwd, &flag);
if ( flag ) {
info = OBJ_NEW(opal_value_t);
info->key = strdup(OPAL_PMIX_PRELOAD_FILES);
Expand All @@ -874,7 +874,7 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
/* see if this is a non-mpi job - if so, then set the flag so ORTE
* knows what to do - job-level key
*/
opal_info_get_bool(array_of_info[i], "ompi_non_mpi", &non_mpi, &flag);
ompi_info_get_bool(array_of_info[i], "ompi_non_mpi", &non_mpi, &flag);
if (flag && non_mpi) {
info = OBJ_NEW(opal_value_t);
info->key = strdup(OPAL_PMIX_NON_PMI);
Expand All @@ -883,15 +883,15 @@ int ompi_dpm_spawn(int count, const char *array_of_commands[],
}

/* see if this is an MCA param that the user wants applied to the child job */
opal_info_get (array_of_info[i], "ompi_param", sizeof(params) - 1, params, &flag);
ompi_info_get (array_of_info[i], "ompi_param", sizeof(params) - 1, params, &flag);
if ( flag ) {
opal_argv_append_unique_nosize(&app->env, params, true);
}

/* see if user specified what to do with stdin - defaults to
* not forwarding stdin to child processes - job-level key
*/
opal_info_get (array_of_info[i], "ompi_stdin_target", sizeof(stdin_target) - 1, stdin_target, &flag);
ompi_info_get (array_of_info[i], "ompi_stdin_target", sizeof(stdin_target) - 1, stdin_target, &flag);
if ( flag ) {
if (0 == strcmp(stdin_target, "all")) {
ui32 = ORTE_VPID_WILDCARD;
Expand Down
18 changes: 12 additions & 6 deletions ompi/file/file.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
* Copyright (c) 2015 Research Organization for Information Science
* and Technology (RIST). All rights reserved.
* Copyright (c) 2016 University of Houston. All rights reserved.
* Copyright (c) 2016 IBM Corp. All rights reserved.
* Copyright (c) 2016-2017 IBM Corporation. All rights reserved.
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -114,11 +114,10 @@ int ompi_file_open(struct ompi_communicator_t *comm, const char *filename,
file->f_comm = comm;
OBJ_RETAIN(comm);

/* Present the info to the info layer */

if (OPAL_SUCCESS != opal_infosubscribe_change_info(&file->super, info)) {
OBJ_RELEASE(file);
return ret;
/* Copy the info for the info layer */
file->super.s_info = OBJ_NEW(opal_info_t);
if (info) {
opal_info_dup(info, &(file->super.s_info));
}

file->f_amode = amode;
Expand Down Expand Up @@ -310,6 +309,13 @@ static void file_destructor(ompi_file_t *file)
#endif
}

if (NULL != file->super.s_info) {
OBJ_RELEASE(file->super.s_info);
#if OPAL_ENABLE_DEBUG
file->super.s_info = NULL;
#endif
}

/* Reset the f_to_c table entry */

if (MPI_UNDEFINED != file->f_f_to_c_index &&
Expand Down
Loading

0 comments on commit ffaf595

Please sign in to comment.