Modify

Opened 4 years ago

Last modified 4 years ago

#1618 new enhancement

Compile with adaptive MPI (based on Charm++)

Reported by: jtao@… Owned by:
Priority: major Milestone:
Component: Cactus Version: development version
Keywords: Cc:

Description

Gengbin Zheng, one of the leading developer of the Charm++ and I managed to get Cactus compile against the AMPI (http://charm.cs.illinois.edu/research/ampi). The patch against Revsion 5120 is attached. The major changes are in the flesh to deal with global variables. We started with PUGH to get started and changes in Carpet could be done later on. NR applications might not benefit from this work though this effort could make Cactus appealing to many other applications as a computational framework.

Attachments (3)

05-21.diff (34.4 KB) - added by Jian Tao 4 years ago.
patches to deal with global variables in Cactus to compile with AMPI
07-29.4.diff (36.9 KB) - added by anonymous 4 years ago.
08-08.diff (36.9 KB) - added by anonymous 4 years ago.

Download all attachments as: .zip

Change History (15)

Changed 4 years ago by Jian Tao

Attachment: 05-21.diff added

patches to deal with global variables in Cactus to compile with AMPI

comment:1 Changed 4 years ago by Erik Schnetter

The reason Jian says that NR may not benefit is that simulation with large number of ghost zones may not benefit. Switching to other numerical methods, such as DG, may change the picture.

Re Carpet: Most of Carpet's global variables live in a namespace. The reason this was done was one of convenience -- it is easier to access a namespace than a singleton class object where the pointer has to be retrieved from cctkGH. The changes to avoid global variables are straightforward, but tedious.

There exist other global variables having to do with profiling, timing, keeping statistics etc. Those should probably be put into thread-local storage.

comment:2 Changed 4 years ago by Roland Haas

We are always happy to increase the user base for Cactus. Supporting AMPI seems like a nice thing to have. The patch as it is can likely not be applied to the public version of Cactus though. We most likely have to replace the AMPI_TLS defines with a CCTK_PROCLOCAL or similar and define it to be empty if AMPI is not used (the trickiest issue being that we'd want this all to work if either OpenMP or AMPI are used so we cannot just make AMPI_TLS into __thread). Deciding whether AMPI is in use could be done either in the flesh (ideally to be avoided since the flesh should not know of MPI) or the thorn ExternalLibraries/MPI. It would also be very nice to limit calls to CmiEnableTLS/CmiDisableTLS to only the two places namely where we call MPI_Init and MPI_Finilize. A second issue is that the patch currently contains a line (line 562 of WarnLevel.c that says "/* Gengbin hack */" which we have to remove). Am I correct in assuming that this particular line is related to #1548 ?

comment:3 Changed 4 years ago by anonymous

Yes, OpenMP is using thread internally. That is why Gengbin added another macro instead of using thread directly. Yes, we may find a more systematic approach to handle thread local storage, which might also be necessary for compiling Cactus with HPX (http://stellar.cct.lsu.edu/tag/hpx/). Yes, #1548 is relevant to this patch which has a dirty hack to get around the issue in that ticket.

comment:4 Changed 4 years ago by Steven R. Brandt

Do we like this patch? Do we need to split it up into smaller pieces?

comment:5 Changed 4 years ago by Erik Schnetter

No, we do not like this patch:

AMPI_TLS needs to be named CCTK_TLS. Possibly, "TLS" should be "THREAD_LOCAL" instead, so that it is more similar to the modern C and C++ standards.

The construct

#ifdef AMPI 
    CmiDisableTLS(); 
#endif

should be wrapped into a single function, probably called "CCTK_DisableThreadLocal()" or so. Same for CmiEnableTLS. In particular, the #ifdefs should go into this function, so that using it requires only 1 instead of 3 lines of code.

The various comments containing the work "hack" should instead contain an explanation of what they do. There is no need to call it "hack".

"AMPI" should become "CCTK_HAVE_AMPI", similar to other capabilities.

The header file mpi.h should be accessed via <mpi.h>, not "mpi.h".

There are some commented out lines of code that are introduced. These should not be introduced.

The code in Boundary.c seems fishy. Are these static variables really correct, even if executed by multiple threads? Does the code need to be redesigned instead?

shelob.ini should not change the default for Shelob.

The newly introduced CCTK_* macros and functions need to be documented.

But before we do any of this, it would be good to describe in an email to the developers' list which new features you propose to introduce to the flesh, and why, and what other packages (except AMPI) may want similar features, how this compares to C11 and C++11, etc.

comment:6 Changed 4 years ago by anonymous

The code in Boundary.c seems fishy. Are these static variables really correct, even if ?>executed by multiple threads? Does the code need to be redesigned instead?

Erik, would you please explain a bit more about your concerns? Other changes shall be straightforward.

Jian

Changed 4 years ago by anonymous

Attachment: 07-29.4.diff added

comment:7 Changed 4 years ago by anonymous

I just attached the updated patch from Gengbin.
Jian

Changed 4 years ago by anonymous

Attachment: 08-08.diff added

comment:8 Changed 4 years ago by anonymous

Replaced AMPI_TLS with CCTK_TLS.

comment:9 Changed 4 years ago by Frank Löffler

Steve: more changes have to be made, but it is on the right track.

comment:10 Changed 4 years ago by Erik Schnetter

Brief notes from the phone discussion:

  • rename TLS to THREAD_LOCAL (both macro and include file)
  • define CCTK_THREAD_LOCAL via autoconf magic, choose whether to use this macro depending on whether AMPI is used
  • add documentation for Enable/DisableThreadLocal
  • simplify logic for redirecting I/O: don't introduce another else branch, rather move #ifdef into else branch
  • declare CmiEnableTLS only when AMPI is available
  • why is mpi.h needed for flesh.cc?
  • don't use -DCCTK_TLS in option list
  • don't use -DCCTK_HAVE_AMPI in option list; rather set this in AMPI thorn

comment:11 Changed 4 years ago by Steven R. Brandt

With regard to the point "add documenation for Enable/Disable" it was requested that documentation for why thread local storage needs to be disabled where it is disabled.

comment:12 Changed 4 years ago by Erik Schnetter

I meant that the reference manual needs to be updates with a description of these routines. The comments in the code can then assume that the reader is familiar with this documentation.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
Next status will be 'review'.
as The resolution will be set.
to The owner will be changed from (none) to the specified user.
Next status will be 'confirmed'.
The owner will be changed from (none) to anonymous.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.