libc/builtin: support per-application priority/stacksize under KERNEL…#19412
Open
hitHuang wants to merge 1 commit into
Open
libc/builtin: support per-application priority/stacksize under KERNEL…#19412hitHuang wants to merge 1 commit into
hitHuang wants to merge 1 commit into
Conversation
… build.
nsh_fileapp() could not apply an application's Kconfig-configured
priority/stacksize via posix_spawn() under CONFIG_BUILD_KERNEL, because
the registry table (struct builtin_s / g_builtins[]) was gated on
CONFIG_BUILTIN, which depends on !BUILD_KERNEL. Those settings were
silently ignored in KERNEL builds.
CONFIG_BUILTIN conflates the table with main_t-based dispatch, which is
meaningless under CONFIG_BUILD_KERNEL. Add a hidden derived symbol,
APP_REGISTRY, that tracks table availability independently of dispatch:
config APP_REGISTRY
bool
default y if BUILTIN || BUILD_KERNEL
Switch the guards on the table itself (Make.defs, builtin.h) from
CONFIG_BUILTIN to CONFIG_APP_REGISTRY. Call sites that dereference
builtin->main stay gated on CONFIG_BUILTIN and remain unreachable
under CONFIG_BUILD_KERNEL.
Signed-off-by: liang.huang <liang.huang@houmo.ai>
| ############################################################################ | ||
|
|
||
| ifeq ($(CONFIG_BUILTIN),y) | ||
| ifneq ($(CONFIG_APP_REGISTRY),) |
Contributor
There was a problem hiding this comment.
need update cmake too
|
|
||
| config APP_REGISTRY | ||
| bool | ||
| default y if BUILTIN || BUILD_KERNEL |
Contributor
There was a problem hiding this comment.
the more natural approach is attached the config as elf symbol, so elf binfmt could initialize them during loading.
Contributor
I think the reason you see most of these issues is because the majority of the devices NuttX is being used on are running the flat build. However, the kernel build is meant to be fully supported and these issues need to fixed. Especially as we're starting to see more targets that have hardware to support kernel builds. So good find! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
I'm porting NuttX to a SiFive U74 core that currently runs Linux; we're moving to an RTOS
there mainly to cut memory footprint. For reasons specific to this port we're using
CONFIG_BUILD_KERNEL, and while adapting existing applications to it I found that anapplication's Kconfig-configured priority and stack size (e.g.
CONFIG_EXAMPLES_HELLO_PRIORITY/CONFIG_EXAMPLES_HELLO_STACKSIZE) had no effect at allunder
CONFIG_BUILD_KERNEL— every app was spawned with the same default priority/stacksize regardless of what was configured.
The root cause is that
nsh_fileapp()looks up an app's configured priority/stacksize inthe compile-time registry table (
struct builtin_s/g_builtins[]) before callingposix_spawn(), but that table was only built underCONFIG_BUILTIN, andCONFIG_BUILTIN depends on !BUILD_KERNEL. So underCONFIG_BUILD_KERNELthe table simplydidn't exist, and the lookup silently never ran.
A companion PR on
nuttx-apps(nshlib: apply configured priority/stacksize under CONFIG_BUILD_KERNEL) switchesapps/builtin/Make.defsandapps/nshlib/nsh_fileapps.cto the same
CONFIG_APP_REGISTRYsymbol sonsh_fileapp()can actually perform the lookupunder
CONFIG_BUILD_KERNEL. This nuttx-side PR is self-contained and safe to merge onits own —
APP_REGISTRYis purely additive and defaults to the exact same behavior astoday's
CONFIG_BUILTINfor every existing FLAT/PROTECTED configuration; the apps-side PRshould be submitted only after this one merges, since it changes an
ifneqguard thatcurrently resolves via
CONFIG_BUILTINand would otherwise regress against nuttxmaster(which doesn't have
CONFIG_APP_REGISTRYyet).Separately, I'd like to ask the community directly: is
CONFIG_BUILD_KERNELconsidered asecond-class citizen at this point? Most of the gaps I've been running into while porting
(this one included) come from code paths that quietly assume FLAT or PROTECTED and were
never exercised under KERNEL build. If the general recommendation is to prefer FLAT/
PROTECTED unless strict process isolation is a hard requirement, that would be useful to
know up front — but if
CONFIG_BUILD_KERNELis meant to be fully supported, I'd like tokeep sending fixes like this one as I find them.
Impact
APP_REGISTRY, hidden (no prompt), purely derived. No effect on anyexisting configuration: it evaluates to the same condition
CONFIG_BUILTINalreadysatisfied under FLAT/PROTECTED, plus
CONFIG_BUILD_KERNEL.CONFIG_BUILD_KERNELwithCONFIG_SCHED_USER_IDENTITY=y, theuid/gid/modefields of
struct builtin_sare now also populated (previously the whole struct didn'texist in that build mode). They remain unused: their only consumer,
fs/binfs/fs_binfs.c, is reached only throughFS_BINFS depends on BUILTIN, which isstill unreachable under
CONFIG_BUILD_KERNEL. This is inert, not a behavior change.CONFIG_BUILTIN, its dependency on!BUILD_KERNEL, or any of themain_t-dispatch call sites.nuttx-appsPR to actually take effect; without it, the new tableis built but nothing reads it yet (same as today).
Testing
Host: Linux
Target: QEMU RISC-V (
rv-virt), configknsh_romfs, with the defaultCONFIG_EXAMPLES_HELLO_STACKSIZE=8192removed so the app's own Kconfig defaults apply.Test app: modified
examples/hello:with
apps/examples/hello/Kconfigchanged to non-default values to make any effectobservable:
config EXAMPLES_HELLO_PRIORITY int "Hello task priority" - default 100 + default 120 config EXAMPLES_HELLO_STACKSIZE int "Hello stack size" - default DEFAULT_TASK_STACKSIZE + default 1000Before this fix (plus the companion apps PR),
hello's configured priority (120) andstack size (1000) are ignored; it runs with priority 100 and a much larger stack:
After this fix (with the companion apps PR applied), the configured priority (120) and
stack size (1000, rounded to 944 usable after bookkeeping) are correctly applied:
Also re-verified with
rv-virt:nsh(CONFIG_BUILTIN=y,CONFIG_BUILD_KERNELnot set) thatexisting FLAT-mode
CONFIG_APP_REGISTRYbehavior (derived fromCONFIG_BUILTIN) isunchanged — builtin lookup and priority/stacksize handling behave exactly as before this
change.