[mpich-discuss] [PATCH v2] test: add attrdeleteget, MPI_Attr_get called from delete_fn

[Fab Tillier]

Jed Brown wrote on Sat, 18 May 2013 at 06:27:03

> Fab Tillier <ftillier at microsoft.com> writes:
> 
>>> +int main(int argc,char **argv)
>>> +{
>>> +  MPI_Comm scomm;
>>> +
>>> +  MTest_Init(&argc,&argv);
>>> +  MPI_Comm_split(MPI_COMM_WORLD,1,0,&scomm);
>>> +  MPI_Keyval_create(MPI_NULL_COPY_FN,delete_fn,&key,(void*)0);
>>> +  MPI_Attr_put(scomm,key,a);
>>> +  MPI_Comm_free(&scomm);
>> 
>> The communicator handle will become invalid at some point after you
>> call MPI_Comm_free (but before the call returns).  I didn't see in the
>> standard any mention of whether using the communicator in the destroy
>> callback is acceptable.
> 
> Why pass the communicator as an argument to delete_fn if the MPI
> implementation is allowed to have already made it invalid?

There are multiple paths that cause the delete callback to be invoked.  It can be explicit, through MPI_COMM_DELETE_ATTR, implicit through MPI_COMM_SET_ATTR if an existing attribute is being updated, or implicit through MPI_COMM_FREE.  In the first two cases, the communicator handle is valid.  I'd like to fix the third case if possible...

> Since delete_fn is called by MPI_Comm_free, any use of the first
> argument passed to delete_fn would be erroneous.  IMO, the presence of
> the argument and lack of any comments stating that it should never be
> used imply that it was intended to be valid.  It is valid with all other
> MPI implementations that we've used (most of them).

I wonder what other implementations would do if you initiated an MPI_Bcast or similar on such a communicator from the delete callback (say you wanted to notify everyone the attribute was being deleted, or whatnot).  I'm reluctant to special case certain functions as working on a freed communicator, but perhaps the structure of our implementation could change to not invalidate the communicator.

>> There's the usual vague notion of the call to MPI_Comm_free being
>> erroneous if the delete callback fails.
> 
> Yes, the test relies on that.  The problem is MPI_Attr_get returning an
> error when called from delete_fn, presumably due to eager invalidation
> of the communicator.

The issue here is that the communicator is reference counted.  It is deleted when the reference count reaches zero (at which point it is no longer valid, as communicator lookup checks that the reference count is non-zero).  Having the reference count bounce through zero (to initiate destruction), then back to 1 so that the attribute delete callbacks can access it, then back to zero, is really messy, especially if the delete callbacks were to initiate I/O operations.

I agree the standard should clarify (I'll take a look at the ticket you created), and I'd like to see a clarification of the behavior with respect to the attribute delete callbacks.  Specifically, the standard does not state explicitly whether the attributes are destroyed within the call to MPI_COMM_FREE, or only once all references on the communicator are released.  Right now, MSMPI implements the latter, and I think we could argue this is too late.  If the logic were changed to destroy all attributes from the context of MPI_COMM_FREE, but before the reference on the communicator is released, the destroy callbacks would be free to use the communicator, and the 'bounce through zero' reference count issue also gets resolved cleanly.

Since the application has freed its handle, it seems reasonable to clean up the attributes (there's no longer any way for the user to access the cached information) from the context of the MPI_COMM_FREE call itself.  It's a lot easier to rationalize the state of the communicator in this case, too.

I think this would be a reasonable change to make, but would like to get your input as to whether you think it would match the expected behavior.  It would be fairly straight forward to do this, and we could potentially share a beta version for you to validate if you're interested.

> 
>>> +  MPI_Finalize();
>>> +  return 0;
>>> +}
>>> +
>>> +int delete_fn(MPI_Comm comm,int keyval,void *attr_val,void *extra_state)
>>> +{
>>> +  int   flg;
>>> +  void *ptr;
>>> +
>>> +  return MPI_Attr_get(comm,key,&ptr,&flg);
>> 
>> Why aren't you using attr_val and extra_state here, rather than
>> calling MPI_Attr_get?  Is this just a symptom of the test case being
>> reduced, and the original code attempts to access a different
>> attribute on that communicator?
> 
> It's a consequence of reducing, but PETSc can restructure (by using
> separate destructors for each of the two relevant keyvals) so that it is
> not necessary to call MPI_Attr_get in a delete_fn.  I'll make that
> change, but I think it is worth fixing in MS-MPI as well.

Yes, I agree that not having surprising behavior for users is a good goal to have, and I appreciate the input.

Cheers,
-Fab