Skip to content

Commit 972de08

Browse files
authored
Fix segfault cuvs bench (#2088)
Resolves #2087 # What does this PR do? There is a race condition on the std::optional `!handle_.has_value()`. While thread 1 works on assigning handle with mmap, thread 2 can race through and try to assign it as well. However, during reassignment it causes destructor of thread 1 to be called which calls unmap, leading to a segfault. To solve this we use `std::once` so that other threads can't enter the critical section. `std::once` is non reentrant, so we need to also refactor from using recursion to a while loop. `std::once` is not movable, so we allocate it on the heap and use a unique_pointer to keep track of it instead. We need to ensure member variables of blob are movable since blob is used in a std::variant Authors: - Anupam (https://github.com/aamijar) Approvers: - Dante Gama Dessavre (https://github.com/dantegd) URL: #2088
1 parent 319920e commit 972de08

2 files changed

Lines changed: 76 additions & 53 deletions

File tree

cpp/bench/ann/src/common/blob.hpp

Lines changed: 69 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* SPDX-FileCopyrightText: Copyright (c) 2025, NVIDIA CORPORATION.
2+
* SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION.
33
* SPDX-License-Identifier: Apache-2.0
44
*/
55
#pragma once
@@ -16,6 +16,8 @@
1616
#include <cassert>
1717
#include <cstdint>
1818
#include <cstdio>
19+
#include <memory>
20+
#include <mutex>
1921
#include <optional>
2022
#include <stdexcept>
2123
#include <string>
@@ -425,66 +427,80 @@ struct blob_mmap {
425427
mutable bool hugepages_2mb_actual_;
426428

427429
mutable std::optional<std::tuple<mmap_owner, ptrdiff_t>> handle_;
430+
// Heap-allocate the once_flag so that blob_mmap remains movable (std::once_flag
431+
// itself is neither copyable nor movable). Multiple threads can race on the
432+
// first call to handle() via blob<T>::data(); without this serialization each
433+
// racing thread would mmap the file and the losing emplace would munmap
434+
// the winner's mapping AND invalidate the reference the winner had already
435+
// returned to its caller -- a flaky SIGSEGV in ann benchmarks.
436+
mutable std::unique_ptr<std::once_flag> handle_once_ = std::make_unique<std::once_flag>();
428437

429438
[[nodiscard]] auto handle() const -> const std::tuple<mmap_owner, ptrdiff_t>&
430439
{
431-
if (!handle_.has_value()) {
432-
size_t page_size = hugepages_2mb_actual_ ? 1024ull * 1024ull * 2ull : sysconf(_SC_PAGE_SIZE);
433-
int flags = 0;
434-
if (hugepages_2mb_actual_) { flags |= MAP_HUGETLB | MAP_HUGE_2MB; }
435-
size_t data_start = sizeof(T) * file_.rows_offset() * file_.n_cols() + sizeof(uint32_t) * 2;
436-
size_t data_end = sizeof(T) * file_.rows_limit() * file_.n_cols() + data_start;
437-
438-
try {
439-
if (copy_in_memory_) {
440-
// Copy the content in-memory
441-
flags |= MAP_ANONYMOUS | MAP_PRIVATE;
442-
size_t size = data_end - data_start;
443-
mmap_owner owner{size, flags};
444-
std::fseek(file_.descriptor().value(), data_start, SEEK_SET);
445-
auto n_elems =
446-
static_cast<size_t>(file_.rows_limit()) * static_cast<size_t>(file_.n_cols());
447-
if (std::fread(owner.data(), sizeof(T), n_elems, file_.descriptor().value()) != n_elems) {
448-
throw std::runtime_error{"cuvs::bench::blob_mmap() fread " + file_.path() + " failed"};
440+
std::call_once(*handle_once_, [this] {
441+
// Loop here so that the huge-page fallback (formerly a recursive call
442+
// to handle()) does not re-enter std::call_once, which would deadlock.
443+
while (!handle_.has_value()) {
444+
size_t page_size =
445+
hugepages_2mb_actual_ ? 1024ull * 1024ull * 2ull : sysconf(_SC_PAGE_SIZE);
446+
int flags = 0;
447+
if (hugepages_2mb_actual_) { flags |= MAP_HUGETLB | MAP_HUGE_2MB; }
448+
size_t data_start = sizeof(T) * file_.rows_offset() * file_.n_cols() + sizeof(uint32_t) * 2;
449+
size_t data_end = sizeof(T) * file_.rows_limit() * file_.n_cols() + data_start;
450+
451+
try {
452+
if (copy_in_memory_) {
453+
// Copy the content in-memory
454+
flags |= MAP_ANONYMOUS | MAP_PRIVATE;
455+
size_t size = data_end - data_start;
456+
mmap_owner owner{size, flags};
457+
std::fseek(file_.descriptor().value(), data_start, SEEK_SET);
458+
auto n_elems =
459+
static_cast<size_t>(file_.rows_limit()) * static_cast<size_t>(file_.n_cols());
460+
if (std::fread(owner.data(), sizeof(T), n_elems, file_.descriptor().value()) !=
461+
n_elems) {
462+
throw std::runtime_error{"cuvs::bench::blob_mmap() fread " + file_.path() +
463+
" failed"};
464+
}
465+
handle_.emplace(std::move(owner), 0);
466+
} else {
467+
// Map the file
468+
// If this is a temporary file, we're supposed to write to it, hence MAP_SHARED.
469+
flags |= file_.is_temporary() ? MAP_SHARED : MAP_PRIVATE;
470+
size_t mmap_start = (data_start / page_size) * page_size;
471+
size_t mmap_size = data_end - mmap_start;
472+
handle_.emplace(
473+
mmap_owner{file_.descriptor(), mmap_start, mmap_size, flags, file_.is_temporary()},
474+
data_start - mmap_start);
449475
}
450-
handle_.emplace(std::move(owner), 0);
451-
} else {
452-
// Map the file
453-
// If this is a temporary file, we're supposed to write to it, hence MAP_SHARED.
454-
flags |= file_.is_temporary() ? MAP_SHARED : MAP_PRIVATE;
455-
size_t mmap_start = (data_start / page_size) * page_size;
456-
size_t mmap_size = data_end - mmap_start;
457-
handle_.emplace(
458-
mmap_owner{file_.descriptor(), mmap_start, mmap_size, flags, file_.is_temporary()},
459-
data_start - mmap_start);
460-
}
461-
} catch (const mmap_error& e) {
462-
bool hugepages_2mb_asked = hugepages_2mb_requested_ == HugePages::kAsk ||
463-
hugepages_2mb_requested_ == HugePages::kRequire;
464-
if (e.code() == EPERM && hugepages_2mb_asked && hugepages_2mb_actual_) {
465-
if (hugepages_2mb_requested_ == HugePages::kRequire) {
466-
log_warn(
467-
"cuvs::bench::blob_mmap: `mmap` failed to map due to EPERM, which is likely caused "
468-
"by the permissions issue. You either need a CAP_IPC_LOCK capability or run the "
469-
"program with sudo. We will try again without huge pages.");
476+
} catch (const mmap_error& e) {
477+
bool hugepages_2mb_asked = hugepages_2mb_requested_ == HugePages::kAsk ||
478+
hugepages_2mb_requested_ == HugePages::kRequire;
479+
if (e.code() == EPERM && hugepages_2mb_asked && hugepages_2mb_actual_) {
480+
if (hugepages_2mb_requested_ == HugePages::kRequire) {
481+
log_warn(
482+
"cuvs::bench::blob_mmap: `mmap` failed to map due to EPERM, which is likely "
483+
"caused by the permissions issue. You either need a CAP_IPC_LOCK capability or "
484+
"run the program with sudo. We will try again without huge pages.");
485+
}
486+
hugepages_2mb_actual_ = false;
487+
continue; // retry the loop with huge pages disabled
470488
}
471-
hugepages_2mb_actual_ = false;
472-
return handle();
473-
}
474-
if (e.code() == EINVAL && hugepages_2mb_asked && hugepages_2mb_actual_ &&
475-
!copy_in_memory_) {
476-
if (hugepages_2mb_requested_ == HugePages::kRequire) {
477-
log_warn(
478-
"cuvs::bench::blob_mmap: `mmap` failed to map due to EINVAL, which is likely caused "
479-
"by the file system not supporting huge pages. We will try again without huge "
480-
"pages.");
489+
if (e.code() == EINVAL && hugepages_2mb_asked && hugepages_2mb_actual_ &&
490+
!copy_in_memory_) {
491+
if (hugepages_2mb_requested_ == HugePages::kRequire) {
492+
log_warn(
493+
"cuvs::bench::blob_mmap: `mmap` failed to map due to EINVAL, which is likely "
494+
"caused by the file system not supporting huge pages. We will try again without "
495+
"huge pages.");
496+
}
497+
hugepages_2mb_actual_ = false;
498+
continue; // retry the loop with huge pages disabled
481499
}
482-
hugepages_2mb_actual_ = false;
483-
return handle();
500+
throw; // The error is not due to huge pages or otherwise unrecoverable
484501
}
485-
throw; // The error is not due to huge pages or otherwise unrecoverable
486502
}
487-
}
503+
});
488504
return handle_.value();
489505
}
490506

cpp/bench/ann/src/common/dataset.hpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,13 @@ struct ground_truth_map {
9292
}
9393
}
9494
};
95+
// Prewarm any lazily-initialized state on the main thread (e.g. memory-mapped
96+
// buffers inside `blob<T>::data()`) before dispatching worker threads, so the
97+
// workers only ever read already-initialized state through `data()`. This
98+
// protects this parallel section against any future lazy paths added inside
99+
// blob.hpp that may not be internally synchronized.
100+
(void)ground_truth_set.data();
101+
if (filter_bitset.has_value()) { (void)filter_bitset->data(MemoryType::kHostMmap); }
95102
// launch worker threads
96103
int start = 0;
97104
for (int tid = 0; tid < num_map_building_worker_threads; tid++) {

0 commit comments

Comments
 (0)