Re: [PATCH v2 1/1] selftests: vm: add process_mrelease tests

[Shuah Khan]

On 5/16/22 2:47 PM, Suren Baghdasaryan wrote:
> On Mon, May 16, 2022 at 1:29 PM Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx> wrote:
> > On 5/16/22 1:55 AM, Suren Baghdasaryan wrote:
> > > Introduce process_mrelease syscall sanity tests which include tests
> > > which expect to fail:
> > > - process_mrelease with invalid pidfd and flags inputs
> > > - process_mrelease on a live process with no pending signals
> > > and valid process_mrelease usage which is expected to succeed.
> > > Because process_mrelease has to be used against a process with a pending
> > > SIGKILL, it's possible that the process exits before process_mrelease
> > > gets called. In such cases we retry the test with a victim that allocates
> > > twice more memory up to 1GB. This would require the victim process to
> > > spend more time during exit and process_mrelease has a better chance of
> > > catching the process before it exits and succeeding.
> > >
> > > On success the test reports the amount of memory the child had to
> > > allocate for reaping to succeed. Sample output:
> > >       Success reaping a child with 1MB of memory allocations
> > >
> > > On failure the test reports the failure. Sample outputs:
> > >       All process_mrelease attempts failed!
> > >       process_mrelease: Invalid argument
> >
> > Nit: Please format this better - include actual example output from the
> > command and how to run the test examples.
>
> Hmm... Those are the actual outputs from the command and it does not
> take any input arguments. Do you mean smth like this:
>
> $ mrelease_test
> Success reaping a child with 1MB of memory allocations
>
> $ mrelease_test
> All process_mrelease attempts failed!
>
> $ mrelease_test
> process_mrelease: Invalid argument
>
> ?

This looks good.

> > > Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > > ---
> > >    tools/testing/selftests/vm/.gitignore      |   1 +
> > >    tools/testing/selftests/vm/Makefile        |   1 +
> > >    tools/testing/selftests/vm/mrelease_test.c | 214 +++++++++++++++++++++
> > >    tools/testing/selftests/vm/run_vmtests.sh  |  16 ++
> > >    4 files changed, 232 insertions(+)
> > >    create mode 100644 tools/testing/selftests/vm/mrelease_test.c

[snip]

> > Okay these above 3 routines are called once. I am not seeing any point
> > in making these separate routines. I made the same comment on v1.
>
> I must have misunderstood your previous comment. Will change.

Thank you.

> >

> > Now the above code can be a separate function which will make it readable.
>
> Ack.
>
> >

> > > +
> >
> > Why do you need these ifdefs - syscall will return ENOSYS and you can
> > key off that. Please take a look at other usages of syscall in the
> > repo.
>
> The issue is that I need to provide the syscall number when calling
> syscall() (in my case __NR_pidfd_open and __NR_process_mrelease) and
> if that number is not defined in the userspace headers on a given
> system then what should I pass instead?
> When implementing this I followed the examples of
> https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/vm/memfd_secret.c#L30
> and https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/vm/userfaultfd.c#L65.
> My original implementation was modeled after this approach:
> https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/vm/mlock2.h#L15.
> If none of these are correct, could you please point me to the example
> you want me to follow?

kselftests include kernel headers. As long as these syscalls are
defined in the kernel headers, the test will build.

Looks it is defined in include/uapi/asm-generic/unistd.h

You can assume it is defined and then if we find architectures that
don't, you can follow what tools/testing/selftests/pidfd/pidfd.h
does.

This way the test can simply call syscall and handle ENOSYS.

thanks,
-- Shuah