Skip to content
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

Jump to bottom

Fix the info subscriber mechanism and hidden info keys #12498

Merged
merged 1 commit into from May 3, 2024
Merged

Fix the info subscriber mechanism and hidden info keys #12498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 0 additions & 13 deletions ompi/communicator/comm.c
View file
Open in desktop
Expand Up @@ -707,11 +707,6 @@ int ompi_comm_split_with_info( ompi_communicator_t* comm, int color, int key,
/* Activate the communicator and init coll-component */
rc = ompi_comm_activate (&newcomp, comm, NULL, NULL, NULL, false, mode);

/* MPI-4 §7.4.4 requires us to remove all unknown keys from the info object */
if (NULL != newcomp->super.s_info) {
opal_info_remove_unreferenced(newcomp->super.s_info);
}

exit:
free ( results );
free ( sorted );
Expand Down Expand Up @@ -1028,9 +1023,6 @@ static int ompi_comm_split_type_core(ompi_communicator_t *comm,
goto exit;
}

/* MPI-4 §7.4.4 requires us to remove all unknown keys from the info object */
opal_info_remove_unreferenced(newcomp->super.s_info);

/* TODO: there probably is better way to handle this case without throwing away the
* intermediate communicator. */
rc = ompi_comm_split (newcomp, local_split_type, key, newcomm, false);
Expand Down Expand Up @@ -1363,9 +1355,6 @@ int ompi_comm_dup_with_info ( ompi_communicator_t * comm, opal_info_t *info, omp
return rc;
}

/* MPI-4 §7.4.4 requires us to remove all unknown keys from the info object */
opal_info_remove_unreferenced(newcomp->super.s_info);

*newcomm = newcomp;
return MPI_SUCCESS;
}
Expand Down Expand Up @@ -1522,8 +1511,6 @@ static int ompi_comm_idup_with_info_finish (ompi_comm_request_t *request)
{
ompi_comm_idup_with_info_context_t *context =
(ompi_comm_idup_with_info_context_t *) request->context;
/* MPI-4 §7.4.4 requires us to remove all unknown keys from the info object */
opal_info_remove_unreferenced(context->newcomp->super.s_info);

/* done */
return MPI_SUCCESS;
Expand Down
3 changes: 0 additions & 3 deletions ompi/file/file.c
View file
Open in desktop
Expand Up @@ -138,9 +138,6 @@ int ompi_file_open(struct ompi_communicator_t *comm, const char *filename,
return ret;
}

/* MPI-4 §14.2.8 requires us to remove all unknown keys from the info object */
opal_info_remove_unreferenced(file->super.s_info);

/* All done */

*fh = file;
Expand Down
2 changes: 1 addition & 1 deletion ompi/info/info.c
View file
Open in desktop
Expand Up @@ -243,7 +243,7 @@ int ompi_mpiinfo_init_env(int argc, char *argv[], ompi_info_t *info)
// related calls:

int ompi_info_dup (ompi_info_t *info, ompi_info_t **newinfo) {
return opal_info_dup (&(info->super), (opal_info_t **)newinfo);
return opal_info_dup_public (&(info->super), (opal_info_t **)newinfo);
}
int ompi_info_set (ompi_info_t *info, const char *key, const char *value) {
return opal_info_set (&(info->super), key, value);
Expand Down
2 changes: 1 addition & 1 deletion ompi/mpi/c/comm_get_info.c
View file
Open in desktop
Expand Up @@ -61,7 +61,7 @@ int MPI_Comm_get_info(MPI_Comm comm, MPI_Info *info_used)

opal_info_t *opal_info_used = &(*info_used)->super;

opal_info_dup(comm->super.s_info, &opal_info_used);
opal_info_dup_public(comm->super.s_info, &opal_info_used);

return MPI_SUCCESS;
}
2 changes: 1 addition & 1 deletion ompi/mpi/c/file_get_info.c
View file
Open in desktop
Expand Up @@ -86,7 +86,7 @@ int MPI_File_get_info(MPI_File fh, MPI_Info *info_used)
}
opal_info_t *opal_info_used = &(*info_used)->super;

