From 9b61fd99e463c6e19c4824f905a232315f6292dd Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 29 Sep 2026 08:50:59 +0000 Subject: [PATCH] fix(scheduler): tear down the simulator and the core's registries at exit (bug-1884, part 1) A batch --sim replay under ASAN exited 1 on 17.2 MB of LeakSanitizer reports. This removes all but 75 KB: - sched_sim_driver_fini() (called by main after a clean run) cancels and joins the picker/allocator threads like sched_fini(), closes the replay, alternate-events and output files, and finalizes the core. The threads' ids now live in core.jobpicker/allocator, as in normal mode. - xce_sched_core_fini() destroys the plugin, job, worker, queue, user and project registries and the shared-resource arrays it creates. - The seqjob/parjob sched_make_allocs() stand-ins set up only lock, nextid and the aliased resmgr. Each ran a full xce_sched_core_init(), which also replaced the process-wide string and selector tables mid-startup. - plugin_unregister(), the plugin registry's datakiller, only runs fini_fn; it key_del()'d from the registry being destroyed and skipped plugins registered with plugin_register_core(). - The limit plugin's usage-instance table used kill_limit as datakiller on xce_vres_t records (bad free on destroy); kill_vres now frees the record. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/lib/xcelimit.c | 5 +++ src/scheduler/include/sim.h | 10 +++++ src/scheduler/src/core.c | 29 +++++++++++++ src/scheduler/src/main.c | 1 + src/scheduler/src/plugin.c | 16 +++---- src/scheduler/src/plugins/limit.c | 4 +- src/scheduler/src/sim.c | 70 ++++++++++++++++++++++++++++++ src/scheduler/src/sim_alloc.c | 72 ++++++++++++++++--------------- src/scheduler/src/sim_loop.c | 16 +++---- 9 files changed, 169 insertions(+), 54 deletions(-) diff --git a/src/lib/xcelimit.c b/src/lib/xcelimit.c index a80e2ed5..84e0049b 100644 --- a/src/lib/xcelimit.c +++ b/src/lib/xcelimit.c @@ -633,9 +633,14 @@ int instantiate_limit(xce_vres_t** pvres, var resmgr, xce_limit_t* limit, return rc; } +/* kill_vres - bug-1884: the usage-instance table's datakiller. Frees only + * the record: its vres/res/percent_res belong to the resource manager and + * its limit to the plugin's limit table. */ void kill_vres(xce_vres_t* vres) { if (vres == NULL) { return; } + + FREEUP(vres); } diff --git a/src/scheduler/include/sim.h b/src/scheduler/include/sim.h index 5f34faa5..019a3ad0 100644 --- a/src/scheduler/include/sim.h +++ b/src/scheduler/include/sim.h @@ -261,6 +261,16 @@ extern int sched_sim_driver_start(xce_sched_sim_driver_t* sim, extern int sched_sim_driver_run(xce_sched_sim_driver_t* sim, const sched_sim_args_t* args); +/* + * sched_sim_driver_fini - bug-1884: releases what a sched_sim_driver_run() + * that returned 0 left behind: cancels and joins the picker and allocator + * threads (the way sched_fini() does in normal mode), closes the replay, + * alternate-events and output files, and finalizes the core. Call it only + * after sched_sim_driver_run() returned 0; a failed run has either + * finalized the core already (a startup failure) or is about to exit. + */ +extern void sched_sim_driver_fini(xce_sched_sim_driver_t* sim); + /* * sim_prescan_next_nonempty_session - user-requested (2026-09-11): looks * ahead in sim->replay_file (from its CURRENT read position) for the next diff --git a/src/scheduler/src/core.c b/src/scheduler/src/core.c index 10865bf0..6d888c98 100644 --- a/src/scheduler/src/core.c +++ b/src/scheduler/src/core.c @@ -215,6 +215,35 @@ void xce_sched_core_fini(xce_sched_core_t* core) dataq_fini(core->pend_updates[0]); dataq_fini(core->pend_updates[1]); + /* bug-1884: every registry xce_sched_core_init() makes. Plugins go + * first -- their fini hooks may still read the entity tables. */ + if (core->plugins != NULL) { + destroy(core->plugins); + core->plugins = NULL; + } + if (core->jobs != NULL) { + destroy(core->jobs); + core->jobs = NULL; + } + if (core->workers != NULL) { + destroy(core->workers); + core->workers = NULL; + } + if (core->queues != NULL) { + destroy(core->queues); + core->queues = NULL; + } + if (core->users != NULL) { + destroy(core->users); + core->users = NULL; + } + if (core->projects != NULL) { + destroy(core->projects); + core->projects = NULL; + } + zarray_fini(&(core->sharedInst)); + zarray_fini(&(core->sharedInUse)); + destroy(core->resmgr); destroy(core->ugmgr); diff --git a/src/scheduler/src/main.c b/src/scheduler/src/main.c index ceab9e18..0051eadf 100644 --- a/src/scheduler/src/main.c +++ b/src/scheduler/src/main.c @@ -403,6 +403,7 @@ int main(int argc, char* argv[]) return 1; } + sched_sim_driver_fini(&sim); return 0; } diff --git a/src/scheduler/src/plugin.c b/src/scheduler/src/plugin.c index f9c2721b..6b3a2bf6 100644 --- a/src/scheduler/src/plugin.c +++ b/src/scheduler/src/plugin.c @@ -209,24 +209,20 @@ int plugin_register_core(xce_sched_core_t* core, xce_sched_plugin_t* plugin) return rc; } +/* plugin_unregister - datakiller of core->plugins (core.c): runs as the + * registry drops the entry, so it only finalizes the plugin, whose fini_fn + * frees it. bug-1884: it used to key_del() the entry from + * plugin->sched's registry -- the one being destroyed -- and skipped fini_fn + * for a plugin registered with plugin_register_core() (plugin->sched NULL); + * no registry was ever destroyed before, so that path never ran. */ void plugin_unregister(void* arg) { xce_sched_plugin_t* plugin = (xce_sched_plugin_t*)arg; - xce_scheduler_t* sched = NULL; if (plugin == NULL) { return; } - sched = (xce_scheduler_t*)plugin->sched; - if (sched == NULL) { - return; - } - - lock(sched->core.plugins, RTTI_LOCK_WRITE); - key_del(sched->core.plugins, plugin->name); - unlock(sched->core.plugins); - if (plugin->fini_fn != NULL) { plugin->fini_fn(plugin); } diff --git a/src/scheduler/src/plugins/limit.c b/src/scheduler/src/plugins/limit.c index 43a597b4..9de9f210 100644 --- a/src/scheduler/src/plugins/limit.c +++ b/src/scheduler/src/plugins/limit.c @@ -26,11 +26,13 @@ static int limit_priv_data_init(limit_plugin_data_t* data) .delta = 64, .datakiller = (destroy_fn_t)kill_limit, }; + /* bug-1884: holds xce_vres_t usage instances, not limits -- kill_limit + * here freed a pointer inside a resource object on destroy. */ kvmgr_params_t instances_params = { .key_type = KVMGR_KEY_STRING, .nslots = 1021, .delta = 64, - .datakiller = (destroy_fn_t)kill_limit, + .datakiller = (destroy_fn_t)kill_vres, }; data->limits = makeInstance(&xkvmgr_type, &limits_params); diff --git a/src/scheduler/src/sim.c b/src/scheduler/src/sim.c index 4c2bd46a..af0a7c63 100644 --- a/src/scheduler/src/sim.c +++ b/src/scheduler/src/sim.c @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -7,6 +8,8 @@ #include "controller.capnp.h" #include "sim.h" +#include "scheduler.h" +#include "plugin.h" #include "utils.h" #include "xstring.h" @@ -681,3 +684,70 @@ int sched_sim_driver_start(xce_sched_sim_driver_t* sim, * wrapper, and sim.c stays scoped to Req 5 AC 13's startup sequence (this * file's own header comment). Its declaration is unchanged in sim.h. */ + +/* bug-1884: the sched_make_allocs() stand-ins sim_alloc.c hangs off the + * seqjob/parjob allocator plugins' `sched` field (only lock, nextid and an + * aliased resmgr are set up). The plugins are freed with the core's plugin + * registry, so take the wrappers before xce_sched_core_fini() and free them + * after it. */ +static void sim_take_alloc_wrappers_(xce_sched_sim_driver_t* sim, + xce_scheduler_t* out[2]) +{ + xce_sched_plugin_t* seq = (xce_sched_plugin_t*)sim->core.seqjob_allocator; + xce_sched_plugin_t* par = (xce_sched_plugin_t*)sim->core.parjob_allocator; + + out[0] = (seq != NULL) ? (xce_scheduler_t*)seq->sched : NULL; + out[1] = (par != NULL) ? (xce_scheduler_t*)par->sched : NULL; +} + +static void sim_free_alloc_wrappers_(xce_scheduler_t* wrappers[2]) +{ + int i; + + for (i = 0; i < 2; i++) { + if (wrappers[i] != NULL) { + mylock_fini(&(wrappers[i]->core.lock)); + free(wrappers[i]); + } + } +} + +void sched_sim_driver_fini(xce_sched_sim_driver_t* sim) +{ + uint64_t jobid = 0; + xce_scheduler_t* wrappers[2] = { NULL, NULL }; + void* res; + + if (sim == NULL) { + return; + } + + if (sim->allocator_thread_live) { + pthread_cancel(sim->core.jobpicker); + pthread_join(sim->core.jobpicker, &res); + + pthread_cancel(sim->core.allocator); + /* try to wake up allocator */ + cbuf_tryput(&(sim->core.jbuf), &jobid); + pthread_join(sim->core.allocator, &res); + + sim->allocator_thread_live = false; + } + + if (sim->record_file != NULL) { + fclose(sim->record_file); + sim->record_file = NULL; + } + if (sim->alt_events_file != NULL) { + fclose(sim->alt_events_file); + sim->alt_events_file = NULL; + } + if (sim->replay_file != NULL) { + fclose(sim->replay_file); + sim->replay_file = NULL; + } + + sim_take_alloc_wrappers_(sim, wrappers); + xce_sched_core_fini(&(sim->core)); + sim_free_alloc_wrappers_(wrappers); +} diff --git a/src/scheduler/src/sim_alloc.c b/src/scheduler/src/sim_alloc.c index eb4bd446..8f07507d 100644 --- a/src/scheduler/src/sim_alloc.c +++ b/src/scheduler/src/sim_alloc.c @@ -68,9 +68,10 @@ * calls sched_get_id(sched) (`sched->core.lock`/`.nextid`, a fresh * allocation-id counter) -- both my_alloc()'s own honest `core` parameter * AND this narrow legacy need are satisfied by keeping a small, - * long-lived (heap-allocated, never freed -- same accepted-leak convention - * this file family already uses for its own never-joined threads) - * throwaway `xce_scheduler_t` wrapper, whose `.core.resmgr` alone is + * long-lived throwaway `xce_scheduler_t` wrapper (bug-1884: only `lock`, + * `nextid` and `resmgr` are set up -- a full xce_sched_core_init() here + * built a whole unused core and re-initialized the process-wide string and + * selector tables -- and sched_sim_driver_fini() (sim.c) frees it), whose `.core.resmgr` alone is * ALIASED (a `var`, core.h -- a plain assignable reference, not an embedded * value) to simulation mode's own real resmgr, so sched_make_allocs() * resolves resource names correctly. This wrapper is set on `plugin->sched` @@ -83,6 +84,34 @@ * wrapper-based fix and outside this task's own Observable.) */ +/* sim_alloc_wrapper_new_ - the sched_make_allocs() stand-in described in + * this file's header comment: only the three fields that function and + * sched_get_id() read. */ +static xce_scheduler_t* sim_alloc_wrapper_new_(xce_sched_sim_driver_t* sim) +{ + xce_scheduler_t* wrapper = (xce_scheduler_t*)calloc(1, sizeof(*wrapper)); + + if (wrapper == NULL) { + return NULL; + } + + mylock_init(&(wrapper->core.lock)); + wrapper->core.nextid = 1; + wrapper->core.resmgr = sim->core.resmgr; + + return wrapper; +} + +static void sim_alloc_wrapper_free_(xce_scheduler_t* wrapper) +{ + if (wrapper == NULL) { + return; + } + + mylock_fini(&(wrapper->core.lock)); + free(wrapper); +} + /* * sim_bootstrap_seqjob_allocator/sim_bootstrap_parjob_allocator - consumed * by sim_loop.c via a plain `extern` declared there, same "extern declared @@ -106,7 +135,7 @@ void sim_bootstrap_seqjob_allocator(xce_sched_sim_driver_t* sim) return; } - wrapper = (xce_scheduler_t*)calloc(1, sizeof(*wrapper)); + wrapper = sim_alloc_wrapper_new_(sim); if (wrapper == NULL) { ERROR("%s out of memory bootstrapping the sim-mode seqjob " "allocator plugin's sched_make_allocs() stand-in", @@ -114,26 +143,12 @@ void sim_bootstrap_seqjob_allocator(xce_sched_sim_driver_t* sim) return; } - rc = xce_sched_core_init(&(wrapper->core)); - if (rc < 0) { - ERROR("%s failed to init the seqjob allocator sched_make_allocs() " - "stand-in core (rc=%d)", - __func__, rc); - free(wrapper); - return; - } - - /* See this file's own header comment: aliases the SAME underlying - * rtti-managed resmgr object, not a disconnected copy -- needed only - * for my_alloc()'s own remaining sched_make_allocs() calls. */ - wrapper->core.resmgr = sim->core.resmgr; - plugin = seqjob_allocator_new_plugin(); if (plugin == NULL) { ERROR("%s out of memory building the sim-mode seqjob allocator " "plugin instance", __func__); - free(wrapper); + sim_alloc_wrapper_free_(wrapper); return; } @@ -151,7 +166,7 @@ void sim_bootstrap_seqjob_allocator(xce_sched_sim_driver_t* sim) "against the simulation core (rc=%d)", __func__, rc); free(plugin); - free(wrapper); + sim_alloc_wrapper_free_(wrapper); return; } @@ -172,7 +187,7 @@ void sim_bootstrap_parjob_allocator(xce_sched_sim_driver_t* sim) return; } - wrapper = (xce_scheduler_t*)calloc(1, sizeof(*wrapper)); + wrapper = sim_alloc_wrapper_new_(sim); if (wrapper == NULL) { ERROR("%s out of memory bootstrapping the sim-mode parjob " "allocator plugin's sched_make_allocs() stand-in", @@ -180,23 +195,12 @@ void sim_bootstrap_parjob_allocator(xce_sched_sim_driver_t* sim) return; } - rc = xce_sched_core_init(&(wrapper->core)); - if (rc < 0) { - ERROR("%s failed to init the parjob allocator sched_make_allocs() " - "stand-in core (rc=%d)", - __func__, rc); - free(wrapper); - return; - } - - wrapper->core.resmgr = sim->core.resmgr; - plugin = parjob_allocator_new_plugin(); if (plugin == NULL) { ERROR("%s out of memory building the sim-mode parjob allocator " "plugin instance", __func__); - free(wrapper); + sim_alloc_wrapper_free_(wrapper); return; } @@ -206,7 +210,7 @@ void sim_bootstrap_parjob_allocator(xce_sched_sim_driver_t* sim) "against the simulation core (rc=%d)", __func__, rc); free(plugin); - free(wrapper); + sim_alloc_wrapper_free_(wrapper); return; } diff --git a/src/scheduler/src/sim_loop.c b/src/scheduler/src/sim_loop.c index 74c4c657..0becda34 100644 --- a/src/scheduler/src/sim_loop.c +++ b/src/scheduler/src/sim_loop.c @@ -1335,8 +1335,6 @@ static void sim_reconcile_unaccounted_jobs(xce_sched_sim_driver_t* sim, int sched_sim_driver_run(xce_sched_sim_driver_t* sim, const sched_sim_args_t* args) { - pthread_t picker_tid; - pthread_t allocator_tid; xce_core_event_t* buf_events = NULL; int buf_cap = 0; int buf_n = 0; @@ -1382,12 +1380,11 @@ int sched_sim_driver_run(xce_sched_sim_driver_t* sim, * Precondition (design.md's Service Interface): sched_resolve_job_picker() * must succeed once either thread's own loop needs it -- satisfied here * because sim_bootstrap_picker() (above) already ran before this point, - * unchanged ordering from the pre-4.1 sequence. Like the picker thread - * these replace, neither is ever joined or cancelled -- no sim-driver - * teardown/fini function exists (this file's own header comment), the - * same pre-existing gap normal mode does not have only because - * sched_fini() exists there. */ - rc = pthread_create(&picker_tid, NULL, sched_job_picker, &(sim->core)); + * unchanged ordering from the pre-4.1 sequence. Their ids live in the + * core, as in normal mode; sched_sim_driver_fini() (sim.c) cancels and + * joins both the way sched_fini() does (bug-1884). */ + rc = pthread_create(&(sim->core.jobpicker), NULL, sched_job_picker, + &(sim->core)); if (rc != 0) { ERROR("%s failed to start the simulation picker thread: %s", __func__, strerror(rc)); @@ -1395,7 +1392,8 @@ int sched_sim_driver_run(xce_sched_sim_driver_t* sim, return -rc; } - rc = pthread_create(&allocator_tid, NULL, sched_allocator, &(sim->core)); + rc = pthread_create(&(sim->core.allocator), NULL, sched_allocator, + &(sim->core)); if (rc != 0) { ERROR("%s failed to start the simulation allocator thread: %s", __func__, strerror(rc));