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