lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

docs: address deep document-review findings for resreq-ast-cse-design

Browse source 
Review by adversarial, feasibility, and product-lens reviewers.
Key changes:

- Add host_eval_ctx_t struct to carry both numeric and string host
  resource values; xvec_idval_t is for job allocation, not host eval
- Fix SEL_FALSE case in eval_select (was returning -1 instead of 0)
- Add bval field to appendix xce_select_node_t struct
- Fix makeSelectNode key_add race (EEXIST now re-finds existing entry)
- Specify two-phase GC sweep to avoid iterator corruption in kvtab
- Add EDA cluster justification for CSE memory savings
- Use %.17g instead of %g for number-to-string coercion (full precision)
- Add #include "kv.h" dependency note for Phase 4
- Add runtime goroutine assertion requirement for CGO bridge safety
- Add gc_sweep_select_nodes() CGO call in Phase 6 session boundary
- Clarify job cleanup happens at session boundaries (no mid-session kill)
- Clarify sel_key_hash/sel_key_cmp must hash fields individually (padding)
This commit is contained in:
Rongsong Shen 2026-04-21 07:05:49 +00:00
parent 98911a2154
commit 19898a198a
1 changed files with 130 additions and 55 deletions
Download patch file Download diff file Expand all files Collapse all files

185
src/docs/resreq-ast-cse-design.md
View file

@ -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