From f11a552e01a596d7589f89c7095717d35e8b3580 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 02:05:14 +0000 Subject: [PATCH 1/2] Render lock: keep patch loads out of an in-progress render A patch load settles due deltas on the sending thread (flush_due_deltas before patches_load_patch, so a queued reset lands before the load rebuilds the synth tables). That flush can free oscs -- a released voice's FREE_OSC, or amy_reset_oscs() for a reset -- while the render thread is reading them, because rendering never held a lock. Desktop AddressSanitizer stress (one thread rendering, another reloading synth 1 back to back) crashes in amy_render/render_*/hold_and_modify on main in about 6 runs of 10. Add a second lock, the render lock, taken before the queue lock: - the render thread holds it for a whole block: flush, render, mix (amy_simple_fill_buffer, and the ESP fill task and single-thread path; released before the i2s write, so a load gets in while we wait on DMA); - a patch load holds it from its flush to the end of patches_load_patch, which also keeps a reset run by the render thread's flush from landing halfway through the load's bookkeeping or its clone-on-grow snapshot; - amy_execute_deltas takes it too, covering callers outside a render loop. Ordinary events only take the queue lock, so note-ons never wait for a render. The render lock is recursive per thread via a thread-local depth (a patch string can load a patch: drum kits open with `if3iv1in38Z`), and each platform's lock primitive is now defined once and used for both locks. Not covered: Pico/Teensy have no lock implementation (unchanged no-ops), and tulipcc's native Tulip/AMYboard render loops need the same wrap. Stress test, 10 runs x 3000 reloads under ASan: main 4 clean / 6 render crashes; this change 10 clean. make ctest passes; make test gives the same 90 pass / 43 fail as main here. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01UW4jqUQ4Xc1sdoqzUPL2YV --- src/amy.c | 133 ++++++++++++++++++++++++++++++++++++------------------ src/amy.h | 4 ++ src/api.c | 7 ++- src/i2s.c | 11 ++++- 4 files changed, 110 insertions(+), 45 deletions(-) diff --git a/src/amy.c b/src/amy.c index 5962a525..db782b6d 100644 --- a/src/amy.c +++ b/src/amy.c @@ -92,70 +92,102 @@ void amy_profiles_print() {} #include "clipping_lookup_table.h" -// Set up the mutex for accessing the queue during rendering (for multicore) +// Two locks, always taken in this order when both are needed: +// +// render lock held by the render thread for a whole block (flush, render, +// mix), and by an ingest thread across a patch load (its flush +// plus patches_load_patch's bookkeeping). So a load's frees and +// resets can't run while a render is reading oscs, and a queued +// reset can't wipe the synth tables halfway through a load. +// Recursive for its owner: a patch string can load a patch +// (drum kits open with `if3iv1in38Z`), and a render-thread hook +// or sequenced message can send one mid-block. +// queue lock guards the delta queue itself (add_delta_to_queue, the +// flush), held briefly. Ordinary events take only this one, so +// a note-on never waits for a render. +// +// Each platform supplies a plain lock; the recursion is built on top, below, +// from a per-thread depth count. + +#if defined(_MSC_VER) +#define AMY_TLS __declspec(thread) +#else +#define AMY_TLS _Thread_local +#endif #ifdef __EMSCRIPTEN__ #include #include -emscripten_lock_t amy_queue_lock = EMSCRIPTEN_LOCK_T_STATIC_INITIALIZER; -void amy_grab_lock() { - emscripten_lock_busyspin_wait_acquire(&amy_queue_lock, 100); -} -void amy_release_lock() { - emscripten_lock_release(&amy_queue_lock); -} -void amy_init_lock() { -} +typedef emscripten_lock_t amy_lock_t; +static void lock_init(amy_lock_t *l) { emscripten_lock_init(l); } +static void lock_take(amy_lock_t *l) { emscripten_lock_busyspin_wait_acquire(l, 100); } +static void lock_give(amy_lock_t *l) { emscripten_lock_release(l); } +#define AMY_THREAD_LOCAL AMY_TLS #elif defined _WIN32 -CRITICAL_SECTION amy_queue_lock; -void amy_grab_lock() { - EnterCriticalSection(&amy_queue_lock); -} -void amy_release_lock() { - LeaveCriticalSection(&amy_queue_lock); -} -void amy_init_lock() { - InitializeCriticalSection(&amy_queue_lock); -} +typedef CRITICAL_SECTION amy_lock_t; +static void lock_init(amy_lock_t *l) { InitializeCriticalSection(l); } +static void lock_take(amy_lock_t *l) { EnterCriticalSection(l); } +static void lock_give(amy_lock_t *l) { LeaveCriticalSection(l); } +#define AMY_THREAD_LOCAL AMY_TLS + #elif defined _POSIX_THREADS -pthread_mutex_t amy_queue_lock; -void amy_grab_lock() { - pthread_mutex_lock(&amy_queue_lock); -} -void amy_release_lock() { - pthread_mutex_unlock(&amy_queue_lock); -} -void amy_init_lock() { - pthread_mutex_init(&amy_queue_lock, NULL); -} -#elif defined ESP_PLATFORM +typedef pthread_mutex_t amy_lock_t; +static void lock_init(amy_lock_t *l) { pthread_mutex_init(l, NULL); } +static void lock_take(amy_lock_t *l) { pthread_mutex_lock(l); } +static void lock_give(amy_lock_t *l) { pthread_mutex_unlock(l); } +#define AMY_THREAD_LOCAL AMY_TLS +#elif defined ESP_PLATFORM #include "freertos/FreeRTOS.h" #include "freertos/task.h" #include "freertos/semphr.h" -SemaphoreHandle_t amy_queue_lock; +// A FreeRTOS mutex, so a low-priority task holding it is boosted while the +// render task waits on it. +typedef SemaphoreHandle_t amy_lock_t; +static void lock_init(amy_lock_t *l) { *l = xSemaphoreCreateMutex(); } +static void lock_take(amy_lock_t *l) { xSemaphoreTake(*l, portMAX_DELAY); } +static void lock_give(amy_lock_t *l) { xSemaphoreGive(*l); } +#define AMY_THREAD_LOCAL AMY_TLS + +#else +// Single-threaded (or not yet locked) platforms. +typedef int amy_lock_t; +static void lock_init(amy_lock_t *l) { (void)l; } +static void lock_take(amy_lock_t *l) { (void)l; } +static void lock_give(amy_lock_t *l) { (void)l; } +// No threads to tell apart, and no promise of thread-local storage. +#define AMY_THREAD_LOCAL +#endif + +amy_lock_t amy_queue_lock; // extern in amy.h on Windows and POSIX +static amy_lock_t amy_render_lock; +// How deep this thread is in the render lock. Thread-local, so each thread +// sees only its own nesting and nothing is shared: > 0 means this thread +// holds the lock. +static AMY_THREAD_LOCAL int render_lock_depth = 0; void amy_grab_lock() { - xSemaphoreTake(amy_queue_lock, portMAX_DELAY); + lock_take(&amy_queue_lock); } void amy_release_lock() { - xSemaphoreGive( amy_queue_lock ); + lock_give(&amy_queue_lock); } -void amy_init_lock() { - amy_queue_lock = xSemaphoreCreateMutex(); -} -#else -void amy_grab_lock() { +void amy_grab_render_lock() { + if (render_lock_depth++ > 0) return; // already ours + lock_take(&amy_render_lock); } -void amy_release_lock() { +void amy_release_render_lock() { + if (--render_lock_depth == 0) + lock_give(&amy_render_lock); } + void amy_init_lock() { + lock_init(&amy_queue_lock); + lock_init(&amy_render_lock); } -#endif - // Global state @@ -851,9 +883,18 @@ void amy_event_to_deltas_queue(amy_event *e, uint16_t base_osc, uint16_t oscs_pe if (AMY_IS_SET(e->patch_number) || AMY_IS_SET(e->num_voices) || AMY_IS_SET(e->oscs_per_voice)) { // Settle pending deltas without running the sequencer tick // service - this can execute on any sending thread (see - // flush_due_deltas). + // flush_due_deltas). A queued reset has to land before the load + // rebuilds the synth tables, or it wipes them afterwards. + // Under the render lock, from the flush to the end of the load: + // the flush can free oscs (a released voice, a reset) that a + // render in progress is reading, and the load's bookkeeping + // (osc_to_voice, instruments, the clone-on-grow snapshot of + // synth[]) must not interleave with a reset the render thread's + // own flush runs. + amy_grab_render_lock(); flush_due_deltas(); patches_load_patch(e); + amy_release_render_lock(); } // Execute any other commands in this event. patches_event_has_voices(e, queue); @@ -2427,12 +2468,18 @@ static void flush_due_deltas() { // this takes scheduled deltas and plays them at the right time void amy_execute_deltas() { AMY_PROFILE_START(AMY_EXECUTE_DELTAS) + // Under the render lock, so a patch load on another thread can't be + // halfway through its bookkeeping while this flush runs a reset. Render + // loops already hold it across the whole block (it's recursive for them); + // this covers callers that execute deltas on their own. + amy_grab_render_lock(); // Advance the sequencer on AMY (sample) time and play any due sequence // events, so sequencing works in any rendering context, real-time or not. sequencer_check_and_fill(); // Make sure any CV-triggered events are added to delta queue update_external_cv_in(); flush_due_deltas(); + amy_release_render_lock(); AMY_PROFILE_STOP(AMY_EXECUTE_DELTAS) } diff --git a/src/amy.h b/src/amy.h index d1aab458..2f65e7fe 100644 --- a/src/amy.h +++ b/src/amy.h @@ -1143,6 +1143,10 @@ int8_t global_init(amy_config_t c); void global_deinit(); void amy_grab_lock(); void amy_release_lock(); +// Held by the render thread across each block, and by an ingest thread across +// a patch load; recursive for its owner. Take it before the queue lock. +void amy_grab_render_lock(); +void amy_release_render_lock(); void amy_deltas_reset(); void add_delta_to_queue(struct delta *d, struct delta **queue); void amy_add_event_internal(amy_event *e, uint16_t base_osc); diff --git a/src/api.c b/src/api.c index d9df5ef4..7060bd3a 100644 --- a/src/api.c +++ b/src/api.c @@ -250,9 +250,14 @@ void *amy_get_external_hook_context(void) { } output_sample_type * amy_simple_fill_buffer() { + // One block under the render lock: no patch load's frees or resets can + // run between this block's flush and the end of its mix. + amy_grab_render_lock(); amy_execute_deltas(); amy_render(0, AMY_OSCS, 0); - return amy_fill_buffer(); + output_sample_type *block = amy_fill_buffer(); + amy_release_render_lock(); + return block; } diff --git a/src/i2s.c b/src/i2s.c index 744aa388..bbb29000 100644 --- a/src/i2s.c +++ b/src/i2s.c @@ -367,6 +367,11 @@ void esp_fill_audio_buffer_task() { #ifdef ARDUINO_SPEEDTEST int64_t _rl_start_t = esp_timer_get_time(); #endif // ARDUINO_SPEEDTEST + // The whole block, flush to mix, under the render lock (released + // before the i2s write, so a patch load gets in while we wait on the + // DMA). esp_render_task renders its half on the other core inside + // this hold without taking the lock itself. + amy_grab_render_lock(); // Get ready to render amy_execute_deltas(); @@ -375,6 +380,7 @@ void esp_fill_audio_buffer_task() { // Write to i2s output_sample_type *block = amy_fill_buffer(); + amy_release_render_lock(); uint32_t busy_us = (uint32_t)(amy_get_us() - t); AMY_PROFILE_STOP(AMY_ESP_FILL_BUFFER) @@ -503,10 +509,13 @@ int16_t *amy_render_audio() { xTaskNotifyGive(amy_fill_buffer_handle); // to esp_fill_audio_buffer_task:!AMY_HAS_I2S } } else { - // No multithread, we have to render here. + // No multithread, we have to render here. (amy_update_tasks() + // already flushed, under the render lock.) int64_t t0 = amy_get_us(); + amy_grab_render_lock(); esp_render_on_cores(); buf = amy_fill_buffer(); + amy_release_render_lock(); amy_overload_check((uint32_t)(amy_get_us() - t0)); } return buf; From 281274379d44f6bfb05f852cc824678951c5959d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 1 Oct 2026 00:36:37 +0000 Subject: [PATCH 2/2] Render lock: hold it for a patch load's flush only, not the load Measured on an AMYboard (ESP32-S3) with AMY_LOCK_TIMING: holding the render lock across patches_load_patch() held off the render for most of the load, and loads take tens of ms of CPU there: load flush hold render stall longest block Juno 1, 6v 6-9 us 19-30 ms 10-11 ms 15-18 ms DX7 130, 6v 6-8 us 47-71 ms 36-37 ms 43-46 ms grow to 8v 6-9 us 37-88 ms 27-29 ms 34-37 ms drum kit 258 6-8 us 12-13 ms 4-6 ms 8-12 ms against a 5.8 ms block and a ~35 ms DMA ring, so a DX7 load ran the ring dry. The flush is the part that frees oscs; it takes microseconds. Hold the render lock only around it, in the patch-load path and in amy_execute_deltas() (not across the sequencer tick, which can itself load a patch). The render thread still holds it for each whole block, so a flush still can't free oscs mid-render. This gives up protecting the load's bookkeeping from a reset that comes due mid-load (the same exposure main has); that needs a fix that doesn't hold the render off for the length of a load. ASan stress, 10 runs x 3000 reloads: 10 clean (main: 4 clean, 6 render crashes). make ctest passes. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01UW4jqUQ4Xc1sdoqzUPL2YV --- src/amy.c | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/src/amy.c b/src/amy.c index db782b6d..5b1b3985 100644 --- a/src/amy.c +++ b/src/amy.c @@ -95,13 +95,13 @@ void amy_profiles_print() {} // Two locks, always taken in this order when both are needed: // // render lock held by the render thread for a whole block (flush, render, -// mix), and by an ingest thread across a patch load (its flush -// plus patches_load_patch's bookkeeping). So a load's frees and -// resets can't run while a render is reading oscs, and a queued -// reset can't wipe the synth tables halfway through a load. -// Recursive for its owner: a patch string can load a patch -// (drum kits open with `if3iv1in38Z`), and a render-thread hook -// or sequenced message can send one mid-block. +// mix), and by an ingest thread for the flush it runs before a +// patch load. So a load's frees and resets can't run while a +// render is reading oscs. Only the flush, not the load: a load +// takes tens of ms on an ESP32-S3, far longer than a block. +// Recursive for its owner: a render-thread hook or sequenced +// message can load a patch mid-block, and the render thread's +// own amy_execute_deltas() takes it inside the block's hold. // queue lock guards the delta queue itself (add_delta_to_queue, the // flush), held briefly. Ordinary events take only this one, so // a note-on never waits for a render. @@ -885,16 +885,15 @@ void amy_event_to_deltas_queue(amy_event *e, uint16_t base_osc, uint16_t oscs_pe // service - this can execute on any sending thread (see // flush_due_deltas). A queued reset has to land before the load // rebuilds the synth tables, or it wipes them afterwards. - // Under the render lock, from the flush to the end of the load: - // the flush can free oscs (a released voice, a reset) that a - // render in progress is reading, and the load's bookkeeping - // (osc_to_voice, instruments, the clone-on-grow snapshot of - // synth[]) must not interleave with a reset the render thread's - // own flush runs. + // The flush runs under the render lock: it can free oscs (a + // released voice, a reset) that a render in progress is reading. + // The load itself does not: it takes tens of ms on an ESP32-S3 + // (a 6-voice DX7 load ~50-70 ms), and a render held off that long + // runs the DMA ring dry. The flush is µs. amy_grab_render_lock(); flush_due_deltas(); - patches_load_patch(e); amy_release_render_lock(); + patches_load_patch(e); } // Execute any other commands in this event. patches_event_has_voices(e, queue); @@ -2468,16 +2467,16 @@ static void flush_due_deltas() { // this takes scheduled deltas and plays them at the right time void amy_execute_deltas() { AMY_PROFILE_START(AMY_EXECUTE_DELTAS) - // Under the render lock, so a patch load on another thread can't be - // halfway through its bookkeeping while this flush runs a reset. Render - // loops already hold it across the whole block (it's recursive for them); - // this covers callers that execute deltas on their own. - amy_grab_render_lock(); // Advance the sequencer on AMY (sample) time and play any due sequence // events, so sequencing works in any rendering context, real-time or not. sequencer_check_and_fill(); // Make sure any CV-triggered events are added to delta queue update_external_cv_in(); + // The flush can free oscs, so it runs under the render lock. Render loops + // already hold it across the whole block (it's recursive for them); this + // covers callers that execute deltas off the render thread (parse.c's + // sample-transfer start). + amy_grab_render_lock(); flush_due_deltas(); amy_release_render_lock(); AMY_PROFILE_STOP(AMY_EXECUTE_DELTAS)