Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220516075538.1276644-1-surenb@google.com> <78c3a163-551b-ef53-4018-7b6ba0640757@linuxfoundation.org>
 <CAJuCfpEW3kif-QAVdFKyZikVRT5QrLGwJ5pzua+yuVq=A=bv8w@mail.gmail.com> <7f0fd407-18f5-2718-40b5-b16804163197@linuxfoundation.org>
In-Reply-To: <7f0fd407-18f5-2718-40b5-b16804163197@linuxfoundation.org>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Mon, 16 May 2022 16:51:43 -0700
Message-ID: <CAJuCfpFn7aRoTJHmnR7oXuXY+LkhMF2R+W7h9pL_3hm9ynUJSA@mail.gmail.com>
Subject: Re: [PATCH v2 1/1] selftests: vm: add process_mrelease tests
To:     Shuah Khan <skhan@linuxfoundation.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Michal Hocko <mhocko@suse.com>,
        David Rientjes <rientjes@google.com>,
        Matthew Wilcox <willy@infradead.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Roman Gushchin <guro@fb.com>, Minchan Kim <minchan@kernel.org>,
        "Kirill A. Shutemov" <kirill@shutemov.name>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Christian Brauner <brauner@kernel.org>,
        Christoph Hellwig <hch@infradead.org>,
        Oleg Nesterov <oleg@redhat.com>,
        David Hildenbrand <david@redhat.com>,
        Jann Horn <jannh@google.com>,
        Shakeel Butt <shakeelb@google.com>,
        Peter Xu <peterx@redhat.com>,
        John Hubbard <jhubbard@nvidia.com>, shuah@kernel.org,
        LKML <linux-kernel@vger.kernel.org>,
        linux-mm <linux-mm@kvack.org>, linux-kselftest@vger.kernel.org,
        kernel-team <kernel-team@android.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, May 16, 2022 at 4:28 PM Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 5/16/22 2:47 PM, Suren Baghdasaryan wrote:
> > On Mon, May 16, 2022 at 1:29 PM Shuah Khan <skhan@linuxfoundation.org> 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@google.com>
> >>> ---
> >>>    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 for the guidance! I'll try that approach.
Suren.

>
> thanks,
> -- Shuah
>
>
>
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>