Linux-kselftest Archive on lore.kernel.org
 help / color / Atom feed
From: Axel Rasmussen <axelrasmussen@google.com>
To: Peter Xu <peterx@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
	Andrea Arcangeli <aarcange@redhat.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Hugh Dickins <hughd@google.com>,
	Jerome Glisse <jglisse@redhat.com>, Joe Perches <joe@perches.com>,
	Lokesh Gidra <lokeshgidra@google.com>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Mike Rapoport <rppt@linux.vnet.ibm.com>, Shaohua Li <shli@fb.com>,
	Shuah Khan <shuah@kernel.org>,
	Stephen Rothwell <sfr@canb.auug.org.au>,
	Wang Qing <wangqing@vivo.com>,
	linux-api@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	LKML <linux-kernel@vger.kernel.org>,
	linux-kselftest@vger.kernel.org, Linux MM <linux-mm@kvack.org>,
	Brian Geffon <bgeffon@google.com>,
	"Dr . David Alan Gilbert" <dgilbert@redhat.com>,
	Mina Almasry <almasrymina@google.com>,
	Oliver Upton <oupton@google.com>
Subject: Re: [PATCH v2 7/9] userfaultfd/selftests: reinitialize test context in each test
Date: Thu, 15 Apr 2021 11:03:14 -0700
Message-ID: <CAJHvVcix2CO_+qQ7+gWMLYL9H5zyDNz_06UnThd84etMNr-SWQ@mail.gmail.com> (raw)
In-Reply-To: <20210413201535.GD4440@xz-x1>

