MPI_Info_dup: allocate info through ompi_info_allocate instead of OBJ_NEW

[devreal]

The call to ompi_info_allocate ensures that the ompi instance is properly retained (similar to MPI_Info_create). The instance is then released in MPI_Info_free.

Thanks to Lisandro Dalcin for reporting and providing a simple reproducer (see #10344 (comment))

Signed-off-by: Joseph Schuchart schuchart@icl.utk.edu

[jsquyres]

@dalcinl FYI

[bosilca]

I don't think this patch is complete. Basically, we should not mix info_allocate/info_free with OBJ_NEW/OBJ_RELEASE. But we do all over the code, so there are many other places where things will fail (check all the uses of OBJ_NEW(ompi_info_t) in the IO part of the code).
After looking at the code, it seems that we should move everything from info_allocate/info_free into the constructor/destructor.

[devreal]

@bosilca My understanding is that the internally allocated info objects you mentioned are attached to another object (files, windows, comms) that have retained the ompi instance already. Info objects created through MPI_Info_dup and MPI_Info_create are free-standing so they have to retain the instance themselves.

[devreal]

In fact, if we retain the instance in every info object attached to predefined comms/windows/files we will run into similar cyclic dependencies as with predefined attributes: #10350

[bosilca]

Let's take mca/io/romio341/src/io_romio341_file_open.c as an example. We allocate with OBJ_NEW, we populate from an opal_object, we then make a copy through MPI_File_open. At this point the local ompi_info object was not yet added to the instance. And then we release with ompi_info_free that will call ompi_mpi_instance_release. Same pattern can be observed around all the other usage of the info in the IO.

[devreal]

OK, that clearly is a bug then :/ OBJ_NEW should be matched with OBJ_RELEASE but I think we need both variants.

[bosilca]

I don't agree. My reason is that we are able to correctly handle the f2c translation via OBJ_NEW/OBJ_RELEASE, so we should be able to handle the instance refcount.

[devreal]

Here is the cycle I'm talking about:

ompi_mpiinfo_init is called from ompi_mpi_instance_retain and in turn calls OBJ_CONSTRUCT(..., ompi_info_t). If we called ompi_mpi_instance_retain from within the ompi_info_t constructor we'd have a cycle and the instance will not be destroyed. Unless we want to restructure more code I don't see how to fix this.

The romio instances of OBJ_NEW(ompi_info_t) is easy to solve and I will push a fix in a minute into this PR. There is one more instance of OBJ_NEW(ompi_info_t) in fs_gpfs_file_get_info.c but I don't see that used anywhere, so it's hard to say what the intention is. Maybe @edgargabriel can say how the info object allocated there would be released?

[edgargabriel]

@devreal I think you are right, this file is a relict of a time long gone, and we forgot to remove it. I do not see any path to invoke this function. We should probably remove it, I do not have however access to a gpfs file system anymore to ensure that we do not break something by removing it.

[bosilca]

@devreal I don't think there is a problematic cycle because the mutex protecting the instance being a recursive mutex it will allow us to go through the fast path on the second call.

[devreal]

@bosilca The mutex is not the problem. The problem is that the destruction of the ompi instance would depend on the destruction of the info object because the info object has retained the instance and would have to call ompi_info_instance_release first. The destruction of the info object depends on the destruction of the instance (which is bound to happen in ompi_mpi_instance_release when the instance ref count reaches zero and it is destroyed). That will never happen because of the circular dependency of the destructors.

[jsquyres]

@devreal's argument sound right to me, but @bosilca and @devreal have looked much more deeply into the relevant code than me; I defer to you guys...

[devreal]

@bosilca and I had a quick chat offline about this and the outcome was that we both didn't know why info objects have to retain the ompi instance in the first place. My guess is that it has to do with the environment info but maybe that can be handled differently? @hjelmn Can you shed some light here?

[bosilca]

To add to @devreal comment, my main issue with the info objects is that while the info object is derived from opal_object_t it should never be used as such, because using them with OBJ_NEW/OBJ_RELEASE will lead to inconsistencies with the instance object. We are basically creating two classes of objects derived from the same opal_object_t.

[dalcinl]

@devreal Any chance you could retarget target this PR to branch v5.0.x? I really hope this fix gets in for the upcoming 5.0.0 release.

[jsquyres]

@hppritcha Can you have a look at this, since a question has come up on this PR that involves sessions?

[devreal]

@dalcinl the backport v5.0.x is at #10420 and should make it into the 5.0 release.