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