Improve efficiency in memory trace and device->host mem-copy by gauravharsha · Pull Request #5 · Green-Phys/green-gpu

gauravharsha · 2024-10-11T20:09:05Z

Proposing the following changes

Current copy of new self-energy from CUDA device to Host memory uses cudaMemcpy. This blocks the processes and further execution of kernels. Replacing this with cudaMemcpyAsync. This should, in principle, speed up the code.
To implement (1), function signatures are updated. Two-step process involving cudaMemcpy to a buffer on host memory, followed by writing the host buffer to shared-memory Self-energy is introduced.
Adding a function to measure total FLOPs achieved (including read/write steps).
Updates in error handling for Cholesky / LU failures when computing polarization P from P0.

…m device to host; also reduce memory trace by removing intermediates and writing directly to shared memory self-energy.

egull

OK. On Monday need to go over shared memory sync and effective locking/unlocking.

For now I propose to split this into the three issues: The FLOP count (harmless), the trap/abort (also harmless), and the shared mem.

The first two we can get done right away. The third needs work.

egull · 2024-10-11T21:51:22Z

src/cu_routines.cu

      qpt.compute_Pq();
      qpt.transform_wt();
      // Write to Sigma(k), k belongs to _ink
+      MPI_Win_lock_all(MPI_MODE_NOCHECK, sigma_tau_host_shared.win());


This we need to discuss. It's fairly catastrophic in amulti-GPU environment: only ONE MPI process will enter the section below at a time... I believe there's no reason for that.

@egull that's not how MPI_Win_lock_all(MPI_MODE_NOCHECK, win) works. This call only asserts the start of the communication epoch, and no synchronization is done here. To do the memory synchronization one has to call MPI_Win_sync, which is done bellow after the loop.

However, there is much more dangerous things going on here. Since all processes that have GPU enter the loop, there would be a guaranteed race condition in this loop, as we run over all k-points and do a summation over all q-points.

Doing similar synchronization pattern as implemented here is safe (and this is actually advised to reduce synchronizations) if we know that there is no overlap between memory regions that are accessed by different processes.

You're a bit late to that party. We'll discuss today how to do that in a way that is both performant (which your solution is not) and correct (which his solution is not). The way I think it works is via

MPI_Win_lock_all
MPI_Win_sync
...then do the update/access
MPI_Win_sync
MPI_Win_flush_all
MPI_Win_unlock_all

the MPI_Win_sync at most syncronizes a private with a public version, but may not syncronize the private version of other threads. The standard section 11 has more.

Which my implementation is not performant?

Also, mpi_win_flush is not needed here, we don't do any RMA operations on shared window here.

egull · 2024-10-11T21:51:58Z

src/cu_routines.cu

-                                                     qpt.Pqk_tQP(qkpt->all_done_event(), qkpt->stream(), need_minus_q));
-                copy_Sigma(Sigma_tskij_host, Sigmak_stij, k_reduced_id, _nts, _ns);
+                qkpt->compute_second_tau_contraction(qpt.Pqk_tQP(qkpt->all_done_event(), qkpt->stream(), need_minus_q));
+                copy_Sigma(sigma_tau_host_shared.object(), Sigmak_stij, k_reduced_id, _nts, _ns);


why can't the lock be around this?

egull · 2024-10-11T21:52:08Z

src/cu_routines.cu

-                                                        qpt.Pqk_tQP(qkpt->all_done_event(), qkpt->stream(), need_minus_q));
-                copy_Sigma_2c(Sigma_tskij_host, Sigmak_stij, k_reduced_id, _nts);
+                qkpt->compute_second_tau_contraction_2C(qpt.Pqk_tQP(qkpt->all_done_event(), qkpt->stream(), need_minus_q));
+                copy_Sigma_2c(sigma_tau_host_shared.object(), Sigmak_stij, k_reduced_id, _nts);


...and around this?

egull · 2024-10-11T21:52:44Z

src/cu_routines.cu

            } else {
              qkpt->set_up_qkpt_second(nullptr, V_Qim.data(), k_reduced_id, k1_reduced_id, need_minus_k1);
-              qkpt->compute_second_tau_contraction(nullptr, qpt.Pqk_tQP(qkpt->all_done_event(), qkpt->stream(), need_minus_q));
+              qkpt->compute_second_tau_contraction(qpt.Pqk_tQP(qkpt->all_done_event(), qkpt->stream(), need_minus_q));


even here, you probably don't want to lock out everybody but do the lock/unlock jsut around the memcpy

egull · 2024-10-11T21:56:08Z

src/cu_routines.cu

      }
