Skip to content

[SYCL][CUDA] Fix unexpected async memcpy #1798

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 5, 2020
Merged

[SYCL][CUDA] Fix unexpected async memcpy #1798

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions sycl/plugins/cuda/pi_cuda.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1521,7 +1521,15 @@ pi_result cuda_piMemBufferCreate(pi_context context, pi_mem_flags flags,
if (piMemObj != nullptr) {
retMemObj = piMemObj.release();
if (performInitialCopy) {
// Operates on the default stream of the current CUDA context.
retErr = PI_CHECK_ERROR(cuMemcpyHtoD(ptr, host_ptr, size));
// Synchronize with default stream implicitly used by cuMemcpyHtoD
// to make buffer data available on device before any other PI call
// uses it.
if (retErr == PI_SUCCESS) {
CUstream defaultStream = 0;
retErr = PI_CHECK_ERROR(cuStreamSynchronize(defaultStream));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "cuMemcpyHtoD" copy is synchronous, so what else do you need to synchronize?
Also the "cuStreamSynchronize" is waiting for all activities on the stream to finish, may it be that you unnecessarily wait for something else too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CUDA default stream in the default legacy mode synchronizes with all other CUDA streams (it synchronizes with the work on them at the point of enqueueing work onto the default stream).

While creating a PI buffer we do not know which PI queue (and its associated CUDA stream) will operate on it. Therefore we use a CUDA function that implicitly uses the default stream.

However, all PI enqueue operations target a specific queue and with that a specific, queue associated CUDA stream. These streams are not synchronizing with each other, nor are they waiting/synchronizing with the default stream.

We observed race conditions when a buffer was created with a host pointer (with the above memcpy happening on the default stream) but then the buffer was used by a kernel enqueued to a different stream (which does not synchronize with other streams). The result is a racecondition. Sometimes the memcpy on the default stream finished in time for the kernel on the other stream getting all the expected input data, sometimes the input data was not completely available on the device and testing would fail.

To make sure, that the memcpy handled by the default stream is finished before any other stream (accessing the same device) operates on it we now explicitly synchronize inside the buffer creation on the default stream.

This has performance implications. However, they are easily avoided by high performance code by first creating all SYCL (and with that PI) objects and then reuse them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: Also, CUDA's async/synchronous naming is sometimes (and annoyingly) misleading. Copying data from page-able memory to a device are synchronous in staging the data into internal, pinned memory, but asynchronous for copying the data from the pinned memory to the device via DMA.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The buffer creation would not return until copy is completed (guaranteed by cuMemcpyHtoD), right? So how is it possible that any command enqueue relying on that buffer be ready on device is attempted before copy is completed? [sorry if I am asking dumb questions]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but asynchronous for copying the data from the pinned memory to the device

Ah, I guess this answers my earlier question.

This has performance implications. However, they are easily avoided by high performance code by first creating all SYCL (and with that PI) objects and then reuse them.

That might be too strict requirement for an average user. With the currently enabled performance testing, do you see any regressions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are seeing non-deterministic fails (as expected by a racecondition) in our work with libraries, when we set up data for testing or profiling.

I tried to create unit test on the PI level to trigger the problem. With certain buffer sizes (and numbers of buffers) I got the test to fail when run through LIT 100% on my machine - but running the unit test directly (not through LIT which runs many tests in parallel and puts load onto the machine) I couldn't get the test to fail and I expect that another machine - or another day with another number of open apps on my machine would also not show the racecondition in LIT.

If this was purely CPU code I would push for thread sanitizer builds and testing, or for testing with valgrind. I am not aware of a CPU-GPU racecondition checker tool 😞 I haven't tried if the cuda-memcheck initcheckl tool can spot these kind of problems but my guess is that it only detects a problem when the race happens while it will miss it when the kernel started just slow enough to hide the non-synchronized data copy...

}
}
} else {
retErr = PI_OUT_OF_HOST_MEMORY;
Expand Down