Skip to content

Add mrb_task_set_scheduler_hook for pre-scheduling deferred work. Remove mrb_hal_task_switch_hook instead - #6982

Merged
matz merged 2 commits into
mruby:masterfrom
hasumikin:task-scheduler-hook
Jul 31, 2026
Merged

matz merged 2 commits into
mruby:masterfrom
hasumikin:task-scheduler-hook

Conversation

@hasumikin

@hasumikin hasumikin commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

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:

  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 #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 #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.

@hasumikin
hasumikin requested a review from matz as a code owner July 31, 2026 02:01
Copilot AI review requested due to automatic review settings July 31, 2026 02:01
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in mrb_state task scheduler state.
  • Invoke the hook at scheduler entry points (task_run_body loop top, mrb_task_run_once, and root-context Task.pass helper) 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.

Comment on lines 124 to 131
* - 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);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hasumikin
hasumikin marked this pull request as draft July 31, 2026 02:10
## 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.
Copilot AI review requested due to automatic review settings July 31, 2026 02:17
@hasumikin
hasumikin force-pushed the task-scheduler-hook branch from 50ee2cc to bc6b582 Compare July 31, 2026 02:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

@hasumikin
hasumikin marked this pull request as ready for review July 31, 2026 02:37
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@sylph01

sylph01 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

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.

Yes, #6942 was indeed proposed for pumping CYW43/lwIP from the scheduler instead of idle_cpu.
From PicoRuby's CYW43/lwIP's perspective:

  • The change in service location still keeps the "pump fires exactly once per timeslice" property
  • The servicing point does not need separate action on GC and task switch (= does not need the reason), so containing both steps into a single servicing point at the beginning of the task does not break CYW43/lwIP servicing.

@matz

matz commented Jul 31, 2026

Copy link
Copy Markdown
Member

Replace. Let's go with the scheduler hook alone.

Your three arguments hold up. I checked the idle path: when every task is WAITING, task_run_body calls mrb_hal_task_idle_cpu() and continues without ever reaching the switch hook, so the one moment an ISR most needs to wake a task is the one moment the old hook is silent. That alone decides it. The ownership point matters too: a compile-time HAL symbol is the wrong shape for work an embedder wants to attach at runtime.

Keeping both would leave two hooks for one job, and mrb_hal_task_switch_hook has exactly one known consumer, which is the one migrating away. It also has not appeared in any release (it landed after 4.0.0), so removing it costs nobody a migration.

The tests are good. Waking a queue-blocked task within a single Task.pass pins the before-ready-read placement without depending on timing, which is the property that actually matters here.

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

@matz
matz merged commit 716261e into mruby:master Jul 31, 2026
20 checks passed
@hasumikin
hasumikin deleted the task-scheduler-hook branch July 31, 2026 03:52
HirohitoHigashi pushed a commit to mrubyc/mrubyc that referenced this pull request Aug 6, 2026
## 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.
harukasan pushed a commit to harukasan/picoruby that referenced this pull request Aug 11, 2026
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants