Hi, Willy, Thomas
Here is the first part of v2 of our tinyconfig support for nolibc-test [1], the patchset subject is reserved as before.
As discussed in v1 thread [1], to easier the review progress, the whole tinyconfig support is divided into several parts, mainly by architecture, here is the first part, include basic preparation and powerpc example.
This patchset should be applied after the 32/64-bit powerpc support [2], exactly these two are required by us:
* selftests/nolibc: add extra config file customize support * selftests/nolibc: add XARCH and ARCH mapping support
In this patchset, we firstly add some misc preparations and at last add the tinyconfig target and use powerpc as the first example.
Tests:
// powerpc run-user $ for arch in powerpc powerpc64 powerpc64le; do \ rm -rf $PWD/kernel-$arch; \ mkdir -p $PWD/kernel-$arch; \ make run-user XARCH=$arch O=$PWD/kernel-$arch RUN_OUT=$PWD/run.$arch.out | grep "status: "; \ done 165 test(s): 157 passed, 8 skipped, 0 failed => status: warning 165 test(s): 157 passed, 8 skipped, 0 failed => status: warning 165 test(s): 157 passed, 8 skipped, 0 failed => status: warning
// powerpc run $ for arch in powerpc powerpc64 powerpc64le; do \ rm -rf $PWD/kernel-$arch; \ mkdir -p $PWD/kernel-$arch; \ make tinyconfig run XARCH=$arch O=$PWD/kernel-$arch RUN_OUT=$PWD/run.$arch.out; \ done
$ for arch in powerpc powerpc64 powerpc64le; do \ make report XARCH=$arch O=$PWD/kernel-$arch RUN_OUT=$PWD/run.$arch.out | grep "status: "; \ done 165 test(s): 156 passed, 9 skipped, 0 failed => status: warning 165 test(s): 156 passed, 9 skipped, 0 failed => status: warning 165 test(s): 156 passed, 9 skipped, 0 failed => status: warning
// the others, randomly choose some $ make run-user XARCH=arm O=$PWD/kernel-arm RUN_OUT=$PWD/run.arm.out CROSS_COMPILE=arm-linux-gnueabi- | grep status: 165 test(s): 156 passed, 9 skipped, 0 failed => status: warning $ make run-user XARCH=x86_64 O=$PWD/kernel-arm RUN_OUT=$PWD/run.x86_64.out CROSS_COMPILE=x86_64-linux-gnu- | grep status: 165 test(s): 157 passed, 8 skipped, 0 failed => status: warning $ make run-libc-test | grep status: 165 test(s): 153 passed, 12 skipped, 0 failed => status: warning
// x86_64, require noapic kernel command line option for old qemu-system-x86_64 (v4.2.1) $ make run XARCH=x86_64 O=$PWD/kernel-x86_64 RUN_OUT=$PWD/run.x86_64.out CROSS_COMPILE=x86_64-linux-gnu- | grep status $ make rerun XARCH=x86_64 O=$PWD/kernel-x86_64 RUN_OUT=$PWD/run.x86_64.out CROSS_COMPILE=x86_64-linux-gnu- | grep status 165 test(s): 159 passed, 6 skipped, 0 failed => status: warning
tinyconfig mainly targets as a time-saver, the misc preparations service for the same goal, let's take a look:
* selftests/nolibc: allow report with existing test log
Like rerun without rebuild, Add report (without rerun) to summarize the existing test log, this may work perfectly with the 'grep status'
* selftests/nolibc: add macros to enhance maintainability
Several macros are added to dedup the shared code to shrink lines and easier the maintainability
The macros are added just before the using area to avoid code change conflicts in the future.
* selftests/nolibc: print running log to screen
Enable logging to let developers learn what is happening at the first glance, without the need to edit the Makefile and rerun it.
These helps a lot when there is a long-time running, a failed poweroff or even a forever hang.
For test summmary, the 'grep status' can be used together with the standalone report target.
* selftests/nolibc: fix up O= option support
With objtree instead srctree for .config and IMAGE, now, O= works.
Using O=$PWD/kernel-$arch avoid the mrproer for every build.
* selftests/nolibc: add menuconfig for development
Allow manually tuning some options, mainly for a new architecture porting.
* selftests/nolibc: add mrproper for development selftests/nolibc: defconfig: remove mrproper target
Split the mrproper target out of defconfig, when with O=, mrproper is not required by defconfig, but reserve it for the other use scenes.
* selftests/nolibc: string the core targets
Allow simply 'make run' instead of 'make defconfig; make extconfig; make kernel; make run'.
* selftests/nolibc: allow quit qemu-system when poweroff fails
When poweroff fails, allow exit while detects the power off string from output or the wait time is too long (specified by QEMU_TIMEOUT).
This helps the boards who have no poweroff support or the kernel not enable the poweroff options (mainly for tinyconfig).
* selftests/nolibc: allow customize CROSS_COMPILE by architecture * selftests/nolibc: customize CROSS_COMPILE for 32/64-bit powerpc
This further saves a CROSS_COMPILE option for 'make run', it is very important when iterates all of the supported architectures and the compilers are not just prefixed with the XARCH variable.
For example, binary of big endian powerpc64 can be compiled with powerpc64le-linux-gnu-, but the prefix is powerpc64le.
Even if the pre-customized compiler not exist, we can configure CROSS_COMPILE_<ARCH> before the test loop to use the code.
* selftests/nolibc: add tinyconfig target selftests/nolibc: tinyconfig: add extra common options selftests/nolibc: tinyconfig: add support for 32/64-bit powerpc
Here is the first architecture(and its variants) support tinyconfig.
powerpc is actually a very good architecture, for it has 'various' variants for test.
Best regards, Zhangjin --- [1]: https://lore.kernel.org/lkml/cover.1687706332.git.falcon@tinylab.org/ [2]: https://lore.kernel.org/lkml/cover.1689713175.git.falcon@tinylab.org/
Zhangjin Wu (14): selftests/nolibc: allow report with existing test log selftests/nolibc: add macros to enhance maintainability selftests/nolibc: print running log to screen selftests/nolibc: fix up O= option support selftests/nolibc: add menuconfig for development selftests/nolibc: add mrproper for development selftests/nolibc: defconfig: remove mrproper target selftests/nolibc: string the core targets selftests/nolibc: allow quit qemu-system when poweroff fails selftests/nolibc: allow customize CROSS_COMPILE by architecture selftests/nolibc: customize CROSS_COMPILE for 32/64-bit powerpc selftests/nolibc: add tinyconfig target selftests/nolibc: tinyconfig: add extra common options selftests/nolibc: tinyconfig: add support for 32/64-bit powerpc
tools/testing/selftests/nolibc/Makefile | 102 ++++++++++++++---- .../selftests/nolibc/configs/common.config | 4 + .../selftests/nolibc/configs/powerpc.config | 3 + .../selftests/nolibc/configs/powerpc64.config | 3 + .../nolibc/configs/powerpc64le.config | 4 + 5 files changed, 98 insertions(+), 18 deletions(-) create mode 100644 tools/testing/selftests/nolibc/configs/common.config create mode 100644 tools/testing/selftests/nolibc/configs/powerpc64.config create mode 100644 tools/testing/selftests/nolibc/configs/powerpc64le.config
After the tests finish, it is valuable to report and summarize with existing test log.
This avoid rerun or run the tests again when not necessary.
Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/testing/selftests/nolibc/Makefile | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index ab17e0d8b7e2..0cd17de2062c 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -206,6 +206,10 @@ rerun: $(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(srctree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out" $(Q)$(REPORT) $(CURDIR)/run.out
+# report with existing test log +report: + $(Q)$(REPORT_RUN_OUT) + clean: $(call QUIET_CLEAN, sysroot) $(Q)rm -rf sysroot
The kernel targets share the same kernel make operations, the same .config file, the same kernel image, add MAKE_KERNEL, KERNEL_CONFIG and KERNEL_IMAGE for them.
Many targets share the same logging related settings, let's add common variables RUN_OUT, LOG_OUT and REPORT_RUN_OUT for them.
The qemu run/rerun targets share the same qemu system run command, add QEMU_SYSTEM_RUN for them.
Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/testing/selftests/nolibc/Makefile | 41 ++++++++++++++++--------- 1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 0cd17de2062c..8c531518bb9f 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -166,45 +166,58 @@ endif libc-test: nolibc-test.c $(QUIET_CC)$(CC) -o $@ $<
+# common macros for logging +RUN_OUT = $(CURDIR)/run.out +LOG_OUT = > "$(RUN_OUT)" +REPORT_RUN_OUT = $(REPORT) "$(RUN_OUT)" + # local libc-test run-libc-test: libc-test - $(Q)./libc-test > "$(CURDIR)/run.out" || : - $(Q)$(REPORT) $(CURDIR)/run.out + $(Q)./libc-test $(LOG_OUT) || : + $(Q)$(REPORT_RUN_OUT)
# local nolibc-test run-nolibc-test: nolibc-test - $(Q)./nolibc-test > "$(CURDIR)/run.out" || : - $(Q)$(REPORT) $(CURDIR)/run.out + $(Q)./nolibc-test $(LOG_OUT) || : + $(Q)$(REPORT_RUN_OUT)
# qemu user-land test run-user: nolibc-test - $(Q)qemu-$(QEMU_ARCH) ./nolibc-test > "$(CURDIR)/run.out" || : - $(Q)$(REPORT) $(CURDIR)/run.out + $(Q)qemu-$(QEMU_ARCH) ./nolibc-test $(LOG_OUT) || : + $(Q)$(REPORT_RUN_OUT)
initramfs: nolibc-test $(QUIET_MKDIR)mkdir -p initramfs $(call QUIET_INSTALL, initramfs/init) $(Q)cp nolibc-test initramfs/init
+# common macros for kernel targets +MAKE_KERNEL = $(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) +KERNEL_CONFIG = $(srctree)/.config +KERNEL_IMAGE = $(srctree)/$(IMAGE) + defconfig: - $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare + $(Q)$(MAKE_KERNEL) mrproper $(DEFCONFIG) prepare
extconfig: - $(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(srctree)" -m "$(srctree)/.config" $(foreach c,$(EXTCONFIG),$(wildcard $(CURDIR)/configs/$c)) - $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) KCONFIG_ALLCONFIG="$(srctree)/.config" allnoconfig + $(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(srctree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(EXTCONFIG),$(wildcard $(CURDIR)/configs/$c)) + $(Q)$(MAKE_KERNEL) KCONFIG_ALLCONFIG="$(KERNEL_CONFIG)" allnoconfig
kernel: initramfs - $(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs + $(Q)$(MAKE_KERNEL) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs + +# common macros for qemu run/rerun targets +QEMU_SYSTEM_RUN = qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(KERNEL_IMAGE)" -serial stdio $(QEMU_ARGS)
# run the tests after building the kernel run: kernel - $(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(srctree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out" - $(Q)$(REPORT) $(CURDIR)/run.out + $(Q)$(QEMU_SYSTEM_RUN) $(LOG_OUT) + $(Q)$(REPORT_RUN_OUT)
# re-run the tests from an existing kernel rerun: - $(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(srctree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out" - $(Q)$(REPORT) $(CURDIR)/run.out + $(Q)$(QEMU_SYSTEM_RUN) $(LOG_OUT) + $(Q)$(REPORT_RUN_OUT)
# report with existing test log report:
On Wed, Jul 19, 2023 at 09:19:10PM +0800, Zhangjin Wu wrote:
The kernel targets share the same kernel make operations, the same .config file, the same kernel image, add MAKE_KERNEL, KERNEL_CONFIG and KERNEL_IMAGE for them.
Many targets share the same logging related settings, let's add common variables RUN_OUT, LOG_OUT and REPORT_RUN_OUT for them.
The qemu run/rerun targets share the same qemu system run command, add QEMU_SYSTEM_RUN for them.
Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/testing/selftests/nolibc/Makefile | 41 ++++++++++++++++--------- 1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 0cd17de2062c..8c531518bb9f 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -166,45 +166,58 @@ endif libc-test: nolibc-test.c $(QUIET_CC)$(CC) -o $@ $< +# common macros for logging +RUN_OUT = $(CURDIR)/run.out +LOG_OUT = > "$(RUN_OUT)" +REPORT_RUN_OUT = $(REPORT) "$(RUN_OUT)"
# local libc-test run-libc-test: libc-test
- $(Q)./libc-test > "$(CURDIR)/run.out" || :
- $(Q)$(REPORT) $(CURDIR)/run.out
- $(Q)./libc-test $(LOG_OUT) || :
- $(Q)$(REPORT_RUN_OUT)
Sorry but I don't find that this improves maintainability, quite the opposite in fact. One reason is that you never visually expect that some shell indirection delimiters are hidden in a macro that seems to only convey a name. Sure there are valid use cases for this, but I think that here it's just adding too much abstraction and it makes it quite hard to unfold all of this mentally.
Please try to keep the number of macros to the minimum that needs to be replaced or forced by the user. Here I'm not seeing a compelling reason for a user to want to force LOG_OUT to something else. Also makefile macros are generally a pain to debug, which is another reason for not putting too many of them.
I noticed that your next patch changes LOG_OUT to tee, it could have done it everywhere and wouldn't affect readability as much.
Thanks, Willy
Hi, Willy
On Wed, Jul 19, 2023 at 09:19:10PM +0800, Zhangjin Wu wrote:
The kernel targets share the same kernel make operations, the same .config file, the same kernel image, add MAKE_KERNEL, KERNEL_CONFIG and KERNEL_IMAGE for them.
Many targets share the same logging related settings, let's add common variables RUN_OUT, LOG_OUT and REPORT_RUN_OUT for them.
The qemu run/rerun targets share the same qemu system run command, add QEMU_SYSTEM_RUN for them.
Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/testing/selftests/nolibc/Makefile | 41 ++++++++++++++++--------- 1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 0cd17de2062c..8c531518bb9f 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -166,45 +166,58 @@ endif libc-test: nolibc-test.c $(QUIET_CC)$(CC) -o $@ $< +# common macros for logging +RUN_OUT = $(CURDIR)/run.out +LOG_OUT = > "$(RUN_OUT)" +REPORT_RUN_OUT = $(REPORT) "$(RUN_OUT)"
# local libc-test run-libc-test: libc-test
- $(Q)./libc-test > "$(CURDIR)/run.out" || :
- $(Q)$(REPORT) $(CURDIR)/run.out
- $(Q)./libc-test $(LOG_OUT) || :
- $(Q)$(REPORT_RUN_OUT)
Sorry but I don't find that this improves maintainability, quite the opposite in fact. One reason is that you never visually expect that some shell indirection delimiters are hidden in a macro that seems to only convey a name. Sure there are valid use cases for this, but I think that here it's just adding too much abstraction and it makes it quite hard to unfold all of this mentally.
Ok, will reserve less ones as possible as we can.
- RUN_OUT is required for architecture specific output - REPORT_RUN_OUT is not necessary, will remove it
Please try to keep the number of macros to the minimum that needs to be replaced or forced by the user. Here I'm not seeing a compelling reason for a user to want to force LOG_OUT to something else. Also makefile macros are generally a pain to debug, which is another reason for not putting too many of them.
I noticed that your next patch changes LOG_OUT to tee, it could have done it everywhere and wouldn't affect readability as much.
I have forgetten to pick an old patch to silence the running log like this:
ifeq ($(QUIET_RUN),1) LOG_OUT = > "$(RUN_OUT)" else LOG_OUT = | tee "$(RUN_OUT)" endif
Without QUIET_RUN, we can silence the running log with:
$ make run LOG_OUT="> /dev/null"
It is not meaningful like QUIET_RUN, seems the QUIET_RUN is not necessary for we have 'grep status' now, so, let's remove this RUN_OUT too.
Thanks, Zhangjin
Thanks, Willy
When poweroff fails, qemu-system will hang there without any output.
It is very hard to debug in such case, let's print the running log to the screen to allow users to learn what is happening at the first glance, without editing the Makefile manually every time.
To get a clean output, the 'grep status' command can be used.
Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/testing/selftests/nolibc/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 8c531518bb9f..f1c8e4a0f1b2 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -168,7 +168,7 @@ libc-test: nolibc-test.c
# common macros for logging RUN_OUT = $(CURDIR)/run.out -LOG_OUT = > "$(RUN_OUT)" +LOG_OUT = | tee "$(RUN_OUT)" REPORT_RUN_OUT = $(REPORT) "$(RUN_OUT)"
# local libc-test
On Wed, Jul 19, 2023 at 09:20:17PM +0800, Zhangjin Wu wrote:
When poweroff fails, qemu-system will hang there without any output.
It is very hard to debug in such case, let's print the running log to the screen to allow users to learn what is happening at the first glance, without editing the Makefile manually every time.
To get a clean output, the 'grep status' command can be used.
The problem with doing this is that it rolls back to the initial version that breaks with qemu. When its stdout is sent to a pipe, we've found that the output got randomly mangled and/or missing contents. It's only when sent to a file that it's OK. I suspect it has something to do with non-blocking writes being used to avoid blocking the emulation but I could be totally wrong. That's the reason why we had to switch to a file.
And I'd rather avoid starting it in the background as well. Maybe you'd want to run the qemu command under the "timeout" one ? That could be better than nothing.
Willy
Hi, Willy
On Wed, Jul 19, 2023 at 09:20:17PM +0800, Zhangjin Wu wrote:
When poweroff fails, qemu-system will hang there without any output.
It is very hard to debug in such case, let's print the running log to the screen to allow users to learn what is happening at the first glance, without editing the Makefile manually every time.
To get a clean output, the 'grep status' command can be used.
The problem with doing this is that it rolls back to the initial version that breaks with qemu. When its stdout is sent to a pipe, we've found that the output got randomly mangled and/or missing contents. It's only when sent to a file that it's OK. I suspect it has something to do with non-blocking writes being used to avoid blocking the emulation but I could be totally wrong. That's the reason why we had to switch to a file.
ok, thanks for sharing the history, it is important.
And I'd rather avoid starting it in the background as well. Maybe you'd want to run the qemu command under the "timeout" one ? That could be better than nothing.
Yeah, with the timeout logic, this may be not really required, so, let's drop this patch and the LOG_OUT from the previous one.
with our timeout logic, we are able to quit qemu and then print the running log to screen to tell users what happens background, let's discuss timeout logic in its own patch.
Thanks, Zhangjin
Willy
To avoid pollute the source code tree and avoid mrproper for every architecture switch, the O= argument must be supported.
Both IMAGE and .config are from the building directory, let's use objtree instead of srctree for them.
If no O= option specified, means building kernel in source code tree, objtree should be srctree in such case.
Suggested-by: Willy Tarreau w@1wt.eu Link: https://lore.kernel.org/lkml/ZK0AB1OXH1s2xYsh@1wt.eu/ Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/testing/selftests/nolibc/Makefile | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index f1c8e4a0f1b2..058e7be070ea 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -9,6 +9,9 @@ ifeq ($(srctree),) srctree := $(patsubst %/tools/testing/selftests/,%,$(dir $(CURDIR))) endif
+# add objtree for O= argument, required by IMAGE and .config +objtree ?= $(srctree) + ifeq ($(ARCH),) include $(srctree)/scripts/subarch.include ARCH = $(SUBARCH) @@ -193,14 +196,14 @@ initramfs: nolibc-test
# common macros for kernel targets MAKE_KERNEL = $(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) -KERNEL_CONFIG = $(srctree)/.config -KERNEL_IMAGE = $(srctree)/$(IMAGE) +KERNEL_CONFIG = $(objtree)/.config +KERNEL_IMAGE = $(objtree)/$(IMAGE)
defconfig: $(Q)$(MAKE_KERNEL) mrproper $(DEFCONFIG) prepare
extconfig: - $(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(srctree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(EXTCONFIG),$(wildcard $(CURDIR)/configs/$c)) + $(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(objtree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(EXTCONFIG),$(wildcard $(CURDIR)/configs/$c)) $(Q)$(MAKE_KERNEL) KCONFIG_ALLCONFIG="$(KERNEL_CONFIG)" allnoconfig
kernel: initramfs
The default DEFCONFIG_<ARCH> may not always work for all architectures, let's allow users to tune some kernel config options with 'menuconfig'.
This is important when porting nolibc to a new architecture, it also allows to speed up nolibc 'run' target testing via manually disabling tons of unnecessary kernel config options.
Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/testing/selftests/nolibc/Makefile | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 058e7be070ea..06954b4b3885 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -202,6 +202,9 @@ KERNEL_IMAGE = $(objtree)/$(IMAGE) defconfig: $(Q)$(MAKE_KERNEL) mrproper $(DEFCONFIG) prepare
+menuconfig: + $(Q)$(MAKE_KERNEL) menuconfig + extconfig: $(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(objtree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(EXTCONFIG),$(wildcard $(CURDIR)/configs/$c)) $(Q)$(MAKE_KERNEL) KCONFIG_ALLCONFIG="$(KERNEL_CONFIG)" allnoconfig
On Wed, Jul 19, 2023 at 09:22:37PM +0800, Zhangjin Wu wrote:
The default DEFCONFIG_<ARCH> may not always work for all architectures, let's allow users to tune some kernel config options with 'menuconfig'.
This is important when porting nolibc to a new architecture, it also allows to speed up nolibc 'run' target testing via manually disabling tons of unnecessary kernel config options.
Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/testing/selftests/nolibc/Makefile | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 058e7be070ea..06954b4b3885 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -202,6 +202,9 @@ KERNEL_IMAGE = $(objtree)/$(IMAGE) defconfig: $(Q)$(MAKE_KERNEL) mrproper $(DEFCONFIG) prepare +menuconfig:
- $(Q)$(MAKE_KERNEL) menuconfig
What is the real benefit of this compared to just running the standard "make menuconfig" ? We should avoid adding makefile targets that do not bring value on top of that the top-level makefile does, because it will make the useful ones much harder to spot, and will tend to encourage users to use only that makefile during the tests, which is a bad practice since many targets will always be missing or work differently. It's important in my opinion that we strictly stick to what is useful to operate the tests themselves and this one doesn't make me feel like it qualifies as such.
Willy
On Wed, Jul 19, 2023 at 09:22:37PM +0800, Zhangjin Wu wrote:
The default DEFCONFIG_<ARCH> may not always work for all architectures, let's allow users to tune some kernel config options with 'menuconfig'.
This is important when porting nolibc to a new architecture, it also allows to speed up nolibc 'run' target testing via manually disabling tons of unnecessary kernel config options.
Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/testing/selftests/nolibc/Makefile | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 058e7be070ea..06954b4b3885 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -202,6 +202,9 @@ KERNEL_IMAGE = $(objtree)/$(IMAGE) defconfig: $(Q)$(MAKE_KERNEL) mrproper $(DEFCONFIG) prepare +menuconfig:
- $(Q)$(MAKE_KERNEL) menuconfig
What is the real benefit of this compared to just running the standard "make menuconfig" ? We should avoid adding makefile targets that do not bring value on top of that the top-level makefile does, because it will make the useful ones much harder to spot, and will tend to encourage users to use only that makefile during the tests, which is a bad practice since many targets will always be missing or work differently. It's important in my opinion that we strictly stick to what is useful to operate the tests themselves and this one doesn't make me feel like it qualifies as such.
Ok, get it.
I did like develop nolibc in tools/testing/selftests/nolibc/ without changing directories frequently or specifying the "-C" option unnecessary ;-)
Since "make menuconfig" is only required during the first porting of a new architecture, so, it is ok to drop this patch.
Thanks, Zhangjin
Willy
Hi, Willy
On Wed, Jul 19, 2023 at 09:22:37PM +0800, Zhangjin Wu wrote:
The default DEFCONFIG_<ARCH> may not always work for all architectures, let's allow users to tune some kernel config options with 'menuconfig'.
This is important when porting nolibc to a new architecture, it also allows to speed up nolibc 'run' target testing via manually disabling tons of unnecessary kernel config options.
Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/testing/selftests/nolibc/Makefile | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 058e7be070ea..06954b4b3885 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -202,6 +202,9 @@ KERNEL_IMAGE = $(objtree)/$(IMAGE) defconfig: $(Q)$(MAKE_KERNEL) mrproper $(DEFCONFIG) prepare +menuconfig:
- $(Q)$(MAKE_KERNEL) menuconfig
What is the real benefit of this compared to just running the standard "make menuconfig" ? We should avoid adding makefile targets that do not bring value on top of that the top-level makefile does, because it will make the useful ones much harder to spot, and will tend to encourage users to use only that makefile during the tests, which is a bad practice since many targets will always be missing or work differently. It's important in my opinion that we strictly stick to what is useful to operate the tests themselves and this one doesn't make me feel like it qualifies as such.
Ok, get it.
I did like develop nolibc in tools/testing/selftests/nolibc/ without changing directories frequently or specifying the "-C" option unnecessary ;-)
Since "make menuconfig" is only required during the first porting of a new architecture, so, it is ok to drop this patch.
Oh, Willy, I did find this is very important again.
I just want to check and test if we need to disable vsx for 32-bit powerpc too, so, I plan to re-configure kernel via menuconfig to disable VSX in kernel side, and forcely enable vsx in application side, but it was very 'painful' when I was running something like this:
$ make defconfig ARCH=ppc CROSS_COMPILE=powerpc-linux-gnu-
To do a further menuconfig, I must switch to something else:
$ make menuconfig ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- -C ../../../../
Especially, our test is able to accept ARCH=ppc, but the top-level kernel still only accept powerpc, it makes the development very comfortable, of course, the typing of the relative or absolute path every time is either not a good idea.
so, this doesn't simply duplicate with the one from top-level, it can get the right ARCH, srctree for us, and this is heavily used by our tinyconfig development to tune the config options very frequently, so, let's still add this one in the new revision?
But I plan to merge the mrproper targets here to save another patch, what about your idea?
menuconfig mrproper: $(Q)$(MAKE_KERNEL) $@
Thanks, Zhangjin
Thanks, Zhangjin
Willy
Hi Zhangjin,
On Thu, Jul 27, 2023 at 09:24:18PM +0800, Zhangjin Wu wrote:
Hi, Willy
On Wed, Jul 19, 2023 at 09:22:37PM +0800, Zhangjin Wu wrote:
The default DEFCONFIG_<ARCH> may not always work for all architectures, let's allow users to tune some kernel config options with 'menuconfig'.
This is important when porting nolibc to a new architecture, it also allows to speed up nolibc 'run' target testing via manually disabling tons of unnecessary kernel config options.
Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/testing/selftests/nolibc/Makefile | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 058e7be070ea..06954b4b3885 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -202,6 +202,9 @@ KERNEL_IMAGE = $(objtree)/$(IMAGE) defconfig: $(Q)$(MAKE_KERNEL) mrproper $(DEFCONFIG) prepare +menuconfig:
- $(Q)$(MAKE_KERNEL) menuconfig
What is the real benefit of this compared to just running the standard "make menuconfig" ? We should avoid adding makefile targets that do not bring value on top of that the top-level makefile does, because it will make the useful ones much harder to spot, and will tend to encourage users to use only that makefile during the tests, which is a bad practice since many targets will always be missing or work differently. It's important in my opinion that we strictly stick to what is useful to operate the tests themselves and this one doesn't make me feel like it qualifies as such.
Ok, get it.
I did like develop nolibc in tools/testing/selftests/nolibc/ without changing directories frequently or specifying the "-C" option unnecessary ;-)
Since "make menuconfig" is only required during the first porting of a new architecture, so, it is ok to drop this patch.
Oh, Willy, I did find this is very important again.
I just want to check and test if we need to disable vsx for 32-bit powerpc too, so, I plan to re-configure kernel via menuconfig to disable VSX in kernel side, and forcely enable vsx in application side, but it was very 'painful' when I was running something like this:
$ make defconfig ARCH=ppc CROSS_COMPILE=powerpc-linux-gnu-To do a further menuconfig, I must switch to something else:
$ make menuconfig ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- -C ../../../../Especially, our test is able to accept ARCH=ppc, but the top-level kernel still only accept powerpc, it makes the development very comfortable, of course, the typing of the relative or absolute path every time is either not a good idea.
It is "able" but that's not what ought to be used, since it's going to be remapped to "powerpc". You're supposed to use XARCH for this one since it's a variant (we could find other names if needed). I suspect it just shows that the "override ARCH :=" is excessive and should not be there. Usually when you put override in a makefile, it's going to pop up as a bug elsewhere. Thus actually you could very well have :
make ARCH=powerpc XARCH=ppc CROSS_COMPILE=powerpc-linux-gnu-
as the prefix for all your commands. Some will prefer to work directly from the kernel dir:
$ make ARCH=powerpc XARCH=ppc CROSS_COMPILE=powerpc-linux-gnu- -C tools/testing/selftests/nolibc-test defconfig $ make ARCH=powerpc XARCH=ppc CROSS_COMPILE=powerpc-linux-gnu- menuconfig
Others would do it from the nolibc-test dir as you did above.
so, this doesn't simply duplicate with the one from top-level, it can get the right ARCH, srctree for us, and this is heavily used by our tinyconfig development to tune the config options very frequently, so, let's still add this one in the new revision?
I'm not *fundamentally* opposed to menuconfig, I just think that it's appearing at the wrong place. Why not oldconfig, allmodconfig, randconfig, allnoconfig etc then ? There is nothing in the test directory that is supposed to be used interactively, either it's run in a batch for cross- arch testing, or you use it to validate your kernel when you're working on it. It feels strange that one would want to configure their kernel from this sub-dir.
But I plan to merge the mrproper targets here to save another patch, what about your idea?
menuconfig mrproper:$(Q)$(MAKE_KERNEL) $@
Please no. Someone doing that in hope to clean only the result of the tests would in fact ruin their config:
$ make -C tools/testing/selftests/nolibc-test mrproper
This is another reason for not encouraging users to develop from within that dir.
Willy
Hi, Willy
[...]
What is the real benefit of this compared to just running the standard "make menuconfig" ? We should avoid adding makefile targets that do not bring value on top of that the top-level makefile does, because it will make the useful ones much harder to spot, and will tend to encourage users to use only that makefile during the tests, which is a bad practice since many targets will always be missing or work differently. It's important in my opinion that we strictly stick to what is useful to operate the tests themselves and this one doesn't make me feel like it qualifies as such.
Ok, get it.
I did like develop nolibc in tools/testing/selftests/nolibc/ without changing directories frequently or specifying the "-C" option unnecessary ;-)
Since "make menuconfig" is only required during the first porting of a new architecture, so, it is ok to drop this patch.
Oh, Willy, I did find this is very important again.
I just want to check and test if we need to disable vsx for 32-bit powerpc too, so, I plan to re-configure kernel via menuconfig to disable VSX in kernel side, and forcely enable vsx in application side, but it was very 'painful' when I was running something like this:
$ make defconfig ARCH=ppc CROSS_COMPILE=powerpc-linux-gnu-To do a further menuconfig, I must switch to something else:
$ make menuconfig ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- -C ../../../../Especially, our test is able to accept ARCH=ppc, but the top-level kernel still only accept powerpc, it makes the development very comfortable, of course, the typing of the relative or absolute path every time is either not a good idea.
It is "able" but that's not what ought to be used, since it's going to be remapped to "powerpc". You're supposed to use XARCH for this one since it's a variant (we could find other names if needed). I suspect it just shows that the "override ARCH :=" is excessive and should not be there. Usually when you put override in a makefile, it's going to pop up as a bug elsewhere. Thus actually you could very well have :
make ARCH=powerpc XARCH=ppc CROSS_COMPILE=powerpc-linux-gnu-
Oh, Seems I have misunderstood your suggestions in another reply, you recommended to add ARCH variable to the help target, at last, I thought it was better to only reserve ARCH as the consistent variable to users, so, the 'override' keyword is added to allow passing anyone of powerpc, ppc, ppc64 and ppc64le to ARCH, since XARCH only accept ppc, ppc64 and ppc64le.
Further passing 'ARCH=powerpc XARCH=ppc' is also ok, but the combination of ARCH and XARCH may require more time to teach a new user, is ok for me to remove the override and carefully document this usage in Makefile.
Seems 'override' is really required, without it, when users wrongly try ARCH=ppc, ARCH=ppc64, ARCH=ppc64le, the ARCH passed to kernel will be completely wrong, it may mislead users ...
With this 'override', it is ok for users to use ARCH only, or XARCH or even together with both of them, no failure and happy, to myself, I'm ok to remove the 'override' keyword, ;-)
as the prefix for all your commands. Some will prefer to work directly from the kernel dir:
$ make ARCH=powerpc XARCH=ppc CROSS_COMPILE=powerpc-linux-gnu- -C tools/testing/selftests/nolibc-test defconfig $ make ARCH=powerpc XARCH=ppc CROSS_COMPILE=powerpc-linux-gnu- menuconfig
Others would do it from the nolibc-test dir as you did above.
Yeah, we still need one more -C, but it is of course ok to do this in a standalone script as you mentioned before.
so, this doesn't simply duplicate with the one from top-level, it can get the right ARCH, srctree for us, and this is heavily used by our tinyconfig development to tune the config options very frequently, so, let's still add this one in the new revision?
I'm not *fundamentally* opposed to menuconfig, I just think that it's appearing at the wrong place. Why not oldconfig, allmodconfig, randconfig, allnoconfig etc then ? There is nothing in the test directory that is supposed to be used interactively, either it's run in a batch for cross- arch testing, or you use it to validate your kernel when you're working on it. It feels strange that one would want to configure their kernel from this sub-dir.
Ok, it is reasonable ;-)
But I plan to merge the mrproper targets here to save another patch, what about your idea?
menuconfig mrproper:$(Q)$(MAKE_KERNEL) $@
Please no. Someone doing that in hope to clean only the result of the tests would in fact ruin their config:
$ make -C tools/testing/selftests/nolibc-test mrproper
Yeah, really a good reason to not add mrproper there.
This is another reason for not encouraging users to develop from within that dir.
Thanks, Zhangjin
Willy
When want to start a new building with O= option, the old generated files in the source code tree must be mrproper-ed, adding mrproper target here to avoid switch to the source code tree manually.
Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/testing/selftests/nolibc/Makefile | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 06954b4b3885..9d9902b54e5e 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -199,6 +199,9 @@ MAKE_KERNEL = $(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROS KERNEL_CONFIG = $(objtree)/.config KERNEL_IMAGE = $(objtree)/$(IMAGE)
+mrproper: + $(Q)$(MAKE_KERNEL) mrproper + defconfig: $(Q)$(MAKE_KERNEL) mrproper $(DEFCONFIG) prepare
On Wed, Jul 19, 2023 at 09:23:48PM +0800, Zhangjin Wu wrote:
+mrproper:
- $(Q)$(MAKE_KERNEL) mrproper
Same as previous one, I'm not convinced about the benefit at all. As a user I would expect that it cleans everything in the build directory while in fact it seems it will only apply to the kernel.
Willy
The O=/path/to/kernel-<ARCH> option allows to build kernel for different architectures in different output directories, in this scene, it doesn't need the mrproper operation for defconfig anymore.
If really require to clean up the source code tree, let users run the standalone mrproper target on demand.
Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/testing/selftests/nolibc/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 9d9902b54e5e..83cb4b017bef 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -203,7 +203,7 @@ mrproper: $(Q)$(MAKE_KERNEL) mrproper
defconfig: - $(Q)$(MAKE_KERNEL) mrproper $(DEFCONFIG) prepare + $(Q)$(MAKE_KERNEL) $(DEFCONFIG) prepare
menuconfig: $(Q)$(MAKE_KERNEL) menuconfig
On Wed, Jul 19, 2023 at 09:24:54PM +0800, Zhangjin Wu wrote:
The O=/path/to/kernel-<ARCH> option allows to build kernel for different architectures in different output directories, in this scene, it doesn't need the mrproper operation for defconfig anymore.
If really require to clean up the source code tree, let users run the standalone mrproper target on demand.
But that's precisely what's going to make it more and more annoying to run simple tests. The mrproper was there precisely because one config at a time was being used, so without it we'll restart to see plenty of failures as it used to be before mrproper was added. I understand what you're trying to do with the O=, but then if you're already passing "O=", why not also pass "defconfig" ?
I mean, let's make sure this makefile is only used to manipulate the tests. It must not become a huge wrapper for the kernel makefile otherwise it will become extremely complicated to use to run just a simple test. And with this one and the last few ones, I'm starting to sense that I'll have to read a README to figure how to reliably run a test.
In my opinion, there are mainly two use cases : - user, manually: commands should be short, forgiving to user mistakes, and easy to remember. I.e. they're compatible with upper-arrow, then enter.
- scripts: these are the ones already running in loops with tons of variables, setting object directories with O=$arch/... and taking care of their own cleanups. These ones will already be user-specific and can very well accomodate one or two extra lines for a make mrproper or make defconfig if needed.
The second ones deserve thinking and control anyway. The first ones should mostly not fail for a user mistake and in the worst case waste a bit of their time by rebuilding something that could have been avoided. But I do want to prioritize the user here. And that's also why I want the makefile to be easy to read with as few macros as possible, because once it works for you, it's easy to figure what is being done, and how to exploit it from your scripts. The opposite is not true. Nobody reads a makefile full of macros to try to figure how to run their first test or why a test that worked once now fails.
Thanks, Willy
On Wed, Jul 19, 2023 at 09:24:54PM +0800, Zhangjin Wu wrote:
The O=/path/to/kernel-<ARCH> option allows to build kernel for different architectures in different output directories, in this scene, it doesn't need the mrproper operation for defconfig anymore.
If really require to clean up the source code tree, let users run the standalone mrproper target on demand.
But that's precisely what's going to make it more and more annoying to run simple tests. The mrproper was there precisely because one config at a time was being used, so without it we'll restart to see plenty of failures as it used to be before mrproper was added. I understand what you're trying to do with the O=, but then if you're already passing "O=", why not also pass "defconfig" ?
I mean, let's make sure this makefile is only used to manipulate the tests. It must not become a huge wrapper for the kernel makefile otherwise it will become extremely complicated to use to run just a simple test. And with this one and the last few ones, I'm starting to sense that I'll have to read a README to figure how to reliably run a test.
In my opinion, there are mainly two use cases :
user, manually: commands should be short, forgiving to user mistakes, and easy to remember. I.e. they're compatible with upper-arrow, then enter.
scripts: these are the ones already running in loops with tons of variables, setting object directories with O=$arch/... and taking care of their own cleanups. These ones will already be user-specific and can very well accomodate one or two extra lines for a make mrproper or make defconfig if needed.
The second ones deserve thinking and control anyway. The first ones should mostly not fail for a user mistake and in the worst case waste a bit of their time by rebuilding something that could have been avoided. But I do want to prioritize the user here. And that's also why I want the makefile to be easy to read with as few macros as possible, because once it works for you, it's easy to figure what is being done, and how to exploit it from your scripts. The opposite is not true. Nobody reads a makefile full of macros to try to figure how to run their first test or why a test that worked once now fails.
Ok, let's reserve mrproper for defconfig and also for tinyconfig if we add standalone target for it.
let's drop patch and the previous one.
Thanks, Zhangjin
Thanks, Willy
To avoid run targets one by one manually and boringly, let's string them with IMAGE and .config, the MAKE command will trigger the dependencies for us.
Note, to allow do menuconfig before extconfig manually, only trigger defconfig while the .config is not there, it means only trigger defconfig for the first run or after a mrproper.
Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/testing/selftests/nolibc/Makefile | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 83cb4b017bef..541f3565e584 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -150,6 +150,7 @@ all: run
sysroot: sysroot/$(ARCH)/include
+PHONY = sysroot/$(ARCH)/include sysroot/$(ARCH)/include: $(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot $(QUIET_MKDIR)mkdir -p sysroot @@ -205,21 +206,28 @@ mrproper: defconfig: $(Q)$(MAKE_KERNEL) $(DEFCONFIG) prepare
-menuconfig: +PHONY += $(KERNEL_CONFIG) +$(KERNEL_CONFIG): + $(Q)if [ ! -f "$(KERNEL_CONFIG)" ]; then $(MAKE) --no-print-directory defconfig; fi + +menuconfig: $(KERNEL_CONFIG) $(Q)$(MAKE_KERNEL) menuconfig
-extconfig: +extconfig: $(KERNEL_CONFIG) $(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(objtree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(EXTCONFIG),$(wildcard $(CURDIR)/configs/$c)) $(Q)$(MAKE_KERNEL) KCONFIG_ALLCONFIG="$(KERNEL_CONFIG)" allnoconfig
-kernel: initramfs +kernel: extconfig + $(Q)$(MAKE) --no-print-directory initramfs $(Q)$(MAKE_KERNEL) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
# common macros for qemu run/rerun targets QEMU_SYSTEM_RUN = qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(KERNEL_IMAGE)" -serial stdio $(QEMU_ARGS)
# run the tests after building the kernel -run: kernel +PHONY += $(KERNEL_IMAGE) +$(KERNEL_IMAGE): kernel +run: $(KERNEL_IMAGE) $(Q)$(QEMU_SYSTEM_RUN) $(LOG_OUT) $(Q)$(REPORT_RUN_OUT)
@@ -244,4 +252,4 @@ clean: $(call QUIET_CLEAN, run.out) $(Q)rm -rf run.out
-.PHONY: sysroot/$(ARCH)/include +.PHONY: $(PHONY)
On Wed, Jul 19, 2023 at 09:26:01PM +0800, Zhangjin Wu wrote:
To avoid run targets one by one manually and boringly, let's string them with IMAGE and .config, the MAKE command will trigger the dependencies for us.
Note, to allow do menuconfig before extconfig manually, only trigger defconfig while the .config is not there, it means only trigger defconfig for the first run or after a mrproper.
Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/testing/selftests/nolibc/Makefile | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 83cb4b017bef..541f3565e584 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile
(...)
-extconfig: +extconfig: $(KERNEL_CONFIG) $(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(objtree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(EXTCONFIG),$(wildcard $(CURDIR)/configs/$c)) $(Q)$(MAKE_KERNEL) KCONFIG_ALLCONFIG="$(KERNEL_CONFIG)" allnoconfig -kernel: initramfs +kernel: extconfig
- $(Q)$(MAKE) --no-print-directory initramfs
There seems to be something wrong here. From what I'm seeing, now if I run "make kernel" it will run extconfig and possibly change the config I just edited.
Or am I missing something ? I must confess all of this is becoming more and more obscure :-(
Willy
On Wed, Jul 19, 2023 at 09:26:01PM +0800, Zhangjin Wu wrote:
To avoid run targets one by one manually and boringly, let's string them with IMAGE and .config, the MAKE command will trigger the dependencies for us.
Note, to allow do menuconfig before extconfig manually, only trigger defconfig while the .config is not there, it means only trigger defconfig for the first run or after a mrproper.
Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/testing/selftests/nolibc/Makefile | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 83cb4b017bef..541f3565e584 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile
(...)
-extconfig: +extconfig: $(KERNEL_CONFIG) $(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(objtree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(EXTCONFIG),$(wildcard $(CURDIR)/configs/$c)) $(Q)$(MAKE_KERNEL) KCONFIG_ALLCONFIG="$(KERNEL_CONFIG)" allnoconfig -kernel: initramfs +kernel: extconfig
- $(Q)$(MAKE) --no-print-directory initramfs
There seems to be something wrong here. From what I'm seeing, now if I run "make kernel" it will run extconfig and possibly change the config I just edited.
Yeah, extconfig will run for every 'make kernel', it is ok for us to let kernel depends on $(KERNEL_CONFIG), but this requires users to run extconfig explictly, the solution may be:
- move extconfig operations to defconfig target and future tinyconfig target (it looks cleaner ...)
- but it is not convenient to trigger additional changes introduced by extconfig, not necessary, but really helpful, something like 'menuconfig'
- let users run extconfig manually after a defconfig or tinyconfig (it is a little complex)
- this make users hard to learn what to do, should give up this method
- run extconfig for every 'make kernel' as it currently do
- this may change something after a menuconfig, but it only trigger minimal required options, the 'hurt' should be minimal, but of course, it may confuse sometimes ;-(
As a summary, let's remove 'extconfig' and move its operations to the defconfig and the future tinyconfig targets? 'extconfig' should be a 'default' config action, let users apply additional ones manually from the top-level 'make menuconfig', what's your idea?
Or am I missing something ? I must confess all of this is becoming more and more obscure :-(
Yeah ... let's find a better solution ;-)
Best regards, Zhangjin
Willy
On Tue, Jul 25, 2023 at 10:20:17PM +0800, Zhangjin Wu wrote:
On Wed, Jul 19, 2023 at 09:26:01PM +0800, Zhangjin Wu wrote:
To avoid run targets one by one manually and boringly, let's string them with IMAGE and .config, the MAKE command will trigger the dependencies for us.
Note, to allow do menuconfig before extconfig manually, only trigger defconfig while the .config is not there, it means only trigger defconfig for the first run or after a mrproper.
Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/testing/selftests/nolibc/Makefile | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 83cb4b017bef..541f3565e584 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile
(...)
-extconfig: +extconfig: $(KERNEL_CONFIG) $(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(objtree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(EXTCONFIG),$(wildcard $(CURDIR)/configs/$c)) $(Q)$(MAKE_KERNEL) KCONFIG_ALLCONFIG="$(KERNEL_CONFIG)" allnoconfig -kernel: initramfs +kernel: extconfig
- $(Q)$(MAKE) --no-print-directory initramfs
There seems to be something wrong here. From what I'm seeing, now if I run "make kernel" it will run extconfig and possibly change the config I just edited.
Yeah, extconfig will run for every 'make kernel', it is ok for us to let kernel depends on $(KERNEL_CONFIG), but this requires users to run extconfig explictly, the solution may be:
move extconfig operations to defconfig target and future tinyconfig target (it looks cleaner ...)
- but it is not convenient to trigger additional changes introduced by extconfig, not necessary, but really helpful, something like 'menuconfig'
let users run extconfig manually after a defconfig or tinyconfig (it is a little complex)
- this make users hard to learn what to do, should give up this method
run extconfig for every 'make kernel' as it currently do
- this may change something after a menuconfig, but it only trigger minimal required options, the 'hurt' should be minimal, but of course, it may confuse sometimes ;-(
As a summary, let's remove 'extconfig' and move its operations to the defconfig and the future tinyconfig targets? 'extconfig' should be a 'default' config action, let users apply additional ones manually from the top-level 'make menuconfig', what's your idea?
Well, it's important to apply the principle of least surprise for the user. You're a developer who spent time working on your config, you believe it's OK and you just remind that you've heard about that nolibc test thing recently and you think "why not give it a try in case it spots something I forgot in my config". Then you just run the test there and once done your config was secretly modified. This is exactly an example of what *not* to do. We should never modify user's config nor files in general without an explicit request from the user. If the user types "make defconfig", they're implicitly requesting to replace the config, so we can do what we want with it. If they type "make kernel", they expect to make a kernel based on this config, not to mollest this precious config file and then make a kernel out of it.
So I'm fine with the idea of adding config snippets on top of defconfig and tinyconfig to allow the user to generate a working config for a given architecture, but not for modifying their config without their consent.
Willy
On Tue, Jul 25, 2023 at 10:20:17PM +0800, Zhangjin Wu wrote:
On Wed, Jul 19, 2023 at 09:26:01PM +0800, Zhangjin Wu wrote:
To avoid run targets one by one manually and boringly, let's string them with IMAGE and .config, the MAKE command will trigger the dependencies for us.
Note, to allow do menuconfig before extconfig manually, only trigger defconfig while the .config is not there, it means only trigger defconfig for the first run or after a mrproper.
Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/testing/selftests/nolibc/Makefile | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 83cb4b017bef..541f3565e584 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile
(...)
-extconfig: +extconfig: $(KERNEL_CONFIG) $(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(objtree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(EXTCONFIG),$(wildcard $(CURDIR)/configs/$c)) $(Q)$(MAKE_KERNEL) KCONFIG_ALLCONFIG="$(KERNEL_CONFIG)" allnoconfig -kernel: initramfs +kernel: extconfig
- $(Q)$(MAKE) --no-print-directory initramfs
There seems to be something wrong here. From what I'm seeing, now if I run "make kernel" it will run extconfig and possibly change the config I just edited.
Yeah, extconfig will run for every 'make kernel', it is ok for us to let kernel depends on $(KERNEL_CONFIG), but this requires users to run extconfig explictly, the solution may be:
move extconfig operations to defconfig target and future tinyconfig target (it looks cleaner ...)
- but it is not convenient to trigger additional changes introduced by extconfig, not necessary, but really helpful, something like 'menuconfig'
let users run extconfig manually after a defconfig or tinyconfig (it is a little complex)
- this make users hard to learn what to do, should give up this method
run extconfig for every 'make kernel' as it currently do
- this may change something after a menuconfig, but it only trigger minimal required options, the 'hurt' should be minimal, but of course, it may confuse sometimes ;-(
As a summary, let's remove 'extconfig' and move its operations to the defconfig and the future tinyconfig targets? 'extconfig' should be a 'default' config action, let users apply additional ones manually from the top-level 'make menuconfig', what's your idea?
Well, it's important to apply the principle of least surprise for the user. You're a developer who spent time working on your config, you believe it's OK and you just remind that you've heard about that nolibc test thing recently and you think "why not give it a try in case it spots something I forgot in my config". Then you just run the test there and once done your config was secretly modified. This is exactly an example of what *not* to do. We should never modify user's config nor files in general without an explicit request from the user. If the user types "make defconfig", they're implicitly requesting to replace the config, so we can do what we want with it. If they type "make kernel", they expect to make a kernel based on this config, not to mollest this precious config file and then make a kernel out of it.
So I'm fine with the idea of adding config snippets on top of defconfig and tinyconfig to allow the user to generate a working config for a given architecture, but not for modifying their config without their consent.
Agree, seems our additional config snippets are minimal and 'necessary' for boot and print, so, I ignored the override issue before.
What about the version in v3 here: https://lore.kernel.org/lkml/9b52e26748eda1ac108d569207bf428bf37b3bbc.169048...
The 'defconfig' will only be triggered while there is no .config there, I do think it is important, at the first time of using nolibc, I directly run kernel but it fails for it has a manual defconfig requirement every time, so, I do think a default defconfig for kernel for the first run or after a mrproper is helpful, it doesn't modify any .config for there is no one there.
+PHONY += $(KERNEL_CONFIG) +$(KERNEL_CONFIG): + $(Q)if [ ! -f "$(KERNEL_CONFIG)" ]; then $(MAKE) --no-print-directory defconfig; fi + +kernel: $(KERNEL_CONFIG) + $(Q)$(MAKE) --no-print-directory initramfs $(Q)$(MAKE_KERNEL) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
Thanks, Zhangjin
Willy
On Sat, Jul 29, 2023 at 05:54:47PM +0800, Zhangjin Wu wrote:
The 'defconfig' will only be triggered while there is no .config there, I do think it is important, at the first time of using nolibc, I directly run kernel but it fails for it has a manual defconfig requirement every time, so, I do think a default defconfig for kernel for the first run or after a mrproper is helpful, it doesn't modify any .config for there is no one there.
On the opposite, that's yet another example of automatic stuff that for me adds zero value and just doubts in the user's head: "is it safe to call this with my own config or should I keep a safe copy of it?", "what will it use for the config?", "will the arch be correct if my current config references 32BIT and the generated default one switches it to 64?" etc.
Please let's not add unneeded dependencies and chaining. It does not help and makes it harder to restart at one specific step, thus lowers the overall value.
Willy
On Sat, Jul 29, 2023 at 05:54:47PM +0800, Zhangjin Wu wrote:
The 'defconfig' will only be triggered while there is no .config there, I do think it is important, at the first time of using nolibc, I directly run kernel but it fails for it has a manual defconfig requirement every time, so, I do think a default defconfig for kernel for the first run or after a mrproper is helpful, it doesn't modify any .config for there is no one there.
On the opposite, that's yet another example of automatic stuff that for me adds zero value and just doubts in the user's head: "is it safe to call this with my own config or should I keep a safe copy of it?", "what will it use for the config?", "will the arch be correct if my current config references 32BIT and the generated default one switches it to 64?" etc.
Please let's not add unneeded dependencies and chaining. It does not help and makes it harder to restart at one specific step, thus lowers the overall value.
Ok, let's do subtraction, drop this one and the timeout two.
Thanks, Zhangjin
Willy
The kernel of some architectures can not poweroff qemu-system normally, especially for tinyconfig.
Some architectures may have no kernel poweroff support, the others may require more kernel config options and therefore slow down the tinyconfig build and test. and also, it's very hard (and some even not possible) to find out the exact poweroff related kernel config options for every architecture.
Since the low-level poweroff support is heavily kernel & qemu dependent, it is not that critical to both nolibc and nolibc-test, let's simply ignore the poweroff required kernel config options for tinyconfig (and even for defconfig) and quit qemu-system after a specified timeout or with an expected system halt or poweroff string (these strings mean our reboot() library routine is perfectly ok).
QEMU_TIMEOUT value can be configured for every architecture based on their time cost requirement of boot+test+poweroff.
Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/testing/selftests/nolibc/Makefile | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 541f3565e584..a03fab020ebe 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -93,6 +93,9 @@ QEMU_ARGS_s390 = -M s390-ccw-virtio -m 1G -append "console=ttyS0 panic=-1 QEMU_ARGS_loongarch = -M virt -append "console=ttyS0,115200 panic=-1 $(TEST:%=NOLIBC_TEST=%)" QEMU_ARGS = $(QEMU_ARGS_$(XARCH)) $(QEMU_ARGS_EXTRA)
+# QEMU_TIMEOUT: some architectures can not poweroff normally, especially for tinyconfig +QEMU_TIMEOUT = $(QEMU_TIMEOUT_$(XARCH)) + # OUTPUT is only set when run from the main makefile, otherwise # it defaults to this nolibc directory. OUTPUT ?= $(CURDIR)/ @@ -224,16 +227,32 @@ kernel: extconfig # common macros for qemu run/rerun targets QEMU_SYSTEM_RUN = qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(KERNEL_IMAGE)" -serial stdio $(QEMU_ARGS)
+ifneq ($(QEMU_TIMEOUT),) +TIMEOUT_CMD = t=$(QEMU_TIMEOUT); \ + while [ $$t -gt 0 ]; do \ + sleep 5; t=$$(expr $$t - 5); echo "detecting power off ..."; \ + if grep -qE "reboot: System halted|reboot: Power down" "$(RUN_OUT)"; then \ + pkill -9 qemu-system-$(QEMU_ARCH); \ + echo "powered off, test finish"; t=1; break; \ + fi; \ + done; \ + if [ $$t -le 0 ]; then pkill -9 qemu-system-$(QEMU_ARCH); echo "qemu-system-$(QEMU_ARCH) timeout"; fi + +TIMEOUT_QEMU_RUN = ($(QEMU_SYSTEM_RUN) $(LOG_OUT) &); $(TIMEOUT_CMD) +else +TIMEOUT_QEMU_RUN = $(QEMU_SYSTEM_RUN) $(LOG_OUT) +endif + # run the tests after building the kernel PHONY += $(KERNEL_IMAGE) $(KERNEL_IMAGE): kernel run: $(KERNEL_IMAGE) - $(Q)$(QEMU_SYSTEM_RUN) $(LOG_OUT) + $(Q)$(TIMEOUT_QEMU_RUN) $(Q)$(REPORT_RUN_OUT)
# re-run the tests from an existing kernel rerun: - $(Q)$(QEMU_SYSTEM_RUN) $(LOG_OUT) + $(Q)$(TIMEOUT_QEMU_RUN) $(Q)$(REPORT_RUN_OUT)
# report with existing test log
On Wed, Jul 19, 2023 at 09:27:08PM +0800, Zhangjin Wu wrote:
The kernel of some architectures can not poweroff qemu-system normally, especially for tinyconfig.
Some architectures may have no kernel poweroff support, the others may require more kernel config options and therefore slow down the tinyconfig build and test. and also, it's very hard (and some even not possible) to find out the exact poweroff related kernel config options for every architecture.
Since the low-level poweroff support is heavily kernel & qemu dependent, it is not that critical to both nolibc and nolibc-test, let's simply ignore the poweroff required kernel config options for tinyconfig (and even for defconfig) and quit qemu-system after a specified timeout or with an expected system halt or poweroff string (these strings mean our reboot() library routine is perfectly ok).
QEMU_TIMEOUT value can be configured for every architecture based on their time cost requirement of boot+test+poweroff.
Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/testing/selftests/nolibc/Makefile | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 541f3565e584..a03fab020ebe 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -93,6 +93,9 @@ QEMU_ARGS_s390 = -M s390-ccw-virtio -m 1G -append "console=ttyS0 panic=-1 QEMU_ARGS_loongarch = -M virt -append "console=ttyS0,115200 panic=-1 $(TEST:%=NOLIBC_TEST=%)" QEMU_ARGS = $(QEMU_ARGS_$(XARCH)) $(QEMU_ARGS_EXTRA) +# QEMU_TIMEOUT: some architectures can not poweroff normally, especially for tinyconfig +QEMU_TIMEOUT = $(QEMU_TIMEOUT_$(XARCH))
# OUTPUT is only set when run from the main makefile, otherwise # it defaults to this nolibc directory. OUTPUT ?= $(CURDIR)/ @@ -224,16 +227,32 @@ kernel: extconfig # common macros for qemu run/rerun targets QEMU_SYSTEM_RUN = qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(KERNEL_IMAGE)" -serial stdio $(QEMU_ARGS) +ifneq ($(QEMU_TIMEOUT),) +TIMEOUT_CMD = t=$(QEMU_TIMEOUT); \
- while [ $$t -gt 0 ]; do \
sleep 5; t=$$(expr $$t - 5); echo "detecting power off ..."; \if grep -qE "reboot: System halted|reboot: Power down" "$(RUN_OUT)"; then \pkill -9 qemu-system-$(QEMU_ARCH); \echo "powered off, test finish"; t=1; break; \fi; \- done; \
- if [ $$t -le 0 ]; then pkill -9 qemu-system-$(QEMU_ARCH); echo "qemu-system-$(QEMU_ARCH) timeout"; fi
Please have a look at the "timeout" command whichi makes all this much simpler. Also, please get used to never ever use kill -9 first. This is exactly the way to leave temporary files and IPCs wandering around while many programs that care about cleanups at least try to do that upon a regular TERM or INT signal.
Willy
Hi, Willy
On Wed, Jul 19, 2023 at 09:27:08PM +0800, Zhangjin Wu wrote:
The kernel of some architectures can not poweroff qemu-system normally, especially for tinyconfig.
Some architectures may have no kernel poweroff support, the others may require more kernel config options and therefore slow down the tinyconfig build and test. and also, it's very hard (and some even not possible) to find out the exact poweroff related kernel config options for every architecture.
Since the low-level poweroff support is heavily kernel & qemu dependent, it is not that critical to both nolibc and nolibc-test, let's simply ignore the poweroff required kernel config options for tinyconfig (and even for defconfig) and quit qemu-system after a specified timeout or with an expected system halt or poweroff string (these strings mean our reboot() library routine is perfectly ok).
QEMU_TIMEOUT value can be configured for every architecture based on their time cost requirement of boot+test+poweroff.
Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/testing/selftests/nolibc/Makefile | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 541f3565e584..a03fab020ebe 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -93,6 +93,9 @@ QEMU_ARGS_s390 = -M s390-ccw-virtio -m 1G -append "console=ttyS0 panic=-1 QEMU_ARGS_loongarch = -M virt -append "console=ttyS0,115200 panic=-1 $(TEST:%=NOLIBC_TEST=%)" QEMU_ARGS = $(QEMU_ARGS_$(XARCH)) $(QEMU_ARGS_EXTRA) +# QEMU_TIMEOUT: some architectures can not poweroff normally, especially for tinyconfig +QEMU_TIMEOUT = $(QEMU_TIMEOUT_$(XARCH))
# OUTPUT is only set when run from the main makefile, otherwise # it defaults to this nolibc directory. OUTPUT ?= $(CURDIR)/ @@ -224,16 +227,32 @@ kernel: extconfig # common macros for qemu run/rerun targets QEMU_SYSTEM_RUN = qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(KERNEL_IMAGE)" -serial stdio $(QEMU_ARGS) +ifneq ($(QEMU_TIMEOUT),) +TIMEOUT_CMD = t=$(QEMU_TIMEOUT); \
- while [ $$t -gt 0 ]; do \
sleep 5; t=$$(expr $$t - 5); echo "detecting power off ..."; \if grep -qE "reboot: System halted|reboot: Power down" "$(RUN_OUT)"; then \pkill -9 qemu-system-$(QEMU_ARCH); \echo "powered off, test finish"; t=1; break; \fi; \- done; \
- if [ $$t -le 0 ]; then pkill -9 qemu-system-$(QEMU_ARCH); echo "qemu-system-$(QEMU_ARCH) timeout"; fi
Please have a look at the "timeout" command whichi makes all this much simpler.
Yeah, I used 'timeout --forgeground' before, but it is still a dead wait and it is very hard for us to configure a just right wait time.
If the configured wait time is short, the qemu will be killed during the test or before running the test, If the configured wait time is long, we will need to dead wait there even if the test is finished, although during interactive running, we can press 'CTRL + A X', but for background running, it is just time waste.
To fix up this issue, the above method used at last allow to detect the output string, when the test finish and print something lines like:
reboot: System halted reboot: Power down.
We will use pkill to send signal to tell qemu quit, so, it is ok for us to configure a even'big' timeout, if the kernel can normally poweroff or if nolibc can successfully send the poweroff syscall, the above message will be detected and qemu will quit as expected, it will completely avoid dead wait, the configured timeout will never happen, this is comfortable.
The worst case is only when qemu or kernel reject to boot, for example, a qemu bios missing or mismatch issue or a kernel regression or a wrong kernel config option, for these cases, the real timeout logic will work for us.
As a summary, our timeout logic here include two parts: one is 'poweroff' related string detection, another is the real timeout logic.
Based on current implementation, I even plan to add the test finish string in the expected strings:
Leaving init with final status
And even further, when a hang detected (no normal poweroff or test finish string detected), print the whole or last part of running log to tell users what happens.
Also, please get used to never ever use kill -9 first. This is exactly the way to leave temporary files and IPCs wandering around while many programs that care about cleanups at least try to do that upon a regular TERM or INT signal.
Ok, thanks, will use TERM or INT signal instead.
Willy
On Tue, Jul 25, 2023 at 10:59:55PM +0800, Zhangjin Wu wrote:
On Wed, Jul 19, 2023 at 09:27:08PM +0800, Zhangjin Wu wrote:
The kernel of some architectures can not poweroff qemu-system normally, especially for tinyconfig.
Some architectures may have no kernel poweroff support, the others may require more kernel config options and therefore slow down the tinyconfig build and test. and also, it's very hard (and some even not possible) to find out the exact poweroff related kernel config options for every architecture.
Since the low-level poweroff support is heavily kernel & qemu dependent, it is not that critical to both nolibc and nolibc-test, let's simply ignore the poweroff required kernel config options for tinyconfig (and even for defconfig) and quit qemu-system after a specified timeout or with an expected system halt or poweroff string (these strings mean our reboot() library routine is perfectly ok).
QEMU_TIMEOUT value can be configured for every architecture based on their time cost requirement of boot+test+poweroff.
Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/testing/selftests/nolibc/Makefile | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 541f3565e584..a03fab020ebe 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -93,6 +93,9 @@ QEMU_ARGS_s390 = -M s390-ccw-virtio -m 1G -append "console=ttyS0 panic=-1 QEMU_ARGS_loongarch = -M virt -append "console=ttyS0,115200 panic=-1 $(TEST:%=NOLIBC_TEST=%)" QEMU_ARGS = $(QEMU_ARGS_$(XARCH)) $(QEMU_ARGS_EXTRA) +# QEMU_TIMEOUT: some architectures can not poweroff normally, especially for tinyconfig +QEMU_TIMEOUT = $(QEMU_TIMEOUT_$(XARCH))
# OUTPUT is only set when run from the main makefile, otherwise # it defaults to this nolibc directory. OUTPUT ?= $(CURDIR)/ @@ -224,16 +227,32 @@ kernel: extconfig # common macros for qemu run/rerun targets QEMU_SYSTEM_RUN = qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(KERNEL_IMAGE)" -serial stdio $(QEMU_ARGS) +ifneq ($(QEMU_TIMEOUT),) +TIMEOUT_CMD = t=$(QEMU_TIMEOUT); \
- while [ $$t -gt 0 ]; do \
sleep 5; t=$$(expr $$t - 5); echo "detecting power off ..."; \if grep -qE "reboot: System halted|reboot: Power down" "$(RUN_OUT)"; then \pkill -9 qemu-system-$(QEMU_ARCH); \echo "powered off, test finish"; t=1; break; \fi; \- done; \
- if [ $$t -le 0 ]; then pkill -9 qemu-system-$(QEMU_ARCH); echo "qemu-system-$(QEMU_ARCH) timeout"; fi
Please have a look at the "timeout" command whichi makes all this much simpler.
Yeah, I used 'timeout --forgeground' before, but it is still a dead wait and it is very hard for us to configure a just right wait time.
If the configured wait time is short, the qemu will be killed during the test or before running the test, If the configured wait time is long, we will need to dead wait there even if the test is finished, although during interactive running, we can press 'CTRL + A X', but for background running, it is just time waste.
To fix up this issue, the above method used at last allow to detect the output string, when the test finish and print something lines like:
reboot: System halted reboot: Power down.We will use pkill to send signal to tell qemu quit, so, it is ok for us to configure a even'big' timeout, if the kernel can normally poweroff or if nolibc can successfully send the poweroff syscall, the above message will be detected and qemu will quit as expected, it will completely avoid dead wait, the configured timeout will never happen, this is comfortable.
The worst case is only when qemu or kernel reject to boot, for example, a qemu bios missing or mismatch issue or a kernel regression or a wrong kernel config option, for these cases, the real timeout logic will work for us.
As a summary, our timeout logic here include two parts: one is 'poweroff' related string detection, another is the real timeout logic.
Based on current implementation, I even plan to add the test finish string in the expected strings:
Leaving init with final statusAnd even further, when a hang detected (no normal poweroff or test finish string detected), print the whole or last part of running log to tell users what happens.
Again, all of this seems ridiculously complicated for what seems to be a very unlikely issue. You mentioned earlier:
The kernel of some architectures can not poweroff qemu-system normally, especially for tinyconfig.
Till now all those I tested do power off correctly. So if the problem is related to one specific arch, first it should be mentioned in the commit message, and maybe we need to look closer why it's doing that instead of papering over it, and if it's caused by tinyconfig, it just means we need to produce a more reasonable config. But I still don't like the idea of causing a problem on one side, and making the other side totally ugly just because of the problem. The timeout solution should be a last resort one which clearly explains why we can't do differently.
Also, defconfig and tinyconfig are for people who work on nolibc. These ones should support working in batch mode, being called from a script iterating over multiple archs. For other config, we should not interfer with what the user does. Maybe some will consider that it's fine to run a test slowly on a simulated platform for example.
Willy
Some cross compilers may not just be prefixed with ARCH or XARCH, customize them by architecture may easier the test a lot, especially, when iterate with XARCH or ARCH.
After customizing this for every architecture, the minimal test argument will be architecture itself, no CROSS_COMPILE will be passed.
If the installed cross compiler is not the same as the one customized, we can also pass CROSS_COMPILE from command line as before, no regression.
Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/testing/selftests/nolibc/Makefile | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index a03fab020ebe..3c2be27747ea 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -42,6 +42,12 @@ IMAGE_loongarch = arch/loongarch/boot/vmlinuz.efi IMAGE = $(IMAGE_$(XARCH)) IMAGE_NAME = $(notdir $(IMAGE))
+# CROSS_COMPILE: cross toolchain prefix by architecture +CROSS_COMPILE ?= $(CROSS_COMPILE_$(XARCH)) + +# make sure CC is prefixed with CROSS_COMPILE +$(call allow-override,CC,$(CROSS_COMPILE)gcc) + # default kernel configurations that appear to be usable DEFCONFIG_i386 = defconfig DEFCONFIG_x86_64 = defconfig
The little-endian powerpc64le compilers provided by Ubuntu and Fedora are able to compile big endian kernel and big endian nolibc-test [1].
These default CROSS_COMPILE settings allow us to test target architectures with:
$ cd /path/to/tools/testing/selftests/nolibc/
$ for arch in powerpc powerpc64 powerpc64le; do \ make run-user XARCH=$arch | grep "status: "; \ done
If want to use another cross compiler, please simply pass CROSS_COMPILE or CC as before.
For example, it is able to build 64-bit nolibc-test with the big endian powerpc64-linux-gcc crosstool from [2]:
$ wget -c https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/13.1.0/... $ tar xvf x86_64-gcc-13.1.0-nolibc-powerpc64-linux.tar.xz $ export PATH=$PWD/gcc-13.1.0-nolibc/powerpc64-linux/bin/:$PATH
$ export CROSS_COMPILE_powerpc64=powerpc64-linux- $ export CROSS_COMPILE_powerpc64le=powerpc64-linux- $ for arch in powerpc64 powerpc64le; do \ make run-user XARCH=$arch | grep "status: "; \ done
Or specify CC directly with full path:
$ export CC=$PWD/gcc-13.1.0-nolibc/powerpc64-linux/bin/powerpc64-linux-gcc $ for arch in powerpc64 powerpc64le; do \ make run-user XARCH=$arch | grep "status: "; \ done
[1]: https://github.com/open-power/skiboot [2]: https://mirrors.edge.kernel.org/pub/tools/crosstool/
Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/testing/selftests/nolibc/Makefile | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 3c2be27747ea..eec2935672ad 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -43,6 +43,9 @@ IMAGE = $(IMAGE_$(XARCH)) IMAGE_NAME = $(notdir $(IMAGE))
# CROSS_COMPILE: cross toolchain prefix by architecture +CROSS_COMPILE_powerpc ?= powerpc-linux-gnu- +CROSS_COMPILE_powerpc64 ?= powerpc64le-linux-gnu- +CROSS_COMPILE_powerpc64le ?= powerpc64le-linux-gnu- CROSS_COMPILE ?= $(CROSS_COMPILE_$(XARCH))
# make sure CC is prefixed with CROSS_COMPILE
The original tinyconfig target only enables minimal kernel config options, it can speed up the kernel build and nolibc test a lot and also brings us with smaller kernel image size.
But the default enabled options are not enough for qemu boot and console print, some additional config options should be added for every architecture individually.
Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/testing/selftests/nolibc/Makefile | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index eec2935672ad..f42782fa78a9 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -218,6 +218,9 @@ mrproper: defconfig: $(Q)$(MAKE_KERNEL) $(DEFCONFIG) prepare
+tinyconfig: + $(Q)$(MAKE_KERNEL) tinyconfig prepare + PHONY += $(KERNEL_CONFIG) $(KERNEL_CONFIG): $(Q)if [ ! -f "$(KERNEL_CONFIG)" ]; then $(MAKE) --no-print-directory defconfig; fi
On Wed, Jul 19, 2023 at 09:30:30PM +0800, Zhangjin Wu wrote:
The original tinyconfig target only enables minimal kernel config options, it can speed up the kernel build and nolibc test a lot and also brings us with smaller kernel image size.
But the default enabled options are not enough for qemu boot and console print, some additional config options should be added for every architecture individually.
Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/testing/selftests/nolibc/Makefile | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index eec2935672ad..f42782fa78a9 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -218,6 +218,9 @@ mrproper: defconfig: $(Q)$(MAKE_KERNEL) $(DEFCONFIG) prepare +tinyconfig:
- $(Q)$(MAKE_KERNEL) tinyconfig prepare
So for the same reasons as defconfig above, I'd actually keep mrproper here. And if we figure that tinyconfig is never called by the user directly but as a dependency from the makefile itself or scripts, then we likely don't even need to create a visible entry for it.
Willy
On Wed, Jul 19, 2023 at 09:30:30PM +0800, Zhangjin Wu wrote:
The original tinyconfig target only enables minimal kernel config options, it can speed up the kernel build and nolibc test a lot and also brings us with smaller kernel image size.
But the default enabled options are not enough for qemu boot and console print, some additional config options should be added for every architecture individually.
Signed-off-by: Zhangjin Wu falcon@tinylab.org
tools/testing/selftests/nolibc/Makefile | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index eec2935672ad..f42782fa78a9 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -218,6 +218,9 @@ mrproper: defconfig: $(Q)$(MAKE_KERNEL) $(DEFCONFIG) prepare +tinyconfig:
- $(Q)$(MAKE_KERNEL) tinyconfig prepare
So for the same reasons as defconfig above, I'd actually keep mrproper here.
Ok, let's add mrproper back here, since tinyconfig is fast enough, so, a mrproper is not that time cost and a cleanup is really good prepare.
And if we figure that tinyconfig is never called by the user directly but as a dependency from the makefile itself or scripts, then we likely don't even need to create a visible entry for it.
Great idea,
At first, tinyconfig can be triggered by something like:
$ make run defconfig DEFCONFIG=tinyconfig
Perhaps we can let $(KERNEL_CONFIG) depends on the top-level 'tinyconfig' and trigger it by default:
$(KERNEL_CONFIG): $(Q)if [ ! -f "$(KERNEL_CONFIG)" ]; then $(MAKE_KERNEL) --no-print-directory mrproper tinyconfig prepare; fi
Of course, we should triger the extra config above.
But this change must delay after we add tinyconfig support for all of the nolibc supported architectures. before that, we should use 'defconfig' as we do currently.
So, it may be ok for us to drop this patch, but we also need to update some commit messages who uses tinyconfig target directly.
Thanks, Zhangjin
Willy
The original kernel tinyconfig target has already enabled some common options, but they are not enough to enable boot and print.
$ find kernel/ arch/*/ -name "tiny*.config" kernel/configs/tiny-base.config kernel/configs/tiny.config arch/x86/configs/tiny.config
To enable qemu boot and console print, additional kernel config options are required, include the common parts and the architecture specific parts.
Here adds minimal extra common parts for all architectures:
* for initrd: CONFIG_BLK_DEV_INITRD * for init executable: CONFIG_BINFMT_ELF * for test result print: CONFIG_PRINTK, CONFIG_TTY
Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/testing/selftests/nolibc/configs/common.config | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 tools/testing/selftests/nolibc/configs/common.config
diff --git a/tools/testing/selftests/nolibc/configs/common.config b/tools/testing/selftests/nolibc/configs/common.config new file mode 100644 index 000000000000..3957f812faac --- /dev/null +++ b/tools/testing/selftests/nolibc/configs/common.config @@ -0,0 +1,4 @@ +CONFIG_BLK_DEV_INITRD=y +CONFIG_BINFMT_ELF=y +CONFIG_PRINTK=y +CONFIG_TTY=y
Firstly, add extra config files for powerpc, powerpc64le and powerpc64.
Second, QEMU_TIMEOUT is configured as 60 seconds for powerpc to allow quit qemu-system-ppc even if poweroff fails. In normal host machine, ~20 seconds may be enough for boot+test+poweroff, but 60 seconds is used here to gurantee it at least finish even in a very slow host machine or the host machine is too busy. Both powerpc64le and powerpc64 can poweroff normally, no need to configure QEMU_TIMEOUT for them.
It is able to use tinyconfig as the minimal config target to speed up the run target of powerpc:
$ for arch in powerpc powerpc64 powerpc64le; do \ rm -rf $PWD/kernel-$arch; \ mkdir -p $PWD/kernel-$arch; \ make tinyconfig run XARCH=$arch O=$PWD/kernel-$arch | grep status ; \ done
rerun with architecture specific run.out (for later report):
$ for arch in powerpc powerpc64 powerpc64le; do \ mkdir -p $PWD/kernel-$arch; \ make rerun XARCH=$arch O=$PWD/kernel-$arch RUN_OUT=$PWD/run.$arch.out | grep status ; \ done
report:
$ for arch in powerpc powerpc64 powerpc64le; do \ make report RUN_OUT=$PWD/run.$arch.out | grep status ; \ done
Signed-off-by: Zhangjin Wu falcon@tinylab.org --- tools/testing/selftests/nolibc/Makefile | 1 + tools/testing/selftests/nolibc/configs/powerpc.config | 3 +++ tools/testing/selftests/nolibc/configs/powerpc64.config | 3 +++ tools/testing/selftests/nolibc/configs/powerpc64le.config | 4 ++++ 4 files changed, 11 insertions(+) create mode 100644 tools/testing/selftests/nolibc/configs/powerpc64.config create mode 100644 tools/testing/selftests/nolibc/configs/powerpc64le.config
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index f42782fa78a9..b01346323e35 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -103,6 +103,7 @@ QEMU_ARGS_loongarch = -M virt -append "console=ttyS0,115200 panic=-1 $(TEST:%=N QEMU_ARGS = $(QEMU_ARGS_$(XARCH)) $(QEMU_ARGS_EXTRA)
# QEMU_TIMEOUT: some architectures can not poweroff normally, especially for tinyconfig +QEMU_TIMEOUT_powerpc = 60 QEMU_TIMEOUT = $(QEMU_TIMEOUT_$(XARCH))
# OUTPUT is only set when run from the main makefile, otherwise diff --git a/tools/testing/selftests/nolibc/configs/powerpc.config b/tools/testing/selftests/nolibc/configs/powerpc.config index b1975f8253f7..29123cee14c4 100644 --- a/tools/testing/selftests/nolibc/configs/powerpc.config +++ b/tools/testing/selftests/nolibc/configs/powerpc.config @@ -1,3 +1,6 @@ +CONFIG_COMPAT_32BIT_TIME=y +CONFIG_PPC_PMAC=y +CONFIG_PPC_OF_BOOT_TRAMPOLINE=y CONFIG_SERIAL_PMACZILOG=y CONFIG_SERIAL_PMACZILOG_TTYS=y CONFIG_SERIAL_PMACZILOG_CONSOLE=y diff --git a/tools/testing/selftests/nolibc/configs/powerpc64.config b/tools/testing/selftests/nolibc/configs/powerpc64.config new file mode 100644 index 000000000000..4e17f0cdb99f --- /dev/null +++ b/tools/testing/selftests/nolibc/configs/powerpc64.config @@ -0,0 +1,3 @@ +CONFIG_PPC64=y +CONFIG_PPC_POWERNV=y +CONFIG_HVC_OPAL=y diff --git a/tools/testing/selftests/nolibc/configs/powerpc64le.config b/tools/testing/selftests/nolibc/configs/powerpc64le.config new file mode 100644 index 000000000000..713b227f506f --- /dev/null +++ b/tools/testing/selftests/nolibc/configs/powerpc64le.config @@ -0,0 +1,4 @@ +CONFIG_PPC64=y +CONFIG_PPC_POWERNV=y +CONFIG_HVC_OPAL=y +CONFIG_CPU_LITTLE_ENDIAN=y
On Wed, Jul 19, 2023 at 09:32:46PM +0800, Zhangjin Wu wrote:
Firstly, add extra config files for powerpc, powerpc64le and powerpc64.
Second, QEMU_TIMEOUT is configured as 60 seconds for powerpc to allow quit qemu-system-ppc even if poweroff fails. In normal host machine, ~20 seconds may be enough for boot+test+poweroff, but 60 seconds is used here to gurantee it at least finish even in a very slow host machine or the host machine is too busy. Both powerpc64le and powerpc64 can poweroff normally, no need to configure QEMU_TIMEOUT for them.
Hmmm call me annoying, but this started with tinyconfig "in order to save build time" and now it's enforcing a 1-minute timeout on a single test. When I run the tests, they hardly last more than a few seconds and sometimes even just about one second. If some tests last too long doing nothing, we should adjust their config (e.g. useless probe of a driver). If they can't power off due to a config option we need to fix that option. If it can't power off due to the architecture, we can also try the reboot (qemu is started with --no-reboot to stop instead of rebooting), and as a last resort we should rely on the timeout in case everything else fails. But then this timeout should be quite short because we'll then have guaranteed from the choice of config options that it boots and executes fast by default.
Finally, if we need to implement a timeout enforcement for at least one arch because we do not control every failure case, then there's no reason for considering that certain archs are safe against this and others not. This means that we can (should?) implement the timeout by default for every arch, and make sure that the timeout is never hit by default, unless there's really absolutely no way to fix the arch that cannot power down nor reboot, in which case the timeout should remain short enough.
What's your opinion ?
Willy
Hi, Willy
On Wed, Jul 19, 2023 at 09:32:46PM +0800, Zhangjin Wu wrote:
Firstly, add extra config files for powerpc, powerpc64le and powerpc64.
Second, QEMU_TIMEOUT is configured as 60 seconds for powerpc to allow quit qemu-system-ppc even if poweroff fails. In normal host machine, ~20 seconds may be enough for boot+test+poweroff, but 60 seconds is used here to gurantee it at least finish even in a very slow host machine or the host machine is too busy. Both powerpc64le and powerpc64 can poweroff normally, no need to configure QEMU_TIMEOUT for them.
Hmmm call me annoying, but this started with tinyconfig "in order to save build time" and now it's enforcing a 1-minute timeout on a single test. When I run the tests, they hardly last more than a few seconds and sometimes even just about one second. If some tests last too long doing nothing, we should adjust their config (e.g. useless probe of a driver). If they can't power off due to a config option we need to fix that option. If it can't power off due to the architecture, we can also try the reboot (qemu is started with --no-reboot to stop instead of rebooting), and as a last resort we should rely on the timeout in case everything else fails. But then this timeout should be quite short because we'll then have guaranteed from the choice of config options that it boots and executes fast by default.
As I just explained in this reply [1], our current timeout logic will detect the 'power off' string at first, so, the 1-minute is the worst case when the qemu even not print a 'power off' string, that should be a bug, normally, after the 'power off' string detected, qemu will quit as expected. the 1-minute is just configured here as a last watchdog to detect a real hang (may be bios related or may be kernel realted) ;-)
So, the 60 seconds will never be reached, even there is a failed poweroff, but a smaller one may be ok, what about 30 seconds?
[1]: https://lore.kernel.org/lkml/20230725145955.37685-1-falcon@tinylab.org/
Finally, if we need to implement a timeout enforcement for at least one arch because we do not control every failure case, then there's no reason for considering that certain archs are safe against this and others not. This means that we can (should?) implement the timeout by default for every arch,
Agree, so, what your suggestion about the default timeout? ;-)
10 or 15 seconds may be not enough especially when running on a very slow host machine, for example, my host will be very slow when the battery is not in charging status ;-(
And also, the architectures like PowerPC using a very slow SLOF will boot very slowly, sometimes 20 seconds may be not enough and it may cost 30+ seconds on a very slow machine.
and make sure that the timeout is never hit by default
Yeah, it is the current behavior.
, unless there's really absolutely no way to fix the arch that cannot power down nor reboot,
Even when the kernel not support poweroff, the 'power off' string will be printed after our 'reboot' syscall, our current timeout logic will detect this and let qemu quit. We even plan to detect the 'Leaving init with final status' line.
so, it is not necessary to spend too much time to find out and enable the kernel power off support for every architecture. and some architectures may simply not support power-off, and also, some architectures require too many 'heavy' options to let power-off work, which may increase build time for tinyconfig a lot, for example, the ACPI+PCI support are required for power-off for x86.
in which case the timeout should remain short enough.
What's your opinion ?
As a summary, with current timeout logic, a big timeout is only hit when a real hang happen. Even when the kernel not support power-off, the power-off string will be detected by us and qemu will quit by pkill.
So, a not that big timeout for every architecture by default, but still allow the architecture to configure a bigger one?
QEMU_TIMEOUT_powerpc = 35 QEMU_TIMEOUT = $(or $(QEMU_TIMEOUT_$(XARCH),30)
I will retest them carefully, I'm still worried about that a too small timeout may kill qemu during test or even before running test, but it would run tests and power-off normally if we not kill them.
And even further, I'm thinking about the detecting of the boot hang as earier as we can, for example, these lines are good for us:
// first line to detect bios hang, may be 5 seconds? Linux version 6.4.0+ ...
// second line to detect kernel boot hang, may be 10 or 15 seconds? Run /init as init process
// third line to detect test hang, ... Leaving init with final status
// forth line to detect power-off reboot: Power down
So, even we configure a big timeout, but we can use a smaller default hang detect setting for bios hang, kernel hang and test hang, it will kill qemu as earier as we can, even hang happens, no need to wait for the timeout value we configured.
Best regards, Zhangjin
Willy
linux-kselftest-mirror@lists.linaro.org
-
Willy Tarreau -
Zhangjin Wu