Re: [PATCH v3 01/22] KVM: selftests: Allow many vCPUs and reader threads per UFFD in demand paging test

From: Robert Hoo <robert.hoo.linux@gmail.com>
To: Anish Moorthy <amoorthy@google.com>
Cc: pbonzini@redhat.com, maz@kernel.org, oliver.upton@linux.dev,
	seanjc@google.com, jthoughton@google.com, bgardon@google.com,
	dmatlack@google.com, ricarkol@google.com,
	axelrasmussen@google.com, peterx@redhat.com, kvm@vger.kernel.org,
	kvmarm@lists.linux.dev
Subject: Re: [PATCH v3 01/22] KVM: selftests: Allow many vCPUs and reader threads per UFFD in demand paging test
Date: Fri, 21 Apr 2023 20:15:24 +0800	[thread overview]
Message-ID: <CA+wubQC0rqteeDdU04vhWahBiwTU5ty5V+pTUXGYu9xd4ppM0g@mail.gmail.com> (raw)
In-Reply-To: <CAF7b7mqMhQmQzJEhZJvEXGUzFB=jSXXOQUr22=Ef+oT-EDyEkg@mail.gmail.com>

Anish Moorthy <amoorthy@google.com> 于2023年4月21日周五 01:56写道：
>
> > > +             /*
> > > +              * With multiple vCPU threads fault on a single page and there are
> > > +              * multiple readers for the UFFD, at least one of the UFFDIO_COPYs
> > > +              * will fail with EEXIST: handle that case without signaling an
> > > +              * error.
> > > +              */
> >
> > But this code path is also gone through in other cases, isn't it? In
> > those cases, is it still safe to ignore EEXIST?
>
> Good point: the answer is no, it's not always safe to ignore EEXISTs
> here. For instance the first UFFDIO_CONTINUE for a page shouldn't be
> allowed to EEXIST, and that's swept under the rug here. I've added the
> following to the comment
>
> + * Note that this does sweep under the rug any EEXISTs occurring
> + * from, e.g., the first UFFDIO_COPY/CONTINUEs on a page. A
> + * realistic VMM would maintain some other state to correctly
> + * surface EEXISTs to userspace or prevent duplicate
> + * COPY/CONTINUEs from happening in the first place.
>
> I could add that extra state to the self test (via for instance, an
> atomic bitmap that threads "or" into before issuing any
> COPY/CONTINUEs) but it's a bit of an extra complication without any
> real payoff. Let me know if you think the comment's inadequate though.
>
IIUC, you could say: in this on demand paging test case, even
duplicate copy/continue doesn't do harm anyway. Am I right?

> > > +             /* See the note about EEXISTs in the UFFDIO_COPY branch. */
> >
> > Personally I would suggest copy the comments here. what if some day above
> > code/comment was changed/deleted?
>
> You might be right: on the other hand, if the comment ever gets
> updated then it would have to be done in two places. Anyone to break
> the tie? :)

The one who updates the place is responsible for the comments. make sense?:)
>
> > > @@ -148,18 +158,32 @@ struct uffd_desc *uffd_setup_demand_paging(int uffd_mode, useconds_t delay,
> > >       TEST_ASSERT((uffdio_register.ioctls & expected_ioctls) ==
> > >                   expected_ioctls, "missing userfaultfd ioctls");
> > >
> > > -     ret = pipe2(uffd_desc->pipefds, O_CLOEXEC | O_NONBLOCK);
> > > -     TEST_ASSERT(!ret, "Failed to set up pipefd");
> > > -
> > >       uffd_desc->uffd_mode = uffd_mode;
> > >       uffd_desc->uffd = uffd;
> > >       uffd_desc->delay = delay;
> > >       uffd_desc->handler = handler;
> >
> > Now that these info are encapsulated into reader args below, looks
> > unnecessary to have them in uffd_desc here.
>
> Good point. I've removed uffd_mode, delay, and handler from uffd_desc.
> I left the "uffd" field in because that's a shared resource, and
> close()ing it as "close(desc->uffd)" makes more sense than, say,
> "close(desc->reader_args[0].uffd)"

Sure, that's also what I originally changed on my side. sorry didn't
mention it earlier.