[CRIU] [PATCH 3/7] page-read: Drop get_remote_pages

[Mike Rapoport]

On Wed, Nov 16, 2016 at 1:40 PM, Pavel Emelyanov <xemul at virtuozzo.com> wrote:
> On 11/16/2016 01:42 PM, Mike Rapoport wrote:
>> On Wed, Nov 16, 2016 at 11:39 AM, Pavel Emelyanov <xemul at virtuozzo.com> wrote:
>>> We already have routines that do send-req, recv-info
>>> and recv-page, so no need in yet another one.
>>>
>>> Signed-off-by: Pavel Emelyanov <xemul at virtuozzo.com>
>>> ---
>>>  criu/include/page-xfer.h |  3 ---
>>>  criu/page-xfer.c         | 34 ++++------------------------------
>>>  criu/pagemap.c           | 21 ++++++++++++---------
>>>  3 files changed, 16 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/criu/include/page-xfer.h b/criu/include/page-xfer.h
>>> index 7cd0055..5b7ec21 100644
>>> --- a/criu/include/page-xfer.h
>>> +++ b/criu/include/page-xfer.h
>>> @@ -52,9 +52,6 @@ extern int check_parent_page_xfer(int fd_type, long id);
>>>   * - dump-side page server sends the raw page data
>>>   */
>>>
>>> -/* sync receive of remote pages */
>>> -extern int get_remote_pages(int pid, unsigned long addr, int nr_pages, void *dest);
>>> -
>>>  /* async request/receive of remote pages */
>>>  extern int request_remote_pages(int pid, unsigned long addr, int nr_pages);
>>>  extern int receive_remote_pages_info(int *nr_pages, unsigned long *addr, int *pid);
>>> diff --git a/criu/page-xfer.c b/criu/page-xfer.c
>>> index 7f1d0f7..2d878e2 100644
>>> --- a/criu/page-xfer.c
>>> +++ b/criu/page-xfer.c
>>> @@ -887,36 +887,6 @@ out:
>>>         return ret ? : status;
>>>  }
>>>
>>> -int get_remote_pages(int pid, unsigned long addr, int nr_pages, void *dest)
>>> -{
>>> -       int ret;
>>> -       int len = PAGE_SIZE * nr_pages;
>>> -
>>> -       struct page_server_iov pi;
>>> -
>>> -       if (send_psi(page_server_sk, PS_IOV_GET, nr_pages, addr, pid))
>>> -               return -1;
>>> -
>>> -       tcp_nodelay(page_server_sk, true);
>>> -
>>> -       ret = recv(page_server_sk, &pi, sizeof(pi), MSG_WAITALL);
>>> -       if (ret != sizeof(pi))
>>> -               return -1;
>>> -
>>> -       /* zero page */
>>> -       if (pi.cmd == PS_IOV_ZERO)
>>> -               return 0;
>>> -
>>> -       if (pi.nr_pages > nr_pages)
>>> -               return -1;
>>> -
>>> -       ret = recv(page_server_sk, dest, len, MSG_WAITALL);
>>> -       if (ret != len)
>>> -               return -1;
>>> -
>>> -       return 1;
>>> -}
>>> -
>>>  int request_remote_pages(int pid, unsigned long addr, int nr_pages)
>>>  {
>>>         struct page_server_iov pi = {
>>> @@ -945,6 +915,10 @@ int receive_remote_pages_info(int *nr_pages, unsigned long *addr, int *pid)
>>>                 return -1;
>>>         }
>>>
>>> +       if (pi.cmd == PS_IOV_ZERO)
>>> +               pr_warn("Unexpected ZERO page received for %d.%lx\n",
>>> +                               (int)pi.dst_id, (unsigned long)pi.vaddr);
>>> +
>>>         *nr_pages = pi.nr_pages;
>>>         *addr = pi.vaddr;
>>>         *pid = pi.dst_id;
>>> diff --git a/criu/pagemap.c b/criu/pagemap.c
>>> index e05a468..86791d7 100644
>>> --- a/criu/pagemap.c
>>> +++ b/criu/pagemap.c
>>> @@ -397,16 +397,19 @@ static int maybe_read_page_local(struct page_read *pr, unsigned long vaddr,
>>>  static int maybe_read_page_remote(struct page_read *pr, unsigned long vaddr,
>>>                 int nr, void *buf, unsigned flags)
>>>  {
>>> -       int ret;
>>> +       int ret, pid;
>>>
>>> -       if (flags & PR_ASYNC)
>>> -               /*
>>> -                * Note, that for async remote page_read, the actual
>>> -                * transfer happens in the lazy-pages daemon
>>> -                */
>>> -               ret = request_remote_pages(pr->pid, vaddr, nr);
>>> -       else
>>> -               ret = get_remote_pages(pr->pid, vaddr, nr, buf);
>>> +       ret = request_remote_pages(pr->pid, vaddr, nr);
>>> +       if ((ret < 0) || (flags & PR_ASYNC))
>>> +               return ret;
>>
>> request_remote_pages uses send(..., MSG_DONTWAIT)
>> Are you sure that for synchronous case we won't get some weird effects here?
>
> Well, "weird effects" no :) In the worst case we'll receive the exit
> code that != sizeof(something) and will abort the operation.
>
> But the same would anyway be a problem even for synchronous case, so
> this particular MSG_DONTWAIT seem to be incorrect by itself.

I've used MSG_DONTWAIT for async case only. This way we can return to
epoll without waiting for net stack.
For the sync case we used to have write(sk, ...). which, AFAIU, waits
for TCP ACK before returning.
Am I missing something?

> -- Pavel



-- 
Sincerely yours,
Mike.