casaroli opened a new pull request, #3889: URL: https://github.com/apache/nuttx-apps/pull/3889
## Summary With `CONFIG_SCHED_CHILD_STATUS`, the kernel keeps the exit status of each child until the parent waits for it. NSH waits only for foreground commands. A background command (`cmd &`) and each pipeline stage before the last one are also children of NSH, but NSH never waits for them. Each one keeps a child status entry in the kernel heap until NSH exits, and the console NSH does not exit. `nsh_builtin()` tried to prevent this with `SA_NOCLDWAIT` around the start of a background builtin. This does not work: `SA_NOCLDWAIT` applies to the whole process, `sigaction()` with it also discards the status of all other children, and the kernel does not clear the flag when the old action comes back. So after the first background builtin, NSH keeps no child status at all, and `wait` can fail with `ECHILD`. The code also needed `CONFIG_ENABLE_ALL_SIGNALS`, and `nsh_fileapp()` (all programs of a kernel build) had no such code. This PR removes the `SA_NOCLDWAIT` code. Before it runs a command, `nsh_parse_command()` now reaps all children that exited, with `waitpid(-1, WNOHANG)`. NSH waits for a foreground command before it starts the next command, so a child that exited at this point is one that no other code waits for. The commit message explains why a `SIGCHLD` handler or a list of background pids was not used. ## Impact - Only configurations with `CONFIG_SCHED_CHILD_STATUS` and `CONFIG_SCHED_WAITPID`. Others do not change. - NSH no longer keeps one kernel heap entry for each background command or pipeline stage. - With `CONFIG_ENABLE_ALL_SIGNALS`, a background builtin no longer turns off the child status of NSH. `wait` for a background command works again in this case. - `wait` still gets the exit status of a background command that runs. As before, `wait` for a command that already exited reports an error. - No documentation change: the NSH documentation does not describe this behavior. ## Testing qemu-armv8a:knsh (kernel build) and qemu-armv8a:nsh (flat build) under HVF, with `CONFIG_SCHED_CHILD_STATUS=y` (the default in kernel builds with apache/nuttx#XXXXX; qemu-armv8a:nsh already sets it) and `CONFIG_PIPES=y`. In the console NSH, a script runs a short program in the background 500 times; this is done 3 times. Then a script runs 100 two-stage pipelines; this is done 2 times. `free` after each round: | Step | knsh, apps master: Kmem used / nused | knsh, this PR | nsh, apps master: Umem used / nused | nsh, this PR | |---|---|---|---|---| | boot | 78552 / 57 | 78552 / 57 | 79848 / 70 | 79848 / 70 | | 500 background jobs | 96984 / 542 | 78608 / 58 | 79848 / 70 | 79848 / 70 | | 1000 background jobs | 116248 / 1042 | 78608 / 58 | 79848 / 70 | 79848 / 70 | | 1500 background jobs | 136248 / 1542 | 78608 / 58 | 79848 / 70 | 79848 / 70 | | 100 pipelines | 140288 / 1647 | 78840 / 61 | 80024 / 72 | 80024 / 72 | | 200 pipelines | 144320 / 1748 | 78840 / 61 | 80024 / 72 | 80024 / 72 | The flat build on master does not grow because of `SA_NOCLDWAIT`, but there the second `wait` fails: `nsh: wait: wait failed: 10`. Both builds, with this PR: - A foreground command keeps its exit status: `bgjob 7` (knsh) and `sh -c "exit 7"` (nsh) give `$?` = 7. - `wait $!` for a background command that sleeps 1 s and exits with 0 gives `$?` = 0, and with exit code 5 gives `$?` = 1, with no error. - ostest passes (`Exiting with status 0`). `tools/checkpatch.sh -c -u -m -g` passes. -- 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]
