Message-ID: <9ad51430-b7c0-47dc-80af-20c86539498d@t-8ch.de>
Date:   Thu, 29 Jun 2023 23:44:31 +0200
From:   Thomas Weißschuh <thomas@...ch.de>
To:     Zhangjin Wu <falcon@...ylab.org>
Cc:     arnd@...db.de, linux-kernel@...r.kernel.org,
        linux-kselftest@...r.kernel.org, w@....eu
Subject: Re: [PATCH v1 05/17] selftests/nolibc: stat_timestamps: remove
 procfs dependency

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 :-)

Powered by blists - more mailing lists