opal_info_dup(fh->super.s_info, &opal_info_used);
opal_info_dup_public(fh->super.s_info, &opal_info_used);

return OMPI_SUCCESS;
}
2 changes: 1 addition & 1 deletion ompi/mpi/c/win_get_info.c
View file
Open in desktop
Expand Up @@ -60,7 +60,7 @@ int MPI_Win_get_info(MPI_Win win, MPI_Info *info_used)
}
opal_info_t *opal_info_used = &(*info_used)->super;

ret = opal_info_dup(win->super.s_info, &opal_info_used);
ret = opal_info_dup_public(win->super.s_info, &opal_info_used);

OMPI_ERRHANDLER_RETURN(ret, win, ret, FUNC_NAME);
}
12 changes: 0 additions & 12 deletions ompi/win/win.c
View file
Open in desktop
Expand Up @@ -266,9 +266,6 @@ ompi_win_create(void *base, size_t size,
return ret;
}

/* MPI-4 §12.2.7 requires us to remove all unknown keys from the info object */
opal_info_remove_unreferenced(win->super.s_info);

*newwin = win;

return OMPI_SUCCESS;
Expand Down Expand Up @@ -300,9 +297,6 @@ ompi_win_allocate(size_t size, int disp_unit, opal_info_t *info,
return ret;
}

/* MPI-4 §12.2.7 requires us to remove all unknown keys from the info object */
opal_info_remove_unreferenced(win->super.s_info);

*((void**) baseptr) = base;
*newwin = win;

Expand Down Expand Up @@ -335,9 +329,6 @@ ompi_win_allocate_shared(size_t size, int disp_unit, opal_info_t *info,
return ret;
}

/* MPI-4 §12.2.7 requires us to remove all unknown keys from the info object */
opal_info_remove_unreferenced(win->super.s_info);

*((void**) baseptr) = base;
*newwin = win;

Expand Down Expand Up @@ -368,9 +359,6 @@ ompi_win_create_dynamic(opal_info_t *info, ompi_communicator_t *comm, ompi_win_t
return ret;
}

/* MPI-4 §12.2.7 requires us to remove all unknown keys from the info object */
opal_info_remove_unreferenced(win->super.s_info);

*newwin = win;

return OMPI_SUCCESS;
Expand Down
87 changes: 34 additions & 53 deletions opal/util/info.c
View file
Open in desktop
Expand Up @@ -65,14 +65,18 @@ OBJ_CLASS_INSTANCE(opal_info_entry_t, opal_list_item_t, info_entry_constructor,
info_entry_destructor);

