[petsc-dev] threadcomm memory leak

[Barry Smith]

On Jul 17, 2012, at 9:58 PM, Shri wrote:

> 
> On Jul 17, 2012, at 4:22 PM, Barry Smith wrote:
> 
>> 
>> 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?
>   Yes, because several communicators could possibly share them. 

   Ok, then the destructor called by the MPI_Comm_del() would decrease the reference count by one and if it gets to zero then does the delete as usual.

> 
>> 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.
> How do we do let MPI do the reference counting if different communicators are being used?
>> 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?
> 
> One way i see this happening is by having a global struct that has the keyval and a reference counter
> 
> typedef struct{
>   PetscMPIInt PetscThreadComm_keyval; /* MPI attribute key *
>   PetscInt refct;       /* reference count */
> }PetscCommAttr;
> 
> PetscCommAttr threadcomm_attr;
> 

   No, No, this is overdesigning it.

   Barry

> instead of just having a global attribute key as done right now.
> 
> and then we could have 
> /* Attaches the attribute on the comm and increments the reference count
> PetscCommAttributeAttach(MPI_Comm comm,PetscCommAttr threadcomm_attr, void* attr_val);
> /* Detaches the attribute and decrements the reference count
> PetscCommAttributeDetach(MPI_Comm comm,PetscCommAttr threadcomm_attr);
> 
> Shri
> 
>> 
>>  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.
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
>