Skip to content

GPU: wait for the O2 track count before sorting the O2 output tracks - #16062

Merged
davidrohr merged 1 commit into
AliceO2Group:devfrom
ktf:pr16062
Nov 7, 2026
Merged

davidrohr merged 1 commit into
AliceO2Group:devfrom
ktf:pr16062

Conversation

@ktf

@ktf ktf commented Nov 6, 2026

Copy link
Copy Markdown
Member

With GPUCA_SPECIALIZE_THRUST_SORTS (CUDA and HIP), the host-side launch
of GPUTPCGMO2Output::sort takes the number of elements from the host
copy of the merger's memory, processors()->tpcMerger.NOutputTracksTPCO2().
That count is computed by GPUTPCGMO2Output::prepare on the device, and
TransferMemoryResourceLinkToHost brings it to the host asynchronously,
on the same stream, right before the sort is launched. The event of the
transfer was only waited for after the sort, so the sort very likely
got the value from before prepare ran: 0, since RunTPCTrackingMerger
clears the merger's memory on the host at its start, and the transfer
is queued behind prepare. Then the O2 output tracks keep the order in
which prepare wrote them. This goes back to b0155c2, which moved
the wait after the sort. With debugLevel >= 1 or serializeGPU the
transfer is synchronous, which would hide it.

The other sorts that read host counts, SortTracks and SortTracksQPt,
wait for their transfer with CondWaitEvent first; do the same here, by
waiting before the sort. The wait was already needed before the
allocation of the O2 output that follows, so it costs no extra
synchronisation.

Found by reading the code, while porting these sorts to the Metal
backend, which reads the count after draining the stream. Not tested on
CUDA or HIP, there is no NVIDIA or AMD GPU here; the CPU build compiles.
To check on a GPU: with debugLevel 0, outputTracksTPCO2 should come out
ordered by the sort's comparator with this change, and not without it.

With GPUCA_SPECIALIZE_THRUST_SORTS (CUDA and HIP), the host-side launch
of GPUTPCGMO2Output::sort takes the number of elements from the host
copy of the merger's memory, processors()->tpcMerger.NOutputTracksTPCO2().
That count is computed by GPUTPCGMO2Output::prepare on the device, and
TransferMemoryResourceLinkToHost brings it to the host asynchronously,
on the same stream, right before the sort is launched. The event of the
transfer was only waited for after the sort, so the sort very likely
got the value from before prepare ran: 0, since RunTPCTrackingMerger
clears the merger's memory on the host at its start, and the transfer
is queued behind prepare. Then the O2 output tracks keep the order in
which prepare wrote them. This goes back to b0155c2, which moved
the wait after the sort. With debugLevel >= 1 or serializeGPU the
transfer is synchronous, which would hide it.

The other sorts that read host counts, SortTracks and SortTracksQPt,
wait for their transfer with CondWaitEvent first; do the same here, by
waiting before the sort. The wait was already needed before the
allocation of the O2 output that follows, so it costs no extra
synchronisation.

Found by reading the code, while porting these sorts to the Metal
backend, which reads the count after draining the stream. Not tested on
CUDA or HIP, there is no NVIDIA or AMD GPU here; the CPU build compiles.
To check on a GPU: with debugLevel 0, outputTracksTPCO2 should come out
ordered by the sort's comparator with this change, and not without it.
@ktf
ktf requested a review from davidrohr as a code owner November 6, 2026 13:03
@ktf

ktf commented Nov 6, 2026

Copy link
Copy Markdown
Member Author

@davidrohr the claim from my friends is that this actually fixes a genuine bug. The change was made in 2021, so I am a bit skeptical it went unnoticed. Could you have a look?

@davidrohr davidrohr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

that is a genuine bug, thx for spotting. Unfortunately I do not see a way to avoid this synchronization.

@davidrohr

Copy link
Copy Markdown
Collaborator

the point is that there is no severe effect, it will simply spit out TPC tracks in undefined order, which is irrelevant if not in deterministic mode. In deterministic mode, the full-intermediate-dump check will not spot it, since it has to serialize it. And the non-serialized deterministic mode only checks number of tracks, and associated / adjacent clusters, which is unaffected by the order. In any case, merging.

@davidrohr
davidrohr merged commit e4270ed into AliceO2Group:dev Nov 7, 2026
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants