| Message ID | 4b4d792299ca5356f8f5af5fc9a27c687b0e4e38.1690489039.git.falcon@tinylab.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/nolibc: add minimal kernel config support - part1 | expand |
Hi, Willy two trivial updates required in this patch. [...] > > To tell users the test running progress in time, some critical running > status are also printed and detected. > [...] > @@ -229,16 +232,39 @@ kernel: $(KERNEL_CONFIG) > # 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) > > +TIMEOUT_CMD = t=$(QEMU_TIMEOUT); past=0; \ > + bios_timeout=$$(expr $$t - 7); kernel_timeout=$$(expr $$t - 5); init_timeout=$$(expr $$t - 3); test_timeout=$$(expr $$t - 1); \ > + err=""; bios=0; kernel=0; init=0; test=0; poweredoff=0; panic=0; \ This 'panic=0;' variable init should be removed, it is not required in the latest version: err=""; bios=0; kernel=0; init=0; test=0; poweredoff=0; \ > + echo "Running $(KERNEL_IMAGE) on qemu-system-$(QEMU_ARCH)"; \ > + while [ $$t -gt 0 ]; do \ > + sleep 2; t=$$(expr $$t - 2); past=$$(expr $$past + 2); \ > + if [ $$bios -eq 0 ] && grep -E "Linux version|Kernel command line|printk: console" "$(RUN_OUT)"; then bios=1; fi; \ > + if [ $$bios -eq 1 -a $$kernel -eq 0 ] && grep -E "Run .* as init process" "$(RUN_OUT)"; then kernel=1; fi; \ > + if [ $$kernel -eq 1 -a $$init -eq 0 ] && grep -E "Running test" "$(RUN_OUT)"; then init=1; fi; \ > + if [ $$init -eq 1 -a $$test -eq 0 ] && grep -E "Leaving init with final status|Exiting with status" "$(RUN_OUT)"; then test=1; fi; \ It is better to get the line of 'Total number of errors' instead of 'Exiting with status', the later never trigger in qemu-system run. if [ $$init -eq 1 -a $$test -eq 0 ] && grep -E "Leaving init with final status|Total number of errors" "$(RUN_OUT)"; then test=1; fi; \ > + if [ $$init -eq 1 ] && grep -E "Kernel panic - not syncing: Attempted to kill init" "$(RUN_OUT)"; then err="test"; sleep 1; break; fi; \ > + if [ $$test -eq 1 ] && grep -E "reboot: System halted|reboot: Power down" "$(RUN_OUT)"; then poweredoff=1; sleep 1; break; fi; \ > + if [ $$past -gt $$bios_timeout -a $$bios -eq 0 ]; then err="bios"; break; fi; \ > + if [ $$past -gt $$kernel_timeout -a $$kernel -eq 0 ]; then err="kernel"; break; fi; \ > + if [ $$past -gt $$init_timeout -a $$init -eq 0 ]; then err="init"; break; fi; \ > + if [ $$past -gt $$test_timeout -a $$test -eq 0 ]; then err="test"; break; fi; \ > + done; \ > + if [ -z "$$err" -a $$poweredoff -eq 0 -a $$panic -eq 0 ]; then err="qemu-system-$(QEMU_ARCH)"; fi; \ And here, we should remove the panic check here too, it is replaced with 'err="test"': if [ -z "$$err" -a $$poweredoff -eq 0 ]; then err="qemu-system-$(QEMU_ARCH)"; fi; \ Thanks, Zhangjin > + if [ -n "$$err" ]; then echo "$$err may timeout, test failed"; tail -10 $(RUN_OUT); else echo "powered off, test finish"; fi; \ > + pkill -15 qemu-system-$(QEMU_ARCH) || true > + > +TIMEOUT_QEMU_RUN = ($(QEMU_SYSTEM_RUN) > "$(RUN_OUT)" &); $(TIMEOUT_CMD) > + > # run the tests after building the kernel > PHONY += $(KERNEL_IMAGE) > $(KERNEL_IMAGE): kernel > run: $(KERNEL_IMAGE) > - $(Q)$(QEMU_SYSTEM_RUN) > "$(RUN_OUT)" > + $(Q)$(TIMEOUT_QEMU_RUN) > $(Q)$(REPORT) "$(RUN_OUT)" > > # re-run the tests from an existing kernel > rerun: > - $(Q)$(QEMU_SYSTEM_RUN) > "$(RUN_OUT)" > + $(Q)$(TIMEOUT_QEMU_RUN) > $(Q)$(REPORT) "$(RUN_OUT)" > > # report with existing test log > -- > 2.25.1
On 2023-07-28 04:30:31+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 can be configured for every architecture based on their > time cost requirement of bios boot + kernel boot + test + poweroff. > > By default, 10 seconds timeout is configured, this is enough for most of > the architectures, otherwise, customize one by architecture. > > To tell users the test running progress in time, some critical running > status are also printed and detected. > > Suggested-by: Willy Tarreau <w@1wt.eu> > Link: https://lore.kernel.org/lkml/20230722130248.GK17311@1wt.eu/ > Signed-off-by: Zhangjin Wu <falcon@tinylab.org> > --- > tools/testing/selftests/nolibc/Makefile | 30 +++++++++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile > index a214745e0f3e..9a57de3b283c 100644 > --- a/tools/testing/selftests/nolibc/Makefile > +++ b/tools/testing/selftests/nolibc/Makefile > @@ -105,6 +105,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 = $(or $(QEMU_TIMEOUT_$(XARCH)),10) > + > # OUTPUT is only set when run from the main makefile, otherwise > # it defaults to this nolibc directory. > OUTPUT ?= $(CURDIR)/ > @@ -229,16 +232,39 @@ kernel: $(KERNEL_CONFIG) > # 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) > > +TIMEOUT_CMD = t=$(QEMU_TIMEOUT); past=0; \ > + bios_timeout=$$(expr $$t - 7); kernel_timeout=$$(expr $$t - 5); init_timeout=$$(expr $$t - 3); test_timeout=$$(expr $$t - 1); \ > + err=""; bios=0; kernel=0; init=0; test=0; poweredoff=0; panic=0; \ > + echo "Running $(KERNEL_IMAGE) on qemu-system-$(QEMU_ARCH)"; \ > + while [ $$t -gt 0 ]; do \ > + sleep 2; t=$$(expr $$t - 2); past=$$(expr $$past + 2); \ > + if [ $$bios -eq 0 ] && grep -E "Linux version|Kernel command line|printk: console" "$(RUN_OUT)"; then bios=1; fi; \ > + if [ $$bios -eq 1 -a $$kernel -eq 0 ] && grep -E "Run .* as init process" "$(RUN_OUT)"; then kernel=1; fi; \ > + if [ $$kernel -eq 1 -a $$init -eq 0 ] && grep -E "Running test" "$(RUN_OUT)"; then init=1; fi; \ > + if [ $$init -eq 1 -a $$test -eq 0 ] && grep -E "Leaving init with final status|Exiting with status" "$(RUN_OUT)"; then test=1; fi; \ > + if [ $$init -eq 1 ] && grep -E "Kernel panic - not syncing: Attempted to kill init" "$(RUN_OUT)"; then err="test"; sleep 1; break; fi; \ > + if [ $$test -eq 1 ] && grep -E "reboot: System halted|reboot: Power down" "$(RUN_OUT)"; then poweredoff=1; sleep 1; break; fi; \ > + if [ $$past -gt $$bios_timeout -a $$bios -eq 0 ]; then err="bios"; break; fi; \ > + if [ $$past -gt $$kernel_timeout -a $$kernel -eq 0 ]; then err="kernel"; break; fi; \ > + if [ $$past -gt $$init_timeout -a $$init -eq 0 ]; then err="init"; break; fi; \ > + if [ $$past -gt $$test_timeout -a $$test -eq 0 ]; then err="test"; break; fi; \ > + done; \ > + if [ -z "$$err" -a $$poweredoff -eq 0 -a $$panic -eq 0 ]; then err="qemu-system-$(QEMU_ARCH)"; fi; \ > + if [ -n "$$err" ]; then echo "$$err may timeout, test failed"; tail -10 $(RUN_OUT); else echo "powered off, test finish"; fi; \ > + pkill -15 qemu-system-$(QEMU_ARCH) || true > + > +TIMEOUT_QEMU_RUN = ($(QEMU_SYSTEM_RUN) > "$(RUN_OUT)" &); $(TIMEOUT_CMD) > + This feels fairly hacky. Before we complicated nolibc-test to handle the no-procfs case to save a few seconds building the kernel and now we have fairly big timeouts. And a statemachine that relies on the specific strings emitted by the testsuite. I would like to get back to something more deterministic and obvious, even at the cost of some time spent compiling the test kernels. (saying this as somebody developing on a 2016 ultrabook) "Since the low-level poweroff support is heavily kernel & qemu dependent" The kernel we can control. How common are qemus with that are missing poweroff support? As this worked before I guess the only architecture where this could pose a problem would be ppc. An alternative I would like to put up for discussion: qemu could provide a watchdog device that is pinged by nolibc-test for each testcase. After nolibc-test is done and didn't poweroff properly the watchdog will reset the machine. ( -watchog-action poweroff ). The disadvantages are that we would need to add watchdog drivers to the kernels and figure out the correct watchdog devices and drivers for each arch. It seems virtio-watchdog is not yet usable. > # run the tests after building the kernel > PHONY += $(KERNEL_IMAGE) > $(KERNEL_IMAGE): kernel > run: $(KERNEL_IMAGE) > - $(Q)$(QEMU_SYSTEM_RUN) > "$(RUN_OUT)" > + $(Q)$(TIMEOUT_QEMU_RUN) > $(Q)$(REPORT) "$(RUN_OUT)" > > # re-run the tests from an existing kernel > rerun: > - $(Q)$(QEMU_SYSTEM_RUN) > "$(RUN_OUT)" > + $(Q)$(TIMEOUT_QEMU_RUN) > $(Q)$(REPORT) "$(RUN_OUT)" > > # report with existing test log > -- > 2.25.1 >
On Sat, Jul 29, 2023 at 09:59:55AM +0200, Thomas Weißschuh wrote: > On 2023-07-28 04:30:31+0800, Zhangjin Wu wrote: > > The kernel of some architectures can not poweroff qemu-system normally, > > especially for tinyconfig. (...) > This feels fairly hacky. and totally unmaintainable in the long term. It may even fail for some users having localization. > Before we complicated nolibc-test to handle the no-procfs case to save a > few seconds building the kernel and now we have fairly big timeouts. > And a statemachine that relies on the specific strings emitted by the > testsuite. > > I would like to get back to something more deterministic and obvious, > even at the cost of some time spent compiling the test kernels. > (saying this as somebody developing on a 2016 ultrabook) Agreed! > "Since the low-level poweroff support is heavily kernel & qemu dependent" > > The kernel we can control. > > How common are qemus with that are missing poweroff support? > As this worked before I guess the only architecture where this could > pose a problem would be ppc. > > > An alternative I would like to put up for discussion: > > qemu could provide a watchdog device that is pinged by nolibc-test for > each testcase. > After nolibc-test is done and didn't poweroff properly the watchdog will > reset the machine. ( -watchog-action poweroff ). > > The disadvantages are that we would need to add watchdog drivers to the > kernels and figure out the correct watchdog devices and drivers for each arch. It's an interesting idea, though at first glance it does not seem to have one for PPC. I think I have a much simpler idea: we don't care about PPC32. I mean OK it can be supported if it happens to work, we will just not include it in default runs, because it will require Ctrl-C to finish, and so what ? nolibc has been in the kernel for 5 years or so, nobody ever cared about PPC, why should we suddenly break or complicate everything just to support a sub-arch that nobody found interesting to add till now? > It seems virtio-watchdog is not yet usable. Then it might become an option for the future when it eventually works. Thanks, Willy
On Sat, Jul 29, 2023 at 08:05:00PM +0800, Zhangjin Wu wrote: (...) > The new added lines are just want to keep the console active to tell > users the test is running at background, even there is potential hang, > users will learn what the progress it is, but if we allow print the > running log to screen (may still have the issues mentioned by Willy in > another email thread), these lines can be simply removed to get a > cleaner version. > > The old version didn't add such logic, beside timeout logic, it simply > try to match the expected 'reboot: ' string, but we do need to match the > 'Kernel panic' string too to catch a crash nolibc-test case, so, a > simplified version looks like this: > > TIMEOUT_CMD = t=$(QEMU_TIMEOUT); finish=0; \ > echo "Running $(IMAGE_NAME) on qemu-system-$(QEMU_ARCH)"; \ > while [ $$t -gt 0 ]; do \ > sleep 1; t=$$(expr $$t - 1); \ > grep -E "reboot: System halted|reboot: Power down|Kernel panic" "$(RUN_OUT)"; then finish=1; sleep 1; break; fi; \ > done; \ > if [ $$finish -eq 1 ]; then echo "test finish"; else echo "test timeout"; fi; \ > pkill -15 qemu-system-$(QEMU_ARCH) || true > > Now, the lines are not too many, and the left expected strings should be > stable enough, do you like this one? Do you realize how unreadable and undebuggable this is for the casual reader who just tries to figure what command line to pass to qemu to run the test manually ? You seem to focus a lot on "if the test hangs" but nobody ever reported such a problem in the last few years. However we've been spending weeks discussing how to add support for extra features or architectures and such hacky stuff definitely steps in the way and complicates everything. > I do expect the timeout command has > a "-m" option like this: > > timeout --foreground 10 -m "reboot: |Kernel panic" qemu-system-$(QEMU_ARCH) ... I suggest that we give up on this timeout thing completely. You've shown that it significantly complicates everything and we yet have to find a single valid use case. Other subsystems also use QEMU without this. rcutorture does perform some process management but it's way more advanced and complete, and there's a reason: the tests are usually not supposed to end by themselves. > > > Before we complicated nolibc-test to handle the no-procfs case to save a > > > few seconds building the kernel and now we have fairly big timeouts. > > In reality, the answer is NO to "now we have fairly big timeouts". > > Firstly, If there is no hang or no poweroff failure, every run will quit > normally. Which is one good reason for not complexifying everything. > Secondly, even there is a poweroff failure or kernel panic, a 'reboot: ' > line will be always printed as expected and quit normally. This is not supposed to happen in automated tests, and archs that fail at this will simply be either excluded from automated tests, or will be run through whatever external timeout mechanism. > Thirdly, just only when there is a hang (not specific to tinyconfig), > such as wrong bios version or missing firmware or even kernel hang with > new changes (for example, the irqstack related riscv kernel hang I > reported), blabla. such hangs will be detected by the fixed timeout > value (like a watchdog). These are local tools issues, we can't fix them all and it's not our job. We're just providing an easy and hopefully convenient framework to test syscalls. We're not supposed to require users to have to go through complex debugging in this. And if they face a failure due to their local tools (like I had with my old qemu version), they'll just work around it by rebuilding it and that will be done. In the worst case, some archs might require a Ctrl-C once in a while during a manual test. No big deal. Definitely not as big as spending 10 minutes trying to figure how to find one's way through a complicated makefile, or wondering what's that runaway qemu process in the background that refuses to die, etc. > When poweroff fails on a target qemu, it still prints the 'reboot: ' > line as below, but will hang there after this line, our timeout logic > will detect this line and tell qemu quit as a normal poweroff. > > Total number of errors: 0 > Leaving init with final status: 0 > reboot: System halted /* This line which be printed when reboot() issues, not depends on qemu or kernel driver */ > ---> detecting reboot: System halted|reboot: Power down|Kernel panic - not syncing: Attempted to kill init|Rebooting ... > test finish /* Even qemu not poweroff successfully, we will kill it while the above line detected */ > LOG: Boot run successfully. > Running boot-finish > > The oldest method I used locally is the 'timeout' command itself only, > it is really a dead wait for a fixed timeout, the new method we used > here to match the expected string does help a lot to simulate a always > sucessful qemu poweroff support, no kernel and qemu dependent, > lightweight enough. I'm still definitely not fond of it because it turns something super simple into something complex and likely unreliable, for very little benefit. > Is the above explaination clearer? ;-) At least given the numerous approaches we've seen, now I'm convinced I don't want to see that anymore. Too much complication, a feeling of hackish and unreliable solution that will very likely cause lots of fixes in the future. > Based on the kernel versions from v2.6.10 - v6.x which I have used on > more than 7 architectures in my own Linux Lab project, the poweroff > support of qemu + kernel is never deterministic, some versions ok, some > versions fail, even with the defconfig, that's why an external timeout > is always required. Again, never ever experienced such a problem with the default configs for the supported archs, with one of my machines having a qemu version as old as 2.7. Paul always runs all the tests as well and never reported this problem either. Thus I would like that we stick to precise facts about problems that occur rather than just papering over them in a generic way. > > > "Since the low-level poweroff support is heavily kernel & qemu dependent" > > > > > > The kernel we can control. > > > > > > How common are qemus with that are missing poweroff support? > > > As this worked before I guess the only architecture where this could > > > pose a problem would be ppc. > > > > > Yes, as explained above, based on the experience I have on tons of > kernel versions of different architecture, it is really hard to make > poweroff work as expected all the time, as the kernel and qemu changes, > it may fail at any version randomly. Please could you provide us with a reproducer for this problem, with the mainline commit ID, arch, toolchain used, qemu version, because I think you're generalizing over a few cases that happened during your tinyconfig tests, for various possible reasons, but which are likely not good reasons for complicating everything. > Beside ppc, all of my local tinyconfig config files of every > architecture are ready for boot and print and also of course for the > 'reboot: ' line print. but it is 'hard' to find and enable the left > options to just further enable 'poweroff' support. If tinyconfig is not fixable, it's not usable, period. Right now all archs stop fine for many of us with defconfig. If only a few tinyconfig fail we'd rather invest time on these specific ones trying to figure what options you need to add to the extra_config. > Firstly, I have tried to enable some of them, but it deviates the > tinyconfig goal, for example, x86 requires to enable both ACPI and PCI > to just let poweroff work, so, I'm not planning to really enable them. > > Secondly, the time cost to just find and enable the poweroff options for > every architecture (and even for the new nolibc portings) is huge, I > give up after several tries, and they may fail in some future versions > randomly, I do think we may be not really interested in fixing up such > bugs in kernel drivers side ;-) OK so better just give up on tinyconfig if it's not suitable for the tests ? The more you present the shortcomings that come with them, the less appealing it looks now. > Thirdly, as Thomas mentioned before, the wireguard test use tinyconfig > too, just found it also gives up the poweroff support in its config > options and it use a simple raw timeout command, but the timeout is > really 'huge', 20m v.s. 10 seconds ;-) > > grep timeout tools/testing/selftests/wireguard/qemu/Makefile > timeout --foreground 20m qemu-system-$(QEMU_ARCH) \ > timeout --foreground 20m $< \ As I previously mentioned, I'm not fundamentally against a simple timeout command *iff* we can validate the exact problem and the conditions where it happens and we decide that it's the best workaround. And I suspect that even once we find it we'll prefer to just not run that specific setup by default and that's all. > > I think I have a much simpler idea: we don't care about PPC32. I mean > > OK it can be supported if it happens to work, we will just not include > > it in default runs, because it will require Ctrl-C to finish, and so > > what ? nolibc has been in the kernel for 5 years or so, nobody ever > > cared about PPC, why should we suddenly break or complicate everything > > just to support a sub-arch that nobody found interesting to add till > > now? > > > > Yes, this timeout logic is ok to be removed from this patchset, till we > get a better solution. Thanks. It would really help if your series could focus on *one thing* at a time. Currently I feel like in almost all patch series you've sent there are good stuff that could have been merged already, but which are mixed with hacks or unacceptable massive reworks that just result in the rest being kept on hold. I would really appreciate it if you thought about clearly presenting the problems you're trying to solve before sending patch series, so that we can collectively decide whether these problems deserve being fixed or can be ignored, and if the cost of addressing them outweigh their cost. It would save many hours of review of patches whose goal is not always very clear. A good rule of thumb is that something that is only added and that provides a specific feature generally suffers not much discussion (beyond the usual style/naming etc). But patches that modify existing process, code organization or affect reliability should be discussed and argumented before even being created. It's easier to discuss a purpose than to try to review a patch for a context that is not very clear. Thanks, Willy
Hi, Willy [...] > You seem to focus a lot on "if the test hangs" > but nobody ever reported such a problem in the last few years. However Willy, as Thomas shared in another reply too, there is really a poweroff failure (even with defconfig) in the default ppc/g3beige 32-bit PowerPC Qemu we are adding support for. Btw, It is possible to switch another 32-bit PowerPC board who support poweroff in qemu+kernel side, but need more investigate and survey, I have tried another two, no lucky, the ppce500 webpage [1] even claims there is 'Power-off functionality via one GPIO pin', but tried to enable the related gpios and reset options, still not work, perhaps I need help from more PowerPc users, will ask somebody to help me ;-) [1]: https://qemu.readthedocs.io/en/latest/system/ppc/ppce500.html > we've been spending weeks discussing how to add support for extra > features or architectures and such hacky stuff definitely steps in the > way and complicates everything. The background is, based on your feedback in another reply, printing the log to screen (another patch I have removed in this revision) may have some issues before. But if without any log and just hang after a poweroff failure and even further no timeout ... then, dead hang, that is really hard to debug, edit of Makefile and rerun and then we will know it fails at poweroff or just during qemu start ..., as you replied below, CTRL+C may help to cat the log file too, but it waste a CTRL+C + cat during the early development stage, especially when we require frequenty modify and test. [...] > > This is not supposed to happen in automated tests, and archs that fail > at this will simply be either excluded from automated tests, or will > be run through whatever external timeout mechanism. > Agree with external timeout mechanism or even excluded from automated tests, but we at least to allow to do basic test without editing the Makefile or without forever Ctrl-C + cat run.out manually ;-) So, what about at least restore part of my old 'print running log to screen' patch like this: # run the tests after building the kernel run: kernel $(Q)$(QEMU_SYSTEM_RUN) | tee "$(RUN_OUT)" $(Q)$(REPORT) "$(RUN_OUT)" # re-run the tests from an existing kernel rerun: $(Q)$(QEMU_SYSTEM_RUN) | tee "$(RUN_OUT)" $(Q)$(REPORT) "$(RUN_OUT)" Or even better, allow use a swtich to control it (the one we also removed from an old patch). ifneq ($(QUIET_RUN),1) LOG_OUT= | tee "$(RUN_OUT)" else LOG_OUT= > "$(RUN_OUT)" endif If missing this, the early stages of a new architecture porting is definitely very hard, not think about poweroff failure, just think about some other potential exceptions which simply stop the booting or testing (not continue and not exit). > > Thirdly, just only when there is a hang (not specific to tinyconfig), > > such as wrong bios version or missing firmware or even kernel hang with > > new changes (for example, the irqstack related riscv kernel hang I > > reported), blabla. such hangs will be detected by the fixed timeout > > value (like a watchdog). > > These are local tools issues, we can't fix them all and it's not our > job. We're just providing an easy and hopefully convenient framework > to test syscalls. We're not supposed to require users to have to go > through complex debugging in this. And if they face a failure due to > their local tools (like I had with my old qemu version), they'll just > work around it by rebuilding it and that will be done. In the worst > case, some archs might require a Ctrl-C once in a while during a > manual test. No big deal. Definitely not as big as spending 10 minutes > trying to figure how to find one's way through a complicated makefile, > or wondering what's that runaway qemu process in the background that > refuses to die, etc. Again Willy, without log and without timeout (with a tail of last 10 lines in the old version), just a 'forever' hang, Ctrl-C helps but not much, still require to check the log to see what happens or editing the Makefile to enable the logging temply, it is realy time-waste during development ;-) [...] > > Again, never ever experienced such a problem with the default configs > for the supported archs, with one of my machines having a qemu version > as old as 2.7. Paul always runs all the tests as well and never reported > this problem either. Thus I would like that we stick to precise facts > about problems that occur rather than just papering over them in a > generic way. > Willy, before, the old versions of this patchset did only configure QEMU_TIMEOUT for the failed 32-bit PowerPC and not enabled for the other boards, but later after a dicussion (perhaps I misunderstood one of your suggestions?), this revision configure a default one for all, I also thought about it is not good for all boards, but still a choice to a board really have no poweroff support (external one is ok if with explicitly log printing to screen). we have used something like this before: ifneq ($(QEMU_TIMEOUT),) ... endif > > > > "Since the low-level poweroff support is heavily kernel & qemu dependent" > > > > > > > > The kernel we can control. > > > > > > > > How common are qemus with that are missing poweroff support? > > > > As this worked before I guess the only architecture where this could > > > > pose a problem would be ppc. > > > > > > > > Yes, as explained above, based on the experience I have on tons of > > kernel versions of different architecture, it is really hard to make > > poweroff work as expected all the time, as the kernel and qemu changes, > > it may fail at any version randomly. > > Please could you provide us with a reproducer for this problem, with > the mainline commit ID, arch, toolchain used, qemu version, because I > think you're generalizing over a few cases that happened during your > tinyconfig tests, for various possible reasons, but which are likely > not good reasons for complicating everything. > It is very hard to list all, the following one is a section [1] (not updated for a long time) of the document of my 'Linux Lab' project: ### 6.2.2 Poweroff hang Both of the `poweroff` and `reboot` commands not work on these boards currently (LINUX=v5.1): * mipsel/malta (exclude LINUX=v2.6.36) * mipsel/ls232 * mipsel/ls1b * mips64el/ls2k * mips64el/ls3a7a * aarch64/raspi3 * arm/versatilepb System will directly hang there while running `poweroff` or `reboot`, to exit qemu, please pressing `CTRL+a x` or using `pkill qemu`. To test such boards automatically, please make sure setting `TEST_TIMEOUT`, e.g. `make test TEST_TIMEOUT=50`. Welcome to fix up them. IMHO, since my project is mainly for kernel learning and development, I even don't care about why it not poweroff successfully at last although I have tried my best to find the realted options but failed at last, the reasons may be complex, from kernel side or from qemu side, both possible, or even simply a kernel option changed and the config options are not updated timely. We have so many boards with kernel version from v2.6.10 to v6.4, many poweroff failures before: $ ls -1 -d */* aarch64/raspi3 aarch64/virt arm/ebf-imx6ull arm/mcimx6ul-evk arm/versatilepb arm/vexpress-a9 arm/virt i386/pc loongarch64/virt mips64el/loongson3-virt mips64el/ls2k mips64el/ls3a7a mipsel/ls1b mipsel/ls232 mipsel/malta ppc64le/powernv ppc64le/pseries ppc64/powernv ppc64/pseries ppc/g3beige ppc/ppce500 riscv32/virt riscv64/virt s390x/s390-ccw-virtio x86_64/pc The kernel we used for i386/pc: v2.6.10 v2.6.11.12 v2.6.12.6 v2.6.21.5 v2.6.24.7 v2.6.34.9 v2.6.35.14 v2.6.36 v3.10.108 v4.6.7 v5.1 v5.13 v5.2 v6.1.1 [1]: https://github.com/tinyclub/linux-lab/blob/master/README.md#622-poweroff-hang > > Beside ppc, all of my local tinyconfig config files of every > > architecture are ready for boot and print and also of course for the > > 'reboot: ' line print. but it is 'hard' to find and enable the left > > options to just further enable 'poweroff' support. > > If tinyconfig is not fixable, it's not usable, period. Right now all > archs stop fine for many of us with defconfig. If only a few tinyconfig > fail we'd rather invest time on these specific ones trying to figure > what options you need to add to the extra_config. > Yes, that's why I plan to send the left patches by architecture, it will give us more time to configure and confirm the missing poweroff options if really required, I will ask somebody else to work with me together. > > Firstly, I have tried to enable some of them, but it deviates the > > tinyconfig goal, for example, x86 requires to enable both ACPI and PCI > > to just let poweroff work, so, I'm not planning to really enable them. > > > > Secondly, the time cost to just find and enable the poweroff options for > > every architecture (and even for the new nolibc portings) is huge, I > > give up after several tries, and they may fail in some future versions > > randomly, I do think we may be not really interested in fixing up such > > bugs in kernel drivers side ;-) > > OK so better just give up on tinyconfig if it's not suitable for the > tests ? The more you present the shortcomings that come with them, the > less appealing it looks now. > The only shortcoming is some 'poweroff' failures currently, I do think the time-saved is huge, with tinyconfig, I even don't want to try the time-consumer defconfig anymore (although it is a requirement of many kernel features for the last release), especially for nolibc development ;-) I'm using tinyconfig+nolibc for most of the boards supported by my Linux Lab project, I'm really happy with it, I use similar timeout + expected string logic now, it works better than the old pure timeout logic, then, I don't care about which poweroff options the target board want, just ignore the requirement. It is a very good kernel learning and development experience, I have used a defconfig based config and also a small config customized for nolibc for them before, but with the new tinyconfig (extra boot/print options) suggested by Thomas, it is really a good and a whole new experience. To be honest, from my side, it is ok to add tinyconfig for nolibc or not, because I can use the test script from Linux Lab project to test new nolibc features, but I do think this will help a lot for both the kernel and nolibc developers, so, I'm happy to continue, even we are facing some real difficulties currently. [...] > > > > > > > Yes, this timeout logic is ok to be removed from this patchset, till we > > get a better solution. > But again as explained above, we do need something to cope with 'dead' hang without any output. > Thanks. > > It would really help if your series could focus on *one thing* at a > time. Currently I feel like in almost all patch series you've sent > there are good stuff that could have been merged already, but which > are mixed with hacks or unacceptable massive reworks that just result > in the rest being kept on hold. > Agree, thanks, I will try my best to split a huge stuff to smaller ones, I was a little hurried to want to upstream the non-rv32 related parts quickly, then, we can back to rv32 asap ;-) > I would really appreciate it if you thought about clearly presenting > the problems you're trying to solve before sending patch series, so > that we can collectively decide whether these problems deserve being > fixed or can be ignored, and if the cost of addressing them outweigh > their cost. It would save many hours of review of patches whose goal > is not always very clear. I like this suggestion, since you have mentioned before, based on your suggestions, like the syscall.h stuff, a disucss or a proposal is fired before a real patchset, although I think it worths a real RFC patchset, but let's delay it and I do need more work to solve the questions asked from you, this may be also related to some ideas mentioned by Arnd before, seems he have planed to generate some syscall templates from the kernel .tbls, let's discuss this in another thread, please ignore this one. > A good rule of thumb is that something that > is only added and that provides a specific feature generally suffers > not much discussion (beyond the usual style/naming etc). But patches > that modify existing process, code organization or affect reliability > should be discussed and argumented before even being created. It's > easier to discuss a purpose than to try to review a patch for a context > that is not very clear. That's reasonable, a good rule is 'subtraction', not 'addition', sometimes, the implementation of new idea does add more impulsion to modify anything, it require calmness to go back to the core issue we want to solve. Thanks a lot. Best regards, Zhangjin > > Thanks, > Willy
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index a214745e0f3e..9a57de3b283c 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -105,6 +105,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 = $(or $(QEMU_TIMEOUT_$(XARCH)),10) + # OUTPUT is only set when run from the main makefile, otherwise # it defaults to this nolibc directory. OUTPUT ?= $(CURDIR)/ @@ -229,16 +232,39 @@ kernel: $(KERNEL_CONFIG) # 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) +TIMEOUT_CMD = t=$(QEMU_TIMEOUT); past=0; \ + bios_timeout=$$(expr $$t - 7); kernel_timeout=$$(expr $$t - 5); init_timeout=$$(expr $$t - 3); test_timeout=$$(expr $$t - 1); \ + err=""; bios=0; kernel=0; init=0; test=0; poweredoff=0; panic=0; \ + echo "Running $(KERNEL_IMAGE) on qemu-system-$(QEMU_ARCH)"; \ + while [ $$t -gt 0 ]; do \ + sleep 2; t=$$(expr $$t - 2); past=$$(expr $$past + 2); \ + if [ $$bios -eq 0 ] && grep -E "Linux version|Kernel command line|printk: console" "$(RUN_OUT)"; then bios=1; fi; \ + if [ $$bios -eq 1 -a $$kernel -eq 0 ] && grep -E "Run .* as init process" "$(RUN_OUT)"; then kernel=1; fi; \ + if [ $$kernel -eq 1 -a $$init -eq 0 ] && grep -E "Running test" "$(RUN_OUT)"; then init=1; fi; \ + if [ $$init -eq 1 -a $$test -eq 0 ] && grep -E "Leaving init with final status|Exiting with status" "$(RUN_OUT)"; then test=1; fi; \ + if [ $$init -eq 1 ] && grep -E "Kernel panic - not syncing: Attempted to kill init" "$(RUN_OUT)"; then err="test"; sleep 1; break; fi; \ + if [ $$test -eq 1 ] && grep -E "reboot: System halted|reboot: Power down" "$(RUN_OUT)"; then poweredoff=1; sleep 1; break; fi; \ + if [ $$past -gt $$bios_timeout -a $$bios -eq 0 ]; then err="bios"; break; fi; \ + if [ $$past -gt $$kernel_timeout -a $$kernel -eq 0 ]; then err="kernel"; break; fi; \ + if [ $$past -gt $$init_timeout -a $$init -eq 0 ]; then err="init"; break; fi; \ + if [ $$past -gt $$test_timeout -a $$test -eq 0 ]; then err="test"; break; fi; \ + done; \ + if [ -z "$$err" -a $$poweredoff -eq 0 -a $$panic -eq 0 ]; then err="qemu-system-$(QEMU_ARCH)"; fi; \ + if [ -n "$$err" ]; then echo "$$err may timeout, test failed"; tail -10 $(RUN_OUT); else echo "powered off, test finish"; fi; \ + pkill -15 qemu-system-$(QEMU_ARCH) || true + +TIMEOUT_QEMU_RUN = ($(QEMU_SYSTEM_RUN) > "$(RUN_OUT)" &); $(TIMEOUT_CMD) + # run the tests after building the kernel PHONY += $(KERNEL_IMAGE) $(KERNEL_IMAGE): kernel run: $(KERNEL_IMAGE) - $(Q)$(QEMU_SYSTEM_RUN) > "$(RUN_OUT)" + $(Q)$(TIMEOUT_QEMU_RUN) $(Q)$(REPORT) "$(RUN_OUT)" # re-run the tests from an existing kernel rerun: - $(Q)$(QEMU_SYSTEM_RUN) > "$(RUN_OUT)" + $(Q)$(TIMEOUT_QEMU_RUN) $(Q)$(REPORT) "$(RUN_OUT)" # report with existing test log
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 can be configured for every architecture based on their time cost requirement of bios boot + kernel boot + test + poweroff. By default, 10 seconds timeout is configured, this is enough for most of the architectures, otherwise, customize one by architecture. To tell users the test running progress in time, some critical running status are also printed and detected. Suggested-by: Willy Tarreau <w@1wt.eu> Link: https://lore.kernel.org/lkml/20230722130248.GK17311@1wt.eu/ Signed-off-by: Zhangjin Wu <falcon@tinylab.org> --- tools/testing/selftests/nolibc/Makefile | 30 +++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)