<p dir="ltr">Sorry for late reply. I just pushed a fix for the crash. It is in master.</p>
<p dir="ltr">Stefano</p>
<div class="gmail_quot<blockquote class=" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, 5 Sep 2014, Jed Brown wrote:<br>
<br>
> Satish Balay <<a href="mailto:balay@mcs.anl.gov">balay@mcs.anl.gov</a>> writes:<br>
> > Perhaps the following is the fix [with proper comments, more error<br>
> > checks?]. But someone more familiar with this code should check this..<br>
> ><br>
> > Satish<br>
> ><br>
> > --------------<br>
> > $ git diff |cat<br>
> > diff --git a/src/ksp/pc/impls/is/pcis.c b/src/ksp/pc/impls/is/pcis.c<br>
> > index dab5836..0fa0217 100644<br>
> > --- a/src/ksp/pc/impls/is/pcis.c<br>
> > +++ b/src/ksp/pc/impls/is/pcis.c<br>
> > @@ -140,6 +140,8 @@ PetscErrorCode  PCISSetUp(PC pc)<br>
> >    ierr = PetscObjectTypeCompare((PetscObject)pc->pmat,MATIS,&flg);CHKERRQ(ierr);<br>
> >    if (!flg) SETERRQ(PetscObjectComm((PetscObject)pc),PETSC_ERR_ARG_WRONG,"Preconditioner type of Neumann Neumman requires matrix of type MATIS");<br>
> >    matis = (Mat_IS*)pc->pmat->data;<br>
> > +  PetscObjectReference((PetscObject)pc->pmat);<br>
> > +  pcis->pmat = pc->pmat;<br>
><br>
> Uh, PCISSetUp can be called more than once?<br>
<br>
I have no idea..<br>
<br>
><br>
> And simply destroying the pcis->pmat reference is not enough because<br>
> that extra reference could significantly increase the peak memory usage.<br>
<br>
Curently the object (pc->pmat) is destroyed at the end anyway [perhaps<br>
duing PCDestroy()]. This fix changes the order a bit so that its<br>
destoryed only after its last use.<br>
<br>
> The right solution is to not hold that reference and not hold the info.<br>
><br>
> >    pcis->pure_neumann = matis->pure_neumann;<br>
> ><br>
> > @@ -378,8 +380,9 @@ PetscErrorCode  PCISDestroy(PC pc)<br>
> >    ierr = VecScatterDestroy(&pcis->global_to_B);CHKERRQ(ierr);<br>
> >    ierr = PetscFree(pcis->work_N);CHKERRQ(ierr);<br>
> >    if (pcis->ISLocalToGlobalMappingGetInfoWasCalled) {<br>
> > -    ierr = ISLocalToGlobalMappingRestoreInfo((ISLocalToGlobalMapping)0,&(pcis->n_neigh),&(pcis->neigh),&(pcis->n_shared),&(pcis->shared));CHKERRQ(ierr);<br>
> > +    ierr = ISLocalToGlobalMappingRestoreInfo(((Mat_IS*)pcis->pmat->data)->mapping,&(pcis->n_neigh),&(pcis->neigh),&(pcis->n_shared),&(pcis->shared));CHKERRQ(ierr);<br>
> >    }<br>
><br>
> Why not restore the info at the place it is gotten, like we do with<br>
> every other accessor?<br>
<br>
Looks like this info is stashed in 'pcis->n_neigh, pcis->neigh' etc -<br>
and reused later multple times. [perhaps preventing multiple<br>
mallocs/frees]<br>
<br>
$ git grep -l 'pcis->n_neigh'<br>
src/ksp/pc/impls/bddc/bddcfetidp.c<br>
src/ksp/pc/impls/is/nn/nn.c<br>
src/ksp/pc/impls/is/pcis.c<br>
<br>
Or perhaps this info should be stashed in the IS so multiple<br>
ISLocalToGlobalMappingGetInfo() calls are cheap [but then the malloc'd<br>
memory will live until IS is destroyed anyway]<br>
<br>
I guess there are 2 issues you are touching on. A fix for this crash -<br>
and code cleanup. My patch gets the examples working.<br>
<br>
But I'll defer both isses to Stefano [asuming he is aquainted with the<br>
above sources].<br>
<br>
Satish<br>
<br>
><br>
> > +  ierr = MatDestroy(&pcis->pmat);CHKERRQ(ierr);<br>
> >    ierr = PetscObjectComposeFunction((PetscObject)pc,"PCISSetUseStiffnessScaling_C",NULL);CHKERRQ(ierr);<br>
> >    ierr = PetscObjectComposeFunction((PetscObject)pc,"PCISSetSubdomainScalingFactor_C",NULL);CHKERRQ(ierr);<br>
> >    ierr = PetscObjectComposeFunction((PetscObject)pc,"PCISSetSubdomainDiagonalScaling_C",NULL);CHKERRQ(ierr);<br>
> > diff --git a/src/ksp/pc/impls/is/pcis.h b/src/ksp/pc/impls/is/pcis.h<br>
> > index 4a42cf9..736ea8c 100644<br>
> > --- a/src/ksp/pc/impls/is/pcis.h<br>
> > +++ b/src/ksp/pc/impls/is/pcis.h<br>
> > @@ -73,6 +73,7 @@ typedef struct {<br>
> >    /* We need:                                                                                 */<br>
> >    /* proc[k].loc_to_glob(proc[k].shared[i][m]) == proc[l].loc_to_glob(proc[l].shared[j][m])   */<br>
> >    /* for all 0 <= m < proc[k].n_shared[i], or equiv'ly, for all 0 <= m < proc[l].n_shared[j]  */<br>
> > +  Mat pmat;<br>
> >  } PC_IS;<br>
> ><br>
> >  PETSC_EXTERN PetscErrorCode PCISSetUp(PC pc);<br>
><br>
><br>
<br>
</div>