[petsc-dev] threadcomm memory leak

[Barry Smith]

On Jul 17, 2012, at 2:24 PM, Shri wrote:

> 
> 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.

   Do they really need to have their own reference counting? If they are ALWAYS imbedded inside an MPI attribute then we can let MPI do the reference counting and there is no need for its own counting. But if you are passing them around directly wily-nily in subroutine calls then they do need their own reference counting.  Can you see if it is possible for them NOT to have their own reference counting?

   Barry

> 
> 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 <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.
>>>> 
>>> 
>> 
>