[CRIU] [RFC PATCH v2 02/23] lazy-pages: track outstanding page faults

[Mike Rapoport]

On Wed, Feb 08, 2017 at 12:15:01PM +0300, Pavel Emelyanov wrote:
> Applied, but there's a question inside.
> 
> On 02/06/2017 02:43 PM, Mike Rapoport wrote:
> > Multithreaded applications may concurrently generate page faults at the
> > same address, which will cause creation of multiple requests for remote
> > page, and, consequently, confuse the page server on the dump side.
> > We can keep track on page fault requests in flight and ensure this way that
> > we request a page from the remote side only once.
> > 
> > Signed-off-by: Mike Rapoport <rppt at linux.vnet.ibm.com>
> > ---
> >  criu/uffd.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/criu/uffd.c b/criu/uffd.c
> > index 557f112..050637c 100644
> > --- a/criu/uffd.c
> > +++ b/criu/uffd.c
> > @@ -537,9 +544,19 @@ static int uffd_copy(struct lazy_pages_info *lpi, __u64 address, int nr_pages)
> >  
> >  static int complete_page_fault(struct lazy_pages_info *lpi, unsigned long vaddr, int nr)
> >  {
> > +	struct lp_req *req;
> > +
> >  	if (uffd_copy(lpi, vaddr, nr))
> >  		return -1;
> >  
> > +	list_for_each_entry(req, &lpi->reqs, l) {
> > +		if (req->addr == vaddr) {
> > +			list_del(&req->l);
> > +			xfree(req);
> > +			break;
> 
> Shouldn't we better keep this forever and ... (below)

We'll probably keep them a bit longer for the fork() + remote lazy-pages
case. I cannot see another reason.
 
> > +		}
> > +	}
> > +
> >  	return update_lazy_iovecs(lpi, vaddr, nr * PAGE_SIZE);
> >  }
> >  
> > @@ -635,6 +652,7 @@ static int handle_remaining_pages(struct lazy_pages_info *lpi)
> >  
> >  static int handle_page_fault(struct lazy_pages_info *lpi, struct uffd_msg *msg)
> >  {
> > +	struct lp_req *req;
> >  	__u64 address;
> >  	int ret;
> >  
> > @@ -642,6 +660,16 @@ static int handle_page_fault(struct lazy_pages_info *lpi, struct uffd_msg *msg)
> >  	address = msg->arg.pagefault.address & ~(page_size() - 1);
> >  	pr_debug("%d: #PF at 0x%llx\n", lpi->pid, address);
> >  
> > +	list_for_each_entry(req, &lpi->reqs, l)
> > +		if (req->addr == address)
> > +			return 0;
> 
> ... punch a hole in lpi->iovs so that we have iovs containing a list
> of not yet transfered memory and reqs -- already transfered?

And why would we want it here? We punch a hole in lpi->iovs after the page
is copied into the process address space.
What we can do is just drop 'struct lp_req' and use list of "pending"
lazy_iov's. Although, I'm still not 100% sure I won't extend lp_req with
some more fields, again for the fork() + remote lazy-pages.

> > +
> > +	req = xzalloc(sizeof(*req));
> > +	if (!req)
> > +		return -1;
> > +	req->addr = address;
> > +	list_add(&req->l, &lpi->reqs);
> > +
> >  	ret = uffd_handle_pages(lpi, address, 1, PR_ASYNC | PR_ASAP);
> >  	if (ret < 0) {
> >  		pr_err("Error during regular page copy\n");
> > @@ -664,6 +692,9 @@ static int handle_uffd_event(struct epoll_rfd *lpfd)
> >  		return 1;
> >  
> >  	if (ret != sizeof(msg)) {
> > +		/* we've already handled the page fault for another thread */
> > +		if (errno == EAGAIN)
> > +			return 0;
> >  		if (ret < 0)
> >  			pr_perror("Can't read userfaultfd message");
> >  		else
> > 
>