+      MPI_Win_sync(sigma_tau_host_shared.win());
+      MPI_Barrier(utils::context.node_comm);
+      MPI_Win_unlock_all(sigma_tau_host_shared.win());


this is silly here? Why? we always have lock/unlock in pairs

egull · 2024-10-11T22:02:39Z

src/cugw_qpt.cu

+  template <typename prec>
+  void gw_qkpt<prec>::cleanup(bool low_memory_mode, cxx_complex* Sigmak_stij_host) {
+    if (cleanup_req_) {
+      std::memcpy(Sigma_stij_host, Sigmak_stij_buffer_, ns_ * ntnao2_ * sizeof(cxx_complex));


we were talking about combining buffers here. Possibly that was done already.

egull · 2024-10-11T22:04:23Z

src/green/gpu/cugw_qpt.h

+    // status of data transfer / copy from Device to Host.
+    // false: not required, stream ready for next calculation
+    // true: required
+    bool cleanup_req_;


propose making a destructor that throws an exception if cleanup_req_ is true and a constructor that sets it to false.

c++ standard advises to not throwing exceptions in destructor.

That's right. I guess we need to print an error and abort the program. This should never happen unless there's a logic error anyway.

src/gw_gpu_kernel.cpp

egull · 2024-10-11T22:06:50Z

src/cugw_qpt.cu


+  template <typename prec>
+  void gw_qkpt<prec>::cleanup(bool low_memory_mode, cxx_complex* Sigmak_stij_host) {
+    if (cleanup_req_) {


I would put the shared window lock just here

egull · 2024-10-11T22:07:04Z

src/cugw_qpt.cu

+  void gw_qkpt<prec>::cleanup(bool low_memory_mode, cxx_complex* Sigmak_stij_host) {
+    if (cleanup_req_) {
+      std::memcpy(Sigma_stij_host, Sigmak_stij_buffer_, ns_ * ntnao2_ * sizeof(cxx_complex));
+      cleanup_req_ = false;


I would put the shared window unlock just here.

iskakoff · 2024-10-14T05:17:10Z

src/cu_routines.cu

      qpt.compute_Pq();
      qpt.transform_wt();
      // Write to Sigma(k), k belongs to _ink
+      MPI_Win_lock_all(MPI_MODE_NOCHECK, sigma_tau_host_shared.win());


@egull that's not how MPI_Win_lock_all(MPI_MODE_NOCHECK, win) works. This call only asserts the start of the communication epoch, and no synchronization is done here. To do the memory synchronization one has to call MPI_Win_sync, which is done bellow after the loop.

However, there is much more dangerous things going on here. Since all processes that have GPU enter the loop, there would be a guaranteed race condition in this loop, as we run over all k-points and do a summation over all q-points.

Doing similar synchronization pattern as implemented here is safe (and this is actually advised to reduce synchronizations) if we know that there is no overlap between memory regions that are accessed by different processes.

iskakoff · 2024-10-14T05:22:26Z

src/green/gpu/cugw_qpt.h

+    // status of data transfer / copy from Device to Host.
+    // false: not required, stream ready for next calculation
+    // true: required
+    bool cleanup_req_;


c++ standard advises to not throwing exceptions in destructor.

iskakoff · 2024-10-14T05:22:51Z

src/green/gpu/cugw_qpt.h

+  template <typename prec>
+  void wait_and_clean_qkpts(std::vector<gw_qkpt<prec>*>& qkpts, bool low_memory_mode,
+                            typename cu_type_map<std::complex<prec>>::cxx_type* Sigmak_stij_host) {
+    static int pos = 0;


next three lines does not make sense to me.

gauravharsha · 2025-11-06T17:10:57Z

Closing. Superseded by #9

gauravharsha added 3 commits October 11, 2024 15:33

function to measure floating point operations per second.

fd6c529

replace cudaMemcpy with cudaMemcpyAsync when updating self-energy fro…

2b4b916

…m device to host; also reduce memory trace by removing intermediates and writing directly to shared memory self-energy.

fix application abort when (if) Cholesky or LU fails.

6dd12c4

gauravharsha requested review from egull and iskakoff October 11, 2024 20:09

fix bugs

3a29fe5

egull requested changes Oct 11, 2024

View reviewed changes

fix bugs for build

e8cb1cc

iskakoff requested changes Oct 14, 2024

View reviewed changes

gauravharsha closed this Nov 6, 2025

gauravharsha deleted the efficiency branch March 11, 2026 18:13

Conversation

gauravharsha commented Oct 11, 2024

Uh oh!

egull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gauravharsha commented Nov 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants