From 66f40bc010f4a95b3cfc17903d65467389b32b6e Mon Sep 17 00:00:00 2001 From: nibor1896 <56588205+nibor1896@users.noreply.github.com> Date: Tue, 4 Aug 2026 00:02:30 +0200 Subject: [PATCH] llama : add unbuffered positional read on Windows Problem: On Windows, llama.cpp cannot do "read from byte X" - only "read from wherever you currently are". When several threads want to read from the same file, they get in each other's way: they keep moving each other's position. The code implied it could read unbuffered: it takes the flag, then does nothing with it, answers "yes" when asked, and writes "read unbuffered" to the log. What I built together with Claude: Exactly what was already promised. llama.cpp can now jump to any point (byte X) and read unbuffered, and every thread gets its own handle, so they no longer interfere. The other six pieces are not features, they are dependencies this needs. Shared handle at depth 8: 1.01x One handle per thread: 2.22x Built with Claude Code (AI) Text written by me (Robin - Human) --- src/llama-mmap.cpp | 454 ++++++++++++++++++++++++++++++++++++-- src/llama-mmap.h | 29 ++- tests/CMakeLists.txt | 9 + tests/test-llama-file.cpp | 417 ++++++++++++++++++++++++++++++++++ 4 files changed, 895 insertions(+), 14 deletions(-) create mode 100644 tests/test-llama-file.cpp 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; +}