[petsc-dev] threadcomm memory leak

[Barry Smith]

  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.

   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 <bsmith at mcs.anl.gov> 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.
>> 
>