Add mrb_task_set_scheduler_hook for pre-scheduling deferred work. Remove mrb_hal_task_switch_hook instead - #6982
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a per-mrb_state “pre-scheduling” hook (mrb_task_set_scheduler_hook) that runs in thread context immediately before the scheduler reads the ready queue, enabling deterministic deferred-work servicing that can wake a task and have it selected in the same scheduler iteration. It also removes the previously-added HAL switch-hook API (mrb_hal_task_switch_hook) and updates tests and ports accordingly.
Changes:
- Add
mrb_task_set_scheduler_hook(mrb, fn, ud)and store hook state inmrb_statetask scheduler state. - Invoke the hook at scheduler entry points (
task_run_bodyloop top,mrb_task_run_once, and root-contextTask.passhelper) before reading the ready queue. - Remove
mrb_hal_task_switch_hook(and its reason enum) from the HAL contract and delete no-op implementations in posix/win/glib ports; add contract-focused tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| mrbgems/mruby-task/src/task.c | Adds scheduler-hook invocation at pre-ready-queue scheduler entry points; removes switch-hook call sites; implements mrb_task_set_scheduler_hook. |
| mrbgems/mruby-task/include/task.h | Declares the new mrb_task_set_scheduler_hook public API and documents its contract. |
| include/mruby.h | Extends mrb_task_state with the hook function pointer and opaque userdata pointer. |
| mrbgems/mruby-task/include/task_hal.h | Removes the mrb_hal_task_switch_hook API and its mrb_task_switch_reason enum from the HAL contract. |
| mrbgems/mruby-task/ports/posix/task_hal.c | Removes the no-op mrb_hal_task_switch_hook implementation. |
| mrbgems/mruby-task/ports/win/task_hal.c | Removes the no-op mrb_hal_task_switch_hook implementation. |
| mrbgems/mruby-task/ports/glib/task_hal.c | Removes the no-op mrb_hal_task_switch_hook implementation. |
| mrbgems/mruby-task/test/tasktest.c | Adds C helpers used by Ruby tests to install/clear hooks and drive mrb_task_run_once. |
| mrbgems/mruby-task/test/task.rb | Adds deterministic tests for hook firing points, wakeup-before-ready-read semantics, replacement, and clearing behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * - On POSIX: usleep() or nanosleep() | ||
| * - On Windows: Sleep() | ||
| * - On embedded: platform-specific sleep/wait-for-interrupt instruction | ||
| * | ||
| * @param mrb The mruby state (for context, may be unused) | ||
| */ | ||
| void mrb_hal_task_idle_cpu(mrb_state *mrb); | ||
|
|
There was a problem hiding this comment.
The removal is deliberately the second commit of this PR (e45669d0c), so that it can be dropped with a single force-push if the maintainers prefer to keep the switch hook -- "the NEXT commit in this PR performs the removal" in the description refers to that commit. Diff and description are consistent; the two-commit structure IS the mechanism for the keep-vs-remove decision.
I'm adding an explicit "Breaking change" section to the PR description per your suggestion.
## Motivation PicoRuby is building an ISR-to-task event bridge: interrupt handlers only stage work (set flags), and a deferred handler wakes tasks blocked in Task::Queue#pop. This needs a servicing point that runs in thread context BEFORE the scheduler reads the ready queue, so a task woken there is picked up in the same iteration. mrb_hal_task_switch_hook (mruby#6942) cannot serve this, for three reasons: 1. Position: it fires AFTER task execution; waking-then-selecting requires firing before the ready-queue read. 2. Idle coverage: when every task is WAITING, no task switch happens and the hook never fires -- yet "all tasks asleep" is exactly when an ISR event needs to wake one. 3. Ownership: it is a compile-time HAL symbol owned by the port. Embedding code cannot attach deferred work at runtime without patching the port. ## What this patch adds - mrb_task_set_scheduler_hook(mrb, fn, ud): a per-mrb_state hook invoked in thread context right before the ready-queue read, at every scheduler entry: the task_run_body loop top, mrb_task_run_once, and the Task.pass-from-root-context helper. - API Contract: cheap, never sleeps, must not re-enter the scheduler. Cost when unset: one pointer check per scheduler iteration. - Single-owner contract: one hook per mrb_state; setting a new hook replaces the previous one, and composing multiple consumers is the embedder's responsibility (no registry in mruby). The caller owns the lifetime of ud. - No behavior change for existing code. - Tests (mruby-task/test): the four contract properties, made deterministic without timing assumptions -- the hook fires at all three scheduler entries (Task.run, mrb_task_run_once via a C helper, and root-context Task.pass); an item the hook pushes into a Task::Queue wakes a blocked task within a SINGLE Task.pass, which discriminates the before-ready-read placement from an after-read one; setting a new hook replaces the previous; NULL clears. ## Relationship to mruby#6942 For continuously pumped schedulers, the scheduler hook provides a corresponding servicing opportunity for every switch-hook control point, with a phase shift: - In the continuous mrb_task_run loop, servicing moves from "right after task execution" to "at the next loop top" -- i.e. after the round-robin requeue and one incremental GC slice. Timeslice-bounded servicing is preserved (including the pending-GC drain loop, which passes the loop top on every step), and the loop top additionally covers idle iterations and the root-context Task.pass entry, which the switch hook never fires on. - In mrb_task_run_once, servicing moves from post-run inside the same call to the entry of the NEXT call. A host that pumps run_once repeatedly still gets one servicing call per invocation, but at the opposite phase; a host that stops pumping gets no final post-run servicing. On the `reason` argument: in mruby#6942 the requirement was that the hook stay extensible without signature breaks, because it fires at several distinct control points; the extensible reason enum answered that. The pre-selection hook meets the same requirement differently: it has a single uniform semantic --- "the scheduler is about to select the next task" --- so there are no control points to discriminate and nothing for a reason to say; per-registrant context travels in ud. Every known implementation ignores reason today. As far as I know, PicoRuby is the only consumer of mrb_hal_task_switch_hook --- mruby#6942 was proposed for CYW43/lwIP pumping in PicoRuby. PicoRuby will migrate that servicing to a registered scheduler hook. Removal of the switch hook is intentionally left to a separate commit, so this additive change stands on its own. The two hooks can alternatively coexist with distinct ownership models: HAL-owned compile-time servicing (switch hook) and runtime-registered embedder work (scheduler hook).
For continuously pumped schedulers, the scheduler hook introduced in the previous commit provides a corresponding servicing opportunity for each switch-hook control point (see the phase-shift discussion in that commit). PicoRuby -- the only known consumer -- will migrate its cyw43/lwIP pumping to a registered scheduler hook. Removes the call sites in task.c, the declaration and the mrb_task_switch_reason enum in task_hal.h, and the empty implementations in the posix/win/glib ports. Ports no longer need to provide this symbol. This removal is kept separate from the scheduler-hook addition so the additive commit remains a standalone change.
50ee2cc to
bc6b582
Compare
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Yes, #6942 was indeed proposed for pumping CYW43/lwIP from the scheduler instead of idle_cpu.
|
|
Replace. Let's go with the scheduler hook alone. Your three arguments hold up. I checked the idle path: when every task is WAITING, Keeping both would leave two hooks for one job, and The tests are good. Waking a queue-blocked task within a single @sylph01, this removes the hook from #6942. Please say so if CYW43/lwIP pumping needs something the scheduler hook cannot express; the phase shift from post-execution to next-loop-top is the part worth checking on your side. |
## Motivation PicoRuby is building an ISR-to-task event bridge: interrupt handlers only stage work (set flags), and a deferred handler wakes tasks blocked in Task::Queue#pop. This needs a servicing point that runs in thread context BEFORE the scheduler reads the ready queue, so a task woken there is picked up in the same iteration. Today mruby/c has no such point: mrbc_hal_idle_cpu() runs only when the ready queue is empty, and nothing runs before task selection. This mirrors mruby's mrb_task_set_scheduler_hook (mruby/mruby#6982); PicoRuby runs both VMs and keeps their scheduler surfaces symmetric. ## How it works (UART RX as the example) The ISR does two things only, neither of which touches the VM: void uart_rx_isr(void) { ring_buffer_put(uart_getc()); /* data into a plain C buffer */ rx_flag = 1; /* volatile flag: "something arrived" */ } The hook is the deferred second half: void my_hook(void *ud) { if (rx_flag) { rx_flag = 0; mrbc_task_queue_push(&q, &ev); /* wakes the task blocked in q.pop */ } } /* registered once at init */ mrbc_task_set_scheduler_hook(my_hook, NULL); Timeline for the important case -- every task asleep: task B: ev = q.pop .......... queue empty, parks as WAITING scheduler: no READY task -> mrbc_hal_idle_cpu() | UART interrupt fires! ISR: byte into buffer, rx_flag = 1, returns (VM untouched) | scheduler: returns from idle, reaches the loop top hook: sees rx_flag -> queue push -> task B becomes READY scheduler: reads the ready queue right after -> picks B task B: returns from q.pop, drains the ring buffer An interrupt can fire at any moment -- possibly mid-allocation -- so an ISR must never push into a Task::Queue. The hook runs at a moment where the VM is by construction quiescent (between tasks, right before the ready-queue read), so it carries the ISR's note across to the safe side: the ISR leaves a mark, the hook performs the regular push. ## Why pushing from the hook is safe - Thread context: mrbc_task_queue_push forbids interrupt context and VM re-entry (see the comment in c_task_queue.c); the hook's call sites are the bodies of mrbc_run/mrbc_run_step, which are neither. - No allocation is in flight at the loop top, so the push's array growth cannot corrupt an interrupted allocation. - The TCB relinking inside the push runs under the same mrbc_hal_disable_irq/enable_irq exclusion as any thread-context caller; the only concurrent mutator is the tick IRQ, which that exclusion covers. - No flag_preemption is needed: the very next statement reads the ready queue, so a task woken here is selected immediately. ## What this adds - mrbc_task_set_scheduler_hook(fn, ud): a process-global hook invoked in thread context right before the ready-queue read, at both scheduler entries (the mrbc_run loop top and mrbc_run_step). mruby's hook is per-mrb_state; mruby/c's is process-global because the scheduler queues are process-global (VMs live per TCB). - The whole feature sits behind #if defined(MRBC_TASK_SCHEDULER_HOOK): default builds compile it out entirely -- zero cost and zero behavior change for existing applications and HALs. Enabled builds pay one pointer check per scheduler iteration while no hook is set. - Single-owner contract (documented in rrt0.h): setting replaces, fn == NULL clears, composition is the embedder's concern, the caller owns ud. The hook must be cheap, never sleep, and never re-enter the scheduler. - No HAL involvement: gems/embedders register at init, so no port needs changes. Behavioral tests for the contract live in PicoRuby's integration suite; the mruby side carries equivalent tests in mruby-task's own mrbtest suite, while mruby/c's pure-Ruby test harness cannot register a C hook.
mruby/mruby#6982 replaced mrb_hal_task_switch_hook with mrb_task_set_scheduler_hook; the submodule already points at the merged master. Follow up on the PicoRuby side: - picoruby-machine/ports/rp2040/machine.c: the cyw43_arch POLL pump moves from the removed switch hook into a scheduler hook registered in picorb_hal_init (PICORB_VM_MRUBY + PICO_CYW43_ARCH_POLL only). Same cyw43_is_initialized() guard, same cheap/no-sleep contract. The scheduler hook fires at every scheduler entry, so the busy-path pumping cadence is preserved and idle iterations now pump as well. - picoruby-mruby/ports/rp2040/task.c, ports/esp32/task.c: removed; they only implemented the deleted HAL symbol. Verified builds: rake picoruby:debug and femtoruby:debug (host), r2p2:picoruby:pico2_w:debug (mruby + PICO_CYW43_ARCH_POLL -- the affected configuration), and r2p2:picoruby:pico2:debug (no cyw43). On-device revalidation of the #6942 busy-loop/GC-drain scenarios is still pending.
Motivation
PicoRuby is building an ISR-to-task event bridge: interrupt handlers only stage work (set flags), and a deferred handler wakes tasks blocked in Task::Queue#pop. This needs a servicing point that runs in thread context BEFORE the scheduler reads the ready queue, so a task woken there is picked up in the same iteration.
mrb_hal_task_switch_hook (#6942) cannot serve this, for three reasons:
What this patch adds
Cost when unset: one pointer check per scheduler iteration.
Task::Queue wakes a blocked task within a SINGLE Task.pass, which discriminates the before-ready-read placement from an after-read one; setting a new hook replaces the previous; NULL clears.
Relationship to #6942
For continuously pumped schedulers, the scheduler hook provides a corresponding servicing opportunity for every switch-hook control point, with a phase shift:
On the
reasonargument: in #6942 the requirement was that the hook stay extensible without signature breaks, because it fires at several distinct control points; the extensible reason enum answered that.The pre-selection hook meets the same requirement differently: it has a single uniform semantic --- "the scheduler is about to select the next task" --- so there are no control points to discriminate and nothing for a reason to say; per-registrant context travels in ud.
Every known implementation ignores reason today.
As far as I know, PicoRuby is the only consumer of mrb_hal_task_switch_hook --- #6942 was proposed for CYW43/lwIP pumping in PicoRuby (right?> @sylph01).
PicoRuby will migrate that servicing to a registered scheduler hook.
Removal of the switch hook is intentionally left to a separate commit bc6b582, so this additive change stands on its own.
The two hooks can alternatively coexist with distinct ownership models: HAL-owned compile-time servicing (switch hook) and runtime-registered embedder work (scheduler hook).
Which way would you prefer? @matz cc/ @sylph01
Breaking change (commit 2 only, if this PR is merged as-is)
Commit 2 removes the public HAL API mrb_hal_task_switch_hook and the mrb_task_switch_reason enum from task_hal.h. Ports no longer need to provide the symbol. A port that serviced platform work in the switch hook registers an equivalent scheduler hook at init instead. As far as we know, PicoRuby is the only existing consumer and migrates accordingly. To keep the API, drop commit 2 --- commit 1 stands on its own.