Segfault when running problem with periodic boundary conditions

[clarkpede]

When I run a mixing-layer problem with periodic boundary conditions, I get a segfault while saving the file. I checked out the most recent develop branch, and I get the same problem.

It may be that I messed up the periodic boundary conditions. I'm still learning how to use them in SU2, and I may have made a mistake. But even if my *.cfg files are set up wrong, I should get a runtime error message, not a segfault.

I did a brief check and it looks like SU2_MSH is doing its job in generating the correct periodic BCs.

Valgrind says that there's a lot of invalid reads in COutput::SortOutputData, which terminate in a segfault. I'm attaching the valgrind output, in case you want to look at it.

Steps to reproduce

1. Build SU2 with parallel support.
2. Put the attached grid, mixing_MSH.cfg and mixing_CFD.cfg files into the same folder.
3. Run SU2_MSH mixing_MSH.cfg.
4. Run SU2_MSH mixing_CFD.cfg.

I've also attached my config output, so you can see what my setup is like.

Here's the files:
PeriodicSegfault.tar.gz

[economon]

@clarkpede: the periodic BCs are something we are working on (we know there are issues). However, thread #416 may be helpful to check.

[clarkpede]

Thanks for the response. I did see that thread, and the discussion there was useful. Please keep me in the loop as you fix the bugs in the periodic BCs.

[clarkpede]

This bug pops up because the periodic mesh reconstruction (in SU2_MSH) doesn't always create a valid mesh in 3D. Some points disappear, and some points are duplicated. When SU2_CFD (specifically, CPhysicalGeometry) tries to get the number of points in the domain, it calculates the wrong number because some of those points are duplicated. That leads to segfaults, because the output routine thinks the extra (missing) points belong to another processor.

I've got a little toy problem with a simple periodic cube. It's easy to see on the simple 4x4x4 (+ periodic) problem that 3 nodes disappear, and 3 nodes are duplicated.

I've never observed this problem in any of the 2D test cases I ran.

As to why SU2_MSH is missing some points and duplicating others, I'm not sure. I tried running Valgrind to check for uninitialized variables or illegal memory access, but nothing there. I'll have to look more into the problem.

The su2_periodic.f90 script mentioned in #416 seems to be a workaround. I don't run into any problems when I use that script.

[clarkpede]

I've tracked down the source of the bug, and I'll submit a pull request later today.

During the output of the periodic mesh in SU2_MSH, CPeriodicGeometry::SetMeshFile sorts the nodes into the order normal, receive, send. This can be found in lines 18798-18819 of Common/src/geometry_structure. I've copied down the lines here for reference:

  /*--- Change the numbering to guarantee that the all the receive
   points are at the end of the file ---*/
  unsigned long OldnPoint = geometry->GetnPoint();
  unsigned long *NewSort = new unsigned long[nPoint];
  for (iPoint = 0; iPoint < nPoint; iPoint++) {
    NewSort[iPoint] = iPoint;
  }
  
  unsigned long Index = OldnPoint-1;
  for (iMarker = 0; iMarker < nMarker; iMarker++) {
    if (bound[iMarker][0]->GetVTK_Type() == VERTEX) {
      if (config->GetMarker_All_SendRecv(iMarker) < 0) {
        for (iElem_Bound = 0; iElem_Bound < nElem_Bound[iMarker]; iElem_Bound++) {
          if (bound[iMarker][iElem_Bound]->GetNode(0) < geometry->GetnPoint()) {
            NewSort[bound[iMarker][iElem_Bound]->GetNode(0)] = Index;
            NewSort[Index] = bound[iMarker][iElem_Bound]->GetNode(0);
            Index--;
          }
        }
      }
    }
  }

In order to sort the points, a list mapping the old points to the new points (NewSort) is created. The problem is that this list is sorted by switching nodes. But it doesn't switch around the contents of the list (i.e. two node numbers from the list). Instead it switched an index from the list and an actual node number pulled from the geometry. This presents two problems:

1. If any of the receive nodes actually are at the end of the list, they will get switched away from the end of the list.
2. If any node is switched twice, data is lost. That's because the sorting assumes that the elements being switched have not already been switched. This is how the three points went missing from the cube problem I previously referenced.

If my write-up is too confusing, I can provide a play-by-play of how the error occurs.

[demanosalvas]

