Re: [PATCH RFC 20/21] migration: Handle page faults using UFFDIO_CONTINUE

[Juan Quintela <quintela@redhat.com>]

Peter Xu <peterx@redhat.com> wrote:
> Teach QEMU to be able to handle page faults using UFFDIO_CONTINUE for
> hugetlbfs double mapped ranges.
>
> To copy the data, we need to use the mirror buffer created per ramblock by
> a raw memcpy(), then we can kick the faulted threads using UFFDIO_CONTINUE
> by installing the pgtables.
>
> Move trace_postcopy_place_page(host) upper so that it'll dump something for
> either UFFDIO_COPY or UFFDIO_CONTINUE.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

> ---
>  migration/postcopy-ram.c | 55 ++++++++++++++++++++++++++++++++++++++--
>  migration/trace-events   |  4 +--
>  2 files changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 8a2259581e..c4bd338e22 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -1350,6 +1350,43 @@ int postcopy_notify_shared_wake(RAMBlock *rb, uint64_t offset)
>      return 0;
>  }
>  
> +/* Returns the mirror_host addr for a specific host address in ramblock */
> +static inline void *migration_ram_get_mirror_addr(RAMBlock *rb, void *host)
> +{
> +    return (void *)((__u64)rb->host_mirror + ((__u64)host - (__u64)rb->host));

This is gross :-(
I hate this C miss-feature.

What about:
    return (char *)rb->host_mirror + (char*)host - (char*)rb->host;

But I don't know if it (much) clearer.  And no, I don't remember if we
ever need more parents.

gcc used to do "the right" thing on void * arithmetic, but it is not in
the standard, and I don't know what is worse.

> +}
> +
> +static int
> +qemu_uffd_continue(MigrationIncomingState *mis, RAMBlock *rb, void *host,
> +                   void *from)
> +{
> +    void *mirror_addr = migration_ram_get_mirror_addr(rb, host);
> +    /* Doublemap uses small host page size */
> +    uint64_t psize = qemu_real_host_page_size();
> +    struct uffdio_continue req;
> +
> +    /*
> +     * Copy data first into the mirror host pointer; we can't directly copy
> +     * data into rb->host because otherwise our thread will get trapped too.
> +     */
> +    memcpy(mirror_addr, from, psize);
> +
> +    /* Kick off the faluted threads to fetch data from the page cache
                       ^^^^^^^
> */

Faulted

> +    req.range.start = (__u64)host;
> +    req.range.len = psize;
> +    req.mode = 0;
> +	if (ioctl(mis->userfault_fd, UFFDIO_CONTINUE, &req)) {
> +        error_report("%s: UFFDIO_CONTINUE failed for start=%p"
> +                     " len=0x%"PRIx64": %s\n", __func__, host,
> +                     psize, strerror(-req.mapped));
> +        return req.mapped;
> +    }
> +
> +    postcopy_mark_received(mis, rb, host, psize / qemu_target_page_size());
> +
> +    return 0;
> +}
> +
>  /*
>   * Place a host page (from) at (host) atomically
>   * returns 0 on success
> @@ -1359,6 +1396,18 @@ int postcopy_place_page(MigrationIncomingState *mis, void *host, void *from,
>  {
>      size_t pagesize = migration_ram_pagesize(rb);
>  
> +    trace_postcopy_place_page(rb->idstr, (uint8_t *)host - rb->host, host);
> +
> +    if (postcopy_use_minor_fault(rb)) {
> +        /*
> +         * If minor fault used, we use UFFDIO_CONTINUE instead.
> +         *
> +         * TODO: support shared uffds (e.g. vhost-user). Currently we're
> +         * skipping them.
> +         */
> +        return qemu_uffd_continue(mis, rb, host, from);
> +    }
> +
>      /* copy also acks to the kernel waking the stalled thread up
>       * TODO: We can inhibit that ack and only do it if it was requested
>       * which would be slightly cheaper, but we'd have to be careful
> @@ -1372,7 +1421,6 @@ int postcopy_place_page(MigrationIncomingState *mis, void *host, void *from,
>          return -e;
>      }
>  
> -    trace_postcopy_place_page(host);
>      return postcopy_notify_shared_wake(rb,
>                                         qemu_ram_block_host_offset(rb, host));
>  }
> @@ -1385,10 +1433,13 @@ int postcopy_place_page_zero(MigrationIncomingState *mis, void *host,
>                               RAMBlock *rb)
>  {
>      size_t pagesize = migration_ram_pagesize(rb);
> -    trace_postcopy_place_page_zero(host);
> +    trace_postcopy_place_page_zero(rb->idstr, (uint8_t *)host - rb->host, host);

It is me, or to be standard compliant, you need to cast also rb->host?


>      /* Normal RAMBlocks can zero a page using UFFDIO_ZEROPAGE
>       * but it's not available for everything (e.g. hugetlbpages)
> +     *
> +     * NOTE: when hugetlb double-map enabled, then this ramblock will never
> +     * have RAM_UF_ZEROPAGE, so it'll always go to postcopy_place_page().
>       */
>      if (qemu_ram_is_uf_zeroable(rb)) {
>          if (qemu_ufd_copy_ioctl(mis, host, NULL, pagesize, rb)) {
> diff --git a/migration/trace-events b/migration/trace-events
> index 6b418a0e9e..7baf235d22 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -265,8 +265,8 @@ postcopy_discard_send_range(const char *ramblock, unsigned long start, unsigned
>  postcopy_cleanup_range(const char *ramblock, void *host_addr, size_t offset, size_t length) "%s: %p offset=0x%zx length=0x%zx"
>  postcopy_init_range(const char *ramblock, void *host_addr, size_t offset, size_t length) "%s: %p offset=0x%zx length=0x%zx"
>  postcopy_nhp_range(const char *ramblock, void *host_addr, size_t offset, size_t length) "%s: %p offset=0x%zx length=0x%zx"
> -postcopy_place_page(void *host_addr) "host=%p"
> -postcopy_place_page_zero(void *host_addr) "host=%p"
> +postcopy_place_page(const char *id, size_t offset, void *host_addr) "id=%s offset=0x%zx host=%p"
> +postcopy_place_page_zero(const char *id, size_t offset, void *host_addr) "id=%s offset=0x%zx host=%p"
>  postcopy_ram_enable_notify(void) ""
>  mark_postcopy_blocktime_begin(uint64_t addr, void *dd, uint32_t time, int cpu, int received) "addr: 0x%" PRIx64 ", dd: %p, time: %u, cpu: %d, already_received: %d"
>  mark_postcopy_blocktime_end(uint64_t addr, void *dd, uint32_t time, int affected_cpu) "addr: 0x%" PRIx64 ", dd: %p, time: %u, affected_cpu: %d"

I think that you can split the part of the patch that changes the
traces.  But again, it is up to you.