On Jul 17, 2012, at 1:17 PM, Barry Smith wrote:

Why all this messy attribute check on Petsc_Counter_keyval ... stuff? Why not just ALWAYS call PetscCommDuplicate() and then check it for Petsc_ThreadComm_keyval and stick in the threadcomm if not there? That is, there is no reason to spread knowledge of Petsc_Counter_keyval into other places since other places can just use PetscCommDuplicate() to manage that.

Thanks, Did what you suggested and pushed. http://petsc.cs.iit.edu/petsc/petsc-dev/rev/70470108a897 Should we generalize the Attach/Detach stuff instead of having specific implementations for each attribute. PetscCommAttachAttribute(MPI_Comm comm,MPI_Keyval,void *attr) PetscCommDetachAttribute(MPI_Comm,MPI_keyval) One hiccup i see is for attributes that have their own reference counting, for e.g., threadcomm and Hong's elemental grid stuff. Thanks, Shri

Barry

+PetscErrorCode PetscThreadCommAttach(MPI_Comm comm,PetscThreadComm tcomm) +{ + PetscErrorCode ierr; + MPI_Comm icomm; + PetscMPIInt flg,flg1,flg2; + void *ptr; + PetscCommCounter *counter; + + PetscFunctionBegin; + ierr = MPI_Attr_get(comm,Petsc_Counter_keyval,&counter,&flg);CHKERRQ(ierr); + if(!flg) { /* Communicator not initialized yet */ + ierr = PetscCommDuplicate(comm,&icomm,PETSC_NULL);CHKERRQ(ierr); + tcomm->refct++; + ierr = MPI_Attr_put(icomm,Petsc_ThreadComm_keyval,tcomm);CHKERRQ(ierr); + } else { + ierr = MPI_Attr_get(comm,Petsc_InnerComm_keyval,&ptr,&flg1);CHKERRQ(ierr); + if(flg1) { + ierr = PetscMemcpy(&ptr,&icomm,sizeof(MPI_Comm));CHKERRQ(ierr); + ierr = MPI_Attr_get(icomm,Petsc_ThreadComm_keyval,&ptr,&flg2);CHKERRQ(ierr); + if(!flg2) { + tcomm->refct++; + ierr = MPI_Attr_put(icomm,Petsc_ThreadComm_keyval,tcomm);CHKERRQ(ierr); + } + } + } + PetscFunctionReturn(0);

On Jul 17, 2012, at 3:14 AM, Shri wrote:

...

Barry, Jed, Please see the attached patch based on Barry's suggestions. I tested this with MPI and MPIUNI and did not see any memory leaks. Let me know what you think.

Thanks, Shri<threadcomm.patch>

On Jul 16, 2012, at 11:00 PM, Barry Smith wrote:

...

Note that in general I would advocate in any code (especially PETSc code) NEVER blinding putting an attribute into a MPI_Comm, always check if the attribute is already there and only put it there if it is not already there.

Barry

On Jul 16, 2012, at 10:48 PM, Jed Brown wrote:

...

On Mon, Jul 16, 2012 at 10:44 PM, Barry Smith <[email protected]> wrote: /* PETSC_COMM_SELF = PETSC_COMM_WORLD for MPIUNI */ #if !defined(PETSC_HAVE_MPIUNI) ierr = PetscCommDuplicate(PETSC_COMM_WORLD,&icomm,PETSC_NULL);CHKERRQ(ierr); ierr = MPI_Attr_put(icomm,Petsc_ThreadComm_keyval,(void*)tcomm);CHKERRQ(ierr); tcomm->refct++; /* Share the threadcomm with PETSC_COMM_SELF */ #endif

ierr = PetscCommDuplicate(PETSC_COMM_SELF,&icomm,PETSC_NULL);CHKERRQ(ierr); ierr = MPI_Attr_put(icomm,Petsc_ThreadComm_keyval,(void*)tcomm);CHKERRQ(ierr);

I would not do it this way. Instead I would write a general routine that attached a threadcomm to a MPI_Comm; this routine would get the threadcomm_keyval and if it did NOT find it then would be put the attribute, otherwise it would know one was already there. Say it is called PetscThreadCommAttach(MPI_Comm, threadcomm); then in this routine you would just write

PetscThreadCommAttach(PETSC_COMM_WORLD, tcomm); PetscThreadCommAttach(PETSC_COMM_SELF,tcomm); /* won't attr it again for MPIUni because it is already there */

This looks good, but there is also a ref-counting check needed in PetscThreadCommDetach/Destroy because the thread pool (presumably) needs to be closed before PetscFinalize returns.