Hi clarkpede, Have you looked into the “enhancement_periodicbc" branch? I recall fixing many of these bugs there but not merging it to master because there are still a few more things that have to be taken care of. David On Jan 17, 2018, at 7:30 AM, clarkpede <notifications@github.com<mailto:notifications@github.com>> wrote: I've tracked down the source of the bug, and I'll submit a pull request later today. During the output of the periodic mesh in SU2_MSH, CPeriodicGeometry::SetMeshFile sorts the nodes into the order normal, receive, send. This can be found in lines 18798-18819 of Common/src/geometry_structure<https://github.com/su2code/SU2/blob/68991cd1005cf118e1ff21b8952bb366bb299cf3/Common/src/geometry_structure.cpp#L18798>. I've copied down the lines here for reference: /*--- Change the numbering to guarantee that the all the receive points are at the end of the file ---*/ unsigned long OldnPoint = geometry->GetnPoint(); unsigned long *NewSort = new unsigned long[nPoint]; for (iPoint = 0; iPoint < nPoint; iPoint++) { NewSort[iPoint] = iPoint; } unsigned long Index = OldnPoint-1; for (iMarker = 0; iMarker < nMarker; iMarker++) { if (bound[iMarker][0]->GetVTK_Type() == VERTEX) { if (config->GetMarker_All_SendRecv(iMarker) < 0) { for (iElem_Bound = 0; iElem_Bound < nElem_Bound[iMarker]; iElem_Bound++) { if (bound[iMarker][iElem_Bound]->GetNode(0) < geometry->GetnPoint()) { NewSort[bound[iMarker][iElem_Bound]->GetNode(0)] = Index; NewSort[Index] = bound[iMarker][iElem_Bound]->GetNode(0); Index--; } } } } } In order to sort the points, a list mapping the old points to the new points (NewSort) is created. The problem is that this list is sorted by switching nodes. But it doesn't switch around the contents of the list (i.e. two node numbers from the list). Instead it switched an index from the list and an actual node number pulled from the geometry. This presents two problems: 1. If any of the receive nodes actually are at the end of the list, they will get switched away from the end of the list. 2. If any node is switched twice, data is lost. That's because the sorting assumes that the elements being switched have not already been switched. This is how the three points went missing from the cube problem I previously referenced. If my write-up is too confusing, I can provide a play-by-play of how the error occurs. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#431 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AE_4SuB5X60MCplLjmFeVUIgSigM7xbrks5tLhH_gaJpZM4PE7sn>.

[clarkpede]

I had forgotten about that branch. Can you be more specific? What do you mean by "there are still a few more things that have to be taken care of"?

[demanosalvas]

After realizing that SU2_MSH was only able to generate halo cells when the periodic BCs didn't share a common edge, @sravya91 and I went through the code that performs this task and fixed a few things to add this capability back. After fixing this we realized that there was an issue within the SU2_CFD when trying to run periodic BCs that touch each other. The issue was traced to the mpi communication routines, and since @economon was working on a revamp of this part of the code, we decided to wait until this was done. I am not sure what has been done here since, but I thought I should mention what has already been done in case you find it useful and can save you some time.

[clarkpede]

Ah. Well, pull request #500 should fix this bug, regardless. It's a single-purpose pull request that's ready to be merged in with the develop branch.

That being said, it looks like the changes implemented in enhancement_periodicbc and enhancement_periodicbc_paralleldraft seem to encompass a lot of the other changes needed to fix periodic boundary conditions. Unfortunately, both branches are based off of v4.3 and seem to be a little rough (there's a lot of cout statements, commented out code, etc.).

@demanosalvas, @sravya91, or @hlkline: Do you anticipate updating these branches in the next few months? Our group wants to run a few periodic problems, and we're currently trying to fix the bugs in the periodic implementation. We want to avoid duplicating effort if at all possible.

[economon]

@clarkpede: thanks for organizing - it would be great to have your help on this topic. I'll let the others comment on the status of those branches, but in general, we have 2 major issues:

1. Numerous bugs in the creation of the initial periodic BCs and new mesh file generated by SU2_MSH (ordering, inability to treat adjacent periodic markers, e.g., triply-periodic cube, and others). The branches above contain some fixes, and we should also merge in your Overhauled node sorting in periodic geometry to be less buggy #500 too.

2. Runtime issues in SU2_CFD, especially in parallel. Some of these issues may be fixed by the branches above, but the second step is to create the periodic BCs on the fly in SU2_CFD and integrate properly with the parallel partitioning, removing the need to call SU2_MSH as a pre-processing. Once the clean up in 1 is finished (a lot of this searching and matching can be reused), and after the fix_partitioning branch is merged, we can start on this task. I am planning to work on this part.

[economon]

Resolved by #652