Repository navigation
[BUG]: Potential race in merge sort due to PDL #3131
Description
Is this a duplicate?
- I confirmed there appear to be no duplicate issues for this bug and that I agree to the Code of Conduct
Type of Bug
Silent Failure
Component
CUB
Describe the bug
#3114 introduced programmatic dependent launch into device merge sort. I think it should cause date races. Dependent launch consists of two steps:
- primary kernel invoking
cudaTriggerProgrammaticLaunchCompletion(), - and secondary kernel invoking
cudaGridDependencySynchronize()
This causes concurrent execution of these kernels:
As written, merge sort now has the following structure:
- BlockSort (primary for partition):
- Dependency sync (why is it needed?)
- Load input keys (A)
- Trigger next launch
- Sort
- Store output keys (B)
- Partition (secondary for block sort / primary for merge):
- Dependency sync (waiting for block sort to trigger launch)
- Read output keys (B)
- Write partitions (P)
- No explicit triggering of the next launch, so happens implicitly on last block exit
- Merge kernel (secondary for previous partition / primary for next partition):
- Dependency sync (waiting for partition to finish)
- Read output keys (B, P)
- Trigger next launch
- Merge
- Write buffer keys (C)
- Partition
- Dependency sync
- Load buffer keys (C)
- Write partition (P)
- No explicit triggering of the next launch, so happens implicitly on last block exit
- ...
Since each pair of primary / secondary kernels is concurrent, we should have a data race between:
- 2.ii and 1.v
- 4.ii and 3.v
- ...
We likely want to trigger dependent launch after we write the data, not before.
How to Reproduce
Likely have to find workload that is under occupancy so that last CTA of primary kernel runs concurrently with last CTA of secondary kernel.
Expected behavior
No race in merge sort
Reproduction link
No response
Operating System
No response
nvidia-smi output
No response
NVCC version
No response
Metadata
Metadata
Assignees
Labels
No labels
Type
Projects
- Status
Activity
bernhardmgruber commentedon Dec 12, 2024
So we can overlap BlockSort with a previous kernel (outside CUB). However, I just saw that I missed launching the kernel with the PDL flag.
I tried adding one, but I always ended up crashing. Under compute-sanitizer, the bug disappeared. I discussed with @ahendriksen and it seems the workaround was to
__syncthreads()before triggering the next launch.I documented this here:
cccl/cub/cub/device/dispatch/dispatch_merge_sort.cuh
Line 215 in 53f69a4
However, the entire kernel is divergent until it exits (because it branches based on whether the thread id is smaller then the problem size), so we could only trigger the next launch at the end of the kernel, which is also done implicitly. Therefore, such a call is missing.
I think you may have fallen for the same misconception as I did, but @ahendriksen could help me out here:
cudaGridDependencySynchronizedoes not wait forcudaTriggerProgrammaticLaunchCompletion, but for the end of the previous kernel.cudaTriggerProgrammaticLaunchCompletionallows the next kernel to ramp-up, but then wait for the previous kernel to complete.bernhardmgruber commentedon Dec 12, 2024
The only data race I could introduce is if I would put the
cudaGridDependencySynchronizeafter reading the first data and enabling PDL for that kernel. Then the kernel could start reading while a previous kernel is still writing.gevtushenko commentedon Dec 12, 2024
My bad, I completely missed that. Thank you for elaborating!