| Message ID | 5f80f3dae60f77c6b746578113e56c8fa6454143.1687344643.git.falcon@tinylab.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/nolibc: allow run with minimal kernel config | expand |
Hi, Thomas
I'm preparing a revision for this series, in the past days, when I was
working on testing our new 'minimal' kernel config support for all of
the architectures, the time cost (and wait) is really appreciable and the
repeated develop and test is really a big pain, I can also image when you
was working on stack-protector and Willy was working on lots of old
features ;-)
As you explained before, I knew the idea of using '/proc/self' here is
important to not using a fixed-time file, besides our proposed method (make
sure it at least not fail, just skip for !procfs):
- CASE_TEST(stat_timestamps); EXPECT_SYSZR(1, test_stat_timestamps()); break;
+ CASE_TEST(stat_timestamps); EXPECT_SYSZR(proc, test_stat_timestamps()); break;
To further avoid skip it for !procfs (I don't mean relaly disable it for the
default tinyconfig support, which need more discuss, at least provide the
possibility to pass without procfs), do you like this change? it doesn't depend
on 'proc' now.
- if (stat("/proc/self/", &st))
+ if (stat("/proc/self/", &st) && stat("/init", &st) && stat("/", &st))
The "/init" is compiled for 'run' target every time, so, the time stamp should
be dynamic enough, for libc-test, the /proc/self should be always there (if
still not enough, we can reuse the init file list here), the "/" here is only
for the worst-case scene ;-)
Thanks,
Zhangjin
> Since it is not really necessary to use /proc/self here, instead of
> adding a condition check, we use the always existing '/' path instead of
> /proc/self, this eventually let it work without procfs.
>
> Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> ---
> tools/testing/selftests/nolibc/nolibc-test.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> index ebec948ec808..2ef44176f7a9 100644
> --- a/tools/testing/selftests/nolibc/nolibc-test.c
> +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> @@ -520,7 +520,7 @@ static int test_stat_timestamps(void)
> if (sizeof(st.st_atim.tv_sec) != sizeof(st.st_atime))
> return 1;
>
> - if (stat("/proc/self/", &st))
> + if (stat("/", &st))
> return 1;
>
> if (st.st_atim.tv_sec != st.st_atime || st.st_atim.tv_nsec > 1000000000)
> --
> 2.25.1
Hi Zhangjin, On 2023-06-28 21:59:22+0800, Zhangjin Wu wrote: > I'm preparing a revision for this series, in the past days, when I was > working on testing our new 'minimal' kernel config support for all of > the architectures, the time cost (and wait) is really appreciable and the > repeated develop and test is really a big pain, I can also image when you > was working on stack-protector and Willy was working on lots of old > features ;-) To be honest I almost never built a kernel. Most of the time I tested my stuff with qemu-user. This made the dev-cycle really fast, especially with a binfmt setup that launches foreign binaries automatically with qemu-user. > As you explained before, I knew the idea of using '/proc/self' here is > important to not using a fixed-time file, besides our proposed method (make > sure it at least not fail, just skip for !procfs): > > - CASE_TEST(stat_timestamps); EXPECT_SYSZR(1, test_stat_timestamps()); break; > + CASE_TEST(stat_timestamps); EXPECT_SYSZR(proc, test_stat_timestamps()); break; > > To further avoid skip it for !procfs (I don't mean relaly disable it for the > default tinyconfig support, which need more discuss, at least provide the > possibility to pass without procfs), do you like this change? it doesn't depend > on 'proc' now. > > - if (stat("/proc/self/", &st)) > + if (stat("/proc/self/", &st) && stat("/init", &st) && stat("/", &st)) > > The "/init" is compiled for 'run' target every time, so, the time stamp should > be dynamic enough, for libc-test, the /proc/self should be always there (if > still not enough, we can reuse the init file list here), the "/" here is only > for the worst-case scene ;-) Both aproaches seem fine. Just skipping on !proc seems good enough. As for enabling proc in the test configs I just tested a plain tinyconfig vs one with CONFIG_PROC_FS enabled: tinyconfig: 375.06user 53.21system 2:05.80elapsed tinyconfig + CONFIG_PROC_FS: 397.77user 56.84system 2:09.24elapsed The overhead seems acceptable. Note as for disabling memfd: It seems currently MEMFD_CREATE is hardwired to only be enabled when either TMPFS or HUGETLBFS is enabled. But the memfd code and syscalls seem to work perfectly fine with those options disabled. I'll send a patch to fix up the Kconfigs to enable that usecase. > Thanks, > Zhangjin > > > Since it is not really necessary to use /proc/self here, instead of > > adding a condition check, we use the always existing '/' path instead of > > /proc/self, this eventually let it work without procfs. > > > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org> > > --- > > tools/testing/selftests/nolibc/nolibc-test.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c > > index ebec948ec808..2ef44176f7a9 100644 > > --- a/tools/testing/selftests/nolibc/nolibc-test.c > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c > > @@ -520,7 +520,7 @@ static int test_stat_timestamps(void) > > if (sizeof(st.st_atim.tv_sec) != sizeof(st.st_atime)) > > return 1; > > > > - if (stat("/proc/self/", &st)) > > + if (stat("/", &st)) > > return 1; > > > > if (st.st_atim.tv_sec != st.st_atime || st.st_atim.tv_nsec > 1000000000) > > -- > > 2.25.1
> Hi Zhangjin, > > On 2023-06-28 21:59:22+0800, Zhangjin Wu wrote: > > I'm preparing a revision for this series, in the past days, when I was > > working on testing our new 'minimal' kernel config support for all of > > the architectures, the time cost (and wait) is really appreciable and the > > repeated develop and test is really a big pain, I can also image when you > > was working on stack-protector and Willy was working on lots of old > > features ;-) > > To be honest I almost never built a kernel. > Most of the time I tested my stuff with qemu-user. > This made the dev-cycle really fast, especially with a binfmt setup that > launches foreign binaries automatically with qemu-user. > Yeah, qemu-user-static + binfmt_misc work perfectly, but my host kernel is not that new, so, I'm still a little worried about that there may be some hidden issues. > > As you explained before, I knew the idea of using '/proc/self' here is > > important to not using a fixed-time file, besides our proposed method (make > > sure it at least not fail, just skip for !procfs): > > > > - CASE_TEST(stat_timestamps); EXPECT_SYSZR(1, test_stat_timestamps()); break; > > + CASE_TEST(stat_timestamps); EXPECT_SYSZR(proc, test_stat_timestamps()); break; > > > > To further avoid skip it for !procfs (I don't mean relaly disable it for the > > default tinyconfig support, which need more discuss, at least provide the > > possibility to pass without procfs), do you like this change? it doesn't depend > > on 'proc' now. > > > > - if (stat("/proc/self/", &st)) > > + if (stat("/proc/self/", &st) && stat("/init", &st) && stat("/", &st)) > > > > The "/init" is compiled for 'run' target every time, so, the time stamp should > > be dynamic enough, for libc-test, the /proc/self should be always there (if > > still not enough, we can reuse the init file list here), the "/" here is only > > for the worst-case scene ;-) > > Both aproaches seem fine. Just skipping on !proc seems good enough. > To get less skips, let's use the second method, just updated my local patches ;-) > As for enabling proc in the test configs I just tested a plain > tinyconfig vs one with CONFIG_PROC_FS enabled: > > tinyconfig: 375.06user 53.21system 2:05.80elapsed > tinyconfig + CONFIG_PROC_FS: 397.77user 56.84system 2:09.24elapsed > > The overhead seems acceptable. > Yeah, only one option is ok, but "multiple options x multiple architectures x multiple repeated runs", that is 'huge' ;-) > > Note as for disabling memfd: > > It seems currently MEMFD_CREATE is hardwired to only be enabled when > either TMPFS or HUGETLBFS is enabled. > > But the memfd code and syscalls seem to work perfectly fine with those > options disabled. I'll send a patch to fix up the Kconfigs to enable > that usecase. Good catch! but for the vfprintf test cases, It is able to open a file from tmpfs directly. If no tmpfs, use the default ramfs (initramfs uses) instead, this will also avoid the new flags trying (to silence the warning). static int expect_vfprintf(int llen, size_t c, const char *expected, const char *fmt, ...) { + static const char *tmpfile = "/tmp/nolibc-vfprintf"; + struct stat stat_buf; int ret, fd, w, r; char buf[100]; FILE *memfile; va_list args; - fd = memfd_create("vfprintf", 0); + if (stat("/tmp/.", &stat_buf)) { + pad_spc(llen, 64, "[SKIPPED]\n"); + return 0; + } + + fd = open(tmpfile, O_CREAT | O_TRUNC | O_RDWR, 0755); ... + unlink(tmpfile); ... tmpfs is mounted (in another patch) like procfs in prepare() for pid==1. I plan to use this method in the revision, do you like this? memfd_create() was designed to do this work, but in current stage, opening tmpfile ourselves may be better. Thanks, Zhangjin
On 2023-06-30 05:23:35+0800, Zhangjin Wu wrote: > > Hi Zhangjin, > > > > On 2023-06-28 21:59:22+0800, Zhangjin Wu wrote: > > > I'm preparing a revision for this series, in the past days, when I was > > > working on testing our new 'minimal' kernel config support for all of > > > the architectures, the time cost (and wait) is really appreciable and the > > > repeated develop and test is really a big pain, I can also image when you > > > was working on stack-protector and Willy was working on lots of old > > > features ;-) > > > > To be honest I almost never built a kernel. > > Most of the time I tested my stuff with qemu-user. > > This made the dev-cycle really fast, especially with a binfmt setup that > > launches foreign binaries automatically with qemu-user. > > > > Yeah, qemu-user-static + binfmt_misc work perfectly, but my host kernel > is not that new, so, I'm still a little worried about that there may be > some hidden issues. qemu-user shouldn't have specific requirements for the host kernel. Could you elaborate? > > > As you explained before, I knew the idea of using '/proc/self' here is > > > important to not using a fixed-time file, besides our proposed method (make > > > sure it at least not fail, just skip for !procfs): > > > > > > - CASE_TEST(stat_timestamps); EXPECT_SYSZR(1, test_stat_timestamps()); break; > > > + CASE_TEST(stat_timestamps); EXPECT_SYSZR(proc, test_stat_timestamps()); break; > > > > > > To further avoid skip it for !procfs (I don't mean relaly disable it for the > > > default tinyconfig support, which need more discuss, at least provide the > > > possibility to pass without procfs), do you like this change? it doesn't depend > > > on 'proc' now. > > > > > > - if (stat("/proc/self/", &st)) > > > + if (stat("/proc/self/", &st) && stat("/init", &st) && stat("/", &st)) > > > > > > The "/init" is compiled for 'run' target every time, so, the time stamp should > > > be dynamic enough, for libc-test, the /proc/self should be always there (if > > > still not enough, we can reuse the init file list here), the "/" here is only > > > for the worst-case scene ;-) > > > > Both aproaches seem fine. Just skipping on !proc seems good enough. > > > > To get less skips, let's use the second method, just updated my local > patches ;-) > > > As for enabling proc in the test configs I just tested a plain > > tinyconfig vs one with CONFIG_PROC_FS enabled: > > > > tinyconfig: 375.06user 53.21system 2:05.80elapsed > > tinyconfig + CONFIG_PROC_FS: 397.77user 56.84system 2:09.24elapsed > > > > The overhead seems acceptable. > > > > Yeah, only one option is ok, but "multiple options x multiple > architectures x multiple repeated runs", that is 'huge' ;-) In your other patchset you mentioned a few options that were needed. But if we can drop CONFIG_NET completely, reduce CONFIG_TMPFS to CONFIG_MEMFD I would like to reevaluate the overall impact. > > > > Note as for disabling memfd: > > > > It seems currently MEMFD_CREATE is hardwired to only be enabled when > > either TMPFS or HUGETLBFS is enabled. > > > > But the memfd code and syscalls seem to work perfectly fine with those > > options disabled. I'll send a patch to fix up the Kconfigs to enable > > that usecase. > > Good catch! > > but for the vfprintf test cases, It is able to open a file from tmpfs > directly. If no tmpfs, use the default ramfs (initramfs uses) instead, > this will also avoid the new flags trying (to silence the warning). > > static int expect_vfprintf(int llen, size_t c, const char *expected, const char *fmt, ...) > { > + static const char *tmpfile = "/tmp/nolibc-vfprintf"; > + struct stat stat_buf; This should not be static. > int ret, fd, w, r; > char buf[100]; > FILE *memfile; > va_list args; > > - fd = memfd_create("vfprintf", 0); > + if (stat("/tmp/.", &stat_buf)) { > + pad_spc(llen, 64, "[SKIPPED]\n"); > + return 0; > + } Instead of checking with stat() here it would be nicer to check the result of open() below. > + > + fd = open(tmpfile, O_CREAT | O_TRUNC | O_RDWR, 0755); Seems like a good usecase for open("/tmp", O_TMPFILE | O_EXCL | ...) > ... > + unlink(tmpfile); ... and drop this. fd = memfd_open(); + if (fd == -1) + fd = open("/tmp", O_TMPFILE | O_EXCL | O_CREAT | O_RDWR, 0600); + if (fd == -1) + skip() May be enough. > ... > > tmpfs is mounted (in another patch) like procfs in prepare() for pid==1. > > I plan to use this method in the revision, do you like this? > > memfd_create() was designed to do this work, but in current stage, > opening tmpfile ourselves may be better. I'm not really a fan of having so much fallback code for features that should always be available. But if we can keep it straightforward it's probably fine. Or Willy has another opinion :-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index ebec948ec808..2ef44176f7a9 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -520,7 +520,7 @@ static int test_stat_timestamps(void) if (sizeof(st.st_atim.tv_sec) != sizeof(st.st_atime)) return 1; - if (stat("/proc/self/", &st)) + if (stat("/", &st)) return 1; if (st.st_atim.tv_sec != st.st_atime || st.st_atim.tv_nsec > 1000000000)
Since it is not really necessary to use /proc/self here, instead of adding a condition check, we use the always existing '/' path instead of /proc/self, this eventually let it work without procfs. Signed-off-by: Zhangjin Wu <falcon@tinylab.org> --- tools/testing/selftests/nolibc/nolibc-test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)