Re: [PATCH v0 3/4] migration: add background snapshot

[Peter Xu]

On Wed, Jul 22, 2020 at 11:11:32AM +0300, Denis Plotnikov wrote:
> +/**
> + * ram_copy_page: make a page copy
> + *
> + * Used in the background snapshot to make a copy of a memeory page.
> + * Ensures that the memeory page is copied only once.
> + * When a page copy is done, restores read/write access to the memory
> + * page.
> + * If a page is being copied by another thread, wait until the copying
> + * ends and exit.
> + *
> + * Returns:
> + *   -1 - on error
> + *    0 - the page wasn't copied by the function call
> + *    1 - the page has been copied
> + *
> + * @block:     RAM block to use
> + * @page_nr:   the page number to copy
> + * @page_copy: the pointer to return a page copy
> + *
> + */
> +int ram_copy_page(RAMBlock *block, unsigned long page_nr,
> +                          void **page_copy)
> +{
> +    void *host_page;
> +    int res = 0;
> +
> +    atomic_inc(&ram_state->page_copier_cnt);
> +
> +    if (test_and_set_bit_atomic(page_nr, block->touched_map)) {
> +        while (!test_bit_atomic(page_nr, block->copied_map)) {
> +            /* the page is being copied -- wait for the end of the copying */
> +            qemu_event_wait(&ram_state->page_copying_done);
> +        }
> +        goto out;
> +    }
> +
> +    *page_copy = ram_page_buffer_get();
> +    if (!*page_copy) {
> +        res = -1;
> +        goto out;
> +    }
> +
> +    host_page = block->host + (page_nr << TARGET_PAGE_BITS);
> +    memcpy(*page_copy, host_page, TARGET_PAGE_SIZE);
> +
> +    if (ram_set_rw(host_page, TARGET_PAGE_SIZE)) {
> +        ram_page_buffer_free(*page_copy);
> +        *page_copy = NULL;
> +        res = -1;
> +        goto out;
> +    }
> +
> +    set_bit_atomic(page_nr, block->copied_map);
> +    qemu_event_set(&ram_state->page_copying_done);
> +    qemu_event_reset(&ram_state->page_copying_done);
> +
> +    res = 1;
> +out:
> +    atomic_dec(&ram_state->page_copier_cnt);
> +    return res;
> +}

Is ram_set_rw() be called on the page only if a page fault triggered?
Shouldn't we also do that even in the background thread when we proactively
copying the pages?

Besides current solution, do you think we can make it simpler by only deliver
the fault request to the background thread?  We can let the background thread
to do all the rests and IIUC we can drop all the complicated sync bitmaps and
so on by doing so.  The process can look like:

  - background thread runs the general precopy migration, and,

    - it only does the ram_bulk_stage, which is the first loop, because for
      snapshot no reason to send a page twice..

    - After copy one page, do ram_set_rw() always, so accessing of this page
      will never trigger write-protect page fault again,

    - take requests from the unqueue_page() just like what's done in this
      series, but instead of copying the page, the page request should look
      exactly like the postcopy one.  We don't need copy_page because the page
      won't be changed before we unprotect it, so it shiould be safe.  These
      pages will still be with high priority because when queued it means vcpu
      writed to this protected page and fault in userfaultfd.  We need to
      migrate these pages first to unblock them.

  - the fault handler thread only needs to do:

    - when get a uffd-wp message, translate into a postcopy-like request
      (calculate ramblock and offset), then queue it.  That's all.

I believe we can avoid the copy_page parameter that was passed around, and we
can also save the two extra bitmaps and the complicated synchronizations.

Do you think this would work?

Besides, have we disabled dirty tracking of memslots?  IIUC that's not needed
for background snapshot too, so neither do we need dirty tracking nor do we
need to sync the dirty bitmap from outside us (kvm/vhost/...).

-- 
Peter Xu