From ec387c3df6a1f3da7c55a6105e526888922f0ba1 Mon Sep 17 00:00:00 2001 From: Filip Leonarski Date: Sat, 10 Oct 2026 11:36:30 +0200 Subject: [PATCH] rugnux: page-locked raw image bytes are freed only after the engine unregisters them The first-pass spot engines (FirstPassRun::SpotEngine, SpotFindingWorkers::Engine) were given back by move-assigning a fresh struct over them. A move-assignment goes in declaration order, so it freed raw_image - whose bytes the analysis engine had page-locked with cudaHostRegister (PinInputRegion, every uncompressed/CBF frame) - and only then destroyed the engine, which unregistered the already freed range. In between, the address may be handed to another thread's buffer. Measured in a standalone test on this card and driver: a copy into a new buffer at a stale registered address goes to the old pages (the data never arrives, no error), and cudaHostRegister / cudaMallocHost on it fail with "already mapped"; eight threads freeing in this order made unrelated threads' ordinary pageable copies return wrong data, the correct order none. The engines now release themselves engine first, buffer last. The image pass had the same hazard where the writer still holds an image: the worker read the next frame into a new one and dropped the old one, which the writer then freed while the worker's engine still had it registered. The old image is now held until the engine has pinned the new one. This is the most likely cause of a sticky copy-engine fault (Xid 31, CE write to an unmapped address) that ended a rotation run's scaling with "unspecified launch failure": the speculative geometry probe released its spot engines on many threads at once while the merges beside it were copying. That failure was not reproduced in 9 reruns before the fix, so the link is inferred, not shown. Merged data unchanged (p.hkl md5 and MTZ data identical to rc175 on 10 sets; p.hkl identical with --write-process-h5 on a CBF set). Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01EizBKhTqrAYago9KJG3dA3 --- rugnux/RugnuxFirstPass.cpp | 11 +++++++++-- rugnux/RugnuxFirstPass.h | 10 ++++++++++ rugnux/RugnuxImagePass.cpp | 9 ++++++++- 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/rugnux/RugnuxFirstPass.cpp b/rugnux/RugnuxFirstPass.cpp index cc44a332b..d2f814092 100644 --- a/rugnux/RugnuxFirstPass.cpp +++ b/rugnux/RugnuxFirstPass.cpp @@ -134,7 +134,7 @@ namespace { start_image(start_image), stride(stride), engines(std::max(nworkers, 1)) {} ~SpotFindingWorkers() { - OnWorkers(engines.size(), [&](size_t t) { engines[t] = Engine{}; }); + OnWorkers(engines.size(), [&](size_t t) { engines[t].Release(); }); } // The spots of each ordinal, in the order given. @@ -164,6 +164,13 @@ namespace { std::unique_ptr indexer; std::unique_ptr analysis; std::unique_ptr profile; + // In the order destruction takes, not the order a move-assignment would: see SpotEngine. + void Release() { + profile.reset(); + analysis.reset(); + indexer.reset(); + raw_image = {}; + } }; // fn(t) on n threads of its own, thread t on the card its engine lives on. @@ -568,7 +575,7 @@ void Rugnux::FirstPassRun::ReleaseSpotEngines() { for (size_t t = 0; t < engines.size(); t++) futures.emplace_back(std::async(std::launch::async, [&, t] { UseGpuOfWorker(t); - engines[t] = SpotEngine{}; + engines[t].Release(); })); for (auto &f : futures) f.get(); diff --git a/rugnux/RugnuxFirstPass.h b/rugnux/RugnuxFirstPass.h index c6fdb2d88..7ca66ce90 100644 --- a/rugnux/RugnuxFirstPass.h +++ b/rugnux/RugnuxFirstPass.h @@ -200,6 +200,16 @@ struct Rugnux::FirstPassRun { JFJochReaderRawImage raw_image; std::unique_ptr analysis; std::unique_ptr profile; + // Give the engine back with the buffer last. Not by assigning a fresh SpotEngine over it: a + // move-assignment goes in declaration order, so it freed raw_image's bytes while the engine + // still had them page-locked, and unregistered them only afterwards - by then the address + // could belong to another thread's buffer, whose copies the driver then took through the + // stale registration (and whose cudaMallocHost it refused). + void Release() { + profile.reset(); + analysis.reset(); + raw_image = {}; + } }; std::vector engines; int prefetch_batches = 0; diff --git a/rugnux/RugnuxImagePass.cpp b/rugnux/RugnuxImagePass.cpp index 63def847a..7d0828cfc 100644 --- a/rugnux/RugnuxImagePass.cpp +++ b/rugnux/RugnuxImagePass.cpp @@ -221,6 +221,8 @@ void Rugnux::ImagePass(PipelineLocals &p) { // Before the analysis engine, which page-locks these bytes for its uploads and unregisters // them when it is destroyed: it must not outlive the buffer it was given. std::shared_ptr img; + // The image the writer holds whose bytes the engine still has page-locked (see below). + std::shared_ptr pinned; MXAnalysisWithoutFPGA analysis(experiment_, mapping, pixel_mask_, *indexer, /*enable_fused_adaptive_gpu=*/true); AzimuthalIntegrationProfile profile(mapping); @@ -235,8 +237,12 @@ void Rugnux::ImagePass(PipelineLocals &p) { // Reuse the raw image frame to frame - a fresh 4-byte 16 Mpx frame is 72 MB of // untouched pages the decoder faults in one by one - except while the writer thread // still holds the previous one's pixels. - if (!img || img.use_count() > 1) + // The engine unregisters the old bytes when it pins the new ones (Analyze), so until then + // the old image is kept here: the writer would otherwise free bytes still registered. + if (!img || img.use_count() > 1) { + pinned = std::move(img); img = std::make_shared(); + } bool read = false; try { read = reader_.ReadRawImage(image_idx, *img); @@ -263,6 +269,7 @@ void Rugnux::ImagePass(PipelineLocals &p) { logger.Error("Error analyzing image {}: {}", image_idx, e.what()); continue; } + pinned.reset(); msg.processing_time_s = std::chrono::duration(std::chrono::steady_clock::now() - t0).count(); msg.run_number = experiment_.GetRunNumber(); msg.run_name = experiment_.GetRunName();