diff --git a/autosave.h b/autosave.h index 12324b819f..fd34b8c3eb 100644 --- a/autosave.h +++ b/autosave.h @@ -23,20 +23,14 @@ RETRO_BEGIN_DECLS -/** - * autosave_lock: - * - * Lock autosave. - **/ +/* Main-thread SRAM scopes may nest; the outermost unlock publishes + * a requested snapshot after core execution and post-frame updates. */ void autosave_lock(void); - -/** - * autosave_unlock: - * - * Unlocks autosave. - **/ void autosave_unlock(void); +/* Service requests on the main thread even when no core frames run. */ +void autosave_check(void); + bool autosave_init(bool compress_files, unsigned autosave_interval); void autosave_deinit(void); diff --git a/runloop.c b/runloop.c index c4dc4c4783..4bda4afc4f 100644 --- a/runloop.c +++ b/runloop.c @@ -8474,6 +8474,11 @@ int runloop_iterate(void) #endif } +#ifdef HAVE_THREADS + if (runloop_st->flags & RUNLOOP_FLAG_AUTOSAVE) + autosave_check(); +#endif + switch ((enum runloop_state_enum)runloop_check_state( input_st, audio_st, video_st, uico_st, diff --git a/save.c b/save.c index 6d852dfbac..ee21d3a9d1 100644 --- a/save.c +++ b/save.c @@ -61,157 +61,55 @@ struct autosave_st { autosave_t **list; unsigned num; -}; - -enum autosave_flags -{ - AUTOSAVE_FLAG_COMPRESS_FILES = (1 << 1), - AUTOSAVE_FLAG_DIRTY = (1 << 2) + unsigned depth; }; struct autosave { - /* The core's frame, published rather than locked: the main thread - * makes this odd before retro_run() and even after the post-core - * work, with a plain release store - no mutex, no read-modify-write - * - and the worker reads it either side of its snapshot. A snapshot - * that overlaps a frame may be torn, and the sequence is how that - * is noticed; it is dropped and retried. The main thread used to - * take every handle's mutex around retro_run(): a lock held across - * core execution, whose cache line moved between the emulation - * thread and the storage worker every frame. */ - retro_atomic_size_t frame_seq; void *buffer; + void *snapshot; const void *retro_buffer; char *path; - slock_t *lock; slock_t *cond_lock; scond_t *cond; sthread_t *thread; size_t bufsize; unsigned interval; - /* Guarded by 'lock'. AUTOSAVE_FLAG_QUIT is deliberately NOT kept - * here: it is guarded by cond_lock, and two locks protecting - * different bits of one byte do not protect the byte -- the - * read-modify-write covers all of it. */ - uint8_t flags; - /* Guarded by cond_lock. */ - uint8_t quit; + /* Only the main thread reads live SRAM. The worker requests a copy + * and waits until autosave_unlock() publishes it under cond_lock. */ + retro_atomic_int_t snapshot_requested; + bool compress; + bool quit; }; static struct autosave_st autosave_state; - -/** - * autosave_thread: - * @data : pointer to autosave object - * - * Callback function for (threaded) autosave. - * - * Performance notes: - * - The dirty flag allows an early-out when the core - * has not touched SRAM since last check, avoiding a - * full memcmp on every wake-up. - * - When a comparison is needed we scan word-at-a-time - * to locate the first differing region, then only - * memcmp/memcpy from that point onward. - * - The file write happens entirely outside the lock, - * so the core is never stalled on disk I/O. - **/ static void autosave_thread(void *data) { autosave_t *save = (autosave_t*)data; + bool retry_write = false; - for (;;) + slock_lock(save->cond_lock); + while (!save->quit) { - bool differ = false; - bool compress = false; - size_t seq_before = 0; + retro_atomic_store_release_int(&save->snapshot_requested, 1); + while (!save->quit && + retro_atomic_load_relaxed_int(&save->snapshot_requested)) + scond_wait(save->cond, save->cond_lock); - /* Taken outside the frame, without the main thread's help: read - * the sequence, take the snapshot, read it again. Odd means the - * core is running now; a change means a frame began or ended - * while it was taken. */ - seq_before = retro_atomic_load_acquire_size(&save->frame_seq); + if (save->quit) + break; + slock_unlock(save->cond_lock); - slock_lock(save->lock); - - /* Fast path: if the core hasn't signalled a write - * since our last check, skip the expensive memcmp. - * Falls back to full comparison if the dirty flag - * was never set (conservative default). */ - if (!(seq_before & 1) && (save->flags & AUTOSAVE_FLAG_DIRTY)) - { - const size_t word_size = sizeof(size_t); - const size_t aligned = save->bufsize / word_size; - const size_t remainder = save->bufsize % word_size; - const size_t *src_w = (const size_t*)save->retro_buffer; - const size_t *dst_w = (const size_t*)save->buffer; - size_t offset = 0; - size_t i; - - /* Word-at-a-time scan to find first difference. - * Avoids touching the entire buffer when only - * a small region changed (common for SRAM). */ - for (i = 0; i < aligned; i++) - { - if (src_w[i] != dst_w[i]) - { - differ = true; - offset = i * word_size; - break; - } - } - - /* Check trailing bytes if no word-level diff found */ - if (!differ && remainder > 0) - { - const unsigned char *src_b = - (const unsigned char*)save->retro_buffer + aligned * word_size; - const unsigned char *dst_b = - (const unsigned char*)save->buffer + aligned * word_size; - if (memcmp(dst_b, src_b, remainder) != 0) - { - differ = true; - offset = aligned * word_size; - } - } - - /* Only copy from first difference onward */ - if (differ) - memcpy((unsigned char*)save->buffer + offset, - (const unsigned char*)save->retro_buffer + offset, - save->bufsize - offset); - - /* Clear dirty flag regardless — we've checked */ - save->flags &= ~AUTOSAVE_FLAG_DIRTY; - } - - /* COMPRESS_FILES never changes after autosave_new(), but it - * shares the byte with DIRTY, which the main thread sets under - * this lock. Sample it here rather than reading save->flags - * again once the lock is dropped. */ - compress = (save->flags & AUTOSAVE_FLAG_COMPRESS_FILES) != 0; - - slock_unlock(save->lock); - - /* A frame began or ended while the snapshot was taken: part of - * it may be from before that frame and part from after. Drop it - * and leave the buffer dirty, so the next interval retries; the - * file is only ever written from a snapshot known whole. */ - if (differ && retro_atomic_load_acquire_size(&save->frame_seq) != seq_before) - { - differ = false; - slock_lock(save->lock); - save->flags |= AUTOSAVE_FLAG_DIRTY; - slock_unlock(save->lock); - } - - if (differ) + /* The snapshot remains ours until the next request. Keep the + * last written image unchanged if opening or writing fails. */ + if (retry_write || + memcmp(save->buffer, save->snapshot, save->bufsize) != 0) { intfstream_t *file = NULL; + retry_write = true; - if (compress) + if (save->compress) file = intfstream_open_rzip_file(save->path, RETRO_VFS_FILE_ACCESS_WRITE); else @@ -220,32 +118,32 @@ static void autosave_thread(void *data) if (file) { - intfstream_write(file, save->buffer, save->bufsize); - intfstream_flush(file); - intfstream_close(file); + bool written = intfstream_write(file, save->snapshot, + save->bufsize) == (int64_t)save->bufsize; + if (intfstream_flush(file) != 0) + written = false; + if (intfstream_close(file) != 0) + written = false; free(file); + if (written) + { + memcpy(save->buffer, save->snapshot, save->bufsize); + retry_write = false; + } } } slock_lock(save->cond_lock); - - if (save->quit) - { - slock_unlock(save->cond_lock); - break; - } - - scond_wait_timeout(save->cond, - save->cond_lock, + if (!save->quit) + scond_wait_timeout(save->cond, save->cond_lock, #if defined(_MSC_VER) && _MSC_VER <= 1200 - save->interval * 1000000 + save->interval * 1000000 #else - save->interval * 1000000LL + save->interval * 1000000LL #endif - ); - - slock_unlock(save->cond_lock); + ); } + slock_unlock(save->cond_lock); } /** @@ -269,18 +167,17 @@ static autosave_t *autosave_new(const char *path, if (!handle) return NULL; - handle->flags = AUTOSAVE_FLAG_DIRTY; - handle->quit = 0; + handle->compress = compress; + handle->quit = false; + retro_atomic_int_init(&handle->snapshot_requested, 0); handle->bufsize = len; handle->interval = interval; handle->buffer = NULL; - handle->lock = NULL; + handle->snapshot = NULL; handle->cond_lock = NULL; handle->cond = NULL; handle->thread = NULL; - if (compress) - handle->flags |= AUTOSAVE_FLAG_COMPRESS_FILES; handle->retro_buffer = data; /* Own the path string rather than borrowing it. The caller's * path comes from task_save_files->elems[i].data, freed by @@ -306,15 +203,14 @@ static autosave_t *autosave_new(const char *path, memcpy(handle->buffer, handle->retro_buffer, handle->bufsize); - handle->lock = slock_new(); + handle->snapshot = malloc(len); handle->cond_lock = slock_new(); handle->cond = scond_new(); - if (!handle->lock || !handle->cond_lock || !handle->cond) + if (!handle->snapshot || !handle->cond_lock || !handle->cond) { RARCH_ERR("[SRAM] Failed to initialize autosave synchronization primitives.\n"); - if (handle->lock) - slock_free(handle->lock); + free(handle->snapshot); if (handle->cond_lock) slock_free(handle->cond_lock); if (handle->cond) @@ -330,7 +226,7 @@ static autosave_t *autosave_new(const char *path, if (!handle->thread) { RARCH_ERR("[SRAM] Failed to create autosave thread.\n"); - slock_free(handle->lock); + free(handle->snapshot); slock_free(handle->cond_lock); scond_free(handle->cond); free(handle->path); @@ -351,12 +247,12 @@ static autosave_t *autosave_new(const char *path, static void autosave_free(autosave_t *handle) { slock_lock(handle->cond_lock); - handle->quit = 1; + handle->quit = true; slock_unlock(handle->cond_lock); scond_signal(handle->cond); sthread_join(handle->thread); - slock_free(handle->lock); + free(handle->snapshot); slock_free(handle->cond_lock); scond_free(handle->cond); @@ -435,54 +331,42 @@ void autosave_deinit(void) autosave_state.num = 0; } -/** - * autosave_lock: - * - * Lock autosave. - **/ void autosave_lock(void) +{ + autosave_state.depth++; +} + +void autosave_check(void) { unsigned i; + if (autosave_state.depth) + return; + for (i = 0; i < autosave_state.num; i++) { autosave_t *handle = autosave_state.list[i]; - if (handle) + if (!handle) + continue; +#ifdef RETRO_ATOMIC_LOCK_FREE + if (!retro_atomic_load_acquire_int(&handle->snapshot_requested)) + continue; +#endif + slock_lock(handle->cond_lock); + if (retro_atomic_load_relaxed_int(&handle->snapshot_requested)) { - /* Odd: the core is running, and any snapshot overlapping - * this window is discarded by the worker. */ - size_t seq = retro_atomic_load_acquire_size(&handle->frame_seq); - retro_atomic_store_release_size(&handle->frame_seq, seq + 1); + memcpy(handle->snapshot, handle->retro_buffer, handle->bufsize); + retro_atomic_store_release_int(&handle->snapshot_requested, 0); + scond_signal(handle->cond); } + slock_unlock(handle->cond_lock); } } -/** - * autosave_unlock: - * - * Unlocks autosave. - * Also marks all buffers as dirty, since the core - * may have written to SRAM while holding the lock. - **/ void autosave_unlock(void) { - unsigned i; - - for (i = 0; i < autosave_state.num; i++) - { - autosave_t *handle = autosave_state.list[i]; - if (handle) - { - /* Even: the frame is over and the buffer is stable until the - * next one begins. The dirty bit is set here without the - * mutex - the worker only clears it, and a lost update costs - * one more comparison, never a missed save: the write below - * is driven by the comparison, not by the bit. */ - size_t seq = retro_atomic_load_acquire_size(&handle->frame_seq); - handle->flags |= AUTOSAVE_FLAG_DIRTY; - retro_atomic_store_release_size(&handle->frame_seq, seq + 1); - } - } + if (autosave_state.depth && !--autosave_state.depth) + autosave_check(); } #endif diff --git a/tests-other/Makefile.autosave_snapshot b/tests-other/Makefile.autosave_snapshot new file mode 100644 index 0000000000..6865397bc2 --- /dev/null +++ b/tests-other/Makefile.autosave_snapshot @@ -0,0 +1,33 @@ +CC ?= cc +ROOT := .. +TARGET := autosave_snapshot_test +SOURCES := autosave_snapshot_test.c $(ROOT)/libretro-common/rthreads/rthreads.c \ + $(ROOT)/libretro-common/time/rtime.c +CFLAGS += -std=gnu89 -Wall -Wextra -g -O1 -DHAVE_THREADS \ + -ffunction-sections -fdata-sections \ + -I$(ROOT) -I$(ROOT)/libretro-common/include +LDFLAGS += -Wl,--gc-sections +LDLIBS += -lpthread + +# PE linkers retain unrelated external save.c functions without LTO. +ifneq ($(filter MSYS% MINGW% CYGWIN%,$(shell uname -s)),) +CFLAGS += -flto +endif + +ifneq ($(SANITIZER),) +CFLAGS += -fsanitize=$(SANITIZER) -fno-omit-frame-pointer +LDFLAGS += -fsanitize=$(SANITIZER) +endif + +all: $(TARGET) + +$(TARGET): $(SOURCES) $(ROOT)/save.c $(ROOT)/autosave.h + $(CC) $(CFLAGS) $(SOURCES) $(LDFLAGS) $(LDLIBS) -o $@ + +check: $(TARGET) + ./$(TARGET) + +clean: + rm -f $(TARGET) $(TARGET).exe + +.PHONY: all check clean diff --git a/tests-other/autosave_snapshot_test.c b/tests-other/autosave_snapshot_test.c new file mode 100644 index 0000000000..c5433b1d48 --- /dev/null +++ b/tests-other/autosave_snapshot_test.c @@ -0,0 +1,237 @@ +/* Build with function sections and --gc-sections: only the autosave + * portion of save.c is linked. Stream stubs control I/O failures. */ +#include +#include +#include +#include "../save.c" + +#define SRAM_SIZE 17 + +static slock_t *io_lock; +static scond_t *io_cond; +static unsigned opens; +static unsigned writes; +static unsigned compressed_opens; +static bool hold_write; +static unsigned fail_stage; +static unsigned char disk_image[SRAM_SIZE]; +static autosave_t *current; +static unsigned char ram_storage[SRAM_SIZE + 1]; +static unsigned char *ram = ram_storage + 1; + +void RARCH_ERR(const char *fmt, ...) +{ + (void)fmt; +} + +intfstream_t *intfstream_open_file(const char *path, + unsigned mode, unsigned hints) +{ + bool fail; + (void)path; + (void)mode; + (void)hints; + slock_lock(io_lock); + opens++; + fail = fail_stage == 1; + slock_unlock(io_lock); + return fail ? NULL : (intfstream_t*)malloc(1); +} + +intfstream_t *intfstream_open_rzip_file(const char *path, unsigned mode) +{ + slock_lock(io_lock); + compressed_opens++; + slock_unlock(io_lock); + return intfstream_open_file(path, mode, 0); +} + +int64_t intfstream_write(intfstream_t *file, const void *data, uint64_t len) +{ + bool fail; + (void)file; + assert(len == SRAM_SIZE); + slock_lock(io_lock); + writes++; + scond_signal(io_cond); + while (hold_write) + scond_wait(io_cond, io_lock); + memcpy(disk_image, data, SRAM_SIZE); + fail = fail_stage == 2; + slock_unlock(io_lock); + return fail ? (int64_t)len - 1 : (int64_t)len; +} + +int intfstream_flush(intfstream_t *file) +{ + int result; + (void)file; + slock_lock(io_lock); + result = fail_stage == 3 ? -1 : 0; + slock_unlock(io_lock); + return result; +} + +int intfstream_close(intfstream_t *file) +{ + int result; + (void)file; + slock_lock(io_lock); + result = fail_stage == 4 ? -1 : 0; + slock_unlock(io_lock); + return result; +} + +static void wait_requested(void) +{ + unsigned i; + for (i = 0; i < 5000; i++) + { + bool requested; + slock_lock(current->cond_lock); + requested = retro_atomic_load_relaxed_int( + ¤t->snapshot_requested) != 0; + slock_unlock(current->cond_lock); + if (requested) + return; + retro_sleep(1); + } + assert(!"snapshot request timed out"); +} + +static void capture_and_wait(void) +{ + autosave_lock(); + autosave_unlock(); + wait_requested(); +} + +static void start_save(bool compress) +{ + memset(ram, 0, SRAM_SIZE); + current = autosave_new("unused.srm", ram, SRAM_SIZE, 0, compress); + assert(current); + autosave_state.list = ¤t; + autosave_state.num = 1; + wait_requested(); +} + +static void stop_save(void) +{ + /* No further frames: shutdown must wake a worker awaiting a copy. */ + autosave_free(current); + current = NULL; + autosave_state.list = NULL; + autosave_state.num = 0; +} + +int main(void) +{ + unsigned i; + unsigned before; + io_lock = slock_new(); + io_cond = scond_new(); + assert(io_lock && io_cond); + start_save(false); + + /* Netplay may nest an SRAM scope inside the runloop's frame. */ + autosave_lock(); + autosave_lock(); + autosave_check(); + autosave_unlock(); + assert(retro_atomic_load_acquire_int(¤t->snapshot_requested)); + autosave_unlock(); + wait_requested(); + + capture_and_wait(); + assert(opens == 0); + + /* Paused/menu iterations service requests without a core frame. */ + memset(ram, 8, SRAM_SIZE); + autosave_check(); + wait_requested(); + assert(memcmp(disk_image, ram, SRAM_SIZE) == 0); + slock_lock(io_lock); + writes = 0; + slock_unlock(io_lock); + + slock_lock(io_lock); + hold_write = true; + slock_unlock(io_lock); + memset(ram, 1, SRAM_SIZE); + autosave_lock(); + autosave_unlock(); + slock_lock(io_lock); + for (i = 0; !writes && i < 5; i++) + scond_wait_timeout(io_cond, io_lock, 1000000); + assert(writes == 1); + slock_unlock(io_lock); + + /* A slow disk must neither stall frames nor observe subsequent SRAM. */ + for (i = 0; i < 100; i++) + { + memset(ram, 2, SRAM_SIZE); + autosave_lock(); + autosave_unlock(); + } + slock_lock(io_lock); + hold_write = false; + scond_signal(io_cond); + slock_unlock(io_lock); + wait_requested(); + for (i = 0; i < SRAM_SIZE; i++) + assert(disk_image[i] == 1); + capture_and_wait(); + assert(memcmp(disk_image, ram, SRAM_SIZE) == 0); + before = opens; + capture_and_wait(); + assert(opens == before); + + /* An unchanged image must be retried after any I/O failure. */ + for (i = 1; i <= 4; i++) + { + memset(ram, (int)i + 2, SRAM_SIZE); + slock_lock(io_lock); + fail_stage = i; + before = opens; + slock_unlock(io_lock); + capture_and_wait(); + assert(opens == before + 1); + assert(memcmp(current->buffer, ram, SRAM_SIZE) != 0); + slock_lock(io_lock); + fail_stage = 0; + slock_unlock(io_lock); + capture_and_wait(); + assert(opens == before + 2); + assert(memcmp(current->buffer, ram, SRAM_SIZE) == 0); + } + /* A partial write also needs repair if SRAM reverts to the last + * successfully saved image before the next request. */ + memset(ram, 9, SRAM_SIZE); + slock_lock(io_lock); + fail_stage = 2; + before = opens; + slock_unlock(io_lock); + capture_and_wait(); + memcpy(ram, current->buffer, SRAM_SIZE); + slock_lock(io_lock); + fail_stage = 0; + slock_unlock(io_lock); + capture_and_wait(); + assert(opens == before + 2); + assert(memcmp(disk_image, ram, SRAM_SIZE) == 0); + stop_save(); + + start_save(true); + memset(ram, 7, SRAM_SIZE); + capture_and_wait(); + assert(compressed_opens == 1); + stop_save(); + /* Shutdown is also safe before the first snapshot is published. */ + start_save(false); + stop_save(); + scond_free(io_cond); + slock_free(io_lock); + puts("autosave snapshot tests passed"); + return 0; +}