casaroli commented on issue #19540:
URL: https://github.com/apache/nuttx/issues/19540#issuecomment-5092600519
## On `task_fork()` — I'd like to argue for keeping it
First, a correction to my own text: §4.2 and §4.4 say the relocated stack
copy
"cannot work at all" on Xtensa's windowed ABI, and that `ARCH_HAVE_TASK_FORK`
would therefore never be available there. Both are wrong. I have
`task_fork()`
working on Xtensa LX7 in flat, protected and kernel builds, passing ostest.
It needed a step no other architecture needs. The windowed ABI spills each
frame's caller stack pointer into a base save area 16 bytes below the frame,
so
a copy taken at a different address carries a chain of *absolute* pointers
still
naming the parent's stack — the child's first `retw` underflows onto the
parent's live frames. The chain is walkable, so I walk it and add the
relocation
offset to every link.
That leaves exactly the residue you're pointing at: spilled `a4`–`a15`, a
local
holding `&local`, anything else in the copy that happens to be a stack
address.
Those cannot be found in general, on any architecture, and I'm not claiming
otherwise.
But that hazard is not something `task_fork()` introduces. It is what NuttX
ships **today**, on every architecture, under the name `fork()` — it's the
residue `arm_fork.c` has always called a *"feeble effort to preserve the
stack
contents"*. Removing `task_fork()` from the proposal doesn't remove the
hazard
from NuttX. It removes the honest name for it, and leaves the only spelling
as
the POSIX one that promises the opposite. I'd rather it be called what it is,
gated behind a symbol that defaults to `n`.
Three concrete reasons to keep it:
**1. It's implemented and tested, on the hardest case.** My branch has
`up_task_fork()` on arm, arm64, risc-v, sim, x86, x86_64 and xtensa. This
isn't
work to be scheduled; it's work to be reviewed.
**2. It gives the per-arch split a spine.** The three primitives differ in
exactly two axes — which stack the child gets (the parent's own, borrowed,
or a
copy) and which address-environment operation runs (duplicate, join, join).
One
`<arch>_fork(type, snapshot)` core serves all three, and the copy case is the
one every architecture already implements today. So keeping `task_fork()`
costs
no new per-arch code — it names the code that's already there. And dropping
it
saves none either: as long as any configuration wants a `vfork()` on a copied
stack rather than a borrowed one, the copy path and its Xtensa relocation
stay
regardless.
**3. It's the compatibility story, and it's what makes this mergeable.** With
`task_fork()` kept, `CONFIG_FORK_IS_TASK_FORK` restores today's exact
behaviour
— same sharing, same concurrency — for anyone who needs `fork()` back, with
one
Kconfig switch. Without it, `fork()` vanishes from `ARCH_ARM`, `ARCH_ARM64`
flat
builds, `ARCH_RISCV`, `ARCH_SIM` and `ARCH_X86_64` with no opt-out at all,
and
out-of-tree users get a link error and no recourse. It also expands rather
than
contracts what's available: Xtensa and x86 gain a fork-family primitive
they've
never had, and on a flat no-MMU target `task_fork()` is the only one that
yields
a concurrently-running child, since `vfork()` suspends the parent by
definition.
---
## Proposed PR ordering
Arranged so each one stands alone and the diff stays reviewable:
**1. Core semantics.** `SYS_vfork`/`up_vfork()`, `SYS_task_fork`/
`up_task_fork()`, `addrenv_fork()` beside `addrenv_join()` and its
`up_addrenv_fork()` hook, the new Kconfig symbols, the `lib_fork.c` split,
and
the parent suspension moved out of libc's `waitpid()` into the kernel vfork
path. No architecture selects anything new, so nothing changes anywhere yet.
**2. `BUILD_PROTECTED` over an MMU.** `ARCH_HAVE_PROTECTED_MMU`, plus arm64
and
armv7-a. Strictly speaking independent of fork semantics and defensible on
its
own, but it comes before the per-arch work because it's what lets each of
those
PRs be tested under all three build models rather than two.
**3. Drop `fork()` everywhere.** Every current `ARCH_HAVE_FORK` select
becomes
`ARCH_HAVE_TASK_FORK` + `ARCH_HAVE_VFORK`. Nothing is deleted — the existing
machinery keeps working under names that describe it — and `ARCH_HAVE_FORK`
ends
up selected by nobody. This is the breaking change, and it's a single
reviewable
commit precisely because `task_fork()` absorbs the old behaviour instead of
discarding it.
**4 onwards — one PR per architecture,** each adding real `fork()` where the
address environment can be duplicated, in decreasing order of what the
hardware
can do:
- **MMU:** risc-v, armv8-a, armv7-a, x86_64, xtensa LX7 — these gain
`fork()`,
`vfork()` and `task_fork()`.
- **MPU:** armv8-m, armv7-m, armv8-r, xtensa LX6 — `vfork()` and
`task_fork()`,
`BUILD_FLAT` and `BUILD_PROTECTED`.
- **Flat-only:** the rest — `vfork()` and `task_fork()` under `BUILD_FLAT`.
Most
of these (avr, renesas, z80, z16, sparc, tricore, or1k, misoc, ceva, hc)
have
no `fork()` today at all, so nothing regresses if they're never done. I'd
treat them as optional follow-ups rather than part of the series.
**One coordination note.** Step 3 is the only point where `apps` can go red:
ostest and about a dozen other files gate on `CONFIG_ARCH_HAVE_FORK`, which
stops being selected. I'd like to land a small `apps` PR just before it that
accepts `CONFIG_ARCH_HAVE_VFORK` as well, and a second one after that drops
the
fallback — so the ostest `vfork` test keeps running throughout instead of
silently compiling out for a release. Happy to prepare both.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