/*
* Duplicate an info
* Duplicate an info into newinfo. If public_info is true we only duplicate
* key-value pairs that are not internal and that had been referenced,
* either through opal_info_get or opal_info_set.
*/
int opal_info_dup(opal_info_t *info, opal_info_t **newinfo)
static int opal_info_dup_impl(opal_info_t *info, opal_info_t **newinfo, bool public_only)
{
opal_info_entry_t *iterator;

OPAL_THREAD_LOCK(info->i_lock);
OPAL_LIST_FOREACH (iterator, &info->super, opal_info_entry_t) {
/* skip keys that are internal if we didn't ask for them */
if (public_only && (iterator->ie_internal || iterator->ie_referenced == 0)) continue;
/* create a new info entry and retain the string objects */
opal_info_entry_t *newentry = OBJ_NEW(opal_info_entry_t);
newentry->ie_key = iterator->ie_key;
Expand All @@ -85,6 +89,16 @@ int opal_info_dup(opal_info_t *info, opal_info_t **newinfo)
return OPAL_SUCCESS;
}

int opal_info_dup_public(opal_info_t *info, opal_info_t **newinfo)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move these into a header file and make them always inline.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hesitant about this since there are no other implementations in info.h. The call to opal_info_dup_impl will be a jmp so there is little to gain from inlining these.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A call into a function that does a simple jmp ? I don't even see the interest of having these functions, with a weird name, instead of just using opal_info_dup with all 3 arguments.

{
return opal_info_dup_impl(info, newinfo, true);
}

int opal_info_dup(opal_info_t *info, opal_info_t **newinfo)
{
return opal_info_dup_impl(info, newinfo, false);
}

static void opal_info_get_nolock(opal_info_t *info, const char *key, opal_cstring_t **value,
int *flag)
{
Expand Down Expand Up @@ -136,7 +150,7 @@ static int opal_info_set_cstring_nolock(opal_info_t *info, const char *key, opal
return OPAL_SUCCESS;
}

static int opal_info_set_nolock(opal_info_t *info, const char *key, const char *value)
static int opal_info_set_nolock(opal_info_t *info, const char *key, const char *value, bool internal)
{
opal_info_entry_t *old_info;

Expand All @@ -147,6 +161,7 @@ static int opal_info_set_nolock(opal_info_t *info, const char *key, const char *
*/
size_t value_len = strlen(value);
old_info->ie_referenced++;
old_info->ie_internal = internal;
if (old_info->ie_value->length == value_len
&& 0 == strcmp(old_info->ie_value->string, value)) {
return OPAL_SUCCESS;
Expand All @@ -171,6 +186,7 @@ static int opal_info_set_nolock(opal_info_t *info, const char *key, const char *
return OPAL_ERR_OUT_OF_RESOURCE;
}
new_info->ie_referenced++;
new_info->ie_internal = internal;
opal_list_append(&(info->super), (opal_list_item_t *) new_info);
}
return OPAL_SUCCESS;
Expand All @@ -184,7 +200,20 @@ int opal_info_set(opal_info_t *info, const char *key, const char *value)
int ret;

OPAL_THREAD_LOCK(info->i_lock);
ret = opal_info_set_nolock(info, key, value);
ret = opal_info_set_nolock(info, key, value, false);
OPAL_THREAD_UNLOCK(info->i_lock);
return ret;
}

/*
* Set a value on the info
*/
int opal_info_set_internal(opal_info_t *info, const char *key, const char *value)
{
int ret;

OPAL_THREAD_LOCK(info->i_lock);
ret = opal_info_set_nolock(info, key, value, true);
OPAL_THREAD_UNLOCK(info->i_lock);
return ret;
}
Expand Down Expand Up @@ -372,6 +401,7 @@ static void info_entry_constructor(opal_info_entry_t *entry)
entry->ie_key = NULL;
entry->ie_value = NULL;
entry->ie_referenced = 0;
entry->ie_internal = false;
}

static void info_entry_destructor(opal_info_entry_t *entry)
Expand Down Expand Up @@ -410,52 +440,3 @@ static opal_info_entry_t *info_find_key(opal_info_t *info, const char *key)
}
return NULL;
}

/**
* Mark the entry \c key as referenced.
*/
int opal_info_mark_referenced(opal_info_t *info, const char *key)
{
opal_info_entry_t *entry;

OPAL_THREAD_LOCK(info->i_lock);
entry = info_find_key(info, key);
entry->ie_referenced++;
OPAL_THREAD_UNLOCK(info->i_lock);

return OPAL_SUCCESS;
}

/**
* Remove a reference from the entry \c key.
*/
int opal_info_unmark_referenced(opal_info_t *info, const char *key)
{
opal_info_entry_t *entry;

OPAL_THREAD_LOCK(info->i_lock);
entry = info_find_key(info, key);
entry->ie_referenced--;
OPAL_THREAD_UNLOCK(info->i_lock);

return OPAL_SUCCESS;
}

/**
* Remove any entries that are not marked as referenced
*/
int opal_info_remove_unreferenced(opal_info_t *info)
{
opal_info_entry_t *iterator, *next;
/* iterate over all entries and remove the ones that are not referenced */
OPAL_THREAD_LOCK(info->i_lock);
OPAL_LIST_FOREACH_SAFE (iterator, next, &info->super, opal_info_entry_t) {
if (!iterator->ie_referenced) {
opal_list_remove_item(&info->super, &iterator->super);
}
}
OPAL_THREAD_UNLOCK(info->i_lock);


return OPAL_SUCCESS;
}
68 changes: 30 additions & 38 deletions opal/util/info.h
View file
Open in desktop
Expand Up @@ -67,6 +67,7 @@ struct opal_info_entry_t {
opal_cstring_t *ie_key; /**< "key" part of the (key, value) pair */
uint32_t ie_referenced; /**< number of times this entry was internally
bosilca marked this conversation as resolved.
Show resolved Hide resolved
referenced */
bool ie_internal; /**< internal keys are not handed back to the user */
};

/**
Expand All @@ -90,7 +91,23 @@ OPAL_DECLSPEC OBJ_CLASS_DECLARATION(opal_info_t);
OPAL_DECLSPEC OBJ_CLASS_DECLARATION(opal_info_entry_t);

/**
* opal_info_dup - Duplicate an 'MPI_Info' object
* opal_info_dup - Duplicate public keys of an 'MPI_Info' object
*
* @param info source info object (handle)
* @param newinfo pointer to the new info object (handle)
*
* @retval OPAL_SUCCESS upon success
* @retval OPAL_ERR_OUT_OF_RESOURCE if out of memory
*
* Not only will the (key, value) pairs be duplicated, the order
* of keys will be the same in 'newinfo' as it is in 'info'. When
* an info object is no longer being used, it should be freed with
* \c opal_info_free.
bosilca marked this conversation as resolved.
Show resolved Hide resolved
*/
int opal_info_dup_public(opal_info_t *info, opal_info_t **newinfo);

/**
* opal_info_dup - Duplicate all entries of an 'MPI_Info' object
*
* @param info source info object (handle)
* @param newinfo pointer to the new info object (handle)
Expand All @@ -117,6 +134,18 @@ int opal_info_dup(opal_info_t *info, opal_info_t **newinfo);
*/
OPAL_DECLSPEC int opal_info_set(opal_info_t *info, const char *key, const char *value);

/**
* Set a new key,value pair on info and mark it as internal.
*
* @param info pointer to opal_info_t object
* @param key pointer to the new key object
* @param value pointer to the new value object
*
* @retval OPAL_SUCCESS upon success
* @retval OPAL_ERR_OUT_OF_RESOURCE if out of memory
*/
OPAL_DECLSPEC int opal_info_set_internal(opal_info_t *info, const char *key, const char *value);

/**
* Set a new key,value pair on info.
*
Expand Down Expand Up @@ -287,43 +316,6 @@ static inline int opal_info_get_nkeys(opal_info_t *info, int *nkeys)
return OPAL_SUCCESS;
}


/**
* Mark the entry \c key as referenced.
*
* This function is useful for lazily initialized components
* that do not read the key immediately but want to make sure
* the key is kept by the object owning the info key.
*
* @param info Pointer to opal_info_t object.
* @param key The key which to mark as referenced.
*
* @retval OPAL_SUCCESS
*/
int opal_info_mark_referenced(opal_info_t *info, const char *key);

/**
* Remove a reference from the entry \c key.
*
* This function should be used by components reading the key
* without wanting to retain it in the object owning the info.
*
* @param info Pointer to opal_info_t object.
* @param key The key which to unmark as referenced.
*
* @retval OPAL_SUCCESS
*/
int opal_info_unmark_referenced(opal_info_t *info, const char *key);

/**
* Remove any entries that are not marked as referenced
*
* @param info Pointer to opal_info_t object.
*
* @retval OPAL_SUCCESS
*/
int opal_info_remove_unreferenced(opal_info_t *info);

END_C_DECLS

#endif /* OPAL_INFO_H */
7 changes: 4 additions & 3 deletions opal/util/info_subscriber.c
View file
Open in desktop
Expand Up @@ -269,9 +269,10 @@ int opal_infosubscribe_change_info(opal_infosubscriber_t *object, opal_info_t *n
updated_value = opal_infosubscribe_inform_subscribers(object, iterator->ie_key->string,
iterator->ie_value->string,
&found_callback);
if (NULL != updated_value
&& 0 != strncmp(updated_value, value_str->string, value_str->length)) {
err = opal_info_set(object->s_info, iterator->ie_key->string, updated_value);
if (NULL != updated_value) {
err = opal_info_set(object->s_info, key_str->string, updated_value);
} else {
err = opal_info_set_internal(object->s_info, key_str->string, value_str->string);
}
OBJ_RELEASE(value_str);
OBJ_RELEASE(key_str);
Expand Down