Anish Moorthy <amoorthy@xxxxxxxxxx> 于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.