On Tue, Apr 13, 2021 at 1:15 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Mon, Apr 12, 2021 at 10:17:19PM -0700, Axel Rasmussen wrote:
> > Currently, the context (fds, mmap-ed areas, etc.) are global. Each test
> > mutates this state in some way, in some cases really "clobbering it"
> > (e.g., the events test mremap-ing area_dst over the top of area_src, or
> > the minor faults tests overwriting the count_verify values in the test
> > areas). We run the tests in a particular order, each test is careful to
> > make the right assumptions about its starting state, etc.
> >
> > But, this is fragile. It's better for a test's success or failure to not
> > depend on what some other prior test case did to the global state.
> >
> > To that end, clear and reinitialize the test context at the start of
> > each test case, so whatever prior test cases did doesn't affect future
> > tests.
> >
> > This is particularly relevant to this series because the events test's
> > mremap of area_dst screws up assumptions the minor fault test was
> > relying on. This wasn't a problem for hugetlb, as we don't mremap in
> > that case.
> >
> > Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
> > ---
> >  tools/testing/selftests/vm/userfaultfd.c | 221 +++++++++++++----------
> >  1 file changed, 127 insertions(+), 94 deletions(-)
> >
> > diff --git a/tools/testing/selftests/vm/userfaultfd.c b/tools/testing/selftests/vm/userfaultfd.c
> > index 1f65c4ab7994..0ff01f437a39 100644
> > --- a/tools/testing/selftests/vm/userfaultfd.c
> > +++ b/tools/testing/selftests/vm/userfaultfd.c
> > @@ -89,7 +89,8 @@ static int shm_fd;
> >  static int huge_fd;
> >  static char *huge_fd_off0;
> >  static unsigned long long *count_verify;
> > -static int uffd, uffd_flags, finished, *pipefd;
> > +static int uffd = -1;
> > +static int uffd_flags, finished, *pipefd;
> >  static char *area_src, *area_src_alias, *area_dst, *area_dst_alias;
> >  static char *zeropage;
> >  pthread_attr_t attr;
> > @@ -342,6 +343,121 @@ static struct uffd_test_ops hugetlb_uffd_test_ops = {
> >
> >  static struct uffd_test_ops *uffd_test_ops;
> >
> > +static int userfaultfd_open(uint64_t *features)
> > +{
> > +     struct uffdio_api uffdio_api;
> > +
> > +     uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
>
> Keep UFFD_USER_MODE_ONLY?
>
> [...]
>
> > @@ -961,10 +1045,9 @@ static int userfaultfd_zeropage_test(void)
> >       printf("testing UFFDIO_ZEROPAGE: ");
> >       fflush(stdout);
> >
> > -     uffd_test_ops->release_pages(area_dst);
> > -
> > -     if (userfaultfd_open(0))
> > +     if (uffd_test_ctx_clear() || uffd_test_ctx_init(0))
> >               return 1;
>
> Would it look even nicer to init() at the entry of each test, and clear() after
> finish one test?

I slightly prefer clearing at the beginning, as it means we don't need
to depend on the previous test being correct for this test to
function. And, we don't need more complex error handling in the test
cases to make sure we don't mess things up for the next test.

But, two things we can do to clean this up as-is:

The initialization function can just call clear itself, so tests don't
need to worry about it.

And, with err(), we don't need these functions to return an int any more.

I'll send a version like that, we can see how it looks.

>
> > +
> >       uffdio_register.range.start = (unsigned long) area_dst;
> >       uffdio_register.range.len = nr_pages * page_size;
> >       uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
>
> The rest looks good to me.  Thanks,
>
> --
> Peter Xu
>

  reply index

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-13  5:17 [PATCH v2 0/9] userfaultfd: add minor fault handling for shmem Axel Rasmussen
2021-04-13  5:17 ` [PATCH v2 1/9] userfaultfd/hugetlbfs: avoid including userfaultfd_k.h in hugetlb.h Axel Rasmussen
2021-04-14  6:43   ` Hugh Dickins
2021-04-13  5:17 ` [PATCH v2 2/9] userfaultfd/shmem: combine shmem_{mcopy_atomic,mfill_zeropage}_pte Axel Rasmussen
2021-04-14  6:51   ` Hugh Dickins
2021-04-13  5:17 ` [PATCH v2 3/9] userfaultfd/shmem: support minor fault registration for shmem Axel Rasmussen
2021-04-13 20:43   ` Peter Xu
2021-04-14  7:36   ` Hugh Dickins
2021-04-14 18:51     ` Peter Xu
2021-04-13  5:17 ` [PATCH v2 4/9] userfaultfd/shmem: support UFFDIO_CONTINUE " Axel Rasmussen
2021-04-16 23:47   ` Hugh Dickins
2021-04-20 18:05     ` Axel Rasmussen
2021-04-13  5:17 ` [PATCH v2 5/9] userfaultfd/selftests: use memfd_create for shmem test type Axel Rasmussen
2021-04-13 20:16   ` Peter Xu
2021-04-13  5:17 ` [PATCH v2 6/9] userfaultfd/selftests: create alias mappings in the shmem test Axel Rasmussen
2021-04-13 20:17   ` Peter Xu
2021-04-13  5:17 ` [PATCH v2 7/9] userfaultfd/selftests: reinitialize test context in each test Axel Rasmussen
2021-04-13 20:15   ` Peter Xu
2021-04-15 18:03     ` Axel Rasmussen [this message]
2021-04-13  5:17 ` [PATCH v2 8/9] userfaultfd/selftests: exercise minor fault handling shmem support Axel Rasmussen
2021-04-13  5:17 ` [PATCH v2 9/9] userfaultfd/shmem: modify shmem_mcopy_atomic_pte to use install_ptes Axel Rasmussen
2021-04-17  0:34   ` Hugh Dickins
2021-04-20 18:43     ` Axel Rasmussen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAJHvVcix2CO_+qQ7+gWMLYL9H5zyDNz_06UnThd84etMNr-SWQ@mail.gmail.com \
    --to=axelrasmussen@google.com \
    --cc=aarcange@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=almasrymina@google.com \
    --cc=bgeffon@google.com \
    --cc=dgilbert@redhat.com \
    --cc=hughd@google.com \
    --cc=jglisse@redhat.com \
    --cc=joe@perches.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=lokeshgidra@google.com \
    --cc=mike.kravetz@oracle.com \
    --cc=oupton@google.com \
    --cc=peterx@redhat.com \
    --cc=rppt@linux.vnet.ibm.com \
    --cc=sfr@canb.auug.org.au \
    --cc=shli@fb.com \
    --cc=shuah@kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    --cc=wangqing@vivo.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-kselftest Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kselftest/0 linux-kselftest/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kselftest linux-kselftest/ https://lore.kernel.org/linux-kselftest \
		linux-kselftest@vger.kernel.org
	public-inbox-index linux-kselftest

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kselftest


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git