From 7ee9a16f532a5b5812a33db846560789274ab08e Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Mon, 3 Aug 2026 08:51:45 +0000 Subject: [PATCH] feat(scheduler-core): wire builtin_fcfs as registered default picker (task 2.4) Registers builtin_fcfs at scheduler startup and makes it the default job_pick_plugin, replacing the hardcoded "xce_sched_fcfs" literal in sched_job_picker() with a fail-loud lookup through sched_resolve_job_picker(). The picker thread now branches legacy job_elect_fn plugins (delegated unchanged, preserving their self-contained wbuf-driven loop) from native xce_job_picker_iface_t pickers (driven by a new per-session loop), and the allocator now calls on_dispatch/on_pend for every picker as Req 1 AC 4 requires. Closes task 2.3's flagged picker/allocator concurrency race with a mutex on fcfs_plugin_data_t, and adds cancel points to next_job(). --- .kiro/specs/scheduler-core/tasks.md | 4 +- src/scheduler/include/plugin.h | 1 + src/scheduler/include/scheduler.h | 15 + src/scheduler/meson.build | 1 + src/scheduler/src/plugins/builtin_fcfs.c | 312 +++++++++-- src/scheduler/src/schedobj.c | 51 ++ src/scheduler/src/scheduler.c | 169 +++++- src/scheduler/testsuite/meson.build | 18 + src/scheduler/testsuite/test_picker_fcfs.c | 15 + .../testsuite/test_picker_fcfs_session.c | 13 + src/scheduler/testsuite/test_picker_wiring.c | 491 ++++++++++++++++++ 11 files changed, 1036 insertions(+), 54 deletions(-) create mode 100644 src/scheduler/testsuite/test_picker_wiring.c diff --git a/.kiro/specs/scheduler-core/tasks.md b/.kiro/specs/scheduler-core/tasks.md index a4be68a4..8a9c2694 100644 --- a/.kiro/specs/scheduler-core/tasks.md +++ b/.kiro/specs/scheduler-core/tasks.md @@ -51,7 +51,7 @@ - _Requirements: 3.2, 3.4, 3.6_ - _Depends: 2.2_ -- [ ] 2.4 Wire builtin_fcfs as the registered default and remove the hardcoded picker string +- [x] 2.4 Wire builtin_fcfs as the registered default and remove the hardcoded picker string - Auto-register `builtin_fcfs` at scheduler startup under the name `"builtin_fcfs"` and set it as the default picker plugin - Replace the hardcoded `"xce_sched_fcfs"` literal with a lookup of the configured picker plugin name through the existing plugin registration/lookup mechanism; fail scheduler startup with a clear error if the configured name isn't registered - Confirm the existing three-thread/cbuf pipeline (picker thread, allocator thread, driver-signaled `wbuf`/`jbuf`/`decisions` cbufs) is unchanged by this wiring — no new threads, no cbuf capacity changes @@ -357,3 +357,5 @@ - Meson build dir is `src/xce.build`, not repo-root `xce.build` — build/test commands must run from `src/` (e.g. `cd src && nix develop . --command bash -c "meson test -C xce.build --suite scheduler"`). - Scheduler testsuite convention: run test binaries directly with `ASAN_OPTIONS=detect_leaks=0` (fixtures aren't torn down, by design — matches existing `meson.build` test registration). - Task 2.3's review found a real concurrency gap for task 2.4 to close: once builtin_fcfs is wired live, `next_job()` (picker thread) and `on_dispatch`/`on_pend` (allocator thread) can race on the same unsynchronized `fcfs_plugin_data_t` private state — `cbuf_get()` unblocks the picker's next `cbuf_put()` before the allocator finishes processing the dequeued job (jbuf capacity is 1), so the picker can re-enter `next_job()` while the allocator is still mutating `priv->queues` via `on_dispatch`/`on_pend`. Today's production path never hits this because the allocator thread calls a different plugin category (`limit.c`'s resource-limit hook), never `fcfs.c`'s own private state. Task 2.4 must add a mutex guarding `fcfs_plugin_data_t` across all five picker hooks (or an equivalent handoff redesign) before registering builtin_fcfs as the live default. +- Task 2.4 closed the above race with `fcfs_plugin_data_t.lock` (mylock_t), taken by exactly 7 hooks (`on_queue_update`, `on_job_push`, `on_job_state_change`, `on_job_update`, `begin_session`, `next_job`, `on_dispatch`) — `on_pend`/`end_session`/`init`/`fini` correctly excluded (they never touch `priv->queues`/`cursor`/`job_cursor`), a verified refinement of this note's original "all five hooks" estimate, not a gap. `sched_job_picker()` now branches `job_elect_fn != NULL` (legacy plugins like `xce_sched_fcfs`, delegates unchanged to their own self-contained loop) vs NULL (native `xce_job_picker_iface_t` pickers, runs a new generic per-session loop) — this split exists because legacy `job_elect_fn` implementations (e.g. `fcfs_elect_job`) already contain their own internal `cbuf_get(&wbuf)`, so a single uniform outer loop would double-consume the wbuf signal for legacy pickers. `sched_allocator()` now calls `picker_iface->on_dispatch()`/`on_pend()` after each allocation attempt (Req 1 AC 4), which was previously called for no picker at all — required for builtin_fcfs to actually remove dispatched jobs from its private state once live. +- Two follow-ups surfaced by task 2.4's review, not yet fixed (outside its file boundary): (1) `main.c` discards `sched_init()`'s return value, so a bad `job_pick_plugin` config fails `sched_init()` correctly but the process doesn't yet halt on it — needs a fast follow-up before relying on "fails startup" in production. (2) `pthread_testcancel()` in `builtin_fcfs_next_job()` is only at the loop entry/exit, not scattered through the queue/job walk (the walk holds `fcfs_plugin_data_t.lock` throughout, and this codebase has no `pthread_cleanup_push` precedent to safely unlock on mid-walk cancellation) — behaviorally inert today since `sched_queue_is_ready`/`sched_job_is_ready` are `return 1` stubs, but becomes a real cancellation-latency gap once Phase 2 implements real filtering. diff --git a/src/scheduler/include/plugin.h b/src/scheduler/include/plugin.h index 326c0976..4f21f27a 100644 --- a/src/scheduler/include/plugin.h +++ b/src/scheduler/include/plugin.h @@ -97,6 +97,7 @@ extern int plugin_job_election(void*, xce_sched_job_t*); extern int plugin_job_rusage_update(void*, xce_sched_job_t*, xce_job_rusages_t*); extern int register_fcfs(void*); +extern int register_builtin_fcfs(void*); extern int register_limit(void*); extern int register_seqjob_allocator(void*); extern int register_parjob_allocator(void*); diff --git a/src/scheduler/include/scheduler.h b/src/scheduler/include/scheduler.h index 34b0452c..a9e5682d 100644 --- a/src/scheduler/include/scheduler.h +++ b/src/scheduler/include/scheduler.h @@ -137,6 +137,21 @@ extern int sched_init_config(xce_scheduler_t* sched, xce_conf4sched_t*); * default to sched->job_pick_plugin when unset. See the field comment on * xce_scheduler_t above -- this does NOT resolve/register the name. */ extern void sched_set_default_job_pick_plugin(xce_scheduler_t*); +/* Req 1 AC 6 / Req 2 AC 5 (scheduler-core spec, task 2.4): resolves + * sched->job_pick_plugin against the plugin registry (the same + * plugin_register()/key_find() mechanism every other plugin category + * uses) and validates the result actually implements the picker MUST + * hooks (xce_job_picker_iface_t: next_job, begin_session, end_session, + * on_dispatch, on_pend). Returns 0 and sets *out on success; a negative + * errno on failure: + * -EINVAL bad arguments, or sched->job_pick_plugin is unset (NULL), + * or the name IS registered but is not a usable picker + * -ENOENT the configured name is not registered at all + * Called from sched_init() (before the picker thread is created, so an + * unregistered/non-picker name fails scheduler startup synchronously) and + * from sched_job_picker() (the picker thread, at thread start) -- one + * implementation, two call sites, no duplicated lookup logic. */ +extern int sched_resolve_job_picker(xce_scheduler_t*, xce_sched_plugin_t**); extern int sched_update(xce_scheduler_t*); extern void sched_go(xce_scheduler_t*); extern void sched_begin_session(xce_scheduler_t*); diff --git a/src/scheduler/meson.build b/src/scheduler/meson.build index ccc3e8b2..dc27350d 100644 --- a/src/scheduler/meson.build +++ b/src/scheduler/meson.build @@ -7,6 +7,7 @@ scheduler_srcs = [ 'src/plugin.c', 'src/resource.c', 'src/plugins/fcfs.c', + 'src/plugins/builtin_fcfs.c', 'src/plugins/seqjob_allocator.c', 'src/plugins/parjob_allocator.c', 'src/plugins/limit.c', diff --git a/src/scheduler/src/plugins/builtin_fcfs.c b/src/scheduler/src/plugins/builtin_fcfs.c index ae3e5b41..8f1c3bb8 100644 --- a/src/scheduler/src/plugins/builtin_fcfs.c +++ b/src/scheduler/src/plugins/builtin_fcfs.c @@ -2,28 +2,19 @@ * builtin_fcfs.c - default xce_job_picker_iface_t implementation * (scheduler-core spec, Req 3: Builtin default picker). * - * This is the eventual replacement for plugins/fcfs.c's job_elect_fn-based - * picker (design.md's File Structure Plan: "plugins/fcfs.c # rewritten - * (Phase 1) to xce_job_picker_iface_t, drops job_elect_fn"). That rewrite - * is delivered incrementally across tasks 2.1-2.4, and plugins/fcfs.c is - * still the LIVE, production picker (registered as "xce_sched_fcfs", - * driving real scheduling decisions) until task 2.4 flips the switch. This - * file is therefore kept SEPARATE and UNREGISTERED for now: - * - separate: so nothing in this task can regress fcfs.c's current - * behavior -- fcfs.c is not touched at all by task 2.1. - * - unregistered: builtin_fcfs is not added to any plugin table here; - * that wiring is task 2.4's "Wire builtin_fcfs as the registered - * default" (tasks.md). Until then this file is exercised only by its - * own testsuite (test_picker_fcfs.c, white-box #include). - * A later task (2.4, per design.md's stated end state) is expected to fold - * this file's content over plugins/fcfs.c (or otherwise retire the old - * file) once every hook here is complete and the switch is flipped -- that - * consolidation decision belongs to that task, not this one. - * - * Task 2.1 scope: init()/fini() and the private ordered-queue state - * container only (Req 3 AC 1, AC 2's init/fini rows, AC 3). Every other - * hook (on_queue_update, on_job_push, next_job, ...) is NULL/unimplemented - * here and lands in tasks 2.2/2.3 -- see tasks.md. + * This is the replacement for plugins/fcfs.c's job_elect_fn-based picker + * (design.md's File Structure Plan: "plugins/fcfs.c # rewritten (Phase 1) + * to xce_job_picker_iface_t, drops job_elect_fn"). That rewrite was + * delivered incrementally across tasks 2.1-2.3 (hooks only, unregistered); + * task 2.4 (this revision) is the "Wire builtin_fcfs as the registered + * default" step: register_builtin_fcfs() (bottom of this file) is now + * called from sched_register_plugins() (scheduler.c) and + * sched_set_default_job_pick_plugin()/sched_resolve_job_picker() + * (schedobj.c) make it the live default picker. plugins/fcfs.c is + * DELIBERATELY left untouched and still registered under "xce_sched_fcfs" + * (task boundary: this task does not remove it, only stops it being the + * thing selected by default) -- it is now dead code on the default path, + * reachable only by explicitly configuring job_pick_plugin="xce_sched_fcfs". * * "picker" calling convention: every xce_job_picker_iface_t hook receives * `void* picker` as its first argument. The already-implemented job_elect_fn @@ -32,16 +23,40 @@ * legacy_picker_next_job() casts `picker` straight to xce_sched_plugin_t* * to reach plugin->job_elect_fn, and test_plugin_picker_registration.c * confirms this by calling `plugin.picker_iface->next_job(&plugin)`. Since - * a single, picker-implementation-agnostic call site (the future picker - * thread loop, tasks.md group 2.4/2.5) must drive both legacy-wrapped and - * native pickers identically, "picker" == xce_sched_plugin_t* for EVERY - * picker, native ones included. builtin_fcfs therefore follows the exact - * convention plugins/fcfs.c's own (old-style) fcfs_init/fcfs_fini already - * use: allocate the private fcfs_plugin_data_t and store it in + * a single, picker-implementation-agnostic call site (scheduler.c's + * sched_job_picker thread, wired in task 2.4) drives both legacy-wrapped + * and native pickers identically, "picker" == xce_sched_plugin_t* for + * EVERY picker, native ones included. builtin_fcfs therefore follows the + * exact convention plugins/fcfs.c's own (old-style) fcfs_init/fcfs_fini + * already use: allocate the private fcfs_plugin_data_t and store it in * plugin->data (the existing plugin_data_t field on xce_sched_plugin_t), * not in a new field of its own. + * + * Concurrency (task 2.4, closing a gap task 2.3's review found): once + * builtin_fcfs is wired live, next_job() (picker thread) and + * on_dispatch()/on_pend() (allocator thread) can be called concurrently -- + * jbuf's capacity-1 cbuf unblocks the picker's NEXT next_job() call for + * job N+1 as soon as the allocator's cbuf_get(&jbuf) dequeues job N, well + * before the allocator finishes on_dispatch()/on_pend() for job N. ordq_t's + * OWN internal lock (mylock.h, src/lib/ordq.c) only protects a single + * ordq_t's own put/get/del/peek calls against each other -- it does NOT + * protect a raw external pointer walk (this file's priv->cursor/job_cursor + * in next_job(), and fcfs_remove_job_everywhere's raw + * `it = priv->queues.head` walk) from a concurrent ordq_del on the very + * same list freeing the node the walk currently holds (a genuine + * use-after-free). fcfs_plugin_data_t.lock (added below) closes this by + * making every hook that touches candidate state (on_queue_update, + * on_job_push, on_job_state_change, on_job_update, begin_session, + * next_job, on_dispatch) mutually exclusive at the fcfs_plugin_data_t + * granularity -- see the lock's own field comment and next_job()'s + * cancel-safety comment for the rest of the reasoning. (on_pend and + * end_session are deliberately NOT in that list -- both are true no-ops + * that never touch queues/cursor/job_cursor, see each function's own + * comment; an earlier revision of this comment incorrectly included + * on_pend here, corrected in task 2.4.) */ #include +#include #include #include #include @@ -89,6 +104,22 @@ typedef struct { * unambiguous inside next_job() -- it always * means "current queue's job list exhausted", * never "haven't started this queue yet". */ + /* Task 2.4 concurrency fix (see file header comment): guards every + * field in this struct -- queues, cursor, job_cursor, and + * transitively each fcfs_queue_t's own pendJobs reached through them + * -- against the picker thread (begin_session/next_job/end_session) + * and the allocator thread (on_dispatch/on_pend) running concurrently. + * Entered/exited by every hook that reads or writes candidate state; + * NOT entered by init()/fini() (nothing else holds a reference to + * plugin->data before init() returns or after fini() begins, so + * lock/unlock there would be a no-op contention-wise, same convention + * ordq_init()/ordq_fini() themselves follow for their own lock) or by + * end_session()/on_pend() (both are true no-ops that never touch + * queues/cursor/job_cursor -- see each function's own comment). Plain + * pthread_mutex_t (mylock_t, non-recursive) -- callers must never call + * back into a lock-taking hook while already holding this lock; no + * code path here does. */ + mylock_t lock; } fcfs_plugin_data_t; /* ordq_data_killer_t for a queue's pendJobs: frees the fcfs_job_t wrapper. @@ -214,6 +245,11 @@ int builtin_fcfs_init(void* picker, xce_sched_core_t* core) return rc; } + /* Task 2.4: mylock_init() return value is not checked -- same + * convention ordq_init() itself follows for this exact call + * (src/lib/ordq.c: `mylock_init(&(q->lock));`, unchecked). */ + mylock_init(&(priv->lock)); + priv->cursor = NULL; priv->job_cursor = NULL; @@ -367,10 +403,17 @@ int builtin_fcfs_on_queue_update(void* picker, xce_sched_queue_t* queue) return -EINVAL; } + /* Task 2.4 concurrency fix: see fcfs_plugin_data_t.lock's field + * comment -- guards priv->queues (and transitively each fcfs_queue_t's + * own pendJobs) against the picker thread's next_job()/begin_session() + * and the allocator thread's on_dispatch(). */ + mylock_lock(&(priv->lock)); + it = ordq_get(&(priv->queues), (ordq_match_fn_t)queue_match, queue->name); if (it == NULL) { fq = (fcfs_queue_t*)calloc(1, sizeof(*fq)); if (fq == NULL) { + mylock_unlock(&(priv->lock)); return -ENOMEM; } @@ -378,6 +421,7 @@ int builtin_fcfs_on_queue_update(void* picker, xce_sched_queue_t* queue) (ordq_cmp_fn_t)fcfs_job_cmp); if (rc < 0) { FREEUP(fq); + mylock_unlock(&(priv->lock)); return rc; } } else { @@ -387,7 +431,11 @@ int builtin_fcfs_on_queue_update(void* picker, xce_sched_queue_t* queue) fq->queue = queue; - return ordq_put(&(priv->queues), (void*)fq); + rc = ordq_put(&(priv->queues), (void*)fq); + + mylock_unlock(&(priv->lock)); + + return rc; } /* @@ -409,6 +457,7 @@ int builtin_fcfs_on_job_push(void* picker, xce_sched_job_t* job) fcfs_plugin_data_t* priv = NULL; fcfs_queue_t* fq = NULL; fcfs_job_t* fj = NULL; + int rc = 0; if ((plugin == NULL) || (job == NULL) || (job->queue == NULL) || (job->queue->name == NULL)) { @@ -424,24 +473,36 @@ int builtin_fcfs_on_job_push(void* picker, xce_sched_job_t* job) return 0; } + /* Task 2.4 concurrency fix: see fcfs_plugin_data_t.lock's field + * comment. job->state was already read above, outside the lock -- + * only priv's own candidate state needs the lock. */ + mylock_lock(&(priv->lock)); + fq = fcfs_find_queue(priv, job->queue->name); if (fq == NULL) { + mylock_unlock(&(priv->lock)); return -EINVAL; } if (ordq_peek(&(fq->pendJobs), (ordq_match_fn_t)job_match, job->jobid) != NULL) { + mylock_unlock(&(priv->lock)); return 0; /* already a candidate -- see header comment */ } fj = (fcfs_job_t*)calloc(1, sizeof(*fj)); if (fj == NULL) { + mylock_unlock(&(priv->lock)); return -ENOMEM; } fj->job = job; - return ordq_put(&(fq->pendJobs), (void*)fj); + rc = ordq_put(&(fq->pendJobs), (void*)fj); + + mylock_unlock(&(priv->lock)); + + return rc; } /* @@ -465,6 +526,7 @@ int builtin_fcfs_on_job_state_change(void* picker, xce_sched_job_t* job, xce_sched_plugin_t* plugin = (xce_sched_plugin_t*)picker; fcfs_plugin_data_t* priv = NULL; fcfs_queue_t* fq = NULL; + int rc = 0; if ((plugin == NULL) || (job == NULL) || (job->queue == NULL) || (job->queue->name == NULL)) { @@ -476,13 +538,19 @@ int builtin_fcfs_on_job_state_change(void* picker, xce_sched_job_t* job, return -EINVAL; } + /* Task 2.4 concurrency fix: see fcfs_plugin_data_t.lock's field + * comment. */ + mylock_lock(&(priv->lock)); + fq = fcfs_find_queue(priv, job->queue->name); if (fq == NULL) { + mylock_unlock(&(priv->lock)); return -EINVAL; } if (!JOB_IS_PEND(state)) { ordq_del(&(fq->pendJobs), (ordq_match_fn_t)job_match, job->jobid); + mylock_unlock(&(priv->lock)); return 0; } @@ -491,14 +559,19 @@ int builtin_fcfs_on_job_state_change(void* picker, xce_sched_job_t* job, fcfs_job_t* fj = (fcfs_job_t*)calloc(1, sizeof(*fj)); if (fj == NULL) { + mylock_unlock(&(priv->lock)); return -ENOMEM; } fj->job = job; - return ordq_put(&(fq->pendJobs), (void*)fj); + rc = ordq_put(&(fq->pendJobs), (void*)fj); + mylock_unlock(&(priv->lock)); + return rc; } + mylock_unlock(&(priv->lock)); + return 0; } @@ -526,6 +599,7 @@ int builtin_fcfs_on_job_update(void* picker, xce_sched_job_t* job) fcfs_plugin_data_t* priv = NULL; fcfs_queue_t* fq = NULL; fcfs_job_t* fj = NULL; + int rc = 0; if ((plugin == NULL) || (job == NULL)) { return -EINVAL; @@ -536,29 +610,41 @@ int builtin_fcfs_on_job_update(void* picker, xce_sched_job_t* job) return -EINVAL; } + /* Task 2.4 concurrency fix: see fcfs_plugin_data_t.lock's field + * comment. */ + mylock_lock(&(priv->lock)); + fcfs_remove_job_everywhere(priv, job->jobid); if (!JOB_IS_PEND(job->state)) { + mylock_unlock(&(priv->lock)); return 0; } if ((job->queue == NULL) || (job->queue->name == NULL)) { + mylock_unlock(&(priv->lock)); return -EINVAL; } fq = fcfs_find_queue(priv, job->queue->name); if (fq == NULL) { + mylock_unlock(&(priv->lock)); return -EINVAL; } fj = (fcfs_job_t*)calloc(1, sizeof(*fj)); if (fj == NULL) { + mylock_unlock(&(priv->lock)); return -ENOMEM; } fj->job = job; - return ordq_put(&(fq->pendJobs), (void*)fj); + rc = ordq_put(&(fq->pendJobs), (void*)fj); + + mylock_unlock(&(priv->lock)); + + return rc; } /* --------------------------------------------------------------------- @@ -611,7 +697,18 @@ int builtin_fcfs_begin_session(void* picker) return -EINVAL; } + /* Task 2.4 concurrency fix: see fcfs_plugin_data_t.lock's field + * comment. begin_session() only runs once per round before any + * next_job()/on_dispatch() call of that round, so it is not proven to + * race the allocator thread under today's synchronous main-loop + * barrier -- locked anyway for defense-in-depth and to keep every + * candidate-state-touching hook uniformly covered by the one + * documented invariant, rather than relying on a scheduling + * assumption that could silently stop holding under a future, + * less-synchronous driver (e.g. the core/driver split, Phase 2). */ + mylock_lock(&(priv->lock)); fcfs_cursor_enter_queue(priv, priv->queues.head); + mylock_unlock(&(priv->lock)); return 0; } @@ -637,14 +734,42 @@ int builtin_fcfs_begin_session(void* picker) * searching. Returns NULL once priv->cursor runs off the end of * priv->queues -- "all candidates exhausted" (task 2.3 Observable). * - * Cancel-point plumbing (pthread_testcancel() calls for the live picker - * thread) is task 2.4 scope -- this task only implements the walking - * contract itself. + * Cancel-point plumbing (task 2.4, Req 1 AC 5 / Req 3 AC 5): next_job() + * runs in the dedicated picker thread, torn down via pthread_cancel + * (sched_fini(), scheduler.c). plugins/fcfs.c's fcfs_elect_job() scatters + * pthread_testcancel() at every loop level (top of session loop, each + * queue, each job) because its own privdata has NO concurrent access from + * any other thread -- nothing else can be disturbed by cancelling it + * anywhere. builtin_fcfs is different: this function holds + * fcfs_plugin_data_t.lock (task 2.4 concurrency fix, see that field's + * comment) for the entire cursor walk, because the walk itself must be + * atomic with respect to the allocator thread's on_dispatch() freeing + * queue-list/job-list nodes out from under it. This codebase has no + * pthread_cleanup_push()/pop() precedent anywhere (verified), so a + * pthread_testcancel() placed INSIDE that locked walk would be a real + * deadlock hazard, not just a style concern: if cancellation landed there, + * this thread would terminate immediately, holding the mutex forever (a + * plain, non-robust pthread_mutex_t) -- the allocator thread's very next + * on_dispatch()/on_pend() call would then block on mylock_lock() forever + * (pthread_mutex_lock() is not itself a cancellation point, so a pending + * cancellation on the allocator thread cannot rescue it either), + * permanently hanging sched_fini()'s pthread_join(sched->allocator). + * Cancellation is therefore only tested here, OUTSIDE the lock, at entry + * and at exit -- both real, safe, per-call cancellation points (mirroring + * sched_job_picker()'s own pthread_testcancel() immediately before every + * next_job() call, which remains in place too -- belt-and-suspenders, not + * redundant dead code). This does not weaken cancel-safety in practice + * today: sched_job_is_ready()/sched_queue_is_ready() are unconditional + * `return 1` stubs (scheduler.c), so this walk always returns on the very + * first candidate examined -- fcfs_elect_job()'s own per-iteration + * testcancel calls are equally never exercised mid-walk today, for the + * same reason. */ xce_sched_job_t* builtin_fcfs_next_job(void* picker) { xce_sched_plugin_t* plugin = (xce_sched_plugin_t*)picker; fcfs_plugin_data_t* priv = NULL; + xce_sched_job_t* ret = NULL; if (plugin == NULL) { return NULL; @@ -655,6 +780,10 @@ xce_sched_job_t* builtin_fcfs_next_job(void* picker) return NULL; } + pthread_testcancel(); + + mylock_lock(&(priv->lock)); + while (priv->cursor != NULL) { fcfs_queue_t* fq = (fcfs_queue_t*)priv->cursor->data; @@ -669,14 +798,23 @@ xce_sched_job_t* builtin_fcfs_next_job(void* picker) priv->job_cursor = priv->job_cursor->next; if (sched_job_is_ready(fj->job)) { - return fj->job; + ret = fj->job; + break; } } + if (ret != NULL) { + break; + } + fcfs_cursor_enter_queue(priv, priv->cursor->next); } - return NULL; + mylock_unlock(&(priv->lock)); + + pthread_testcancel(); + + return ret; } /* @@ -732,7 +870,13 @@ int builtin_fcfs_on_dispatch(void* picker, uint64_t jobid) return -EINVAL; } + /* Task 2.4 concurrency fix: see fcfs_plugin_data_t.lock's field + * comment -- this runs in the allocator thread and can race the + * picker thread's next_job() (jbuf's capacity-1 handoff unblocks the + * picker for job N+1 before this call for job N returns). */ + mylock_lock(&(priv->lock)); fcfs_remove_job_everywhere(priv, jobid); + mylock_unlock(&(priv->lock)); return 0; } @@ -764,3 +908,97 @@ int builtin_fcfs_on_pend(void* picker, uint64_t jobid) return 0; } + +/* --------------------------------------------------------------------- + * Task 2.4: registration (Req 3 AC 1) -- wires builtin_fcfs into the + * scheduler's plugin registry under "builtin_fcfs" via the EXISTING + * plugin_register()/key_find() mechanism every other plugin category + * already uses (plugin.c, unmodified by this task), mirroring + * plugins/fcfs.c's own register_fcfs(): a static template struct, + * calloc'd + memcpy'd into a heap instance, plugin_register()'d. + * ------------------------------------------------------------------- */ + +/* + * xce_job_picker_iface_t.init/.fini use the (void* picker, + * xce_sched_core_t* core) / (void* picker) signatures documented in + * plugin.h -- xce_sched_plugin_t's OLDER init_fn/fini_fn fields + * (plugin_init_fn_t/plugin_fini_fn_t) predate that contract and only take + * a single void* argument. plugin_register()/plugin_unregister() (plugin.c, + * unmodified by this task) already invoke init_fn/fini_fn unconditionally + * at register/unregister time -- exactly the "driver thread, at plugin + * register/teardown" timing the picker lifecycle-hook contract (plugin.h) + * documents for init/fini -- so builtin_fcfs plugs into that EXISTING call + * site via this thin adapter rather than by changing plugin_register() + * itself. A raw `(plugin_init_fn_t)builtin_fcfs_init` cast would silently + * drop the `core` argument at the call site -- a real arity mismatch + * (undefined behavior per C11 6.3.2.3p8), not merely a style concern, so + * this adapter supplies it explicitly (NULL, per builtin_fcfs_init's own + * doc comment: "core is accepted per the interface signature but + * deliberately never dereferenced" -- xce_sched_core_t does not exist + * until the Phase 2 core extraction). builtin_fcfs_fini()'s signature + * already matches plugin_fini_fn_t exactly (single void* picker arg), so + * no adapter is needed for fini -- it is wired directly below. */ +static int builtin_fcfs_init_fn(void* picker) +{ + return builtin_fcfs_init(picker, NULL); +} + +/* Every MUST hook from tasks 2.1-2.3, wired to the real builtin_fcfs_* + * implementations; on_config/on_job_rusage NULL per Req 3 AC 2's table + * ("FCFS is config-agnostic" / "FCFS is not resource-aware"). */ +static xce_job_picker_iface_t builtin_fcfs_picker_iface = { + .init = builtin_fcfs_init, + .fini = builtin_fcfs_fini, + .begin_session = builtin_fcfs_begin_session, + .next_job = builtin_fcfs_next_job, + .end_session = builtin_fcfs_end_session, + .on_dispatch = builtin_fcfs_on_dispatch, + .on_pend = builtin_fcfs_on_pend, + .on_queue_update = builtin_fcfs_on_queue_update, + .on_job_push = builtin_fcfs_on_job_push, + .on_job_state_change = builtin_fcfs_on_job_state_change, + .on_job_update = builtin_fcfs_on_job_update, + .on_config = NULL, + .on_job_rusage = NULL, +}; + +/* + * register_builtin_fcfs - registers builtin_fcfs under the name + * "builtin_fcfs" (Req 3 AC 1). Called from sched_register_plugins() + * (scheduler.c) alongside register_fcfs() -- both plugins end up + * registered; builtin_fcfs additionally becomes the *default* picker via + * sched_set_default_job_pick_plugin()/sched_resolve_job_picker() + * (schedobj.c), not through anything in this function. + * + * builtin_fcfs sets ONLY picker_iface (native xce_job_picker_iface_t) plus + * the init_fn/fini_fn adapters above -- .hooks (the old plugin_hooks_t + * category), .begin_session_fn/.end_session_fn, and .job_elect_fn all stay + * NULL/zeroed (calloc'd template): builtin_fcfs participates only in the + * picker category, not the legacy per-plugin hook/session categories + * fcfs.c's OWN registration still uses for ITS OWN (untouched, still + * "xce_sched_fcfs"-registered) plugin instance. Because picker_iface is + * already non-NULL here, plugin_register()'s legacy-wrapper substitution + * (task 1.2, `if ((plugin->picker_iface == NULL) && (plugin->job_elect_fn + * != NULL))`) never fires for builtin_fcfs -- it registers as-is, no + * deprecation warning. + */ +int register_builtin_fcfs(void* sched) +{ + static xce_sched_plugin_t builtin_fcfs_plugin_template + = { .name = "builtin_fcfs", + .picker_iface = &builtin_fcfs_picker_iface, + .init_fn = builtin_fcfs_init_fn, + .fini_fn = (plugin_fini_fn_t)builtin_fcfs_fini, + .data = NULL }; + + xce_sched_plugin_t* plugin + = (xce_sched_plugin_t*)calloc(1, sizeof(*plugin)); + + if (plugin == NULL) { + return -ENOMEM; + } + + memcpy(plugin, &builtin_fcfs_plugin_template, sizeof(*plugin)); + + return plugin_register(sched, plugin); +} diff --git a/src/scheduler/src/schedobj.c b/src/scheduler/src/schedobj.c index 8380ca4d..62cf82a7 100644 --- a/src/scheduler/src/schedobj.c +++ b/src/scheduler/src/schedobj.c @@ -409,6 +409,57 @@ void sched_set_default_job_pick_plugin(xce_scheduler_t* sched) } } +/* + * sched_resolve_job_picker - Req 1 AC 6 / Req 2 AC 5 (scheduler-core spec, + * task 2.4). See the declaration comment in scheduler.h for the full + * contract. + * + * Kept here (rather than in scheduler.c, where both its callers -- + * sched_init() and sched_job_picker() -- live) for the same testability + * reason sched_set_default_job_pick_plugin() was: this function only needs + * sched->job_pick_plugin and sched->plugins (the plugin registry kvmgr), + * not the network/xcecli dependencies sched_init() otherwise pulls in, so + * it can be exercised directly by a white-box test of this translation + * unit (mirroring test_picker_config_fields.c's own #include "schedobj.c" + * convention) without linking scheduler.c. + */ +int sched_resolve_job_picker(xce_scheduler_t* sched, xce_sched_plugin_t** out) +{ + xce_sched_plugin_t* plugin = NULL; + int rc = 0; + + if ((sched == NULL) || (out == NULL)) { + return -EINVAL; + } + + *out = NULL; + + if (sched->job_pick_plugin == NULL) { + return -EINVAL; + } + + lock(sched->plugins, RTTI_LOCK_READ); + rc = key_find(sched->plugins, sched->job_pick_plugin, (void**)&plugin); + unlock(sched->plugins); + + if (rc <= 0) { + return (rc < 0) ? rc : -ENOENT; + } + + if ((plugin == NULL) || (plugin->picker_iface == NULL) + || (plugin->picker_iface->next_job == NULL) + || (plugin->picker_iface->begin_session == NULL) + || (plugin->picker_iface->end_session == NULL) + || (plugin->picker_iface->on_dispatch == NULL) + || (plugin->picker_iface->on_pend == NULL)) { + return -EINVAL; + } + + *out = plugin; + + return 0; +} + static void host_uncommit_killer(void* obj) { xvec_t* v = (xvec_t*)obj; diff --git a/src/scheduler/src/scheduler.c b/src/scheduler/src/scheduler.c index adbf3568..b3cf96ae 100644 --- a/src/scheduler/src/scheduler.c +++ b/src/scheduler/src/scheduler.c @@ -36,28 +36,98 @@ static int sched_staging_merge_complete(xce_scheduler_t* sched) memory_order_acquire); } +/* + * sched_job_picker - the scheduler's dedicated picker thread entry point + * (Req 1 AC 3/AC 4, Req 3 AC 5, scheduler-core spec, task 2.4). + * + * Replaces the hardcoded "xce_sched_fcfs" literal (Req 1 AC 6) with + * sched_resolve_job_picker() (schedobj.c), which looks up + * sched->job_pick_plugin through the same plugin_register()/key_find() + * mechanism every plugin category uses. sched_init() already performed + * this same resolution synchronously before this thread was even created + * (so an unregistered/non-picker name fails scheduler startup, not this + * thread quietly) -- re-resolving here is the redundant "one + * implementation, two call sites" the scheduler.h doc comment describes, + * not duplicated logic. + * + * Branches on which picker kind was resolved (see this file's own header + * discussion / builtin_fcfs.c's file header comment for the full + * reasoning): + * - job_elect_fn != NULL (a plugin still using the deprecated legacy + * hook, e.g. "xce_sched_fcfs" explicitly configured): delegate to it + * EXACTLY as before this task -- job_elect_fn (fcfs_elect_job) is + * ITSELF the entire forever per-session loop, with its own + * cbuf_get(&sched->wbuf) at the top and its own pthread_testcancel() + * calls throughout (fcfs.c). Running the generic loop below *as well* + * for this case would install a second, competing consumer of wbuf's + * single "go" signal per round -- fcfs_elect_job's own cbuf_get would + * then block waiting for a signal this outer loop already stole, + * hanging the legacy picker (a real bug, not just redundant work). + * - job_elect_fn == NULL (a native xce_job_picker_iface_t implementor -- + * builtin_fcfs, or any future native picker): run the generic + * per-session outer loop against picker_iface's hooks (Req 1 AC 4 + * pseudocode), pushing each returned job's jobid onto sched->jbuf and + * an end-of-session 0 marker -- exactly what sched_allocator() (below, + * unchanged pipeline: same wbuf/jbuf/decisions cbufs, same two + * threads) already expects on the other end. + */ static void* sched_job_picker(void* arg) { - xce_sched_plugin_t* job_picker = NULL; - xce_scheduler_t* sched = (xce_scheduler_t*)arg; - char* job_pick_plugin = "xce_sched_fcfs"; - int rc = 0; - - lock(sched->plugins, RTTI_LOCK_READ); - rc = key_find(sched->plugins, job_pick_plugin, (void**)&job_picker); - unlock(sched->plugins); + xce_scheduler_t* sched = (xce_scheduler_t*)arg; + xce_sched_plugin_t* job_picker = NULL; + int rc = 0; + rc = sched_resolve_job_picker(sched, &job_picker); if (rc < 0) { + ERROR("%s failed to resolve job picker plugin '%s' (rc=%d)", __func__, + sched->job_pick_plugin ? sched->job_pick_plugin : "(null)", rc); errno = -rc; return NULL; } - if ((job_picker == NULL) || (job_picker->job_elect_fn == NULL)) { - return NULL; + if (job_picker->job_elect_fn != NULL) { + return job_picker->job_elect_fn(job_picker); } - return job_picker->job_elect_fn(job_picker); + while (1) { + int go = 0; + uint64_t jobid = 0; + + pthread_testcancel(); + + /* Cancel-safety (Req 1 AC 5, Req 3 AC 5): cbuf_get()'s internal + * blocking wait is pthread_cond_wait() (src/lib/mylock.c), a + * POSIX cancellation point -- this remains a valid, defined + * cancel point for the picker thread between sessions, the same + * property plugins/fcfs.c's own fcfs_elect_job() already relies on + * for its identical cbuf_get(&sched->wbuf) call. */ + cbuf_get(&(sched->wbuf), &go); + + job_picker->picker_iface->begin_session(job_picker); + + while (1) { + xce_sched_job_t* job = NULL; + + pthread_testcancel(); + + job = job_picker->picker_iface->next_job(job_picker); + if (job == NULL) { + break; + } + + jobid = job->jobid; + cbuf_put(&(sched->jbuf), &jobid); + } + + /* end of scheduling session */ + jobid = 0; + cbuf_put(&(sched->jbuf), &jobid); + + job_picker->picker_iface->end_session(job_picker); + } + + return NULL; } static int sched_make_seqjob_allocation(xce_scheduler_t* sched, @@ -220,16 +290,36 @@ static int commit_decision(xce_scheduler_t* sched, xce_decision_t* decision) static void* sched_allocator(void* arg) { - xce_scheduler_t* sched = (xce_scheduler_t*)arg; + xce_scheduler_t* sched = (xce_scheduler_t*)arg; + xce_sched_plugin_t* job_picker = NULL; + int rc = 0; sel_register_alloc_thread(); xce_use(sched); + + /* Req 1 AC 4 (scheduler-core spec, task 2.4): the allocator thread's + * pseudocode calls picker->on_dispatch()/picker->on_pend() after each + * allocation attempt -- resolve the same picker sched_job_picker() + * resolves (one implementation, two call sites, scheduler.h). By the + * time this thread is created, sched_init() has already gated on this + * exact resolution succeeding (before either thread is started), so + * failure here should not occur in practice; handled the same + * fail-return-NULL way sched_job_picker() handles it, for symmetry. */ + rc = sched_resolve_job_picker(sched, &job_picker); + if (rc < 0) { + ERROR("%s failed to resolve job picker plugin '%s' (rc=%d)", __func__, + sched->job_pick_plugin ? sched->job_pick_plugin : "(null)", rc); + errno = -rc; + + return NULL; + } + while (1) { uint64_t jobid; char jobidString[64]; xce_decision_t* decision = NULL; xce_sched_job_t* job = NULL; - int rc = 0; + int alloc_rc = 0; pthread_testcancel(); @@ -251,14 +341,22 @@ static void* sched_allocator(void* arg) plugin_job_election(sched, job); /* calculate decision for jobid */ - rc = sched_make_allocation(sched, job->schedobj); - if (rc < 0) { + alloc_rc = sched_make_allocation(sched, job->schedobj); + if (alloc_rc < 0) { /* something wrong */ continue; - } else if (rc == 0) { + } else if (alloc_rc == 0) { /* job is pend */ xce_pendreason_t* reason = job->schedobj->reason; + /* Req 1 AC 4 / Req 3 AC 2 "on_pend" row: keep the job as a + * candidate for the next round (no-op for builtin_fcfs; the + * legacy job_elect_fn path's picker_iface is + * legacy_picker_wrapper_iface, whose on_pend is also a no-op + * -- fcfs.c's own removal happens via its separate commit_hook, + * untouched by this call). */ + job_picker->picker_iface->on_pend(job_picker, jobid); + if (reason == NULL) { fmt_jobid(jobidString, sizeof(jobidString), jobid); WARN("job %s is pending but pend reason is NULL", jobidString); @@ -267,6 +365,11 @@ static void* sched_allocator(void* arg) decision = make_pend_decision(reason); } } else { + /* Req 1 AC 4 / Req 3 AC 2 "on_dispatch" row: remove the job + * from the picker's candidate set permanently, before the + * decision is built (matches Req 1 AC 4's pseudocode order). */ + job_picker->picker_iface->on_dispatch(job_picker, jobid); + MOVE_PTR(job->allocs, job->schedobj->alloc); job->allocs->jobid = job->jobid; @@ -353,6 +456,12 @@ static void sched_pend_update_killer(data_item_t* it) static int sched_register_plugins(xce_scheduler_t* sched) { register_fcfs(sched); + /* Req 3 AC 1 (scheduler-core spec, task 2.4): register the new native + * picker under "builtin_fcfs" alongside the untouched legacy + * "xce_sched_fcfs" -- becoming the *default* selection is + * sched_set_default_job_pick_plugin()/sched_resolve_job_picker()'s job + * (sched_init(), below), not this function's. */ + register_builtin_fcfs(sched); register_limit(sched); register_seqjob_allocator(sched); register_parjob_allocator(sched); @@ -382,6 +491,12 @@ int sched_init(xce_scheduler_t* sched, xce_client_config_t* config, int rc = 0; int ndecisions = 5; /* FIXME: make this configurable */ int njobs = 1; /* FIXME: make this configurable */ + /* Req 1 AC 6 / Req 2 AC 5 (scheduler-core spec, task 2.4): resolved + * picker plugin, validated synchronously below before the picker + * thread is created. Only used for the validation side-effect here -- + * the picker thread (sched_job_picker) re-resolves it independently at + * thread start (scheduler.h's sched_resolve_job_picker doc comment). */ + xce_sched_plugin_t* resolved_picker = NULL; kvmgr_params_t jobs_params = { .key_type = KVMGR_KEY_INT, .nslots = 5381, /* FIXME */ .delta = 1024, @@ -460,6 +575,28 @@ int sched_init(xce_scheduler_t* sched, xce_client_config_t* config, sched_register_plugins(sched); + /* Req 1 AC 6 / Req 2 AC 1, AC 5, Req 3 AC 1 (scheduler-core spec, task + * 2.4): default sched->job_pick_plugin to "builtin_fcfs" when the + * operator left it unset (task 1.3's sched_set_default_job_pick_plugin(), + * deliberately not wired into sched_init() before this task -- see + * tasks.md's Implementation Notes), then resolve and validate the + * configured name against the plugin registry BEFORE any thread that + * depends on it is created. An unregistered or non-picker name fails + * scheduler startup here, synchronously and loudly, instead of + * surfacing later as a silent NULL return from the picker thread. + * Placed before the network/HA join below so a bad picker config fails + * fast, without first paying for a controller connection attempt. */ + sched_set_default_job_pick_plugin(sched); + + rc = sched_resolve_job_picker(sched, &resolved_picker); + if (rc < 0) { + ERROR("%s configured picker plugin '%s' failed to resolve as a " + "valid registered picker (rc=%d)", + __func__, + sched->job_pick_plugin ? sched->job_pick_plugin : "(null)", rc); + return rc; + } + rc = xcecli_ctx_init(&(sched->apictx), config); if (rc < 0) { return rc; diff --git a/src/scheduler/testsuite/meson.build b/src/scheduler/testsuite/meson.build index e0c137ef..d5b6cfdf 100644 --- a/src/scheduler/testsuite/meson.build +++ b/src/scheduler/testsuite/meson.build @@ -118,6 +118,22 @@ sched_picker_fcfs_session_test = executable('sched_picker_fcfs_session_test', dependencies: [scheduler_test_deps], install: false) +# White-box test for wiring builtin_fcfs in as the registered default picker +# and removing the hardcoded "xce_sched_fcfs" picker string (scheduler-core +# spec, task 2.4). It #includes ../src/plugin.c, ../src/schedobj.c, and +# ../src/plugins/builtin_fcfs.c directly (same white-box pattern as the +# other picker tests) so it exercises the real plugin_register(), +# register_builtin_fcfs(), sched_set_default_job_pick_plugin(), and +# sched_resolve_job_picker() together -- not a re-implementation of any of +# them. See the file's own header comment for why sched_job_picker() itself +# (scheduler.c) is covered at the resolved-plugin level rather than by +# whole-file-including scheduler.c. +sched_picker_wiring_test = executable('sched_picker_wiring_test', + ['test_picker_wiring.c'], + include_directories:[incdir], + dependencies: [scheduler_test_deps], + install: false) + # Register as meson tests so `meson test --suite scheduler` runs them all. # detect_leaks=0 matches the suite convention (fixtures are not torn down). test('scheduler test', scheduler_test, suite: 'scheduler', @@ -140,3 +156,5 @@ test('scheduler picker fcfs test', sched_picker_fcfs_test, suite: 'scheduler', env: ['ASAN_OPTIONS=detect_leaks=0']) test('scheduler picker fcfs session test', sched_picker_fcfs_session_test, suite: 'scheduler', env: ['ASAN_OPTIONS=detect_leaks=0']) +test('scheduler picker wiring test', sched_picker_wiring_test, suite: 'scheduler', + env: ['ASAN_OPTIONS=detect_leaks=0']) diff --git a/src/scheduler/testsuite/test_picker_fcfs.c b/src/scheduler/testsuite/test_picker_fcfs.c index 1f9c6ac6..7bc23cc5 100644 --- a/src/scheduler/testsuite/test_picker_fcfs.c +++ b/src/scheduler/testsuite/test_picker_fcfs.c @@ -53,6 +53,21 @@ int sched_job_is_ready(xce_sched_job_t* j) return 1; } +/* Task 2.4 added register_builtin_fcfs() (bottom of builtin_fcfs.c), which + * calls plugin_register() (plugin.c, not linked into this white-box TU -- + * this file's own tests never registered anything before task 2.4 either). + * Stub matching plugin.c's real signature/calling convention closely + * enough to satisfy the linker; this file's tests (tasks 2.1/2.2) don't + * exercise registration, so no behavior beyond "don't crash" is needed -- + * test_picker_wiring.c (task 2.4) is where register_builtin_fcfs() is + * exercised against the REAL plugin_register(). */ +int plugin_register(void* arg1, void* arg2) +{ + (void)arg1; + (void)arg2; + return 0; +} + #include "../src/plugins/builtin_fcfs.c" void setUp(void) { return; } diff --git a/src/scheduler/testsuite/test_picker_fcfs_session.c b/src/scheduler/testsuite/test_picker_fcfs_session.c index 773faed3..6479e98c 100644 --- a/src/scheduler/testsuite/test_picker_fcfs_session.c +++ b/src/scheduler/testsuite/test_picker_fcfs_session.c @@ -62,6 +62,19 @@ int sched_queue_is_ready(xce_sched_queue_t* q) int sched_job_is_ready(xce_sched_job_t* j) { return (j != g_not_ready_job); } +/* Task 2.4 added register_builtin_fcfs() (bottom of builtin_fcfs.c), which + * calls plugin_register() (plugin.c, not linked into this white-box TU). + * Stub to satisfy the linker -- this file's tests (task 2.3's session/ + * post-allocation hooks) don't exercise registration; see + * test_picker_wiring.c (task 2.4) for coverage against the REAL + * plugin_register(). */ +int plugin_register(void* arg1, void* arg2) +{ + (void)arg1; + (void)arg2; + return 0; +} + #include "../src/plugins/builtin_fcfs.c" void setUp(void) { return; } diff --git a/src/scheduler/testsuite/test_picker_wiring.c b/src/scheduler/testsuite/test_picker_wiring.c new file mode 100644 index 00000000..8e107652 --- /dev/null +++ b/src/scheduler/testsuite/test_picker_wiring.c @@ -0,0 +1,491 @@ +/* + * test_picker_wiring.c - white-box tests for wiring builtin_fcfs in as the + * registered default picker and removing the hardcoded picker string + * (scheduler-core spec, task 2.4, Req 1 AC 6, Req 2 AC 5, Req 3 AC 1). + * + * RED: before task 2.4, register_builtin_fcfs() did not exist (declared in + * plugin.h but no body in builtin_fcfs.c) -- this file failed to link. + * sched_resolve_job_picker() (schedobj.c, task 2.4) existed but nothing + * had ever registered "builtin_fcfs" for it to find, so the "default + * resolves to builtin_fcfs" half of this file's coverage could not pass; + * sched_job_picker() (scheduler.c) still hardcoded "xce_sched_fcfs". + * GREEN: register_builtin_fcfs() registers builtin_fcfs's native + * xce_job_picker_iface_t under "builtin_fcfs" via the real plugin_register() + * (job_elect_fn stays NULL -- no legacy-wrapper substitution); + * sched_set_default_job_pick_plugin() + sched_resolve_job_picker() together + * resolve an unset job_pick_plugin to the registered builtin_fcfs plugin; an + * unregistered configured name fails resolution with -ENOENT (distinct from + * -EINVAL for "registered but not a valid picker" and "unset name") rather + * than silently falling back to anything. + * + * Scope note (this file does NOT drive sched_job_picker() itself): that + * function is `static` in scheduler.c, which has a much larger external + * dependency surface (xcecli_ctx_*, sel_register_alloc_thread, resource.c's + * allocation path, decision.c's decision constructors, ...) than any other + * white-box test in this directory pulls in -- no existing test #includes + * scheduler.c directly (test.c only pulls in resource.c), and doing so here + * would mean stubbing on the order of twenty external symbols this file's + * tests would never actually exercise, just to satisfy the linker. Per the + * task's own guidance to prefer a tractable direct test over spinning real + * threads, this file instead directly tests the exact condition + * sched_job_picker() branches on + * (`job_picker->job_elect_fn != NULL` selects the legacy delegation path, + * `== NULL` selects the generic picker_iface loop): resolving a + * legacy-registered plugin (job_elect_fn set, no picker_iface of its own) + * comes back with job_elect_fn non-NULL and picker_iface pointing at + * plugin.c's legacy_picker_wrapper_iface (visible in this TU -- see below); + * resolving builtin_fcfs comes back with job_elect_fn NULL and a fully + * populated native picker_iface. That is the entire input the branch reads. + * + * #include's the real plugin.c, schedobj.c, and plugins/builtin_fcfs.c + * (white-box, same convention as test_plugin_picker_registration.c / + * test_picker_config_fields.c / test_picker_fcfs.c) so this test exercises + * the real plugin_register(), the real sched_set_default_job_pick_plugin()/ + * sched_resolve_job_picker(), and the real register_builtin_fcfs() -- not a + * re-implementation of any of them. No symbol name collisions exist between + * the three files (verified: distinct static helper names throughout). + */ +#include +#include +#include +#include +#include + +#include +#include + +#include "kvmgr.h" +#include "scheduler.h" +#include "xstring.h" + +/* ---------------------------------------------------------------------- */ +/* Stubs for externs referenced by plugin.c/schedobj.c/builtin_fcfs.c that + * are defined in other .c files not linked into this whole-file-#include + * TU. None of this file's tests exercise these paths -- present only to + * satisfy the linker, same stub set (minus plugin_job_update/ + * plugin_queue_update/plugin_job_rusage_update/plugin_prepare_allocation, + * which the REAL plugin.c now provides in this TU) as + * test_picker_config_fields.c uses for its own schedobj.c-only include. + * ---------------------------------------------------------------------- */ +xce_sched_user_t* sched_get_user(xce_scheduler_t* s, char* n) +{ + (void)s; + (void)n; + return NULL; +} +xce_sched_project_t* sched_get_project(xce_scheduler_t* s, char* n) +{ + (void)s; + (void)n; + return NULL; +} +xce_sched_queue_t* sched_get_queue(xce_scheduler_t* s, char* n) +{ + (void)s; + (void)n; + return NULL; +} +xce_sched_job_t* sched_get_job(xce_scheduler_t* s, uint64_t jobid) +{ + (void)s; + (void)jobid; + return NULL; +} +xce_job_account_t* make_job_account(xce_job_alloc_t* a) +{ + (void)a; + return NULL; +} +void free_job_account(xce_job_account_t* a) { (void)a; } +int sched_add_user(xce_scheduler_t* s, xce_sched_user_t* u) +{ + (void)s; + (void)u; + return 0; +} +int sched_add_project(xce_scheduler_t* s, xce_sched_project_t* p) +{ + (void)s; + (void)p; + return 0; +} +void sched_worker_release_host_ctx(xce_sched_worker_t* w) { (void)w; } + +/* plugin.c's plugin_prepare_allocation() (a category unrelated to picker + * resolution) calls this -- stub matching test_plugin_picker_registration.c's + * own. */ +int sched_fill_limit_requests(xce_scheduler_t* sched, xvec_idval_t* req, + xlist_t* limits) +{ + (void)sched; + (void)req; + (void)limits; + return 0; +} + +/* builtin_fcfs.c's next_job() (compiled into this TU even though this + * file's tests never call it) calls these -- always-ready stub, same + * convention as test_picker_fcfs.c. */ +int sched_queue_is_ready(xce_sched_queue_t* q) +{ + (void)q; + return 1; +} +int sched_job_is_ready(xce_sched_job_t* j) +{ + (void)j; + return 1; +} + +/* Pull in the real implementations under test. */ +#include "../src/plugin.c" +#include "../src/plugins/builtin_fcfs.c" +#include "../src/schedobj.c" + +void setUp(void) { return; } +void tearDown(void) { return; } + +/* ---------------------------------------------------------------------- */ +/* Fixture: a real sched->plugins kvmgr (same setup scheduler.c's + * sched_plugin_init() uses) plus the xstring/CSE intern tables + * sched_set_default_job_pick_plugin()'s makeString() call needs. Nothing is + * torn down between tests (suite convention, ASAN_OPTIONS=detect_leaks=0). */ +/* ---------------------------------------------------------------------- */ +static void fixture_init(xce_scheduler_t* sched) +{ + kvmgr_params_t params = { .key_type = KVMGR_KEY_STRING, + .nslots = 16, + .delta = 8, + .datakiller = plugin_unregister }; + + memset(sched, 0, sizeof(*sched)); + resreq_init(1); + xstring_table_init(1024 * 16); + sched->plugins = makeInstance(&xkvmgr_type, ¶ms); + TEST_ASSERT_NOT_NULL(sched->plugins); +} + +/* Minimal legacy (job_elect_fn-only) plugin, mirroring + * test_plugin_picker_registration.c's legacy_plugin_init -- a standalone + * stand-in for plugins/fcfs.c's actual shape, not a dependency on fcfs.c + * itself. */ +static void* legacy_job_elect_fn_stub(void* arg) +{ + (void)arg; + return NULL; +} + +static xce_sched_plugin_t* make_legacy_plugin(char* name) +{ + xce_sched_plugin_t* plugin + = (xce_sched_plugin_t*)calloc(1, sizeof(*plugin)); + + TEST_ASSERT_NOT_NULL(plugin); + plugin->name = name; + plugin->job_elect_fn = (plugin_job_election_fn_t)legacy_job_elect_fn_stub; + + return plugin; +} + +/* ---------------------------------------------------------------------- */ +/* Req 3 AC 1: register_builtin_fcfs() */ +/* ---------------------------------------------------------------------- */ + +static void test_register_builtin_fcfs_registers_under_name_builtin_fcfs(void) +{ + xce_scheduler_t sched; + xce_sched_plugin_t* found = NULL; + int rc; + + fixture_init(&sched); + + rc = register_builtin_fcfs(&sched); + TEST_ASSERT_GREATER_OR_EQUAL(0, rc); + + lock(sched.plugins, RTTI_LOCK_READ); + rc = key_find(sched.plugins, "builtin_fcfs", (void**)&found); + unlock(sched.plugins); + + TEST_ASSERT_GREATER_THAN(0, rc); + TEST_ASSERT_NOT_NULL(found); + TEST_ASSERT_EQUAL_STRING("builtin_fcfs", found->name); +} + +/* builtin_fcfs registers as a NATIVE picker: picker_iface non-NULL, + * job_elect_fn NULL -- this is the exact input sched_job_picker()'s branch + * condition reads (see this file's own header comment). Also confirms + * every MUST hook from tasks 2.1-2.3 is wired, and on_config/on_job_rusage + * are NULL per Req 3 AC 2's table. */ +static void test_register_builtin_fcfs_is_native_not_legacy(void) +{ + xce_scheduler_t sched; + xce_sched_plugin_t* found = NULL; + + fixture_init(&sched); + TEST_ASSERT_GREATER_OR_EQUAL(0, register_builtin_fcfs(&sched)); + + lock(sched.plugins, RTTI_LOCK_READ); + key_find(sched.plugins, "builtin_fcfs", (void**)&found); + unlock(sched.plugins); + + TEST_ASSERT_NOT_NULL(found); + TEST_ASSERT_NULL(found->job_elect_fn); + TEST_ASSERT_NOT_NULL(found->picker_iface); + TEST_ASSERT_EQUAL_PTR(builtin_fcfs_next_job, found->picker_iface->next_job); + TEST_ASSERT_EQUAL_PTR(builtin_fcfs_begin_session, + found->picker_iface->begin_session); + TEST_ASSERT_EQUAL_PTR(builtin_fcfs_end_session, + found->picker_iface->end_session); + TEST_ASSERT_EQUAL_PTR(builtin_fcfs_on_dispatch, + found->picker_iface->on_dispatch); + TEST_ASSERT_EQUAL_PTR(builtin_fcfs_on_pend, found->picker_iface->on_pend); + TEST_ASSERT_EQUAL_PTR(builtin_fcfs_on_queue_update, + found->picker_iface->on_queue_update); + TEST_ASSERT_EQUAL_PTR(builtin_fcfs_on_job_push, + found->picker_iface->on_job_push); + TEST_ASSERT_EQUAL_PTR(builtin_fcfs_on_job_state_change, + found->picker_iface->on_job_state_change); + TEST_ASSERT_EQUAL_PTR(builtin_fcfs_on_job_update, + found->picker_iface->on_job_update); + TEST_ASSERT_NULL(found->picker_iface->on_config); + TEST_ASSERT_NULL(found->picker_iface->on_job_rusage); +} + +/* register_builtin_fcfs() wires init_fn/fini_fn adapters into the EXISTING + * plugin_register()/plugin_unregister() call sites (see builtin_fcfs.c's + * own comment on why) -- confirms init actually ran: plugin->data is + * non-NULL immediately after registration, not left NULL until some other + * call. */ +static void test_register_builtin_fcfs_runs_init_via_init_fn_adapter(void) +{ + xce_scheduler_t sched; + xce_sched_plugin_t* found = NULL; + + fixture_init(&sched); + TEST_ASSERT_GREATER_OR_EQUAL(0, register_builtin_fcfs(&sched)); + + lock(sched.plugins, RTTI_LOCK_READ); + key_find(sched.plugins, "builtin_fcfs", (void**)&found); + unlock(sched.plugins); + + TEST_ASSERT_NOT_NULL(found); + TEST_ASSERT_NOT_NULL(found->data); +} + +/* ---------------------------------------------------------------------- */ +/* Req 1 AC 6 / Req 2 AC 1 / Req 2 AC 5: default resolution + fail-loud */ +/* ---------------------------------------------------------------------- */ + +/* tasks.md Observable, half 1: "starting the scheduler with no picker + * config uses builtin_fcfs automatically." */ +static void test_unset_job_pick_plugin_resolves_to_builtin_fcfs_by_default(void) +{ + xce_scheduler_t sched; + xce_sched_plugin_t* resolved = NULL; + int rc; + + fixture_init(&sched); + TEST_ASSERT_GREATER_OR_EQUAL(0, register_builtin_fcfs(&sched)); + TEST_ASSERT_NULL(sched.job_pick_plugin); + + sched_set_default_job_pick_plugin(&sched); + TEST_ASSERT_EQUAL_STRING("builtin_fcfs", sched.job_pick_plugin); + + rc = sched_resolve_job_picker(&sched, &resolved); + + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_NOT_NULL(resolved); + TEST_ASSERT_EQUAL_STRING("builtin_fcfs", resolved->name); + /* This is the exact condition sched_job_picker() (scheduler.c) branches + * on: NULL job_elect_fn selects the generic picker_iface loop. */ + TEST_ASSERT_NULL(resolved->job_elect_fn); + TEST_ASSERT_NOT_NULL(resolved->picker_iface); +} + +/* tasks.md Observable, half 2: "starting it with an unregistered picker + * name fails startup with a clear error instead of silently falling + * back." -ENOENT specifically (distinct from -EINVAL below) -- + * sched_resolve_job_picker's documented contract (scheduler.h). */ +static void +test_unregistered_configured_name_fails_with_enoent_not_silent_fallback(void) +{ + xce_scheduler_t sched; + xce_sched_plugin_t* resolved = (xce_sched_plugin_t*)0x1; /* sentinel */ + int rc; + + fixture_init(&sched); + TEST_ASSERT_GREATER_OR_EQUAL(0, register_builtin_fcfs(&sched)); + + sched.job_pick_plugin = "totally_unregistered_picker_name"; + + rc = sched_resolve_job_picker(&sched, &resolved); + + TEST_ASSERT_EQUAL_INT(-ENOENT, rc); + /* Not a silent fallback to builtin_fcfs (still registered) or to + * whatever the sentinel was -- *out is reset to NULL on any failure + * path (schedobj.c's sched_resolve_job_picker, `*out = NULL;` up + * front). */ + TEST_ASSERT_NULL(resolved); +} + +/* A DIFFERENT failure mode than "not registered at all": the name IS + * registered, but the registered plugin is neither a native picker nor a + * legacy job_elect_fn plugin (e.g. today's limit/allocator categories) -- + * -EINVAL, not -ENOENT. Confirms the two failure modes are genuinely + * distinguishable, not the same generic "no" collapsed into one code. */ +static void test_registered_but_not_a_picker_fails_with_einval_not_enoent(void) +{ + xce_scheduler_t sched; + xce_sched_plugin_t* non_picker = NULL; + xce_sched_plugin_t* resolved = NULL; + int rc; + + fixture_init(&sched); + + non_picker = (xce_sched_plugin_t*)calloc(1, sizeof(*non_picker)); + TEST_ASSERT_NOT_NULL(non_picker); + non_picker->name = "not_a_picker"; + /* job_elect_fn and picker_iface both left NULL -- today's + * allocator/limit plugin shape (register_fcfs's own sibling + * categories), which plugin_register() leaves untouched (task 1.2). */ + TEST_ASSERT_GREATER_OR_EQUAL(0, plugin_register(&sched, non_picker)); + + sched.job_pick_plugin = "not_a_picker"; + + rc = sched_resolve_job_picker(&sched, &resolved); + + TEST_ASSERT_EQUAL_INT(-EINVAL, rc); + TEST_ASSERT_NULL(resolved); +} + +/* Unset job_pick_plugin, and the caller never called + * sched_set_default_job_pick_plugin() either -- -EINVAL (scheduler.h's + * documented contract), distinct from both cases above. */ +static void +test_unset_job_pick_plugin_without_defaulting_fails_with_einval(void) +{ + xce_scheduler_t sched; + xce_sched_plugin_t* resolved = NULL; + int rc; + + fixture_init(&sched); + TEST_ASSERT_NULL(sched.job_pick_plugin); + + rc = sched_resolve_job_picker(&sched, &resolved); + + TEST_ASSERT_EQUAL_INT(-EINVAL, rc); + TEST_ASSERT_NULL(resolved); +} + +/* ---------------------------------------------------------------------- */ +/* sched_job_picker()'s legacy-vs-native branch condition (scheduler.c) -- + * see this file's header comment for why this is tested at the resolved- + * plugin level rather than by driving the picker thread itself. + * ---------------------------------------------------------------------- */ + +/* Legacy path input: a job_elect_fn-only plugin, once registered and + * resolved, comes back with job_elect_fn set AND picker_iface pointing at + * plugin.c's legacy_picker_wrapper_iface (visible in this TU -- same + * assertion test_plugin_picker_registration.c makes for plugin_register() + * itself). This is exactly the condition + * `if (job_picker->job_elect_fn != NULL) return + * job_picker->job_elect_fn(job_picker);` in sched_job_picker() reads to + * choose the "delegate to fcfs_elect_job-style forever loop" path. */ +static void +test_resolved_legacy_plugin_has_job_elect_fn_set_selecting_legacy_branch(void) +{ + xce_scheduler_t sched; + xce_sched_plugin_t* legacy = NULL; + xce_sched_plugin_t* resolved = NULL; + int rc; + + fixture_init(&sched); + legacy = make_legacy_plugin("test_legacy_picker_for_wiring"); + TEST_ASSERT_GREATER_OR_EQUAL(0, plugin_register(&sched, legacy)); + + sched.job_pick_plugin = "test_legacy_picker_for_wiring"; + + rc = sched_resolve_job_picker(&sched, &resolved); + + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_NOT_NULL(resolved); + TEST_ASSERT_NOT_NULL(resolved->job_elect_fn); + TEST_ASSERT_EQUAL_PTR(&legacy_picker_wrapper_iface, resolved->picker_iface); +} + +/* Native path input: builtin_fcfs, once registered and resolved, comes + * back with job_elect_fn NULL -- the condition that selects the generic + * picker_iface session loop instead. Distinct test from the "default" + * test above: this one asserts on an EXPLICITLY configured name, not the + * unset-defaults-to-builtin_fcfs path, isolating the branch condition + * itself from the defaulting behavior. */ +static void +test_resolved_builtin_fcfs_has_job_elect_fn_null_selecting_native_branch(void) +{ + xce_scheduler_t sched; + xce_sched_plugin_t* resolved = NULL; + int rc; + + fixture_init(&sched); + TEST_ASSERT_GREATER_OR_EQUAL(0, register_builtin_fcfs(&sched)); + + sched.job_pick_plugin = "builtin_fcfs"; /* explicit, not defaulted */ + + rc = sched_resolve_job_picker(&sched, &resolved); + + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_NOT_NULL(resolved); + TEST_ASSERT_NULL(resolved->job_elect_fn); + TEST_ASSERT_NOT_NULL(resolved->picker_iface); + TEST_ASSERT_EQUAL_PTR(builtin_fcfs_next_job, + resolved->picker_iface->next_job); +} + +/* Both builtin_fcfs and a legacy plugin can be registered simultaneously + * (Req 3 AC 1's "wire builtin_fcfs as the registered default" does not + * mean "the only" -- tasks.md's own builtin_fcfs.c header comment: fcfs.c + * "stays registered ... reachable only by explicit config"). Confirms + * choosing one by name does not disturb the other's own registration or + * resolution. */ +static void +test_legacy_and_native_pickers_coexist_and_resolve_independently(void) +{ + xce_scheduler_t sched; + xce_sched_plugin_t* legacy = NULL; + xce_sched_plugin_t* resolved = NULL; + int rc; + + fixture_init(&sched); + legacy = make_legacy_plugin("xce_sched_fcfs_stub"); + TEST_ASSERT_GREATER_OR_EQUAL(0, plugin_register(&sched, legacy)); + TEST_ASSERT_GREATER_OR_EQUAL(0, register_builtin_fcfs(&sched)); + + sched.job_pick_plugin = "xce_sched_fcfs_stub"; + rc = sched_resolve_job_picker(&sched, &resolved); + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_NOT_NULL(resolved->job_elect_fn); + + sched.job_pick_plugin = "builtin_fcfs"; + rc = sched_resolve_job_picker(&sched, &resolved); + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_NULL(resolved->job_elect_fn); +} + +int main(void) +{ + UNITY_BEGIN(); + RUN_TEST(test_register_builtin_fcfs_registers_under_name_builtin_fcfs); + RUN_TEST(test_register_builtin_fcfs_is_native_not_legacy); + RUN_TEST(test_register_builtin_fcfs_runs_init_via_init_fn_adapter); + RUN_TEST(test_unset_job_pick_plugin_resolves_to_builtin_fcfs_by_default); + RUN_TEST( + test_unregistered_configured_name_fails_with_enoent_not_silent_fallback); + RUN_TEST(test_registered_but_not_a_picker_fails_with_einval_not_enoent); + RUN_TEST(test_unset_job_pick_plugin_without_defaulting_fails_with_einval); + RUN_TEST( + test_resolved_legacy_plugin_has_job_elect_fn_set_selecting_legacy_branch); + RUN_TEST( + test_resolved_builtin_fcfs_has_job_elect_fn_null_selecting_native_branch); + RUN_TEST(test_legacy_and_native_pickers_coexist_and_resolve_independently); + return UNITY_END(); +}