[petsc-dev] threadcomm memory leak

[Shri]

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