On 2023-06-23 02:45:59+0800, Zhangjin Wu wrote: > > some general comments for the whole series. > > > > On 2023-06-21 20:52:30+0800, Zhangjin Wu wrote: > > > Hi, Willy > > > > > > This patchset mainly allows speed up the nolibc test with a minimal > > > kernel config. > > > > > > As the nolibc supported architectures become more and more, the 'run' > > > test with DEFCONFIG may cost several hours, which is not friendly to > > > develop testing and even for release testing, so, smaller kernel configs > > > may be required, and firstly, we should let nolibc-test work with less > > > kernel config options, this patchset aims to this goal. > > > > > > This patchset mainly remove the dependency from procfs, tmpfs, net and > > > memfd_create, many failures have been fixed up. > > > > > > When CONFIG_TMPFS and CONFIG_SHMEM are disabled, kernel will provide a > > > ramfs based tmpfs (mm/shmem.c), it will be used as a choice to fix up > > > some failures and also allow skip less tests. > > > > Did you look into how much this duplicates from the kernels already > > existing "tinyconfig" and "kvm_guest.config" functionality? > > > > Very good question and suggestion, thanks. it is just between tinyconfig > and kvm_guest.config, the former is not enough, the later provides more. > tinyconfig may be a very good base for us. > > Just tested some architectures, based on tinyconfig, seems we only need > to enable very few options, for example, TTY, PRINTK, CONSOLE related > and target virtual board related options, but it requires more time to > just list the required options. > > The 'minimal' ones I have prepared were shrunk from the 'defconfig', now > we need to add options from 'tinyconfig', with allnoconfig, it should be > smaller and therefore faster ;-) > > Based on my local powerpc porting, I have prepared some changes like > this: > > # extra kernel configs by architecture > EXTCONFIG_powerpc = --enable SERIAL_PMACZILOG --enable CONFIG_SERIAL_PMACZILOG_CONSOLE > EXTCONFIG = --set-str CONFIG_INITRAMFS_SOURCE $(CURDIR)/initramfs $(EXTCONFIG_$(ARCH)) > > ... >also > menuconfig: > $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) menuconfig > > extconfig: > $(Q)$(srctree)/scripts/config --file $(srctree)/.config $(EXTCONFIG) > $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) olddefconfig > > kernel: initramfs extconfig > $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) > > > 'menuconfig' is added for development, for example, find why something not work > and add the missing options. > > 'extconfig' is added to enable additional options (before, based on > defconfig) to let nolibc-test happy (for powerpc, add missing console > options which has been added as modules in default config). > > Based on your suggestion, this may be a good new target: > > tinyconfig: > $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper tinyconfig prepare > > And this one, use 'allnoconfig' instead of 'olddefconfig': > > extconfig: > $(Q)$(srctree)/scripts/config --file $(srctree)/.config $(EXTCONFIG) > $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) KCONFIG_ALLCONFIG=$(srctree)/.config allnoconfig > > So, the new 'tinyconfig' may function as the smallest test environment, > for faster compile and as a boundary test of nolibc-test itself. > > But again, still need time to list the minimally required options, if they are > few, listing them in the EXTCONFIG_<ARCH> line may be acceptable, but if the > options are 'huge', standalone nolibc.config may be required, let's wait for > one or two days. FYI there are many more tests in tools/testing/selftests/ that need custom configs to run. Maybe we can reuse some of their configuration machinery. (And qemu machinery maybe) > > And it would be interesting how much impact the enablement of procfs, > > tmpfs, net and memfd_create has in constrast to the minimal > > configuration. > > For the test speed (mainly kernel compile) itself, when for one architecture on > a very good test host, the time cost increment is very little (see below), but it > does save some, especially when for lots of architectures ;-) > > Comparing the rv64 testing speed on a Ubuntu 20.04 over '4G Mem + 4 Cores of > i7-8550U CPU @ 1.80GHz' Virtual Machne, this time include: > > * nolibc-test sysroot install + build > * kernel config + build > * qemu boot with opensbi + u-boot + kernel v6.4-rcx > > Testing results: > > 'minimal': > > arch/board | result > ------------|------------ > riscv64/virt | 136 test(s) passed, 3 skipped, 0 failed. See all results in /labs/linux-lab/logging/nolibc/riscv64-virt-nolibc-test.log > > LOG: see all results for all boards in /labs/linux-lab/logging/nolibc/nolibc-test.log > > > real 1m57.395s > user 4m50.002s > sys 1m0.866s > > 'minimal' + procfs, net, shmem/tmpfs, devtmpfs/devmtmpfs_mount ...: > > arch/board | result > ------------|------------ > riscv64/virt | 138 test(s) passed, 1 skipped, 0 failed. See all results in /labs/linux-lab/logging/nolibc/riscv64-virt-nolibc-test.log > > LOG: see all results for all boards in /labs/linux-lab/logging/nolibc/nolibc-test.log > > > real 2m17.812s > user 6m4.695s > sys 1m7.061s > > It did save 20s (~17.1%) for us, not too much, but really faster. > > > It seems unfortunate to me to complicate the testsuite to handle such > > uncommon scenarios. > > Yeah, such a config is not common, but as explained above, beside the compile > speedup improvement, it is really a good boundary test environment for > nolibc-test itself to make sure it work (no failure, less skips) at an > extremely worst-case scene, although our changes looks many, but every one is > as simple as CLOC ;-) > > And that also means, nolibc is able to run with a very 'tiny' kernel > config and users could reuse our config fragments and add their own for > their embedded devices. It would mean that nolibc-test is able to run on *really* trimmed down systems, which seems of limited use. If the testsuite has more dependencies it would not stop nolibc itself to run on them. As for the CONFIG_NET dependency, which I would guess is one of the more expensive configs to enable: link_cross can be easily adapted to instead use /proc/self. chmod_net relies on /proc/$PID/net accepting chmod(). It is the only file in /proc/$PID/ that works that way. Maybe its a kernel bug anyways and we shouldn't rely on it anyways? I'm taking a look. > > > Besides, it also adds musl support, improves glibc support and fixes up > > > a kernel cmdline passing use case. > > > > > > This is based on the dev.2023.06.14a branch of linux-rcu [1], all of the > > > supported architectures are tested (with local minimal configs, [5] > > > pasted the one for i386) without failures: > > > > > > arch/board | result > > > ------------|------------ > > > arm/vexpress-a9 | 138 test(s) passed, 1 skipped, 0 failed. See all results in /labs/linux-lab/logging/nolibc/arm-vexpress-a9-nolibc-test.log > > > aarch64/virt | 138 test(s) passed, 1 skipped, 0 failed. See all results in /labs/linux-lab/logging/nolibc/aarch64-virt-nolibc-test.log > > > ppc/g3beige | not supported > > > i386/pc | 136 test(s) passed, 3 skipped, 0 failed. See all results in /labs/linux-lab/logging/nolibc/i386-pc-nolibc-test.log > > > x86_64/pc | 138 test(s) passed, 1 skipped, 0 failed. See all results in /labs/linux-lab/logging/nolibc/x86_64-pc-nolibc-test.log > > > mipsel/malta | 138 test(s) passed, 1 skipped, 0 failed. See all results in /labs/linux-lab/logging/nolibc/mipsel-malta-nolibc-test.log > > > loongarch64/virt | 138 test(s) passed, 1 skipped, 0 failed. See all results in /labs/linux-lab/logging/nolibc/loongarch64-virt-nolibc-test.log > > > riscv64/virt | 136 test(s) passed, 3 skipped, 0 failed. See all results in /labs/linux-lab/logging/nolibc/riscv64-virt-nolibc-test.log > > > riscv32/virt | no test log found > > > s390x/s390-ccw-virtio | 138 test(s) passed, 1 skipped, 0 failed. See all results in /labs/linux-lab/logging/nolibc/s390x-s390-ccw-virtio-nolibc-test.log > > > > > (snipped) > > > > It is able to build and run nolibc-test with musl libc now, but there > > > are some failures/skips due to the musl its own issues/requirements: > > > > > > $ sudo ./nolibc-test | grep -E 'FAIL|SKIP' > > > 8 sbrk = 1 ENOMEM [FAIL] > > > 9 brk = -1 ENOMEM [FAIL] > > > 46 limit_int_fast16_min = -2147483648 [FAIL] > > > 47 limit_int_fast16_max = 2147483647 [FAIL] > > > 49 limit_int_fast32_min = -2147483648 [FAIL] > > > 50 limit_int_fast32_max = 2147483647 [FAIL] > > > 0 -fstackprotector not supported [SKIPPED] > > > > > > musl disabled sbrk and brk for some conflicts with its malloc and the > > > fast version of int types are defined in 32bit, which differs from nolibc > > > and glibc. musl reserved the sbrk(0) to allow get current brk, we > > > added a test for this in the v4 __sysret() helper series [2]. > > > > We could add new macros > > > > #define UINT_MAX(t) (~(t)0) > > #define SINT_MAX(t) (((t)1 << (sizeof(t) * 8 - 2)) - (t)1 + ((t)1 << (sizeof(t) * 8 - 2))) > > > > to get whatever is appropriate for the respective type. > > > > They work perfectly, thanks: > > /* for fast int test cases in stdlib test, musl use 32bit fast int */ > #undef UINT_MAX > #define UINT_MAX(t) (~(t)0) > #undef SINT_MAX > #define SINT_MAX(t) (((t)1 << (sizeof(t) * 8 - 2)) - (t)1 + ((t)1 << (sizeof(t) * 8 - 2))) > #undef SINT_MIN > #define SINT_MIN(t) (-SINT_MAX(t) - 1) > > ... > > CASE_TEST(limit_int_fast16_min); EXPECT_EQ(1, INT_FAST16_MIN, (int_fast16_t) SINT_MIN(int_fast16_t)); break; > CASE_TEST(limit_int_fast16_max); EXPECT_EQ(1, INT_FAST16_MAX, (int_fast16_t) SINT_MAX(int_fast16_t)); break; > CASE_TEST(limit_uint_fast16_max); EXPECT_EQ(1, UINT_FAST16_MAX, (uint_fast16_t) UINT_MAX(uint_fast16_t)); break; > CASE_TEST(limit_int_fast32_min); EXPECT_EQ(1, INT_FAST32_MIN, (int_fast32_t) SINT_MIN(int_fast32_t)); break; > CASE_TEST(limit_int_fast32_max); EXPECT_EQ(1, INT_FAST32_MAX, (int_fast32_t) SINT_MAX(int_fast32_t)); break; > CASE_TEST(limit_uint_fast32_max); EXPECT_EQ(1, UINT_FAST32_MAX, (uint_fast32_t) UINT_MAX(uint_fast32_t)); break; > > To avoid overriding the existing macros, perhaps we should add something > like UINT_TYPE_MAX(t), SINT_TYPE_MAX(t) and SINT_TYPE_MIN(t) ? They should only be visible inside nolibc-test.c I think. But yes the UINT_MAX naming is bad. Also when going away from testing constants maybe we can get back some test strength by validating the sizeof() of the datatypes. <snip> > > > > > > * selftests/nolibc: vfprintf: silence memfd_create() warning > > > selftests/nolibc: vfprintf: skip if neither tmpfs nor hugetlbfs > > > selftests/nolibc: vfprintf: support tmpfs and hugetlbfs > > > > > > memfd_create from kernel >= v6.2 forcely warn on missing > > > MFD_NOEXEC_SEAL flag, the first one silence it with such flag, for > > > older kernels, use 0 flag as before. > > > > Given this is only a problem when nolibc-test is PID1 and printing to > > the system console, we could also just disable warnings on the system > > console through syscall() or /proc/sys/kernel/printk. > > Ok, I did think about disabling console for this call, but I was worried about > the requirement of root (euid0) to do so, limiting it under PID1 may solve the > root permission issue, but still need to find the right syscall to avoid the > dependency of /proc/sys/kernel/printk, otherwise, to avoid failure for !procfs, > the whole vfprintf will be skipped for such a warning, to be honest, it looks > not a good direction. This should work: syslog(__NR_syslog, 6 /* SYSLOG_ACTION_CONSOLE_OFF */); > > > > It would also avoid cluttering the tests for limited gain. > > > > Hmm, if consider the more code lines about disabling/enabling console and the > dependency of /proc/sys/kernel/printk, I do prefer current change. It should really only be the single line above. > But I'm also interested in how the other applications developers to treat this > warning, from the new kernel version side, we should use the latest non > executable flags for security, but to let applications work with old kernels, > we must support old flags, checking the kernel versions may be another choice. I know that systemd does it the same way as you proposed it, with non-negligible code overhead. But for nolibc-test I really don't see any security issue. > Perhaps it's time for us to add the 'uname()' for nolibc, but the > version comparing may be not that easy when we are in c context ;-) > > https://www.man7.org/linux/man-pages/man2/uname.2.html Please no :-) > So, the current method may be still a 'balanced' solution, it tries supported > flags from new kernel to old kernel to get a better and working memfd_create() > without the version checking, is this cleaner? > > int i; > /* kernel >= v6.2 require MFD_NOEXEC_SEAL (0x0008U), but older ones not support this flag */ It is not required, only desired. The functionality still works as expected. I don't think the "old" way can ever stop working as it would break userspace ABI. > unsigned int flags[2] = {0x0008U, 0}; > > for (i = 0; i < 2; i ++) { Loops like this should use ARRAY_SIZE() to calculate the termination condition. > /* try supported flags from new kernels to old kernels */ > fd = memfd_create("vfprintf", flags[i]); > if (fd != -1) > break; > } > > if (fd == -1) { > ... > } <snip> Thomas
