diff --git a/src/docs/resreq-ast-cse-design.md b/src/docs/resreq-ast-cse-design.md index 1339313e..443bef93 100644 --- a/src/docs/resreq-ast-cse-design.md +++ b/src/docs/resreq-ast-cse-design.md @@ -56,7 +56,12 @@ This document describes the design to: implementation. A direct example: when a user submits a large job array, all elements of the array share the same resource requirement string, so CSE allows a single interned AST to represent thousands of - jobs. + jobs. In a busy EDA cluster (10,000 nodes, 10M jobs, 1M/hour +throughput), the number of unique EDA tool configurations is limited +(< 100), so many jobs share the same select expression. Node-level CSE +provides structural sharing even when full resreq strings differ (e.g., +same `select[...]` but different `rusage[...]`), which string-level +interning alone cannot achieve. - Evaluate that AST efficiently in the context of a host's live resource values. - Run a **mark-and-sweep GC** between scheduling sessions to recycle @@ -371,10 +376,17 @@ xce_select_node_t* makeSelectNode(sel_node_type_t type, } int rc3 = key_add(sel_node_table.table, &key, node); if (rc3 < 0) { - /* key_add failed (likely ENOMEM or EEXIST from a race) */ + /* key_add failed — if EEXIST from a race, re-find and return existing */ + if (rc3 == -EEXIST) { + xce_select_node_t* raced = NULL; + int rc4 = key_find(sel_node_table.table, &key, (void**)&raced); + mylock_unlock(&sel_node_table.lock); + free(node); + return (rc4 > 0) ? raced : NULL; /* return existing or NULL on double-failure */ + } mylock_unlock(&sel_node_table.lock); free(node); - return NULL; + return NULL; /* ENOMEM or other error */ } mylock_unlock(&sel_node_table.lock); @@ -409,12 +421,13 @@ gc_sweep_select_nodes() the root node and set the GC mark flag on every reachable node. (If a node is referenced by multiple AST trees, it is marked only once — the flag is boolean.) - 2. Sweep pass: - a. Iterate sel_node_table. - b. For each node whose GC mark flag is NOT set: - - remove the node from the intern table (key_del) - - free the node - c. Clear all GC mark flags. + 2. Sweep pass (two-phase to avoid modifying the hash table during iteration): + a. Iterate sel_node_table using the kvtab iterator interface + (`iter_make`/`iter_next`). Collect keys of unmarked nodes into a + temporary array. + b. For each collected key: remove the node from the intern table + (`key_del`), then free the node. + c. Clear all GC mark flags on remaining (reachable) nodes. ``` This approach is simple and correct: @@ -429,9 +442,16 @@ This approach is simple and correct: The scheduler is responsible for calling `gc_sweep_select_nodes()` between scheduling sessions, typically after all dispatch/defer decisions -for the current batch of jobs have been made. The function is not -thread-safe with respect to concurrent `eval_select` calls, so the -scheduler must ensure no evaluations are in progress when GC runs. +for the current batch of jobs have been made. **Job cleanup +(`primary->select->root = NULL`) also happens between sessions** — +there is no mid-session job deletion. When a job completes, its +entry is marked for removal; the select root pointer is unlinked and +the job struct freed at the session boundary, before GC runs. This +eliminates the risk of shared sub-nodes becoming unreachable mid-session. + +The function is not thread-safe with respect to concurrent +`eval_select` calls, so the scheduler must ensure no evaluations are +in progress when GC runs. Like IBM LSF, the XCE scheduler operates in sessions — each session evaluates all pending jobs against candidate hosts, dispatches what it @@ -582,16 +602,16 @@ always have matching types — the `switch (lv.type)` dispatch in /* action for '==' '!=' — coerce literal to resource type */ if (lnode->type == SEL_RESNAME && rnode->type == SEL_NUM && lnode->u.resname.res_type == RES_TYPE_STRING) { - /* convert numeric literal to string: 5 → "5" */ + /* convert numeric literal to string: 5 → "5", preserving full double precision */ char buf[64]; - snprintf(buf, sizeof(buf), "%g", rnode->u.numval); + snprintf(buf, sizeof(buf), "%.17g", rnode->u.numval); rnode = makeSelectNode(SEL_STR, (uintptr_t)makeString(buf), 0); } /* symmetric: coerce if right is RESNAME and left is numeric literal */ if (rnode->type == SEL_RESNAME && lnode->type == SEL_NUM && rnode->u.resname.res_type == RES_TYPE_STRING) { char buf[64]; - snprintf(buf, sizeof(buf), "%g", lnode->u.numval); + snprintf(buf, sizeof(buf), "%.17g", lnode->u.numval); lnode = makeSelectNode(SEL_STR, (uintptr_t)makeString(buf), 0); } $$ = (char*)makeSelectNode(SEL_EQ, (uintptr_t)lnode, (uintptr_t)rnode); @@ -639,11 +659,11 @@ typedef struct { ```c static sel_val_t get_leaf_value(const xce_select_node_t* node, - const xvec_idval_t* host_resvals) + const host_eval_ctx_t* host) { switch (node->type) { case SEL_RESNAME: - return host_resval_typed(host_resvals, + return host_resval_typed(host, node->u.resname.res_idx, node->u.resname.res_type); case SEL_NUM: @@ -662,32 +682,49 @@ static sel_val_t get_leaf_value(const xce_select_node_t* node, goes wrong during the visit, `-1` is returned immediately and propagated up — `&&`, `||`, and `!` never silently swallow errors. +**Host evaluation context:** `xvec_idval_t` stores per-job resource +allocation data, not host resource values. The select evaluator needs a +host evaluation context that provides resource values by `res_idx`. A +`host_eval_ctx_t` struct bundles the host's numeric resource vector and +a string resource lookup table: + +```c +typedef struct { + const xvec_idval_t* numvals; /* numeric resource values by res_idx */ + const char** strvals; /* string resource values by res_idx */ +} host_eval_ctx_t; +``` + +The Go CGO bridge populates `host_eval_ctx_t` from the host's live +resource data before each evaluation call. The `strvals` array is +indexed by `res_idx`; entries for non-string resources are NULL. + ```c /* Returns: 1 = host matches, 0 = host does not match, -1 = error */ -int eval_select(const xce_select_node_t* node, const xvec_idval_t* host_resvals) +int eval_select(const xce_select_node_t* node, const host_eval_ctx_t* host) { int lv, rv; switch (node->type) { /* logical — explicit error propagation, not C boolean operators */ case SEL_AND: - lv = eval_select(node->u.binop.left, host_resvals); + lv = eval_select(node->u.binop.left, host); if (lv < 0) return -1; if (lv == 0) return 0; - rv = eval_select(node->u.binop.right, host_resvals); + rv = eval_select(node->u.binop.right, host); if (rv < 0) return -1; return rv; case SEL_OR: - lv = eval_select(node->u.binop.left, host_resvals); + lv = eval_select(node->u.binop.left, host); if (lv < 0) return -1; if (lv == 1) return 1; - rv = eval_select(node->u.binop.right, host_resvals); + rv = eval_select(node->u.binop.right, host); if (rv < 0) return -1; return rv; case SEL_NOT: - lv = eval_select(node->u.operand, host_resvals); + lv = eval_select(node->u.operand, host); if (lv < 0) return -1; return !lv; @@ -695,18 +732,22 @@ int eval_select(const xce_select_node_t* node, const xvec_idval_t* host_resvals) case SEL_TRUE: return 1; + /* SEL_FALSE: const-folded from expressions like `!(type == any)` */ + case SEL_FALSE: + return 0; + /* standalone boolean use of a RESNAME node */ case SEL_RESNAME: switch (node->u.resname.res_type) { - case RES_TYPE_BOOL: return host_get_bool (host_resvals, node->u.resname.res_idx); - case RES_TYPE_NUMERIC: return host_get_float(host_resvals, node->u.resname.res_idx) != 0.0f; + case RES_TYPE_BOOL: return host_get_bool (host, node->u.resname.res_idx); + case RES_TYPE_NUMERIC: return host_get_float(host, node->u.resname.res_idx) != 0.0f; default: return -1; /* string standalone rejected at parse time */ } /* numeric comparisons — operand types validated at parse time */ case SEL_GT: case SEL_LT: case SEL_GE: case SEL_LE: { - double l = get_leaf_value(node->u.binop.left, host_resvals).u.num; - double r = get_leaf_value(node->u.binop.right, host_resvals).u.num; + double l = get_leaf_value(node->u.binop.left, host).u.num; + double r = get_leaf_value(node->u.binop.right, host).u.num; if (node->type == SEL_GT) return l > r; if (node->type == SEL_LT) return l < r; if (node->type == SEL_GE) return l >= r; @@ -715,8 +756,8 @@ int eval_select(const xce_select_node_t* node, const xvec_idval_t* host_resvals) /* type-dispatched equality — type guaranteed compatible at parse time */ case SEL_EQ: case SEL_NE: { - sel_val_t l = get_leaf_value(node->u.binop.left, host_resvals); - sel_val_t r = get_leaf_value(node->u.binop.right, host_resvals); + sel_val_t l = get_leaf_value(node->u.binop.left, host); + sel_val_t r = get_leaf_value(node->u.binop.right, host); int eq; switch (l.type) { case RES_TYPE_NUMERIC: eq = l.u.num == r.u.num; break; @@ -763,11 +804,16 @@ int eval_select(const xce_select_node_t* node, const xvec_idval_t* host_resvals) goroutine** (the scheduler goroutine). `eval_select` accesses interned nodes without holding references — if a concurrent goroutine triggered GC sweep while another was evaluating, a use-after-free would occur. - The mark-and-sweep GC (§5.6) runs only between sessions when the + The mark-and-sweep GC (§5.5) runs only between sessions when the scheduler goroutine is not evaluating, so single-goroutine eval - guarantees safety. If concurrent evaluation is needed in the future, + guarantees safety. +- **Runtime enforcement:** The Go layer should include a runtime assertion + that panics if `EvalSelectForHost` is called from a goroutine other than + the one that initialized the C state. This catches misuse at + development time rather than producing silent data corruption in + production. If concurrent evaluation is needed in the future, an eval-time reference count (increment before eval, decrement after) - must be added. + must be added to the C API. --- @@ -803,7 +849,7 @@ Within a single scheduling session: ``` for each pending_job: for each candidate_host: - 1. rc = eval_select(job.resreq.primary.select, host.resvals) + 1. rc = eval_select(job.resreq.primary.select, &host.eval_ctx) → 1 (true): proceed to rusage check → 0 (false): skip host (select filter failed) → -1 (error): log error, skip host — do NOT silently treat as false @@ -879,6 +925,8 @@ signatures and assertions are defined before implementation code.* #### Phase 4: Implementation - `lib/resreq.c` + - Add `#include "kv.h"` (required for `kv_key_ops_t`; not transitively + included via `kvmgr.h`). - `sel_node_table` static with `mylock_t`. - `sel_key_ops_t` for `sel_node_key_t` (hash, cmp, assign, destroy). - `sel_node_table_init(int nslots)` / `sel_node_table_fini()`. @@ -905,16 +953,27 @@ signatures and assertions are defined before implementation code.* #### Phase 6: Go bridge - `backend/clib.go` — replace `EvalSelectForHost` stub with real CGO call. - Must marshal host resource values (`xvec_idval_t*`) from Go to C. + Must marshal a `host_eval_ctx_t` from Go to C, populating both the + numeric resource vector (`numvals`) and the string resource table + (`strvals`, indexed by `res_idx`). The `strvals` array maps `res_idx` + to interned string pointers obtained via `makeString()` for CSE + compatibility. - `controllers/hosts.go` — wire `FilterHostByResReqSelect` into `preCheckHostRequest`. +- **GC trigger:** Add a CGO call `C.gc_sweep_select_nodes()` invoked by + the scheduler goroutine at the end of each scheduling session, after + all dispatch/defer decisions and job cleanup (unlinking select roots) + are complete. This must be a well-defined session boundary — no + `eval_select` calls may be in progress during the sweep. - **CGO concurrency:** `FilterHostByResReqSelect` must be called from a single, pinned scheduler goroutine (§8.4). This is a hard invariant: `eval_select` accesses interned nodes without holding references, so concurrent evaluation while GC sweep runs would cause use-after-free. - If concurrent scheduling is needed in the future, eval-time reference - counting (increment before eval, decrement after) must be added to the - C API. + Add a runtime assertion in the Go layer that panics if + `EvalSelectForHost` is called from a goroutine other than the one that + initialized the C state. If concurrent scheduling is needed in the + future, eval-time reference counting (increment before eval, decrement + after) must be added to the C API. --- @@ -943,6 +1002,7 @@ struct xce_select_node_ { struct { int res_idx; res_type_t res_type; } resname; double numval; char* strval; + int bval; /* SEL_BOOL_LITERAL — 1 for true, 0 for false */ } u; }; ``` @@ -966,6 +1026,15 @@ typedef struct { } sel_val_t; ``` +### Host Evaluation Context + +```c +typedef struct { + const xvec_idval_t* numvals; /* numeric resource values by res_idx */ + const char** strvals; /* string resource values by res_idx */ +} host_eval_ctx_t; +``` + ### Select Parse Context ```c @@ -993,8 +1062,10 @@ static int operand_is_numeric(const xce_select_node_t* node) ### CSE Key Operations The `sel_node_table` uses `kvtab_type` keyed by `sel_node_key_t`. A -custom `kv_key_ops_t` is required since `sel_node_key_t` is a packed -struct, not a string or int: +custom `kv_key_ops_t` is required since `sel_node_key_t` is a struct +with potential padding between `type` and `op1` — `sel_key_hash` and +`sel_key_cmp` must operate on the three fields individually, not by +`memcpy`-ing the entire struct (which would hash padding bytes): ```c static kv_key_ops_t sel_node_key_ops = { @@ -1009,34 +1080,38 @@ These must be defined in `resreq.c` alongside the intern table. ### Host Resource Value Accessors -`eval_select` calls three accessor functions that must be defined: +`eval_select` calls accessor functions that use the `host_eval_ctx_t` +context (not `xvec_idval_t` directly). `xvec_idval_t` stores per-job +resource allocation data; host resource values are retrieved through +the evaluation context's `numvals` and `strvals` members: ```c -/* Retrieve typed value from host resource vector */ -static sel_val_t host_resval_typed(const xvec_idval_t* host_resvals, +/* Retrieve typed value from host evaluation context */ +static sel_val_t host_resval_typed(const host_eval_ctx_t* host, int res_idx, res_type_t res_type); /* Boolean resource: nonzero → true */ -static int host_get_bool(const xvec_idval_t* host_resvals, int res_idx); +static int host_get_bool(const host_eval_ctx_t* host, int res_idx); /* Numeric resource: return float value */ -static double host_get_float(const xvec_idval_t* host_resvals, int res_idx); +static double host_get_float(const host_eval_ctx_t* host, int res_idx); /* String resource: return interned string pointer */ -static const char* host_get_string(const xvec_idval_t* host_resvals, +static const char* host_get_string(const host_eval_ctx_t* host, int res_idx); ``` -**String resource storage:** `xvec_idval_t` stores only `(idx, float)` -pairs for numeric resources. String host resources (e.g., `type`, -`arch`) are stored in a parallel lookup structure alongside each host's -`xvec_idval_t`. When host data is loaded into the scheduler, both the -numeric `xvec_idval_t` vector and a `(res_idx → const char*)` mapping -are populated from the cluster configuration. `libxce` already employs a -de-duplication mechanism that maps identical strings to the same pointer -(via `makeString`), so `host_get_string` returns an interned pointer — -`xce_str_match` (which starts as `strcmp`) can be replaced with pointer -equality when both operands are known-interned. +**String resource storage:** `xvec_idval_t` stores per-job resource +allocation data (not host values). The `host_eval_ctx_t` bundles host +resource data: `numvals` provides numeric and boolean resource values +indexed by `res_idx`, and `strvals` provides string resource values +indexed by `res_idx`. When host data is loaded into the scheduler, both +the numeric vector and the string mapping are populated from the cluster +configuration. `libxce` already employs a de-duplication mechanism that +maps identical strings to the same pointer (via `makeString`), so +`host_get_string` returns an interned pointer — `xce_str_match` (which +starts as `strcmp`) can be replaced with pointer equality when both +operands are known-interned. ### PEG Grammar Preamble