diff --git a/src/llama-mmap.cpp b/src/llama-mmap.cpp index ed572da7fb54..1b39716ceda6 100644 --- a/src/llama-mmap.cpp +++ b/src/llama-mmap.cpp @@ -34,6 +34,11 @@ #define PATH_MAX MAX_PATH #endif #include + // IOCTL_STORAGE_QUERY_PROPERTY and STORAGE_ACCESS_ALIGNMENT_DESCRIPTOR: the device + // is asked for its sector sizes rather than told what they are. + #include + // _aligned_malloc / _aligned_free, the Windows counterpart to posix_memalign. + #include #endif #if defined(__APPLE__) @@ -67,7 +72,57 @@ static std::string llama_format_win_err(DWORD err) { struct llama_file::impl { #if defined(_WIN32) - HANDLE fp_win32; + HANDLE fp_win32 = INVALID_HANDLE_VALUE; + + // True only when this class opened the handle itself with CreateFileW, which is + // exactly when direct I/O is in effect. On the buffered path fp_win32 is derived + // from the CRT stream and belongs to it - closing it here would be a double close. + // The destructor asks about OWNERSHIP, not about mode, which is why this is not + // called is_direct_io. + bool owns_handle = false; + + // One private handle per concurrent reader. This is the whole point of the pool, and + // it is not an optimisation of the OVERLAPPED read - it is the thing the OVERLAPPED + // read could not buy. + // + // Measured 2026-08-03 on this machine, 12.75 MiB blocks, FILE_FLAG_NO_BUFFERING, + // same file and offsets throughout: one shared handle read through an OVERLAPPED + // offset reaches 1.01x at queue depth 8; one handle per thread reaches 2.22x. The + // same pair through SetFilePointerEx gives 0.98x and 2.19x. The read mechanism makes + // no difference at all; sharing the handle makes all of it. Windows serialises on the + // file object, not on the file pointer. + // + // The count is a ceiling on worker ids, not a throughput setting. Saturation sits at + // depth 8: depth 64 measured 2.15x against 2.22x at depth 8, so slots past the eighth + // buy no bandwidth. They are there so that a caller running more readers than the + // saturation point still gets a private handle for each of them - a worker id at or + // past n_pool falls back to the shared handle and takes the serialisation with it, + // which is the one failure this pool exists to prevent. 18 is that headroom, and the + // price for it is 18 open handles on one file. + // + // n_pool counts what was actually opened, always contiguous from 0. That is why the + // array needs no sentinel: slots at or past n_pool were never opened, so the + // destructor closes exactly [0, n_pool) and an aggregate initialiser that fills with + // NULL rather than INVALID_HANDLE_VALUE cannot turn into a CloseHandle(NULL). + static constexpr int POOL_SLOTS = 18; + HANDLE h_pool[POOL_SLOTS] = {}; + int n_pool = 0; + + // The file position under direct I/O, kept here instead of in the kernel. + // + // Measured 2026-08-03: FILE_FLAG_NO_BUFFERING makes SetFilePointerEx reject any + // unaligned position outright with ERROR_INVALID_PARAMETER. The POSIX branch does + // not have this problem - lseek positions freely and only read() must be aligned - + // which is why read_aligned_chunk, copied from there, could not work here at all. + // + // So seek() writes to this variable and every read goes through read_raw_at, whose + // OVERLAPPED offset never touches the kernel's pointer. Note that this does NOT + // lift the alignment requirement: the OVERLAPPED offset must be sector-aligned too. + // It only removes the requirement from POSITIONING, which is what was fatal. + size_t logical_pos = 0; + + std::string fname; + std::string GetErrorMessageWin32(DWORD error_code) const { std::string ret; LPSTR lpMsgBuf = NULL; @@ -83,10 +138,150 @@ struct llama_file::impl { return ret; } - impl(const char * fname, const char * mode, [[maybe_unused]] const bool use_direct_io = false) { - fp = ggml_fopen(fname, mode); + impl(const char * fname, const char * mode, const bool use_direct_io = false) : fname(fname) { + // Try unbuffered I/O for read only, mirroring what the POSIX branch does with + // O_DIRECT. Until now this parameter was accepted and dropped: the constructor + // took use_direct_io, ignored it, and has_direct_io() reported true regardless. + // That combination is a promise the code did not keep. + if (use_direct_io && std::strcmp(mode, "rb") == 0) { + if (init_direct()) { + return; + } + LLAMA_LOG_WARN("Failed to open file '%s' unbuffered: %s. Falling back to buffered I/O\n", + fname, GetErrorMessageWin32(GetLastError()).c_str()); + } + init_fp(mode); + } + + // Ask the device for its sector sizes. Returns 0 when the question cannot be + // answered, which makes the caller fall back to buffered I/O WITH A WARNING. + // + // Deliberately no default of 4096. That constant is written three times across this + // ecosystem and on this drive it happens to be right - measured 2026-08-03, 512 + // logical and 4096 physical. Correct by luck is not correct by construction, and a + // silent default would turn "the device never answered" into something that looks + // like an answer. + // + // The alignment DUTY of FILE_FLAG_NO_BUFFERING is the LOGICAL sector size; the + // PHYSICAL one is Microsoft's performance recommendation. Reading aligned to the + // logical size on a 512e drive is permitted and costs read-modify-write on the + // controller, so the physical size is what we take. + size_t query_sector_size(char drive_letter) const { + char volume[] = "\\\\.\\X:"; + volume[4] = drive_letter; + + // Access 0 asks for metadata only; this needs no elevation. + HANDLE hv = CreateFileA(volume, 0, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, + OPEN_EXISTING, 0, NULL); + if (hv == INVALID_HANDLE_VALUE) { + return 0; + } + + STORAGE_PROPERTY_QUERY query = {}; + query.PropertyId = StorageAccessAlignmentProperty; + query.QueryType = PropertyStandardQuery; + + STORAGE_ACCESS_ALIGNMENT_DESCRIPTOR desc = {}; + DWORD returned = 0; + const BOOL ok = DeviceIoControl(hv, IOCTL_STORAGE_QUERY_PROPERTY, + &query, sizeof(query), + &desc, sizeof(desc), &returned, NULL); + CloseHandle(hv); + + if (!ok || returned < sizeof(desc) || desc.BytesPerPhysicalSector == 0) { + return 0; + } + return (size_t) desc.BytesPerPhysicalSector; + } + + bool init_direct() { + // UTF-8 to UTF-16. ggml_fopen does the same conversion, but its helper is + // static inside ggml.c and there is no exported equivalent. + const int wlen = MultiByteToWideChar(CP_UTF8, 0, fname.c_str(), -1, NULL, 0); + if (wlen == 0) { + return false; + } + std::vector wname(wlen); + if (MultiByteToWideChar(CP_UTF8, 0, fname.c_str(), -1, wname.data(), wlen) == 0) { + return false; + } + + // Resolve to an absolute path BEFORE deriving the volume. The sector size can + // only be asked of a drive letter, and a relative path has none - so a relative + // path used to make direct I/O silently unavailable and fall back to buffered + // reads. The loader happens to pass absolute paths, which is luck rather than + // construction; the test below passes a relative one and is how this was found. + wchar_t abs_path[MAX_PATH]; + const DWORD abs_len = GetFullPathNameW(wname.data(), MAX_PATH, abs_path, NULL); + if (abs_len == 0 || abs_len >= MAX_PATH) { + return false; + } + if (abs_path[1] != L':') { + // A UNC path has no volume to ask, so the sector size stays unknown and + // unbuffered reads cannot be aligned safely. + SetLastError(ERROR_NOT_SUPPORTED); + return false; + } + + HANDLE h = CreateFileW(abs_path, GENERIC_READ, FILE_SHARE_READ, NULL, + OPEN_EXISTING, FILE_FLAG_NO_BUFFERING, NULL); + if (h == INVALID_HANDLE_VALUE) { + return false; + } + + const size_t sector = query_sector_size((char) abs_path[0]); + if (sector == 0) { + CloseHandle(h); + SetLastError(ERROR_NOT_SUPPORTED); + return false; + } + + LARGE_INTEGER li; + if (!GetFileSizeEx(h, &li)) { + const DWORD err = GetLastError(); + CloseHandle(h); + SetLastError(err); + return false; + } + + fp_win32 = h; + owns_handle = true; + alignment = sector; + size = (size_t) li.QuadPart; + + // Open the private handles here: single-threaded, after the path, the flags and + // the sector size are all known good, and before anyone can read. + // + // A short pool is NOT fatal. read_raw_at falls back to the shared handle for any + // slot that is missing, which costs throughput and correctness nothing. It is + // logged rather than swallowed, because a pool that came up empty reads exactly + // the same bytes as one that works - it simply never scales, and that is the + // failure shape this file keeps finding. + // + // FILE_SHARE_READ, the same as the handle above. Win32 grants the second open + // only if its access is permitted by every existing handle's share mode AND its + // own share mode permits every existing handle's access; read-only on both sides + // satisfies that. Asking for FILE_SHARE_WRITE here as well would still be granted + // and would still mean two different sharing contracts on one file. + for (int i = 0; i < POOL_SLOTS; i++) { + const HANDLE hp = CreateFileW(abs_path, GENERIC_READ, FILE_SHARE_READ, NULL, + OPEN_EXISTING, FILE_FLAG_NO_BUFFERING, NULL); + if (hp == INVALID_HANDLE_VALUE) { + LLAMA_LOG_WARN("%s: opened %d of %d private read handles for '%s': %s. " + "Concurrent reads fall back to the shared handle and will not scale\n", + __func__, i, POOL_SLOTS, fname.c_str(), + GetErrorMessageWin32(GetLastError()).c_str()); + break; + } + h_pool[n_pool++] = hp; + } + return true; + } + + void init_fp(const char * mode) { + fp = ggml_fopen(fname.c_str(), mode); if (fp == NULL) { - throw std::runtime_error(format("failed to open %s: %s", fname, strerror(errno))); + throw std::runtime_error(format("failed to open %s: %s", fname.c_str(), strerror(errno))); } fp_win32 = (HANDLE) _get_osfhandle(_fileno(fp)); seek(0, SEEK_END); @@ -94,7 +289,7 @@ struct llama_file::impl { seek(0, SEEK_SET); } - impl(FILE * file) : owns_fp(false) { + impl(FILE * file) : fname("(file*)"), owns_fp(false) { fp = file; fp_win32 = (HANDLE) _get_osfhandle(_fileno(fp)); seek(0, SEEK_END); @@ -103,6 +298,10 @@ struct llama_file::impl { } size_t tell() const { + if (owns_handle) { + return logical_pos; + } + LARGE_INTEGER li; li.QuadPart = 0; BOOL ret = SetFilePointerEx(fp_win32, li, &li, FILE_CURRENT); @@ -113,11 +312,25 @@ struct llama_file::impl { return li.QuadPart; } - void seek(size_t offset, int whence) const { + void seek(size_t offset, int whence) { static_assert(SEEK_SET == FILE_BEGIN, "SEEK_SET != FILE_BEGIN"); static_assert(SEEK_CUR == FILE_CURRENT, "SEEK_CUR != FILE_CURRENT"); static_assert(SEEK_END == FILE_END, "SEEK_END != FILE_END"); + if (owns_handle) { + // Never ask the kernel. See the note on logical_pos: an unaligned position + // is refused outright here, and refusing to seek is not something callers + // expect from seek(). + switch (whence) { + case SEEK_SET: logical_pos = offset; break; + case SEEK_CUR: logical_pos += offset; break; + case SEEK_END: logical_pos = size + offset; break; + default: + throw std::runtime_error(format("seek error: bad whence %d", whence)); + } + return; + } + LARGE_INTEGER li; li.QuadPart = offset; BOOL ret = SetFilePointerEx(fp_win32, li, NULL, whence); @@ -126,7 +339,24 @@ struct llama_file::impl { } } - void read_raw(void * ptr, size_t len) { + void read_raw_unsafe(void * ptr, size_t len) { + if (owns_handle) { + // Direct I/O never uses the kernel's file pointer - see logical_pos. + const size_t got = read_raw_at(ptr, len, logical_pos); + if (got < len) { + // End of file. An aligned request necessarily overshoots a file whose + // size is not a sector multiple, and none of the four model files here + // ends on a boundary, so this is the normal case rather than an edge + // one. Measured 2026-08-03 on all four: ReadFile returns TRUE and + // reports exactly the bytes up to the logical EOF. Zero the padding and + // carry on; the caller knows the tensor size and trims it. Same + // behaviour as the POSIX branch. + std::memset(reinterpret_cast(ptr) + got, 0, len - got); + } + logical_pos += got; + return; + } + size_t bytes_read = 0; while (bytes_read < len) { size_t chunk_size = std::min(len - bytes_read, 64*1024*1024); @@ -136,6 +366,19 @@ struct llama_file::impl { throw std::runtime_error(format("read error: %s", GetErrorMessageWin32(GetLastError()).c_str())); } if (chunk_read < chunk_size || chunk_read == 0) { + // A short count at the end of the file. Under direct I/O this is the + // NORMAL case, not an edge one: an aligned request necessarily runs past + // a file whose size is not a sector multiple, and none of the four model + // files on this machine ends on a sector boundary. + // + // Measured 2026-08-03 on all four: with FILE_FLAG_NO_BUFFERING, ReadFile + // returns TRUE and reports exactly the bytes up to the logical EOF. The + // padding is zeroed and the caller - which knows the tensor size and + // trims the padding itself - carries on. This mirrors the POSIX branch. + // + // Reached only on the buffered path now, which never asks for more than + // it wants - so a short count here is still a real failure, exactly as + // it was before this file learned about direct I/O. throw std::runtime_error("unexpectedly reached end of file"); } @@ -143,6 +386,102 @@ struct llama_file::impl { } } + // Read `size_to_read` bytes from the current position, coping with an offset or + // length that direct I/O will not accept. Reads the enclosing sector-aligned range + // into an aligned bounce buffer and copies out the part that was asked for. + // + // Lives here as an implementation detail, not in the header. The header used to + // declare read_aligned_chunk() publicly while no llama_file method of that name + // existed on any platform and nobody called it - a promise with nothing behind it. + void read_aligned_chunk(void * dest, size_t size_to_read) { + const size_t offset = tell(); + const size_t aligned_offset = offset & ~(alignment - 1); + const size_t offset_from_alignment = offset - aligned_offset; + const size_t bytes_to_read = (offset_from_alignment + size_to_read + alignment - 1) & ~(alignment - 1); + + // The buffer ADDRESS has to be sector-aligned too, not just the offset and the + // length. Missing that is what turned the first attempt at this into a + // 0xC0000409 with no output at all. + void * raw_buffer = _aligned_malloc(bytes_to_read, alignment); + if (raw_buffer == nullptr) { + throw std::runtime_error(format("_aligned_malloc of %zu bytes failed", bytes_to_read)); + } + + struct aligned_buffer_deleter { + void operator()(void * p) const { _aligned_free(p); } + }; + std::unique_ptr buffer(raw_buffer); + + // Reads at the ALIGNED offset directly, without moving any file pointer. The + // earlier form seek(aligned_offset) + read was copied from POSIX and could not + // work here: the caller's unaligned position had already been rejected by the + // kernel before this function was ever reached. + const size_t got = read_raw_at(buffer.get(), bytes_to_read, aligned_offset); + if (got < offset_from_alignment + size_to_read) { + throw std::runtime_error("unexpectedly reached end of file"); + } + + std::memcpy(dest, reinterpret_cast(buffer.get()) + offset_from_alignment, size_to_read); + + // The logical position advances by what the caller asked for, not by what the + // bounce buffer had to read around it. + logical_pos = offset + size_to_read; + } + + void read_raw(void * ptr, size_t len) { + if (has_direct_io()) { + read_aligned_chunk(ptr, len); + } else { + read_raw_unsafe(ptr, len); + } + } + + // The positional read Windows never had in llama_file. The offset travels in the + // OVERLAPPED structure, so no seek is needed and the shared file pointer is not + // touched - which is what makes it SAFE to call from several threads on one handle. + // + // Safe was never the same as concurrent, and that distinction cost a measurement to + // establish. Passing OVERLAPPED to a handle opened without FILE_FLAG_OVERLAPPED does + // not make the read asynchronous: measured 2026-08-03, one shared handle across 8 + // threads reaches 1.01x the throughput of a single thread whether the offset comes + // from the structure or from SetFilePointerEx. A private handle each reaches 2.22x. + // + // Hence worker_id. It selects this caller's own handle out of the pool, and no lock + // guards that selection because none is needed: each index is read by exactly one + // thread, and the array is filled once in init_direct before any reader exists. + size_t read_raw_at(void * ptr, size_t len, size_t offset, int worker_id = -1) { + // Anything the pool does not cover - a negative id, an id past what opened, or a + // buffered file that has no pool at all - reads through the shared handle. That + // is correct and merely serialised, which is the right way round: a caller that + // knows nothing about pools must not be able to index past the array. + const HANDLE h_read = (worker_id >= 0 && worker_id < n_pool) ? h_pool[worker_id] : fp_win32; + + size_t total = 0; + while (total < len) { + const size_t chunk_size = std::min(len - total, 64*1024*1024); + const size_t pos = offset + total; + + OVERLAPPED ov = {}; + ov.Offset = (DWORD) (pos & 0xFFFFFFFFull); + ov.OffsetHigh = (DWORD) (pos >> 32); + + DWORD chunk_read = 0; + if (!ReadFile(h_read, reinterpret_cast(ptr) + total, (DWORD) chunk_size, &chunk_read, &ov)) { + const DWORD err = GetLastError(); + if (err == ERROR_HANDLE_EOF) { + return total; + } + throw std::runtime_error(format("read error: %s", GetErrorMessageWin32(err).c_str())); + } + total += chunk_read; + if (chunk_read < chunk_size) { + // Short read means end of file - see the note in read_raw_unsafe. + return total; + } + } + return total; + } + uint32_t read_u32() { uint32_t val; read_raw(&val, sizeof(val)); @@ -170,12 +509,41 @@ struct llama_file::impl { write_raw(&val, sizeof(val)); } + // Used to return true unconditionally, on a branch that never opened anything + // unbuffered. The single in-tree caller sits in the POSIX branch, so the lie was + // harmless in this repository and would only have been found by external code + // asking the question. Any caller that logs "O_DIRECT in effect" on the strength of + // this answer would have printed it on Windows while reading through the page cache. + // + // Asks about ownership AND alignment, so it stays honest when the constructor fell + // back to buffered I/O. bool has_direct_io() const { - return true; + return owns_handle && alignment > 1; + } + + size_t direct_io_handles() const { + return (size_t) n_pool; } ~impl() { - if (fp && owns_fp) { + // The pool first. These handles are always ours - nothing else can hold them, + // since they are opened in init_direct and handed out by value nowhere. Closing + // exactly n_pool of them is what lets the array go without a sentinel: the slots + // from n_pool upward were never opened, so they are NULL rather than + // INVALID_HANDLE_VALUE and must not reach CloseHandle at all. + for (int i = 0; i < n_pool; i++) { + CloseHandle(h_pool[i]); + } + n_pool = 0; + + if (owns_handle) { + if (fp_win32 != INVALID_HANDLE_VALUE) { + CloseHandle(fp_win32); + } + } else if (fp && owns_fp) { + // Buffered path: the CRT stream owns fp_win32. Closing both would be a + // double close, and closing fp when owns_fp is false would close a stream + // that belongs to the caller. std::fclose(fp); } } @@ -348,6 +716,39 @@ struct llama_file::impl { } } + // Positional read; see the Windows counterpart for what it is for. pread does not + // move the file pointer, which is what lets two threads share one descriptor. + size_t read_raw_at(void * ptr, size_t len, size_t offset, int worker_id = -1) { + // POSIX needs no handle pool. pread carries its offset in the call and does not + // hold the file object while it runs, so several threads on ONE descriptor are + // already concurrent. Windows has no such call, which is why the pool exists over + // there and not here. The parameter is accepted on both so that a caller need not + // know which platform it is compiled for. + (void) worker_id; + +#if defined(fileno) + const int use_fd = (fd != -1) ? fd : fileno(fp); +#else + const int use_fd = (fd != -1) ? fd : ::fileno(fp); +#endif + size_t total = 0; + while (total < len) { + const ssize_t ret = ::pread(use_fd, reinterpret_cast(ptr) + total, + len - total, (off_t) (offset + total)); + if (ret == -1) { + if (errno == EINTR) { + continue; // Interrupted by signal, retry + } + throw std::runtime_error(format("read error: %s", strerror(errno))); + } + if (ret == 0) { + return total; // end of file + } + total += (size_t) ret; + } + return total; + } + uint32_t read_u32() { uint32_t ret; read_raw(&ret, sizeof(ret)); @@ -373,6 +774,13 @@ struct llama_file::impl { return fd != -1 && alignment > 1; } + // Always zero here, and that is the honest answer rather than a stub: this branch + // holds no private handles because pread does not need them. A caller comparing the + // number against its thread count learns the right thing on both platforms. + size_t direct_io_handles() const { + return 0; + } + ~impl() { if (fd != -1) { close(fd); @@ -410,6 +818,20 @@ bool llama_file::has_direct_io() const { return pimpl->has_direct_io(); } int llama_file::file_id() const { #ifdef _WIN32 + if (pimpl->owns_handle) { + // A direct-I/O file has no CRT descriptor, and a HANDLE cannot be squeezed + // through an int: it is a 64-bit pointer, so a cast drops the upper half and + // yields something that still looks like a valid handle and reads zero bytes + // without raising. That exact truncation is easy to hit through any FFI layer + // that defaults a handle-returning call to a 32-bit result type. + // + // An exception rather than GGML_ASSERT on purpose: an assert can be compiled + // out by build configuration, and a guard that disappears in release is no + // guard at all. Failing here is safe because the two are mutually exclusive by + // construction - the model loader only opens files unbuffered for + // LLAMA_LOAD_MODE_DIRECT_IO, and that mode does not use mmap. + throw std::runtime_error("file_id() is not available on a direct-I/O handle on Windows"); + } return _fileno(pimpl->fp); #else if (pimpl->fd != -1) { @@ -425,11 +847,17 @@ int llama_file::file_id() const { void llama_file::seek(size_t offset, int whence) const { pimpl->seek(offset, whence); } void llama_file::read_raw(void * ptr, size_t len) { pimpl->read_raw(ptr, len); } -#ifdef _WIN32 -void llama_file::read_raw_unsafe(void * ptr, size_t len) { pimpl->read_raw(ptr, len); } -#else + +// The #ifdef that used to sit here routed Windows to read_raw, because the Windows +// impl had no read_raw_unsafe at all. It has one now - the same loop as before plus +// the end-of-file case - so the buffered path is unchanged. void llama_file::read_raw_unsafe(void * ptr, size_t len) { pimpl->read_raw_unsafe(ptr, len); } -#endif + +size_t llama_file::read_raw_at(void * ptr, size_t len, size_t offset, int worker_id) { + return pimpl->read_raw_at(ptr, len, offset, worker_id); +} + +size_t llama_file::direct_io_handles() const { return pimpl->direct_io_handles(); } uint32_t llama_file::read_u32() { return pimpl->read_u32(); } diff --git a/src/llama-mmap.h b/src/llama-mmap.h index b7d5c61e95ff..5dcb2c81092c 100644 --- a/src/llama-mmap.h +++ b/src/llama-mmap.h @@ -27,7 +27,26 @@ struct llama_file { void read_raw(void * ptr, size_t len); void read_raw_unsafe(void * ptr, size_t len); - void read_aligned_chunk(void * dest, size_t size); + + // Positional read: does not move the file pointer, so callers need not seek first + // and two threads may read one file without fighting over a shared position. + // Returns the bytes actually read, which is short only at end of file. + // + // This is the RAW form. Under direct I/O the caller is responsible for alignment + // of all three of offset, length and the address of `ptr` - unlike read_raw(), + // which bounces through an aligned buffer and accepts anything. + // + // worker_id names a caller that reads concurrently with other callers, and it is + // what makes those callers actually concurrent on Windows. Removing the race on the + // file position was not enough: measured 2026-08-03, several threads on one handle + // reach 1.01x the throughput of a single thread at queue depth 8, while a private + // handle each reaches 2.22x. Windows serialises on the file OBJECT. + // + // Pass a dense index starting at 0 - one per thread, stable for that thread's life. + // -1, or an index the pool does not cover, reads through the shared handle: correct, + // simply serialised. Callers that do not read concurrently pass nothing. + size_t read_raw_at(void * ptr, size_t len, size_t offset, int worker_id = -1); + uint32_t read_u32(); void write_raw(const void * ptr, size_t len) const; @@ -35,6 +54,14 @@ struct llama_file { size_t read_alignment() const; bool has_direct_io() const; + + // How many private per-worker handles this file holds. 0 whenever direct I/O is not + // in effect, and 0 on POSIX, where pread on one descriptor is already parallel-safe. + // + // Exists so the pool can be asserted rather than assumed: a pool that failed to open + // and a pool that works look identical from the outside - both read the right bytes, + // one of them just never scales. + size_t direct_io_handles() const; private: struct impl; std::unique_ptr pimpl; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 419e1eba4c2c..e86ed2a582a5 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -262,6 +262,15 @@ llama_build_and_test(test-model-resolution.cpp) # the test serves its repos from an httplib server, and the library links it privately target_link_libraries(test-model-resolution PRIVATE cpp-httplib) +# Covers llama_file's read path, which nothing else does. llama_file is internal and +# the llama library does not export it - WINDOWS_EXPORT_ALL_SYMBOLS is set on common +# and on the tools, but not on llama itself, which is the same reason the block above +# is disabled on Windows. So the sources are compiled into the test rather than linked +# from the library. llama-impl.cpp comes along because llama-mmap.cpp calls format() +# and llama_log_internal() from it, and neither is exported either. +llama_build_and_test(test-llama-file.cpp ../src/llama-mmap.cpp ../src/llama-impl.cpp) +target_include_directories(test-llama-file PRIVATE ${PROJECT_SOURCE_DIR}/src) + if (NOT LLAMA_SANITIZE_ADDRESS AND NOT GGML_SCHED_NO_REALLOC) # TODO: repair known memory leaks llama_build_and_test(test-opt.cpp) diff --git a/tests/test-llama-file.cpp b/tests/test-llama-file.cpp new file mode 100644 index 000000000000..c4bdbac7f206 --- /dev/null +++ b/tests/test-llama-file.cpp @@ -0,0 +1,417 @@ +// Covers llama_file's read path, which until now no test touched at all: grepping +// tests/ for load_mode found only argument parsing (test-arg-parser.cpp), load +// cancellation (test-model-load-cancel.cpp) and quantisation statistics +// (test-quantize-stats.cpp). Nothing went red when the reading itself broke. +// +// The cases that MUST fail are the point of this file. A test that only calls +// functions and checks they return proves that they return. +// +// llama_file is internal and the llama library does not export it - see the comment +// at tests/CMakeLists.txt above the block of tests disabled on Windows for that very +// reason - so this test compiles llama-mmap.cpp in directly rather than linking it. + +#include "llama-mmap.h" + +#include +#include +#include +#include +#include +#include + +#ifdef _WIN32 +#include +#endif + +// Deliberately NOT a multiple of any plausible sector size. All four model files on +// this machine end mid-sector, so an aligned read running past EOF is the normal case +// rather than an edge one, and a test file ending on a boundary would never reach it. +static const size_t FILE_SIZE = 1024 * 1024 + 1234; +static const size_t SECTOR = 4096; + +static int g_failed = 0; + +// Whether a direct-I/O open is expected to succeed at all on this platform. +// +// llama_file implements an unbuffered path on Windows (FILE_FLAG_NO_BUFFERING) and on +// Linux (O_DIRECT). Everywhere else the constructor has nothing to open unbuffered and +// correctly falls back to buffered I/O, so failing the run there would report a defect +// where the behaviour is right. On the two platforms that do have the path, that same +// fallback is exactly the defect these cases exist to catch, so it must stay a failure +// there - skipping everywhere would turn this file into a test that cannot go red. +#if defined(_WIN32) || defined(__linux__) +# define DIRECT_IO_EXPECTED 1 +#else +# define DIRECT_IO_EXPECTED 0 +#endif + +// The buffer address must be sector-aligned for unbuffered reads, exactly as much as +// the offset and the length must be. Handing a std::vector to read_raw_at is what +// killed the first attempt at this test with 0xC0000409 and no output at all. +struct aligned_buffer { + size_t size; + uint8_t * data; + + explicit aligned_buffer(size_t raw_size) { + size = (raw_size + SECTOR - 1) & ~(SECTOR - 1); +#ifdef _WIN32 + data = (uint8_t *) _aligned_malloc(size, SECTOR); +#else + data = nullptr; + if (posix_memalign((void **) &data, SECTOR, size) != 0) { + data = nullptr; + } +#endif + if (data == nullptr) { + throw std::bad_alloc(); + } + std::memset(data, 0, size); + } + + ~aligned_buffer() { + if (data) { +#ifdef _WIN32 + _aligned_free(data); +#else + free(data); +#endif + } + } + + aligned_buffer(const aligned_buffer &) = delete; + aligned_buffer & operator=(const aligned_buffer &) = delete; +}; + +static void check(bool condition, const char * what) { + if (condition) { + printf(" ok %s\n", what); + } else { + printf(" FAIL %s\n", what); + g_failed++; + } +} + +static uint8_t pattern_byte(size_t i) { + // The first term alone repeats every 256 bytes, and 256 divides every sector size in + // play - so two sector-ALIGNED reads of the same length used to return byte-identical + // data no matter which sector they came from. Every unbuffered read in this file is + // aligned by definition, which made the pattern unable to tell one sector from + // another: a reader that ignored the offset it was given would have passed. + // + // Found on 2026-08-03 by the worker-slot case, which compared offset 2*SECTOR against + // the bytes of 5*SECTOR and found them equal. That looked like a defect in the handle + // pool and was a defect in this function. + // + // The second term changes once per sector and 17 is odd, so it is invertible modulo + // 256 and two sectors fewer than 256 apart can never carry the same bytes. + return (uint8_t) ((i * 31 + 7 + (i >> 12) * 17) & 0xFF); +} + +static bool write_test_file(const std::string & path) { + FILE * f = fopen(path.c_str(), "wb"); + if (f == nullptr) { + return false; + } + std::vector buf(FILE_SIZE); + for (size_t i = 0; i < FILE_SIZE; i++) { + buf[i] = pattern_byte(i); + } + const size_t written = fwrite(buf.data(), 1, FILE_SIZE, f); + fclose(f); + return written == FILE_SIZE; +} + +int main() { + // Unbuffered stdout, and it is not cosmetic: when this test crashed the buffered + // output was never flushed, so the failure looked like it happened before main() + // started. Every line below has to be on screen the moment it is written, or the + // next crash is just as blind as the last one. + setvbuf(stdout, NULL, _IONBF, 0); + + // A RELATIVE path on purpose. The first attempt could not derive a volume from one + // and fell back to buffered I/O without saying so, which made every case below + // silently skip while the run still reported PASS. + const std::string path = "test-llama-file.tmp"; + + if (!write_test_file(path)) { + printf("SETUP ERROR: could not write %s\n", path.c_str()); + return 2; + } + printf("test file %s, %zu bytes (%zu past the last %zu boundary)\n\n", + path.c_str(), FILE_SIZE, FILE_SIZE % SECTOR, SECTOR); + + // ----------------------------------------------------------------------------- + // Case 1: has_direct_io() must say FALSE when nobody asked for direct I/O. On + // Windows this returned a hard true on every path - it answered a question it had + // never been in a position to answer. + // ----------------------------------------------------------------------------- + printf("buffered open, use_direct_io = false:\n"); + { + llama_file f(path.c_str(), "rb"); + check(!f.has_direct_io(), + "has_direct_io() is false when direct I/O was never requested"); + check(f.read_alignment() == 1, "read_alignment() is 1 on a buffered file"); + check(f.size() == FILE_SIZE, "size() matches the file on disk"); + + aligned_buffer got(64); + const size_t n = f.read_raw_at(got.data, 64, 1000); + bool match = (n == 64); + for (size_t i = 0; i < 64 && match; i++) { + match = got.data[i] == pattern_byte(1000 + i); + } + check(match, "read_raw_at returns the bytes at the requested offset"); + + // The failing half of the same check. If a wrong expectation also passed, the + // check above would only prove that the function returns. + bool wrong_matches = true; + for (size_t i = 0; i < 64; i++) { + if (got.data[i] != pattern_byte(2000 + i)) { + wrong_matches = false; + break; + } + } + check(!wrong_matches, + "the same read does NOT match the bytes of a different offset"); + } + + // ----------------------------------------------------------------------------- + // Case 2: the direct-I/O open. Windows accepted use_direct_io and dropped it; + // FILE_FLAG_NO_BUFFERING appeared nowhere in llama-mmap.cpp. + // ----------------------------------------------------------------------------- + printf("\nunbuffered open, use_direct_io = true:\n"); + { + llama_file f(path.c_str(), "rb", true); + + if (!f.has_direct_io()) { + // Reporting PASS here would be worse than having no test: a green light for + // something nobody checked. The first version of this file did exactly that + // and hid a real defect for a whole run. On a platform without an unbuffered + // path the same state is the documented behaviour, so it is reported and + // skipped rather than counted as a failure. +#if DIRECT_IO_EXPECTED + printf(" FAIL direct I/O was requested and is not in effect\n"); + printf(" The constructor fell back to buffered I/O, so none of the\n"); + printf(" cases this test exists for were exercised.\n"); + g_failed++; +#else + printf(" SKIP this platform has no unbuffered read path in llama_file\n"); + printf(" The fallback to buffered I/O is correct here, so the\n"); + printf(" direct-I/O cases do not apply.\n"); +#endif + } else { + const size_t align = f.read_alignment(); + check(align > 1, "read_alignment() reports the device's sector size, not 1"); + printf(" alignment = %zu\n", align); + check(f.size() == FILE_SIZE, "size() matches the file on disk"); + + // An unaligned offset AND an unaligned length - exactly what direct I/O + // refuses at the syscall level. This works only if the bounce buffer does + // its job, and the destination here may be an ordinary vector: read_raw + // copies out of its own aligned buffer. + std::vector plain(777); + bool match = false; + try { + f.seek(333, SEEK_SET); + f.read_raw(plain.data(), plain.size()); + match = true; + for (size_t i = 0; i < plain.size(); i++) { + if (plain[i] != pattern_byte(333 + i)) { + match = false; + break; + } + } + } catch (const std::exception & e) { + // Catching is not politeness, it is the measurement. An uncaught C++ + // exception ends in MSVC's abort(), which raises + // __fastfail(FAST_FAIL_FATAL_APP_EXIT) - reported as 0xC0000409, the + // same code a stack buffer overrun produces. Without this handler the + // failure is indistinguishable from memory corruption, and that is + // exactly how it was misread once already. + printf(" EXCEPTION during unaligned read_raw: %s\n", e.what()); + } + check(match, "read_raw handles an unaligned offset and length"); + + // ------------------------------------------------------------------- + // Case 3: the read whose aligned end lies past the logical EOF. + // Measured 2026-08-03 on all four model files: ReadFile returns TRUE and + // reports exactly the bytes up to EOF. This pins that behaviour so a + // future change cannot quietly turn it into a throw or a full count. + // + // Offset, length and buffer address are all sector-aligned here. This is + // the raw positional read; it does not bounce. + // ------------------------------------------------------------------- + const size_t last_aligned = (FILE_SIZE / SECTOR) * SECTOR; + aligned_buffer tail(SECTOR); + const size_t n = f.read_raw_at(tail.data, SECTOR, last_aligned); + + check(n == FILE_SIZE - last_aligned, + "a read past EOF reports exactly the bytes up to the end of file"); + printf(" offset %zu, asked %zu, got %zu (file ends %zu in)\n", + last_aligned, SECTOR, n, FILE_SIZE - last_aligned); + + // The failing half: it must NOT report the full request. If it did, the + // caller would copy uninitialised bytes into a tensor and never know. + check(n != SECTOR, + "it does NOT report the full request when the file ends first"); + + bool tail_match = (n == FILE_SIZE - last_aligned); + for (size_t i = 0; i < n && tail_match; i++) { + tail_match = tail.data[i] == pattern_byte(last_aligned + i); + } + check(tail_match, "the bytes it did return are the right ones"); + + // Wholly past the end, and the offset is sector-aligned like every other + // unbuffered read - an unaligned one here is what took the first attempt + // down together with the unaligned buffer. + const size_t beyond_offset = last_aligned + 4 * SECTOR; + aligned_buffer beyond(SECTOR); + const size_t none = f.read_raw_at(beyond.data, SECTOR, beyond_offset); + check(none == 0, "a read starting entirely past EOF returns 0 bytes"); + } + } + + // ----------------------------------------------------------------------------- + // Case 4: the per-worker handle pool. + // + // This is the one case whose absence cannot be seen in the bytes. A pool that never + // opened reads exactly what a working pool reads - it just serialises every thread + // behind one file object, which is what was measured at 1.01x against 2.22x on + // 2026-08-03. So the pool is asserted by COUNT first and by behaviour second; + // checking only that the reads come back right would pass on the old code. + // ----------------------------------------------------------------------------- + printf("\nper-worker handle pool:\n"); + { + llama_file buffered(path.c_str(), "rb"); + check(buffered.direct_io_handles() == 0, + "a buffered file holds no private handles"); + + llama_file f(path.c_str(), "rb", true); + if (!f.has_direct_io()) { +#if DIRECT_IO_EXPECTED + printf(" FAIL direct I/O not in effect, the pool cannot be checked\n"); + g_failed++; +#else + printf(" SKIP no unbuffered read path on this platform, no pool to check\n"); +#endif + } else { + const size_t n_handles = f.direct_io_handles(); + printf(" private handles = %zu\n", n_handles); + + // The failing case for this stage. On the build before the pool this is 0, + // and the whole section below would otherwise still be green. + check(n_handles > 1, + "direct I/O opened more than one handle on the file"); + + // Every slot must read the file, not just slot 0. An off-by-one in the bounds + // check would leave the last worker silently on the shared handle - correct + // bytes, no concurrency, and nothing to see. + bool all_slots_ok = true; + for (size_t slot = 0; slot < n_handles; slot++) { + aligned_buffer b(SECTOR); + const size_t n = f.read_raw_at(b.data, SECTOR, 2 * SECTOR, (int) slot); + if (n != SECTOR) { + all_slots_ok = false; + break; + } + for (size_t i = 0; i < SECTOR; i++) { + if (b.data[i] != pattern_byte(2 * SECTOR + i)) { + all_slots_ok = false; + break; + } + } + if (!all_slots_ok) { + break; + } + } + check(all_slots_ok, + "every worker slot returns the bytes at the requested offset"); + + // The failing half again: a slot that read the right bytes must not also + // match a different offset, or the loop above proves only that it returned. + aligned_buffer probe(SECTOR); + f.read_raw_at(probe.data, SECTOR, 2 * SECTOR, 0); + bool wrong_matches = true; + for (size_t i = 0; i < SECTOR; i++) { + if (probe.data[i] != pattern_byte(5 * SECTOR + i)) { + wrong_matches = false; + break; + } + } + check(!wrong_matches, + "a worker-slot read does NOT match the bytes of a different offset"); + + // Out of range in both directions. A caller that knows nothing about pools + // must land on the shared handle rather than past the array - serialised is + // an acceptable answer here, reading somebody else's memory is not. + bool oob_ok = true; + for (int bad : { -5, (int) n_handles, 999 }) { + aligned_buffer b(SECTOR); + const size_t n = f.read_raw_at(b.data, SECTOR, 3 * SECTOR, bad); + if (n != SECTOR) { + oob_ok = false; + break; + } + for (size_t i = 0; i < SECTOR; i++) { + if (b.data[i] != pattern_byte(3 * SECTOR + i)) { + oob_ok = false; + break; + } + } + if (!oob_ok) { + break; + } + } + check(oob_ok, + "an out-of-range worker id falls back to the shared handle and still reads"); + + // All slots at once. This does not measure throughput - the file is a + // megabyte and lives in RAM by now - it checks that nothing is shared that + // should not be. If the OVERLAPPED structure or the byte counter were held + // per FILE instead of per call, this is where the offsets would cross. + const size_t n_threads = n_handles < 8 ? n_handles : 8; + std::vector ok((size_t) n_threads, 1); + std::vector ts; + for (size_t t = 0; t < n_threads; t++) { + ts.emplace_back([&f, &ok, t]() { + // Each thread reads its OWN offset. Reading the same one everywhere + // would pass even if every thread ignored the offset it was given. + const size_t off = (t + 4) * SECTOR; + for (int rep = 0; rep < 16; rep++) { + aligned_buffer b(SECTOR); + if (f.read_raw_at(b.data, SECTOR, off, (int) t) != SECTOR) { + ok[t] = 0; + return; + } + for (size_t i = 0; i < SECTOR; i++) { + if (b.data[i] != pattern_byte(off + i)) { + ok[t] = 0; + return; + } + } + } + }); + } + for (auto & t : ts) { + t.join(); + } + bool concurrent_ok = true; + for (size_t t = 0; t < n_threads; t++) { + if (!ok[t]) { + concurrent_ok = false; + } + } + check(concurrent_ok, + "all slots read their own offsets correctly at the same time"); + } + } + + remove(path.c_str()); + + printf("\n"); + if (g_failed != 0) { + printf("RESULT: FAIL - %d check(s) failed\n", g_failed); + return 1; + } + printf("RESULT: PASS - all checks behaved as required\n"); + return 0; +}